Ticket #566 (closed enhancement: fixed)

Opened 7 years ago

Last modified 6 years ago

Revert handler and mass reversion utility

Reported by: DarTar Owned by: BrianKoontz
Priority: normal Milestone: 1.1.6.4
Component: administration Version: 1.1.6.3
Severity: normal Keywords: administration antispam 1.1.7-ported
Cc:

Description

A mass reversion utility (porting the existing perl script). It uses the pageadmin action (#26) as an interface to facilitate mass reversions. Revert itself will be a simple page handler than can be invoked on a per-page basis if necessary.

Attachments

0000.PNG Download (21.9 KB) - added by DotMG 6 years ago.
screenshot of date set to 0000.00.00 00:00:00

Change History

  Changed 6 years ago by BrianKoontz

(In [808]) Several modifications to support mass reversions (refs #566): * Pages are searchable by last edit date in the pageadmin view * Multiple pages can be selected (perhaps after searching by edit

date range) and then be mass-reverted to each page's previous version

* A new revert handler has been created that reverts a page to its

previous version, and also supports multiple page reversions

  Changed 6 years ago by BrianKoontz

(In [816]) Refactored reversion functions into separate library, cleaned up handler code (refs #566).

  Changed 6 years ago by BrianKoontz

(In [817]) Display confirmation page for mass reversions (refs #566).

  Changed 6 years ago by BrianKoontz

(In [818]) Confirmation page text field for reversion comment (refs #566).

  Changed 6 years ago by BrianKoontz

(In [819]) Changed default comment for reversion to "Reverting last edit by UserName [id] to previous version [id]" (refs #566).

  Changed 6 years ago by BrianKoontz

(In [821]) Renamed revert.lib.php to admin.lib.php to better reflect library purpose (refs #566).

  Changed 6 years ago by DarTar

Mass reversion fails with a DB error if the text of the revision contains single quotes, I guess you should escape them.

follow-up: ↓ 9   Changed 6 years ago by DarTar

Brian, a few further comments on top of the bug above:

  • I don't know what you think of this, but my idea was that the confirmation screen should be part of the handler itself, not the admin module. So for instance when I append /revert (with a comma separated list of page names or page id's), I can call the revert handler from anywhere. Making handlers and actions as self-contained and interoperable as possible (see the case with userfeedback for instance) is IMO a good idea, what do you think?
  • Do we want to have confirmation screens for single pages as well or keep the 1 click reversion? I see the advantages of both solutions, but a confirmation screen for single reverts can be useful too (see below)
  • Ideally, no JS/popup should be used. Maybe you can display an < em class="success">...</em> message at the top of the admin module?
  • pagenames in the confirmation screen for mass reverts should be clickable and use a <ul>. Also it'd be really nice and user friendly to add something like:
    PageName (current [31] :: previous [30] :: diff)

in reply to: ↑ 8   Changed 6 years ago by BrianKoontz

Replying to DarTar:

Brian, a few further comments on top of the bug above: * I don't know what you think of this, but my idea was that the confirmation screen should be part of the handler itself, not the admin module. So for instance when I append /revert (with a comma separated list of page names or page id's), I can call the revert handler from anywhere. Making handlers and actions as self-contained and interoperable as possible (see the case with userfeedback for instance) is IMO a good idea, what do you think?

JavaWoman is very adamant that a handler is a resource, and should not be forced into "double-duty" by using them for processing internal, non-browser-generated calls. So in this case, the revert handler does only one thing: Processes a browser-generated revert call, then passes off handling to a library.

* Do we want to have confirmation screens for single pages as well or keep the 1 click reversion? I see the advantages of both solutions, but a confirmation screen for single reverts can be useful too (see below)

I think it would be advantageous to be consistent. I'm not a big fan of message dialogs (they are very intrusive IMO), so there's no reason why we couldn't display a confirmation page.>

* Ideally, no JS/popup should be used. Maybe you can display an < em class="success">...</em> message at the top of the admin module?

Agreed. I was simply using the legacy code already in place.

* pagenames in the confirmation screen for mass reverts should be clickable and use a <ul>. Also it'd be really nice and user friendly to add something like:
PageName (current [31] :: previous [30] :: diff)

OK.

  Changed 6 years ago by BrianKoontz

(In [824]) Escaped page body output from DB so it can be fed back into the DB without error (refs #566).

  Changed 6 years ago by BrianKoontz

(In [825]) Reformatted revert confirmation page to include page links, current and previous versions, and a diff link (refs #566).

  Changed 6 years ago by BrianKoontz

(In [826]) Minor edit (refs #566).

  Changed 6 years ago by DarTar

Brian, I think the revert link in adminpages should be disabled when there is just one revision.

  Changed 6 years ago by DarTar

There is also a problem in the mass reversion screen for pages with a single revision

  Changed 6 years ago by BrianKoontz

(In [835]) Revert link disabled in case of page having only single revision; error message displayed during mass reversions for pages with only single revision (refs #566).

  Changed 6 years ago by DotMG

When reverting, I'd like to see the modification date set to now(). The problem is that if we do like this, last reverted pages will appear on top of the list.

  Changed 6 years ago by BrianKoontz

A new page is created that is an exact replica of the page immediately preceding the target page, except for the 'time' field, which is set to now() (via strftime()). I chose to do this so that the page history would be accurate and would reflect actual events (rather than simply resetting the 'latest' field to Y).

Changed 6 years ago by DotMG

screenshot of date set to 0000.00.00 00:00:00

  Changed 6 years ago by DotMG

The date is set to 000000 on my local host (Wamp 5). Maybe a bug in PHP's strftime()? see screenshot attached

  Changed 6 years ago by BrianKoontz

Indeed, it appears that Microsoft (as usual) is the culprit here:

(from PHP docs)

This means that e.g. %e, %T, %R and %D (there might be more) and dates prior to Jan 1, 1970 will not work on Windows

So, I will investigate the MSDN resource provided and determine what needs to be done.

  Changed 6 years ago by DotMG

Can't we just re-use the SavePage() method here? Since we have already a method to save a page, it should be reused wherever possible, at least this would facilitate maintenance and portability! Look: we will support other DB engine later, (even flat files if I have enough inspiration), so it's a good idea to try to keep SQL instructions in fewer places (as possible).

  Changed 6 years ago by BrianKoontz

(In [855]) Replaced SQL with SavePage() call; added optional parameter to SavePage() to allow for page owner override. Refs #566.

  Changed 6 years ago by DotMG

Now, it's fine. Thanks.

  Changed 6 years ago by BrianKoontz

  • status changed from new to closed
  • resolution set to fixed

Tests successful in revision 863.

  Changed 6 years ago by DarTar

(In [988]) Better layout for mass reversion handler, refs #566 and #650

  Changed 6 years ago by DarTar

  • keywords 1.1.7-ported added
Note: See TracTickets for help on using tickets.