Detect and block conflicting edits #212

Closed
wants to merge 5 commits into from

5 participants

@pantsmanuk

Fixes http://www.mantisbt.org/bugs/view.php?id=5466, whereby
concurrent edits to a single issue can overwrite field data.

These changes allow MantisBT to spot a conflicting edit, stopping
it from overwriting the first saved edit with the second saved edit.

It's very much a blunt tool (flat-out refusal to save), but it works.

@pantsmanuk pantsmanuk Detect and block conflicting edits
Fixes the (oh so old) issue on the MantisBT site 5466, whereby
concurrent edits to a single issue can overwrite field data.

These changes allow MantisBT to spot a conflicting edit, stopping
it from overwriting the first edit with the second. It's very much
a blunt tool (flat-out refusal to save), but it works.
2a1796f
@rombert rombert and 1 other commented on an outdated diff Jun 18, 2014
lang/strings_english.txt
@@ -1710,3 +1710,4 @@ $MANTIS_ERROR[ERROR_UPDATING_TIMEZONE] = 'Unable to update timezone.';
$MANTIS_ERROR[ERROR_DEPRECATED_SUPERSEDED] = 'Deprecated functionality: "%1$s", use "%2$s" instead.';
$MANTIS_ERROR[ERROR_DISPLAY_USER_ERROR_INLINE] = 'Warning: The system is configured to display MantisBT errors (E_USER_ERROR) inline. Program execution will continue; this may lead to system/data integrity issues.';
$MANTIS_ERROR[ERROR_TYPE_MISMATCH] = 'Data Type mismatch. Enable detailed error messages for further information.';
+$MANTIS_ERROR[ERROR_BUG_CONFLICTING_EDIT] = 'A conflicting edit has been detected.';
@rombert
Mantis Bug Tracker member
rombert added a note Jun 18, 2014

Do we want a more detailed message here? Like 'A conflicting edit has been detected, please reopen the issue and re-apply your changes' ?

The text of the error message is/was a concern, so any suggestions are welcomed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rombert rombert and 1 other commented on an outdated diff Jun 18, 2014
bug_update.php
@@ -121,6 +122,17 @@
$t_bug_note->view_state = gpc_get_bool( 'private', config_get( 'default_bugnote_view_status' ) == VS_PRIVATE ) ? VS_PRIVATE : VS_PUBLIC;
$t_bug_note->time_tracking = gpc_get_string( 'time_tracking', '0:00' );
+# Detect and prevent conflicting edits - this is a *very* blunt tool.
+/*echo "<pre>" . PHP_EOL;
@rombert
Mantis Bug Tracker member
rombert added a note Jun 18, 2014

The commented out debugging code must be removed.

Oops :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rombert
Mantis Bug Tracker member

This looks good to me after a cursory review ( not tested )

@pantsmanuk pantsmanuk Changes as advised by Rombert
Removed the debugging code and comment from bug_update.php
Modified the error message to be clearer/more detailed.
1d5a426
@vboctor vboctor commented on an outdated diff Jun 19, 2014
bug_change_status_page.php
@@ -150,6 +150,7 @@
<td class="form-title" colspan="2">
<input type="hidden" name="bug_id" value="<?php echo $f_bug_id ?>" />
<input type="hidden" name="status" value="<?php echo $f_new_status ?>" />
+ <input type="hidden" name="last_updated" value="<?php echo $t_bug->last_updated ?>" />
@vboctor
Mantis Bug Tracker member
vboctor added a note Jun 19, 2014

Indentation looks inconsistent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@vboctor
Mantis Bug Tracker member

+1

@dregad
Mantis Bug Tracker member

Thanks for your contribution. I tested this and it gets the job done.

The only thing I don't like is the wording of the error message. I would propose instead

This issue has been updated by another user. Please reapply your changes.
@pantsmanuk pantsmanuk Changed error message
As requested by dregad, I've updated the error message.
b2a4466
@grangeway

1) In bug_update.php, bug->last_updated should be an int I believe, so we should be using gpc_get_int not gpc_get_string

2) Ok, now that the error message has been through N revisions, lets go for N+1 ->

"This issue has been updated by another user".

This part makes more sense to an ordinary person, and looks like it would translate quite well.

"Please reapply your changes"

I'm not sure that 'reapply' is a good word for non-technical users:

First Part:
A "Please refresh the Page ."
B "Please go back" (bad?)

2nd part:
C ... and then continue with your changes."
D.... and try again."

A+C or A+D feels like a better string to translate then the current option to me.

@pantsmanuk

@grangeway

  1. Logically, I agree completely, but when I tried using gpc_get_int in our 1.2.17 install (where I initially developed this feature), it wouldn't "type and value equality" compare to the existing value (the database value coming back as a string). Update: in master BugData->last_updated is a protected string; I can cast it to an int, but it seems wasteful.

  2. If I get a vote, B+D reads best from an English viewpoint: you have to use the Back button to "recover" from the error. However, I suspect the exact wording needs further discussion (and will hold of any further wording changes until that happens).

@dregad
Mantis Bug Tracker member

@grangeway I intentionally did not add the "go back" bit in the message itself, because when we display an error message it is always followed by error_no_proceed, i.e. the full message actually reads:

APPLICATION ERROR #1105
This issue has been updated by another user. Please reapply your changes.
Please use the "Back" button in your web browser to return to the previous page. There you can correct whatever problems were identified in this error or select another action. You can also click an option from the menu bar to go directly to a new section.

That said, I agree that "reapply" may not be the best wording, "Please try again" actually sounds better. What about "Please submit your changes again." ?

"Refresh the page" does not work because as already pointed out by @pantsmanuk we're on an error page and refreshing will just resubmit the same data and get us back to the same point.

In summary, I'd go for

  1. This issue has been updated by another user, please try again. or
  2. This issue has been updated by another user, please submit your changes again. (my preference)
@pantsmanuk

Given the explanation by @dregad, I too prefer the second option presented by him, "This issue has been updated by another user, please submit your changes again."; it details the problem clearly, offers the solution, and does so in language that I would think will be easy to translate.
The only possible problem comes from the second stanza and the text that will immediately follow it: going back one page will not suffice, the user needs to return to the issue and "start again". If you want to be unambiguous, perhaps: This issue has been updated by another user, please return to the issue and submit your changes again.

@pantsmanuk pantsmanuk Error message change
Changed to my suggested value. Hold off pushing until there is
agreement on the exact wording.
f7483bf
@dregad dregad added a commit that referenced this pull request Jul 7, 2014
@pantsmanuk pantsmanuk Detect and block conflicting edits
Fixes the (oh so old) issue on the MantisBT site #5466, whereby
concurrent edits to a single issue can overwrite field data.

These changes allow MantisBT to spot a conflicting edit, stopping
it from overwriting the first edit with the second. It's very much
a blunt tool (flat-out refusal to save), but it works.

Signed-off-by: Damien Regad <dregad@mantisbt.org>

- Error message revised as discussed in the pull request
- Squashed commits

Fixes #5466, PR #212
4ef0e69
@dregad dregad closed this Jul 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment