[WIP] [3.2][feature/revisions] Implement post revision tracking by imkingdavid · Pull Request #1200 · phpbb/phpbb

imkingdavid · 2013-01-19T16:56:03Z

The majority of the functionality has been tested and works. There are still a few minor things that need to be done and tested, but I would like to go ahead and get the peer testing/reviewing started.

To test:

Install this branch
Go to the Post Settings module in the main ACP tab, scroll down to the last category of settings, and ensure the feature is enabled. All other settings added by this PR can be left as default unless you wish to test a specific setting.
Ensure that your account has moderator permissions for viewing and managing post revisions, or at least that you can view your own posts' revisions.
Edit a post at least once (if you're testing the user f_revisions permission, it has to be one of the user's own posts or a wiki post).
When viewing the post, click "View edit history". When javascript is enabled, this will review a short list of the latest revisions directly beneath the post. If you click the "View edit history" link in the "LAST 5 REVISIONS (VIEW EDIT HISTORY)" line, or if javascript is disabled, it will take you to the full revisions view page.
Play around with it some and let me know what does and doesn't work, and what needs to be changed.

Still to do:

Testing
Wiki Posts functionality
Permissions
Automated tests (unit, functional)
Other
Create migrations (instead of database_update.php changes)

moddfish · 2013-01-19T22:45:43Z

Probably not from this PR but ACP -> Permissions -> * Roles -> any role:
Strict Standards: Non-static method auth_admin::build_permission_array() should not be called statically, assuming $this from incompatible context in ../revisions/includes/acp/acp_permission_roles.php on line 465

Attempting to delete topic starter (protected by default) in revision history:
Catchable fatal error: Argument 1 passed to phpbb_revisions_comparison::__construct() must be an instance of phpbb_revisions_revision, boolean given, called in ../revisions/viewtopic.php on line 1561 and defined in ../revisions/includes/revisions/comparison.php on line 67

Reverse compare between any revision # (e.g. 2 & 3 or 4 & 9) other than the first and x (i.e. 1 & 3 or 1 & 9) gives \n\n for line spacing. image: http://www.oddmods.co/mods/revision_line_spacing_reverse_compare.png

imkingdavid · 2013-01-19T22:49:17Z

Permissions -> * Roles -> any role:
Strict Standards: Non-static method auth_admin::build_permission_array() should not be called statically, assuming $this from incompatible context in ../revisions/includes/acp/acp_permission_roles.php on line 465

Can you check if you get this error on develop. I'm not sure how that would be an issue with this feature specifically.

Attempting to delete topic starter (protected by default) in revision history:
Catchable fatal error: Argument 1 passed to phpbb_revisions_comparison::__construct() must be an instance of phpbb_revisions_revision, boolean given, called in ../revisions/viewtopic.php on line 1561 and defined in ../revisions/includes/revisions/comparison.php on line 67

By topic starter, you mean the original revision of the topic? I'll see if I can reproduce this. To note, protecting a revision does not prevent it from being deleted; it only prevents it from being removed by the automatic pruning tasks.

Reverse compare between any revision # (e.g. 2 & 3 or 4 & 9) other than the first and x (i.e. 1 & 3 or 1 & 9) gives \n\n for line spacing. image: http://www.oddmods.co/mods/revision_line_spacing_reverse_compare.png

I will see if I can reproduce this.

Thanks for testing!

moddfish · 2013-01-19T23:11:50Z

The Strict Standards error is in develop so nothing to do with this PR.

Original topic (topic starter or first post) yes. The specified post ID has no associated revisions which is correct but attempt to view the post by selecting the topic from the index and the error shows.

More testing on the Reverse compare shows if only one edit, the error shows /n/n for a line spacing. More than one and the scenario I posted above is true.

Would be nice to see a description above each column in revision history (although it does not take long to understand it) and the delete button inline with (protect) (restore) etc.

imkingdavid · 2013-01-19T23:30:44Z

Okay, I see it displaying \n\n when doing a reverse comparison. I will take a look and see why it is doing that.

I have reproduced the error caused by deleting all of the revisions and am working on an issue. The problem was that the delete function was not updating the post edit count, so it thought it had revisions when it didn't, and that was generating an error on viewtopic.

imkingdavid · 2013-01-19T23:49:11Z

I have fixed the issue with deleting revisions.

In regards to the other issue, it appears that any time you view a deletion of a line break, it displays \n, but viewing addition of a line break does not display \n. I'll keep looking into it to see why it does this.

