Edgewall Software
Modify

Opened 14 years ago

Closed 13 years ago

Last modified 10 years ago

#2618 closed defect (duplicate)

ticket mods are lost on submit if ticket modified by someone else

Reported by: bboisvert@… Owned by: Christian Boos
Priority: normal Milestone:
Component: ticket system Version: 0.9
Severity: major Keywords: conflict
Cc: bboisvert@…, mankoff@… Branch:
Release Notes:
API Changes:

Description

When you submit a modification to a ticket, Trac correctly disallows it if an update has happened since the time you opened the ticket page (because you aren't aware of the most current information). However, instead of throwing an uncaught error and abandoning the modification, it should treat the submission as a 'preview' request, rather than a real submission, rerender the page (with the now-current mods) and a message as to what it did, and allow the user to edit their message if needed, before resubmitting.

Attachments (7)

source_patch.txt (2.2 KB ) - added by bboisvert@… 13 years ago.
patch for installed 0.9.5 source to add submitted fields to the HDF on a "mid air collision" error
templates_patch.txt (851 bytes ) - added by bboisvert@… 13 years ago.
patch for installed 0.9.5 templates to display the submitted fields on a "mid air collision" error
templates_patch.2.txt (983 bytes ) - added by bboisvert@… 13 years ago.
patch for installed 0.9.5 templates to display the submitted fields on a "mid air collision" error - formatted as an HTML table, rather than an HDF dump
0.10dev-error.cs-patch.2.txt (1.1 KB ) - added by bboisvert@… 13 years ago.
Patch for 0.10dev to display page parameters on all error messages
mid_air_collision_retry-r3341.patch (4.2 KB ) - added by Christian Boos 13 years ago.
Implements a Retry button on the "Mid Air Collision" error page.
mid_air_collision_retry-bboisvert.patch (6.3 KB ) - added by bboisvert@… 13 years ago.
Implements a Retry button with a manual merge form on Mid-Air collision errros. Is an EXTENSION of attachment:mid_air_collision_retry-r3341.patch
mid_air_collision_retry-bboisvert2.patch (3.1 KB ) - added by bboisvert@… 13 years ago.
futher tweaks to attachment:mid_air_collision_retry-bboisvert.patch to deal with some weird timestamp resolution conflicts and improve formatting

Download all attachments as: .zip

Change History (27)

comment:1 by Emmanuel Blot, 14 years ago

I'm not sure to understand how this would actually work:

Imagine that the user who receives the middle-air collision warning has changed the CC field while another user has updated the same ticket with a different CC field content: what would be the content of the "preview" page with such an event ?

  • it cannot be the other user's CC field, as it would overwrite the current user's CC field without warning to the current user
  • it cannot be the current user's CC field, as it would overwrite the other user's CC field changes
  • it cannot be a merge of both field as the merge may fail in some cases

The trouble is that the ticket changes are not only about the "description" field which is added to the current ticket contents, but other fields that are overwritten.

comment:2 by anonymous, 14 years ago

At the very least, how about displaying back to the user a summary of the requested (but rejected) changes, with instructions about what to do.

I wouldn't care except that around here when we encounter this scenario, you hit the back button, and the form has reverted back to the original state - you've lost all your work.

comment:3 by bboisvert@…, 14 years ago

Cc: bboisvert@… added

I'd say that it should use the committed data to populate the fields. It's no big deal to reenter you CC or keywords, but it's a big deal to have to reenter a longish comment on a ticket. Perhaps have a "your value" displayed above the mutable fields, if different, so you can easily get back what you entered.

More concisely: do the preview using the data from the DB where there's a conflict, and optionally [hopefully] display the just-submitted data above/below the field where it's changed.

comment:4 by mankoff@…, 14 years ago

Cc: mankoff@… added
Severity: normalmajor

This is a severe bug for Safari users, where the back button doesn't work for some reason, so all edits are 100% lost. :(

comment:5 by bboisvert@…, 13 years ago

Any word on this? It's becoming more and more of a problem for us, as we expand.

by bboisvert@…, 13 years ago

Attachment: source_patch.txt added

patch for installed 0.9.5 source to add submitted fields to the HDF on a "mid air collision" error

by bboisvert@…, 13 years ago

Attachment: templates_patch.txt added

patch for installed 0.9.5 templates to display the submitted fields on a "mid air collision" error

comment:6 by bboisvert@…, 13 years ago

I've implemented a very hackish solution that will simply rerender the submitted fields as a raw HDF dump on the "mid air collision" error page. There are two patches, one for the installed source (/usr/lib/python2.x/site-packages/trac), and one for the installed templates (/usr/share/trac). Note that if you're running the compiled bytecode (rather than from source), you'll need to recompile after applying the patch.

This patch is far from a good solution, as it is ugly, unusable, and leverages the exception handling subsystem in a rather poor way. But it prevents the loss of data.

by bboisvert@…, 13 years ago

Attachment: templates_patch.2.txt added

patch for installed 0.9.5 templates to display the submitted fields on a "mid air collision" error - formatted as an HTML table, rather than an HDF dump

comment:7 by bboisvert@…, 13 years ago

Unfortunately, after upgrading to the trunk (i.e 0.10dev) these patches don't seem to work anymore, as the request dispatching stuff is quite a bit changed.

by bboisvert@…, 13 years ago

Patch for 0.10dev to display page parameters on all error messages

comment:8 by bboisvert@…, 13 years ago

The patch for 0.10dev is of rather a different style. Rather than the kludgy hackery that was in the previous patches, this time the patch is only for templates/error.cs, and causes the page parameters to be displayed on all error pages. Far simpler, no dependance on internal APIs and error handling mechanisms, and it's not constrained to just the mid air collision error. In particular, forgetting a summary on a new ticket.

comment:9 by Christian Boos, 13 years ago

Milestone: 0.10
Owner: changed from Jonas Borgström to Christian Boos

I was thinking about doing something like this myself. What I had in mind was to add at the end of the error message a Retry button that would simply resubmit all the changed fields in order to get a new preview: in that preview, you could see what changed and decide to keep or adapt your own changes (i.e. a kind of manual merge).

by Christian Boos, 13 years ago

Implements a Retry button on the "Mid Air Collision" error page.

comment:10 by Christian Boos, 13 years ago

Status: newassigned

See proof-of-concept implementation in attachment:mid_air_collision_retry-r3341.patch

comment:11 by bboisvert@…, 13 years ago

Shouldn't the ClearSilver template be ticket_error_midair.cs, not ticket_error.cs? There are other ticket errors (such as forgetting the summary on a new ticket) that aren't handled by it, though it's probably desirable if they all are. You don't need the merge stuff for new tickets, but not having to manually copy and paste your ticket data back in would be nice.

One problem with it, however, is that the preview gives you a preview only of YOUR data, so if the collision was for anything except the addition of a comment/attachement, there's no way to view what the updated value is. E.g. if someone updates 'keywords', then you submit and get a mid-air, when you hit 'retry', it's going to show your keywords, with no indication of what the current commited keyword's value is (which you've never seen).

