[PATCH] "are you sure you want to delete this page?" question needs more details

[team@…]

When I'm trying to delete a page from Wiki, I see the following message:

Delete MyPageName
Are you sure you want to completely delete this page? 
This is an irreversible operation.

Would be much better to see this text:

Delete MyPageName
Are you sure you want to completely delete this page? 
This is an irreversible operation (23 revisions since 3-Jan-2010).

[Christian Boos]

… given the rate at which you're making (nice) suggestions, I think you should now also seriously start to consider bringing directly the changes themselves on the table in form of patches. See TracDev/SubmittingPatches if you're not yet familiar with the process.

Especially for the kind of change suggested here, it would be for us just a matter of a quick review and applying the change. Could be much more efficient than waiting for us to eventually implement the changes… Plus you'd be able to test how your proposed change would look and feel like and the suggestion could come in a more refined form.

[team@…]

will do, once I finish reading the book about Python :)

[Christian Boos]

2 months is more than enough to learn Python, no?

Anyway, this was a good suggestion and we'll eventually do it by ourselves when time permits.

[Mikael Relbe]

No, 2 months is a very short time for some of us ;) I'll do this…

[erne.castro@…]

Patch for current 0.12-stable to add more info on the page delete confirmation.

[erne.castro@…]

I just playaround a bit with this and came out with this patch.

I'm sure its not the perfect solution, but is just my five cents.

Please don't be cruel… this is my first patch submitted! :)

[Christian Boos]

Not bad for a first patch ;-)

I made a few follow-up changes,

• the <a href="${href.wiki(...)}">... and last one ${dateinfo(last_date)} ago. line was a bit long
• the wording could be improved: instead of e.g. 5 revisions, first one 1 month ago and last one 1 day ago) I prefer to display: Removed all 5 revisions of the page, which was created 1 month ago and last modified 1 day ago. earlier in the page (this just a detail, of course, and you may disagree ;-) )
• the message was lacking i18n support (well, that link doesn't show much info for now, that will be hopefully be expanded one day)
• the Python code is OK, but after duplicating the loop, you could have continued and simplified the code a bit

As an example of how could things be polished further, I add a few more patches on top of yours. I hope this will help you to get started!

[Christian Boos]

some improvements to t9220-page-delete-more-info-0.12-stable-r10138.patch​ (1/3)

[Christian Boos]

applies on top of wiki_delete_improvements.patch​ (2/3)

[Christian Boos]

applies on top of no-loop.patch​ (3/3)

[Mikael Relbe]

Argh… I was also producing a patch today… I'm supplying it anyway, perhaps it can create some inspiration: t9220-mrelbe-r10140.diff​

[Mikael Relbe]

#9220 - Another approach, fully working

[Mikael Relbe]

The template file in t9220-mrelbe-r10140.diff​ is intentionally not indented as you probably want, Christian. As you can see, I changed to the whole lot here. The current indentation makes it easy to see what's going on, and easy to "correct".

[erne.castro@…]

Replying to cboos:

Not bad for a first patch ;-)

I made a few follow-up changes,