moddfish · 2013-01-19T23:57:07Z

Restoring unprotected post shows this before submit (NOTE the selected compare of the word "edit"): http://www.oddmods.co/mods/revision_restore_compare.png
Submitting the above results in this error:
Fatal error: Call to undefined method phpbb_revisions_post::restore() in /home/me/public_html/revisions/includes/controller/post_revisions.php on line 486

imkingdavid · 2013-01-20T00:04:09Z

The issue with the undefined function was because i had forgotten to update my repository file with the file I was using on my local test board. This has been fixed.

As for how the diff displays the change with the word "edit", that is unfortunately the way it is shown, and there's not really much I can do to change that.

moddfish · 2013-01-20T00:17:07Z

Delete the first in revision history.
Deleting the second edit with a third edit as the Current Revision results in "The specified revision could not be found."
Any chance of a "Return to .." link or to somewhere practical from there?

imkingdavid · 2013-01-20T00:20:58Z

I created a post and edited it twice (two revisions plus the current version). I deleted the first revision, and then without a page refresh, deleted the second revision. I did not see any error about revisions not being found. Are you using with javascript or without?

EDIT: If you mean you would like a "Return to post" link from the "This post has no associated revisions" page, that can be done.

moddfish · 2013-01-20T00:30:40Z

argh! back buttoning is not a good idea sorry.
With javascript. Made 2 edits, deleted the original then deleted the first edit (#2) and yeah I get this: "The specified post ID has no associated revisions."
Any chance of a "Return to .." link or to somewhere practical from there?

EDIT* funny thing is even with that ("The specified post ID has no associated revisions."), the edit is removed.

moddfish · 2013-01-20T01:23:07Z

Restore post line space \n\n : http://www.oddmods.co/mods/revision_restore_line_space_compare.png

Would there be a simple way to distinguish between an edit and a restore?

moddfish · 2013-01-20T01:34:54Z

phpBB/language/en/acp/revisions.php

edited, an a copy -> edited, a copy

moddfish · 2013-01-20T20:34:52Z

Upgrade from 3.0.11..
database_update.php not adding post_revisions table.
Post Revision Tracking module not loaded.
EDIT: post_wiki and post_revision_count not added to posts table

moddfish · 2013-01-27T00:08:29Z

revision.USERNAME.

I would have thought as the edited name appears on the post (i.e. "Last edited by Admin"), then the Username who's revision it is should follow into revision history.

Also what about headings for the columns in the revision page : http://www.oddmods.co/mods/revision_head.png

imkingdavid · 2013-02-14T16:33:24Z

As for the database_update issue, I've decided I'm just going to wait for the migrations pull requests to be merged, rather than fix database_update, only to scrap it anyway.

revision.USERNAME.

I would have thought as the edited name appears on the post (i.e. "Last edited by Admin"), then the Username who's revision it is should follow into revision history.

I don't understand the problem you are describing.

Also what about headings for the columns in the revision page : http://www.oddmods.co/mods/revision_head.png

That looks good thanks for the suggestion.

imkingdavid · 2013-02-14T16:36:15Z

Also, I have made the revisions page show up for all existing posts, even if no revisions have been created. In that case it simply shows the current post.

moddfish · 2013-02-14T17:46:10Z

revision.USERNAME.

I would have thought as the edited name appears on the post (i.e. "Last edited by Admin"), then the Username who's revision it is should follow into revision history.

When someone edits a post, the username shows as the editor under the post ("Last edited by username"). When viewing post revisions, the username always shows as the original poster, not the editor. In the revision_head.png image above, all show GModerator - most of those revisions (edits) were made by Admin.

imkingdavid · 2013-02-14T18:26:37Z

Oh, I didn't realize that since I've been testing with only one user. xD

imkingdavid · 2013-02-14T18:35:19Z

It looks to me like I have it storing the wrong user ID when posts are edited; right now it's storing the original post author's user ID, so I need to change it to store the current user's ID when an edit occurs.

imkingdavid · 2013-02-14T19:00:30Z

Alright, just fixed that issue, I think.

moddfish · 2013-02-15T17:58:44Z

so I need to change it to store the current user's ID when an edit occurs.

Looks much betterra.

revisions_comparison_list.html loses shape when minimising.
0bb5559 negates the progress of df0d92b

EXreaction · 2013-02-16T04:45:52Z

phpBB/config/routing.yml

If possible, I'd prefer that we use some good organization for this.

config/routing/post_revisions.yml (for all /post/revisions routes).

Good organization from the start will be very helpful in the longrun. :)

