Identified at UB Usability Testing: http://www.drupalusability.org/node/154

Create a node and save
Edit the node, save
Hit back to return to the edit form, alter the body and save
Given "This content has been modified by another user, changes cannot be saved." error message

While an error is correct, the text is confusing to users, as they are in fact the actual user, not "another user".

Related issues about node diff/merge
http://drupal.org/node/106953

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chrisshattuck’s picture

Status: Active » Needs review
FileSize
717 bytes

The diff/merge idea would be great, is there any way that's getting into core?

Here is a patch which just removes the 'by another user'. It seems like the text could still be improved, but this is at least succinct.

Status: Needs review » Needs work

The last submitted patch failed testing.

chrisshattuck’s picture

Status: Needs work » Needs review
FileSize
781 bytes

Failed because I didn't roll the patch from root (thanks, webchick). Re-rolled, uploading again.

ultimateboy’s picture

Status: Needs review » Needs work

There are two times when this message appears (on a usual basis).

  • A user edits a node, saves the changes, uses the back button to navigate back, makes additional changes, and saves.
  • Two users attempt to edit the same node at the same time. The second user to click save will receive this error.

The problem with the current error message is that it does not work for the first case. The problem with the proposed change is that it does not work for the second.

Let's keep working until both meanings of this message are relayed.

chrisshattuck’s picture

Thanks for the feedback, ultimateboy,

Could you clarify on how the proposed text doesn't address the second issue? "This content has been modified, changes cannot be saved" doesn't state which user did the modifications, which (while it can no doubt be improved) seems like it should work for both cases.

Understood if this still seems like an incomplete solution. I wonder if it would be worth looking at splitting the messaging based on the scenario. I played with the text for a while, but it just kept getting unwieldy having to address both issues in the same message. i.e "The content on this page has either been modified by another user, or you have already submitted modifications using this form. As a result, your changes cannot be saved.".

Also, it seems like there is consistency in Drupal error reporting in that errors will state the problem, but not a solution. I can see good reasons for this, such as if there are multiple solutions. Do you know of any places where this convention is bypassed an a solution is offered? For example:

"The content on this page has either been modified by another user, or you have already submitted modifications using this form. As a result, your changes cannot be saved. To modify, [start edit link] refresh the node edit page [end link]."

ultimateboy’s picture

The proposed change in #3 could be very confused for the second scenario outlined in #4. Seeing this message, users could possibly think "well yes, I understand that I modified the page, why can't it be saved?" I do not think it is clear that another user has probably altered the node at the same time.

Given that, I do think that the split solution outlined in #5 is the direction to go. I think it makes it extremely clear what the two possibilities are, and gives a solution.. this is very nice.

I guess the other possibility is to figure out which scenario is true by looking at who last modified the content and if it is the same uid, then report accordingly. But I think the former solution is the better way to go.

chrisshattuck’s picture

FileSize
870 bytes

I like the idea of checking the for the last user to modify the node, but I feel like there might be some edge cases where the text might not be accurate.

Attached is a patch with the text proposed in #5: "The content on this page has either been modified by another user, or you have already submitted modifications using this form. As a result, your changes cannot be saved."

Thanks again, ultimateboy.

chrisshattuck’s picture

Status: Needs work » Needs review
Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

seanburlington’s picture

Status: Closed (fixed) » Needs work

While I think the above changes helps a bit - I think it is the behaviour that is confusing.

Users expect to be able to use the back button (without thinking too much about the semantics of it)

For my current project I've cooked up this quick bit of javascript which is working OK for me in test at the moment.

This is for Drupal 6 and is only loaded on a specific type of node edit page.