• the <a href="${href.wiki(...)}">... and last one ${dateinfo(last_date)} ago. line was a bit long
• the wording could be improved: instead of e.g. 5 revisions, first one 1 month ago and last one 1 day ago) I prefer to display: Removed all 5 revisions of the page, which was created 1 month ago and last modified 1 day ago. earlier in the page (this just a detail, of course, and you may disagree ;-) )
• the message was lacking i18n support (well, that link doesn't show much info for now, that will be hopefully be expanded one day)
• the Python code is OK, but after duplicating the loop, you could have continued and simplified the code a bit

As an example of how could things be polished further, I add a few more patches on top of yours. I hope this will help you to get started!

• Regarding the line to long… its ok… didnt notice it.
• The wording was not ok, I know… my mother tong is not english and some times simple phrases like this gets too complicated…. sorry.
• Yes… I didn't know how to do that… again… sorry.
• About the Python code… it way better how you rewrite it… thanks for the feedback…

Replying to mrelbe:

Argh… I was also producing a patch today… I'm supplying it anyway, perhaps it can create some inspiration: t9220-mrelbe-r10140.diff​

Sorry, I was searching the tickets and saw this one and it seemed easy enough for me to give it a try… and as it was with no news for 3 months… I took a chance.

If you prefer to you your patch is ok for, no hard feelings.

[Mikael Relbe]

Replying to erne.castro@…:

Replying to mrelbe:

Argh… /…/

Sorry, /…/ no hard feelings.

Don't excuse yourself! We are all glad that new people join in to help here, your initiative is very appreciated!

I am the sloppy guy who have let this ticket been sleeping for too long. Christian sent me a polite notice the other day, which led me to finally sit down today and do some job on this. (Disclaimer: I have recently switched working career, which totally drains my energy right now, and calls for 100% attention.)

But please pick up on Christians comments, and if you have the energy take a look at my patch as well, if you like, to get inspiration for functionality. If you want, we can work together with this, however I will be somewhat busy this week (but there's no hurry with this ticket). Note that I am also a newbie and have not yet earned/deserved the rights to make commits to the SVN repo. ;-)

[Christian Boos]

Replying to mrelbe:

The template file in t9220-mrelbe-r10140.diff​ is intentionally not indented as you probably want, Christian.

Actually, the <element><i18n:msg ...> on the same line is not my preferred way either, I just did this for keeping the same style that was already in use elsewhere.

The current indentation makes it easy to see what's going on, and easy to "correct".

Agreed, we can keep that, and more generally, I think I prefer this alternative approach ;-) I'll test it.

[Christian Boos]

Replying to mrelbe:

Replying to erne.castro@…:

Replying to mrelbe:

Argh… /…/

Sorry, /…/ no hard feelings.

Don't excuse yourself! We are all glad that new people join in to help here, your initiative is very appreciated!

Seconded.

I am the sloppy guy who have let this ticket been sleeping for too long.

Yep, sometimes it just goes that way in open source, no activity on a topic in months, then a flurry of patches ;-) Of course, Trac helps by seeing what's going on, but can't prevent people to start on their own at the same time. Any kind of "locking" scheme (like we had this "accept ⇒ assigned" state for signaling work in progress) is going to fail on the side of "stale" locks and could lead to block any activity.

But please pick up on Christians comments, and if you have the energy take a look at my patch as well, if you like, to get inspiration for functionality. If you want, we can work together with this, however I will be somewhat busy this week (but there's no hurry with this ticket).

And in the end, the whole point of the open source exercise is that often the collaborative outcome makes for a better result than what we could build alone.

That being said, there are a number of bitesized issues which are perfect candidates for your next patches ;-)

Note that I am also a newbie and have not yet earned/deserved the rights to make commits to the SVN repo. ;-)

Making your way to it ;-)

[Mikael Relbe]

#9220 - Replaces former patch. Final candidate (from me).

[Mikael Relbe]

The patch t9220-mrelbe-r10140-2.diff​ replaces the former patch and is my final take on this. It presents informative information for all of the use cases (delete single/multiple version and the whole page).

This patch takes Christians remarks into account regarding language style (which I liked). Tag indentation in the template file is now consistent (i.e. the header part, lines 17-31 in the new version, are identical to the header part in the former version except for re-indentation.)

However, I kept the loop, disregarding the simplicity inferred by the no-loop.patch​ since that simplicity will not enable the detailed info that is presented for all other use cases (deleting multiple versions in particular). But I'm pretty sure that Christian will find a way to simplify this anyway ;-)

[Christian Boos]

Thanks, looks great! I'll test and apply this evening (and no, the only simplification I can find is to do new_date = old_date = None, which isn't worth editing the patch ;-) ).

Keep an eye on #9222, which will probably need to adapt this "confirm deletion" page further.

[Christian Boos]

There's a glitch when you try to remove a revision range. If you have a wiki history looking like:

	5	92 seconds	cboos	something else
  o	4	109 seconds	cboos	undo aa, bb
  	3	2 minutes	cboos	bb
  	2	2 minutes	cboos	aa
 o 	1	3 years	trac	

If you select the diff from v1 to v4, you see no diffs (correct according to the changes I did), and a Delete version 2 to version 4 button (correct). Following this, we come to the Confirm Delete page which shows:

Removing 4 versions of the page, which was first modified 14 seconds ago and last modified 67 seconds ago. 

Should be 3 versions and the dates are permuted.