IIRC only routing.yml is loaded automatically. Is there a way to "include" other routing files within this one? Otherwise, we'll have to modify this on the PHP side every time we want to add routing for another component.

I would assume it works the same as the other yml files (see the top of services.yml).

Found this: http://symfony.com/doc/current/book/routing.html#including-external-routing-resources

I'll work on adapting what I have to that.

Have you tried the prefix option to see if that works? That would be really nice to use.

Well right now I'm trying to get the resource loading to work.

Cannot import resource "config\routing/post_revisions.yml" from "routing.yml".

I can't find where it inserts the "config" part. I'm wondering if it's the \ instead of /. Maybe it's because I'm on Windows?

Maybe it doesn't work with subdirectories? You try just in the config/ directory?

imkingdavid · 2013-03-01T05:08:54Z

@EXreaction any testing and feedback you can provide on this would be great. I'll take a look at this tomorrow afternoon sometime.

EXreaction · 2013-03-01T15:08:27Z

Missing the modules from the migration

EXreaction · 2013-03-01T15:18:56Z

When you mention permissions in your todo, I assume you mean add the new permissions to existing roles, right? If not, that needs to be done. :)

…re/revisions

PHPBB3-110755

PHPBB3-10755

…ve bolding PHPBB3-10755

PHPBB3-10755

Fixes

…revisions # By Marc Alexander (160) and others # Via Joas Schilling (52) and others * 'develop' of github.com:imkingdavid/phpbb3: (607 commits) [prep-release-3.1.0-a3] Update version to 3.1.0-a4-dev for future development [ticket/11880] migration helper file [prep-release-3.1.0-a3] Commit changelog for 3.1.0-a3 [ticket/11880] Move get_schema_steps function to a migrator helper class [prep-release-3.1.0-a3] Prepare Changelog [prep-release-3.1.0-a3] Add migration for 3.1.0-a3 [prep-release-3.1.0-a3] Update version to 3.1.0-a3 [ticket/12163] Use posts_per_page to calculate current page in viewtopic.php [develop] Remove left over conflict marker from merge 1a31f03 [ticket/12162] Set language to C before running diff to ensure its english [ticket/11509] Be exact when referencing the branch name. [ticket/12141] Disable broken opcache extension on PHP 5.5.7. [ticket/12141] PHP extension setup must happen before webserver + PHP-FPM. [ticket/12161] Do not use subdirectories in build/save directory. [ticket/11985] Do not try to setup PHP extensions on HHVM. [ticket/11985] Rename install -> setup. This is more generic and consistent. [ticket/11509] Run commit message checker only in one environment. [ticket/11509] Contrary to docs, COMMIT_RANGE is not available in PR mode. [ticket/12159] Fix codesniffer complaints due to profilefields and passwords [ticket/11880] Break up schema changes in the migrator ... Conflicts: phpBB/install/schemas/mssql_schema.sql tests/test_framework/phpbb_functional_test_case.php

naderman · 2014-03-20T02:02:37Z

status on this / test failures?

imkingdavid · 2014-03-21T02:32:15Z

@naderman i haven't really looked at this recently. I'll try to make some more progress soon

Conflicts: phpBB/install/schemas/mssql_schema.sql

…develop Changed url() to route() and removed user location field that is now a custom field. PHPBB3-10755

PHPBB3-10755

naderman · 2014-05-08T14:33:15Z

@nickvergessen ?

imkingdavid · 2014-05-08T14:38:18Z

[10:36:58] <imkingdavid> nickvergessen: https://github.com/phpbb/phpbb/pull/1200 ?
[10:37:13] <nickvergessen> imkingdavid: reopen when you work on it
[10:37:19] <imkingdavid> okay
[10:37:23] <nickvergessen> i just closed it to have a more useful PR list
[10:37:27] <imkingdavid> sounds good

naderman · 2014-05-08T14:38:44Z

