Edgewall Software
Modify

Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#9220 closed enhancement (fixed)

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

Reported by: team@… Owned by: Mikael Relbe
Priority: highest Milestone: 1.0
Component: wiki system Version: 0.12dev
Severity: normal Keywords: delete
Cc: erne.castro@… Branch:
Release Notes:

Wiki page deletion confirmation page has more detailed information about what's going to be deleted.

API Changes:
Internal Changes:

Description

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).

Attachments (7)

t9220-page-delete-more-info-0.12-stable-r10138.patch (1.8 KB ) - added by erne.castro@… 14 years ago.
Patch for current 0.12-stable to add more info on the page delete confirmation.
wiki_delete_improvements.patch (4.4 KB ) - added by Christian Boos 14 years ago.
some improvements to t9220-page-delete-more-info-0.12-stable-r10138.patch (1/3)
no-loop.patch (1.4 KB ) - added by Christian Boos 14 years ago.
applies on top of wiki_delete_improvements.patch (2/3)
dates-in-template.patch (2.8 KB ) - added by Christian Boos 14 years ago.
applies on top of no-loop.patch (3/3)
t9220-mrelbe-r10140.diff (5.7 KB ) - added by Mikael Relbe 14 years ago.
#9220 - Another approach, fully working
t9220-mrelbe-r10140-2.diff (8.3 KB ) - added by Mikael Relbe 14 years ago.
#9220 - Replaces former patch. Final candidate (from me).
t9220-mrelbe-r10140-2-fixup-r10211.diff (1.4 KB ) - added by Mikael Relbe 13 years ago.
Fixup of prev. patch (apply the prev. patch on trunk r10140, then this one on trunk r10211)

Download all attachments as: .zip

Change History (32)

comment:1 by Christian Boos, 14 years ago

Component: generalwiki system
Milestone: next-minor-0.12.x

… 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.

comment:2 by team@…, 14 years ago

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

comment:3 by Christian Boos, 14 years ago

Keywords: delete added
Milestone: next-minor-0.12.xnext-major-0.1X
Owner: set to Christian Boos
Priority: normallow

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.

comment:4 by Mikael Relbe, 14 years ago

Owner: changed from Christian Boos to Mikael Relbe
Status: newassigned

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

Last edited 14 years ago by Mikael Relbe (previous) (diff)

by erne.castro@…, 14 years ago

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

comment:5 by erne.castro@…, 14 years ago

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! :)

comment:6 by Christian Boos, 14 years ago

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!

by Christian Boos, 14 years ago

by Christian Boos, 14 years ago

Attachment: no-loop.patch added

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

by Christian Boos, 14 years ago

Attachment: dates-in-template.patch added

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

comment:7 by Mikael Relbe, 14 years ago

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

Last edited 14 years ago by Mikael Relbe (previous) (diff)

by Mikael Relbe, 14 years ago

Attachment: t9220-mrelbe-r10140.diff added

#9220 - Another approach, fully working

comment:8 by Mikael Relbe, 14 years ago

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".

in reply to:  7 ; comment:9 by erne.castro@…, 14 years ago

Cc: erne.castro@… added

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.

in reply to:  9 ; comment:10 by Mikael Relbe, 14 years ago

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. ;-)

in reply to:  8 comment:11 by Christian Boos, 14 years ago

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.

in reply to:  10 comment:12 by Christian Boos, 14 years ago

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 ;-)

by Mikael Relbe, 14 years ago

Attachment: t9220-mrelbe-r10140-2.diff added

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

comment:13 by Mikael Relbe, 14 years ago

Milestone: next-major-0.1X0.13
Summary: "are you sure you want to delete this page?" question needs more details[PATCH] "are you sure you want to delete this page?" question needs more details

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 ;-)

Last edited 14 years ago by Mikael Relbe (previous) (diff)

comment:14 by Christian Boos, 14 years ago

Release Notes: modified (diff)
Version: 0.12dev

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.

comment:15 by Christian Boos, 13 years ago

Keywords: needfixup added

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

	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.

Version 0, edited 13 years ago by Christian Boos (next)

comment:16 by Mikael Relbe, 13 years ago

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.)

in reply to:  16 ; comment:17 by Christian Boos, 13 years ago

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).

by Mikael Relbe, 13 years ago

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

comment:18 by Mikael Relbe, 13 years ago

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!

Last edited 13 years ago by Mikael Relbe (previous) (diff)

in reply to:  17 ; comment:19 by Mikael Relbe, 13 years ago

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.

in reply to:  19 comment:20 by osimons, 13 years ago

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.

comment:21 by Christian Boos, 13 years ago

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.

comment:22 by Mikael Relbe, 13 years ago

Keywords: needfixup removed

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

comment:23 by Mikael Relbe, 13 years ago

Priority: lowhighest

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.

comment:24 by Christian Boos, 13 years ago

Resolution: fixed
Status: assignedclosed

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)

in reply to:  24 comment:25 by Mikael Relbe, 13 years ago

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Mikael Relbe.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Mikael Relbe to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.