Deleting itself works as expected (The versions 2 to 4 of the page TracInstall~ have been deleted. notice, the history showing versions 1 and 5 left).

[Mikael Relbe]

That was a weird use case, which I wasn't aware of. If I understand you, this case applies only when the actual content is unchanged? How do you go about to store several versions of a wiki page without content changes?

Anyway, I confirm the finding (but I had to use an SQL tool to inject duplicates of the same wiki page).

BTW: Why don't we allow deleting any range of wiki pages, no matter content changes or not? (I've seen lots of people storing intermediate changes of wiki pages during an editing session, typically meeting minutes. To be able to "purge" intermediate versions would be a nice-to-have feature.)

[Christian Boos]

Replying to mrelbe:

That was a weird use case, which I wasn't aware of. If I understand you, this case applies only when the actual content is unchanged? How do you go about to store several versions of a wiki page without content changes?

Well, the actual use case is spam deletion. E.g. the page gets spammed at version 12, 13, then someone comes and reverts spam in 14, further edit happen, and at some point an admin wants to "purge" the intermediate no-op range. No need for a SQL tool ;-)

BTW: Why don't we allow deleting any range of wiki pages, no matter content changes or not? (I've seen lots of people storing intermediate changes of wiki pages during an editing session, typically meeting minutes. To be able to "purge" intermediate versions would be a nice-to-have feature.)

That could be an easy and useful extension, but be sure to first review the arguments in favor of restricting the deletion to no net changes ranges (well, I don't remember the ticket, but source annotate of the corresponding lines of code should get you there).

[Mikael Relbe]

Fixup of prev. patch (apply the prev. patch on trunk r10140, then this one on trunk r10211)

[Mikael Relbe]

Two errors were corrected; the template mixed up new and old dates (simple error, but stupid, I'm really sorry for that), the other error were more serious since the code assumed that deleting multiple versions would always include deletion of the newest version.

Thank you "Hawkeye", for finding this!

[Mikael Relbe]

Replying to cboos:

Replying to mrelbe:

BTW: Why don't we allow deleting any range of wiki pages /…/

That could be an easy and useful extension, but be sure to first review the arguments /…/

Ok, thanks for the heads up on this. That is anyway out of the scope for this ticket, but I'll keep this in mind.

[osimons]

Replying to mrelbe:

Replying to cboos:

Replying to mrelbe:

BTW: Why don't we allow deleting any range of wiki pages /…/

That could be an easy and useful extension, but be sure to first review the arguments /…/

Ok, thanks for the heads up on this. That is anyway out of the scope for this ticket, but I'll keep this in mind.

BTW, you may want to review #4112 and #1114 for background on current restrictions.

[Christian Boos]

Thanks, I just re-read them (4 years already! he).

The idea that deleting intermediate spam versions is only possible if the change is reverted exactly later (i.e. with no net change) is not quite correct, as practice has shown in the meantime that it's quite common that new versions undoing spam also introduce some spurious changes. So I'd be OK if the WIKI_ADMIN had the last word on what he decides to delete.

[Mikael Relbe]

I've been using this for some weeks now, I think it's is ready to go into trunk.

[Mikael Relbe]

Raising prio to indicate that the patches are ready for trunk.

Apply t9220-mrelbe-r10140-2.diff​ on trunk at rev 10140, then apply t9220-mrelbe-r10140-2-fixup-r10211.diff​ on trunk at rev 10211

Please give me a note of you find any problems with the patches, or want me to produce a single patch-file based on latest rev of trunk.

[Christian Boos]

Thanks for the reminder Mikael!

Both patches applied in r10357, and just the second I pressed enter I noticed I goofed the reference to the ticket number… So if next time you want a perfect changeset, also contribute the commit message ;-)

Note that if you'd like to fine tune this further, I'd suggest adding proper plural support for:

Removing <a ...>$num_versions versions</a> of the page

(see ​i18:choose in Genshi doc)

[Mikael Relbe]

Replying to cboos:

Note that if you'd like to fine tune this further, I'd suggest adding proper plural support for /…/

There is no need for plural support since that text is only used when several versions are to be removed, see marked lines in trunk/trac/wiki/templates/wiki_delete.html@10629:37,43-44,47#L35​