Ah :(

imkingdavid · 2015-05-24T00:38:30Z

The rebase for this is going to take too much work. When I get back to this, I'm just going to start a new branch and a new PR, following along with this one as I go.

Nicofuma · 2015-05-25T15:55:57Z

as you wish :) and don't hesitate to ask if you need something. I'll be glad to help you (I'd love to have that feature in 3.2 :p)

moddfish reviewed Jan 20, 2013
View reviewed changes

phpBB/language/en/acp/revisions.php Outdated

Copy link
Copy Markdown

moddfish Jan 20, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

edited, an a copy -> edited, a copy

EXreaction reviewed Feb 16, 2013
View reviewed changes

imkingdavid and others added 14 commits October 26, 2013 14:39

Merge remote-tracking branch 'prototech/feature/revisions' into featu…

cb0cf97

…re/revisions

Merge remote-tracking branch 'upstream/develop' into feature/revisions

6831a89

Merge branch 'develop' of github.com:phpbb/phpbb3 into feature/revisions

9647610

[feature/revisions] Add icons on comparison page

cbdc2b9

PHPBB3-110755

[feature/revisions] Use method instead of property

f0f8628

PHPBB3-10755

[feature/revisions] Add avatars back to the comparison list.

ebc9fc5

PHPBB3-10755

[feature/revisions] Add some space below the success message and remo…

8591c53

…ve bolding PHPBB3-10755

[feature/revisions] Replace "return to" with breadcrumbs

6d12b94

PHPBB3-10755

[feature/revisions] Display revision array position instead of id

e504bca

PHPBB3-10755

[feature/revisions] Fix title of icons

9465a89

PHPBB3-10755

[feature/revisions] Fix test request URL

9b7ad00

PHPBB3-10755

[feature/revisions] Fixed the revisions test

a054444

PHPBB3-10755

Merge pull request #11 from prototech/feature/revisions

97adc5c

Fixes

nickvergessen added this to the 3.2.0-a1 milestone Mar 14, 2014

imkingdavid added 3 commits March 20, 2014 19:49

Merge branch 'develop' of github.com:phpbb/phpbb into feature/revisions

1eadb23

Conflicts: phpBB/install/schemas/mssql_schema.sql

[feature/revisions] Get this branch back up to speed with the latest …

7179bb3

…develop Changed url() to route() and removed user location field that is now a custom field. PHPBB3-10755

[feature/revisions] Address some PR comments

0908a49

PHPBB3-10755

nickvergessen changed the title ~~WIP: [feature/revisions] Implement post revision tracking~~ [WIP] [feature/revisions] Implement post revision tracking Apr 15, 2014

nickvergessen changed the title ~~[WIP] [feature/revisions] Implement post revision tracking~~ [WIP] [3.2][feature/revisions] Implement post revision tracking May 8, 2014

nickvergessen closed this May 8, 2014

Nicofuma added the 3.2 (Rhea) label Feb 26, 2015

Nicofuma added this to the 3.2.0 milestone Feb 26, 2015

Uh oh!

Conversation

imkingdavid commented Jan 19, 2013

Uh oh!

moddfish commented Jan 19, 2013

Uh oh!

imkingdavid commented Jan 19, 2013

Uh oh!

moddfish commented Jan 19, 2013

Uh oh!

imkingdavid commented Jan 19, 2013

Uh oh!

imkingdavid commented Jan 19, 2013

Uh oh!

moddfish commented Jan 19, 2013

Uh oh!

imkingdavid commented Jan 20, 2013

Uh oh!

moddfish commented Jan 20, 2013

Uh oh!

imkingdavid commented Jan 20, 2013

Uh oh!

moddfish commented Jan 20, 2013

Uh oh!

moddfish commented Jan 20, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moddfish commented Jan 20, 2013

Uh oh!

moddfish commented Jan 27, 2013

Uh oh!

imkingdavid commented Feb 14, 2013

Uh oh!

imkingdavid commented Feb 14, 2013

Uh oh!

moddfish commented Feb 14, 2013

Uh oh!

imkingdavid commented Feb 14, 2013

Uh oh!

imkingdavid commented Feb 14, 2013

Uh oh!

imkingdavid commented Feb 14, 2013

Uh oh!

moddfish commented Feb 15, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imkingdavid commented Mar 1, 2013

Uh oh!

EXreaction commented Mar 1, 2013

Uh oh!

EXreaction commented Mar 1, 2013

Uh oh!

naderman commented Mar 20, 2014

Uh oh!

imkingdavid commented Mar 21, 2014

Uh oh!

naderman commented May 8, 2014

Uh oh!

imkingdavid commented May 8, 2014

Uh oh!

naderman commented May 8, 2014

Uh oh!

imkingdavid commented May 24, 2015

Uh oh!

Nicofuma commented May 25, 2015

Uh oh!