What I'm thinking is maybe on the error page, it can display the committed values and the submitted values in two columns, and you can run down the list and pick which to keep. Then when you hit 'retry' it submits the collection of values you picked. You get your ability to manually merge, and without much more fuss. Just need to load the committed data into the HDF for the template to use, and throw in a little JavaScript magic for dynamically valued form fields based on which side of the merge is requested for each field.

by bboisvert@…, 13 years ago

Implements a Retry button with a manual merge form on Mid-Air collision errros. Is an EXTENSION of attachment:mid_air_collision_retry-r3341.patch

comment:12 by bboisvert@…, 13 years ago

cboos, I extended your patch to display a manual merge form above the retry button, if there were any fields with discrepancies in the ticket data (i.e. not the comment). It's not particularly attractive, but it's functional. It's a patch for a slightly modified source:/trunk#3335, though I don't think any of my custom mods interfere at all. They're not included in the patch in any case.

Note also that I named my template ticket_error_midair.cs.

by bboisvert@…, 13 years ago

futher tweaks to attachment:mid_air_collision_retry-bboisvert.patch to deal with some weird timestamp resolution conflicts and improve formatting

comment:13 by anonymous, 13 years ago

This second patch is an immediate successor to my first page that deals with a weird timestamp issue that causes mid-air collisions repeatedly. There's probably a better way to handle it, but I don't know enough about Trac's guts to know what it is.

I also cleaned up the merge form to render nothing if there aren't any conflicts, rather than rendering the headers with no fields.

comment:14 by bboisvert@…, 13 years ago

That last comment was by me, of course. Forgot to enter my email address before I submitted.

comment:15 by Christian Boos, 13 years ago

Thanks for your patches, I'll have a look at them. But before that, I wanted to mention that in my patch, the preview did show the other changes: it's simply the last comment added, which as usual contains a summary of the field changes.

comment:16 by bboisvert@…, 13 years ago

You are correct, of course, excepting the description. Description changes are not recorded as comments (nor versioned anywhere else, except for notification emails), so dealing with collisions on the description requires some "extra" mechanism somewhere.

comment:17 by anonymous, 13 years ago

This issue just blew 1.5 hours of my life away ≥(

With or without a merge function, it would help to display the item's preview so that someone could collect their text, update if necessary, and re-submit.

comment:18 by Christian Boos, 13 years ago

I'm sorry, too many things changed lately in the ticket web_ui.py, so the patch(es) don't apply cleanly anymore. Could you help out and make an updated patch on top of a recent trunk (e.g. r3477)?

comment:19 by Christian Boos, 13 years ago

Keywords: conflict added
Milestone: 0.100.11

Post-poning to 0.11 (and probably after the WorkFlow merge)

comment:20 by Christian Boos, 13 years ago

Milestone: 0.11
Resolution: duplicate
Status: assignedclosed

I think the approach suggested in #4100 should be preferred, so I'd like to move the discussion for this topic there.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Christian Boos 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.