$(document).ready(function(){

	var location = "" + top.location;
	if (location.search(/#back/) != -1){
		top.location.replace('#');
		top.location.reload();
	}
	
	$('input#edit-submit').click(function(event){
		top.location.replace('#back');
	
	});

}

What it does is adds a fragment identifier to mark the history as dirty - and checks for this on page load - clearing the marker and reloading the page if it is found.

Needs work (and cross browser testing) but what do people think in principle?

Vako’s picture

I like your logic, I hope it can be implemented even on version 6.

But in general, we should have a better solution like this:
User1 is Editing a node.
User2 clicks on the Edit button for the same node. Immediately User2 should get a message indicating that the node is being used and if (s)he need to be notified when it is available. i.e.: some kind of a locking mechanism.
Microsoft Word has this feature already, so as many other applications.

YK85’s picture

@seanburlington - I was wondering if your code from #11 has been further developed?

I am having this issue where user edits a node, saves the changes, uses the back button to navigate back, makes additional changes, and saves => results in the message.

Regards

seanburlington’s picture

@yaz085 - no - I'm afraid not

the code worked well enough for my client at the time and I haven't had cause to look at it again.

@vako - a locking system would be much more complex - it can be done but is complicated by the stateless nature of HTTP
You'd have to have a bit of ajax going on behind the scenes to let the server know that user 1 is till editing the node and hasn't closed the browser or navigated away.
But even then you'd still need something separate to catch use of the back button.

AaronELBorg’s picture

subscribing

jumpfightgo’s picture

#11 would be very, very helpful.

j0e’s picture

tried #11, but it didn't work for me, any tips on getting a solution like this one to work...

amc’s picture

#655388: Many ways to lose data on form input in the overlay is a different issue (closing the overlay before saving), but may be related and anything here needs to play nice with whatever happens there.

Vako’s picture

Sorry for the late response.

@seanburlington for post#15: I think it's not that complex, what we can do is put a flag in the database and when the second user trying to edit; if the flag is true, it means the file is being edited, otherwise it's ok.

what do you think?

seanburlington’s picture

My Javascript snippet at #11 just addresses the issue where a user gets a "content has been modified" message after clicking the back button

in this case the content was modified since the form was loaded - but by the current user.

Database flags could be used to help the issue where two users attempt to modify the same node at the same time.

tmsimont’s picture

Status: Needs review » Needs work

#11 javascript does not work in Chrome, also it is missing a closing ")"

);

It doesn't work in webkit because $(document).ready() doesn't fire when you use the back button. Also, top.location.replace() doesn't seem to affect the history in Chrome, so when you use the back button, you just go right to the un-hashed URL...

tmsimont’s picture

Status: Needs work » Needs review

After some digging around to the root of this issue, I came up with a cross-browser solution that I don't think will have any negative side effects...

The downside is that it's javascript-dependent.

For any page with a form, include this:

$(window).load(function(){	
	var ts = Math.round(new Date().getTime() / 1000);
	$('#edit-changed').val(ts);	
});

$(window).load() instead of $(document).ready() is important because webkit doesn't call $(document).ready() on back-button reload.

What does this do?

According to http://drupal.org/node/407418#comment-3298640 the problem comes from this code:

<?php
if (isset($node->nid) && (node_last_changed($node->nid) > $node->changed)) {
    form_set_error('changed', t('This content has been modified by another user, changes cannot be saved.'));
}
?>

So all I'm doing with that javascript is updating $node->changed on the form with the current time-stamp, instead of letting the user use the timestamp they used the last time that form loaded.

Does anyone see a problem with doing this?

Mac Clemmens’s picture

Status: Needs work » Needs review
FileSize
3.2 KB

Regarding #23, are there any implications if there is a time mismatch between the server and the user agent?

I recommend storing the form_build_id of the last submitted form in a session variable, e.g. "drupal_last_form_build_id_submitted". Then I recommend using javascript to check an ajax callback to see if the current form's build ID matches the one stored in the session. If the callback returns "true" (meaning the current form_build_id matched the last one stored in the user's session variable) then we would know that the user is editing an expired form, because that form has already been submitted. (The form_build_id gets reset each time a form is submitted).

As a proof of concept, I have attached a module which does this for Drupal 6. If the user clicks the back button, it simply reloads the form for them. Note this module only works on node forms, by design.

I hope this helps stimulate discussion around a more robust solution, especially on the node form where an expired form can really confuse and frustrate users. I would be happy submit a patch if the community wants to take this approach.

Here's how I did it in D6:

function dd_back_button_protect_init() {
  if (arg(0) == 'node' && arg(2) == 'edit') {
    //Load the javascript if editing a node
    drupal_add_js(drupal_get_path('module', 'dd_back_button_protect') . '/dd_back_button_protect.js', 'module', 'header', FALSE, TRUE, FALSE);
  }
}

function dd_back_button_protect_form_alter(&$form, &$form_state, $form_id) {
  //capture the form_build_id. this should probably be limited to only node forms
  $form['#submit'][] = 'dd_back_button_protect_submit';
}

function dd_back_button_protect_submit($form, &$form_state) {
  //capture the form_build_id to a session variable
  if ($form['form_build_id']["#id"]) {
    $_SESSION['dd_back_button_protect_last_form_build_id'] = $form['form_build_id']["#id"];
  }
}

function dd_back_button_protect_menu() {
  //define a simple ajax callback which checks the current form's build_id against the session 
  $items['dd_back_button_protect/%'] = array(
    'title' => 'Form check', 
    'page callback' => 'dd_back_button_protect_check', 
    'access arguments' => array('access content'), 
    'page arguments' => array(1),
    'type' => MENU_CALLBACK,
  );
  
  return $items;
}

function dd_back_button_protect_check($form_build_id) {
  // return true if the current form_build_id is the same as the last one submitted
  // important not to give away any other information for security reasons
  drupal_json(array('staleForm' => $form_build_id == $_SESSION['dd_back_button_protect_last_form_build_id']));
}

And the following javascript, only loaded on the node/*/edit pages:

$(window).load(function(){	
  //Necessary because webkit browsers do not load document.ready when the back button is clicked.
  
	jQuery.getJSON('/dd_back_button_protect/' + $("#node-form").find('input[name="form_build_id"]').val(), function(json, textStatus) {
    if (json.staleForm) {
      // Hide the form so the user doesn't try to work on it while it reloads
      $("#node-form").hide();
      // @TODO: Need to add this message properly per Drupal coding standards
      $("#content-header").append('<div class="messages warning">This post has been updated. Loading latest version...</div>');
      //Unset any pop-up dialogs by other modules
      window.onbeforeunload = function() {};
      //Reload the form
      window.location.reload(true);
    }
  });
	
});
southweb’s picture

This can be done on the server side.

The following hack does not protect against back button risk though.

First in hook_form_alter

function mymodule_form_alter(&$form, &$form_state, $form_id) {
  if (substr($form_id, -10) == '_node_form') { 
      // Ensure our own validation is called first to overcome the node changed error node.module: 971
       array_unshift($form['#validate'], "mymodule_node_edit_validate");
   }
 }

Then we need:

function  mymodule_node_edit_validate($form,&$form_state){
   // Update changed value to avoid node changed error node.module: 971
    $form_state['values']['changed'] =  $form_state['node']->changed;
}
SergO’s picture

bluffit, thank you, it's a fast solution and it works for me.

southweb’s picture

You're welcome.

Refineo’s picture

Is this addressed in core 8.x already ?

joshuautley’s picture

@25

First, will this same solution work for D6?

Then, am I correct in thinking I need to create 2 custom modules?

9tin’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Usability, -Needs text review, -UBUserTesting2009

The last submitted patch, node.module.patch, failed testing.

Status: Needs review » Needs work

deanflory’s picture

Component: node.module » ajax system
Issue summary: View changes

Just keeping this issue fresh since I ran into it the other day.

deanflory’s picture

Component: ajax system » node system

Um, how did the Component get changed when I made a comment? I didn't do that, and there is no "node.module" option to select anymore. What's going on?

Changing it to "node system" since that appears to be the best fit.

Someone please correct me if that's wrong.

adriancid’s picture

#25 works for me, thanks @southweb

  • Dries committed c85fa2b on 8.3.x
    - Patch #394694 by stompeers: improve node edit conflict message. Result...

  • Dries committed c85fa2b on 8.3.x
    - Patch #394694 by stompeers: improve node edit conflict message. Result...

  • Dries committed c85fa2b on 8.4.x
    - Patch #394694 by stompeers: improve node edit conflict message. Result...

  • Dries committed c85fa2b on 8.4.x
    - Patch #394694 by stompeers: improve node edit conflict message. Result...

  • Dries committed c85fa2b on 9.1.x
    - Patch #394694 by stompeers: improve node edit conflict message. Result...