Several changes to handle the case where a note is changed out from under you, due to being saved

from a different window:
 * Made controller.notebooks responsible for preventing unmodified notes from being saved, instead
   of model.Notebook handling this task.
 * Created a revision validator for passing revisions as arguments to exposed methods.
 * controller.Notebooks.save_note() now requires a previous_revision parameter, used to determine
   whether the note has been modified in the particular window it's being saved from.
 * save_note() returns a new previous_revision value, so the client can determine whether a save
   has occurred from another window.
 * controller.Notebooks.undelete_note() fixed to quietly bail if the note to undelete isn't
   actually deleted, which can happen if it was undeleted in another window.
 * Editor() now responsible for making revisions list if it doesn't exist
 * No longer giving an "undo" message when the user deletes an empty note.
 * On the client side, detecting whether the previous_revision as reported by save_note() looks
   correct, and if not, alerting the user about the conflict. Also displaying a "compare versions"
   button that opens both the current version and the previous version.

controller/Database.py

@@ -259,7 +259,23 @@ class Valid_id( object ):
     self.__none_okay = none_okay
 
   def __call__( self, value ):
-    if self.__none_okay and value in ( None, "" ): return None
+    if self.__none_okay and value in ( None, "None", "" ): return None
     if self.ID_PATTERN.search( value ): return str( value )
 
     raise ValueError()
+
+
+class Valid_revision( object ):
+  """
+  Validator for an object id.
+  """
+  REVISION_PATTERN = re.compile( "^\d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d\.\d+$" )
+
+  def __init__( self, none_okay = False ):
+    self.__none_okay = none_okay
+
+  def __call__( self, value ):
+    if self.__none_okay and value in ( None, "None", "" ): return None
+    if self.REVISION_PATTERN.search( value ): return str( value )
+
+    raise ValueError()

controller/Notebooks.py

@@ -2,7 +2,7 @@ import cherrypy
 from Scheduler import Scheduler
 from Expose import expose
 from Validate import validate, Valid_string, Validation_error, Valid_bool
-from Database import Valid_id
+from Database import Valid_id, Valid_revision
 from Users import grab_user_id
 from Updater import wait_for_update, update_client
 from Expire import strongly_expire
@@ -52,7 +52,7 @@ class Notebooks( object ):
   @validate(
     notebook_id = Valid_id(),
     note_id = Valid_id(),
-    revision = Valid_string( min = 19, max = 30 ),
+    revision = Valid_revision(),
   )
   def default( self, notebook_id, note_id = None, revision = None ):
     """
@@ -135,7 +135,7 @@ class Notebooks( object ):
   @validate(
     notebook_id = Valid_id(),
     note_id = Valid_id(),
-    revision = Valid_string( min = 19, max = 30 ),
+    revision = Valid_revision(),
     user_id = Valid_id( none_okay = True ),
   )
   def load_note( self, notebook_id, note_id, revision = None, user_id = None ):
@@ -269,14 +269,15 @@ class Notebooks( object ):
     note_id = Valid_id(),
     contents = Valid_string( min = 1, max = 25000, escape_html = False ),
     startup = Valid_bool(),
+    previous_revision = Valid_revision( none_okay = True ),
     user_id = Valid_id( none_okay = True ),
   )
-  def save_note( self, notebook_id, note_id, contents, startup, user_id ):
+  def save_note( self, notebook_id, note_id, contents, startup, previous_revision, user_id ):
     """
     Save a new revision of the given note. This function will work both for creating a new note and
     for updating an existing note. If the note exists and the given contents are identical to the
-    existing contents, then no saving takes place and a new_revision of None is returned. Otherwise
+    existing contents for the given previous_revision, then no saving takes place and a new_revision
-    this method returns the timestamp of the new revision.
+    of None is returned. Otherwise this method returns the timestamp of the new revision.
 
     @type notebook_id: unicode
     @param notebook_id: id of notebook the note is in
@@ -286,10 +287,16 @@ class Notebooks( object ):
     @param contents: new textual contents of the note, including its title
     @type startup: bool
     @param startup: whether the note should be displayed on startup
+    @type previous_revision: unicode or NoneType
+    @param previous_revision: previous known revision timestamp of the provided note, or None if
+      the note is new
     @type user_id: unicode or NoneType
     @param user_id: id of current logged-in user (if any), determined by @grab_user_id
     @rtype: json dict
-    @return: { 'new_revision': new revision of saved note, or None }
+    @return: {
+      'new_revision': new revision of saved note, or None if nothing was saved,
+      'previous_revision': revision immediately before new_revision, or None if the note is new
+    }
     @raise Access_error: the current user doesn't have access to the given notebook
     @raise Validation_error: one of the arguments is invalid
     """
@@ -306,26 +313,40 @@ class Notebooks( object ):
     self.__database.load( note_id, self.__scheduler.thread )
     note = ( yield Scheduler.SLEEP )
 
-    # if the note is already in the database, load it and update it. otherwise, create it
+    # if the note is already in the database, load it and update it
     if note and note in notebook.notes:
-      orig_revision = note.revision
+      # check whether the provided note contents have been changed since the previous revision
-      notebook.update_note( note, contents )
+      self.__database.load( note_id, self.__scheduler.thread, previous_revision )
+      old_note = ( yield Scheduler.SLEEP )
+
+      # the note hasn't been changed, so bail without updating it
+      if contents == old_note.contents:
+        previous_revision = note.revision
+        new_revision = None
+      # the note has changed, so update it
+      else:
+        previous_revision = note.revision
+        notebook.update_note( note, contents )
+        new_revision = note.revision
+    # the note is not already in the database, so create it
     else:
-      orig_revision = None
+      previous_revision = None
       note = Note( note_id, contents )
       notebook.add_note( note )
+      new_revision = note.revision
 
     if startup:
-      notebook.add_startup_note( note )
+      startup_changed = notebook.add_startup_note( note )
     else:
-      notebook.remove_startup_note( note )
+      startup_changed = notebook.remove_startup_note( note )
 
-    self.__database.save( notebook )
+    if new_revision or startup_changed:
+      self.__database.save( notebook )
 
-    if note.revision == orig_revision:
+    yield dict(
-      yield dict( new_revision = None )
+      new_revision = new_revision,
-    else:
+      previous_revision = previous_revision,
-      yield dict( new_revision = note.revision )
+    )
 
   @expose( view = Json )
   @wait_for_update
@@ -508,6 +529,12 @@ class Notebooks( object ):
     note = ( yield Scheduler.SLEEP )
 
     if note and notebook.trash:
+      # if the note isn't deleted, and it's already in this notebook, just return
+      if note.deleted_from is None and notebook.lookup_note( note.object_id ):
+        yield dict()
+        return
+
+      # if the note was deleted from a different notebook than the notebook given, raise
       if note.deleted_from != notebook_id:
         raise Access_error()
 

controller/test/Test_notebooks.py

@@ -310,9 +310,11 @@ class Test_notebooks( Test_controller ):
       note_id = self.note.object_id,
       contents = new_note_contents,
       startup = startup,
+      previous_revision = previous_revision,
     ), session_id = self.session_id )
 
     assert result[ "new_revision" ] and result[ "new_revision" ] != previous_revision
+    assert result[ "previous_revision" ] == previous_revision
 
     # make sure the old title can no longer be loaded
     result = self.http_post( "/notebooks/load_note_by_title/", dict(
@@ -359,6 +361,154 @@ class Test_notebooks( Test_controller ):
   def test_save_startup_note_without_login( self ):
     self.test_save_note_without_login( startup = True )
 
+  def test_save_unchanged_note( self, startup = False ):
+    self.login()
+
+    # save over an existing note supplying new contents and a new title
+    previous_revision = self.note.revision
+    new_note_contents = u"<h3>new title</h3>new blah"
+    self.http_post( "/notebooks/save_note/", dict(
+      notebook_id = self.notebook.object_id,
+      note_id = self.note.object_id,
+      contents = new_note_contents,
+      startup = startup,
+      previous_revision = previous_revision,
+    ), session_id = self.session_id )
+
+    # now attempt to save over that note again without changing the contents
+    previous_revision = self.note.revision
+    result = self.http_post( "/notebooks/save_note/", dict(
+      notebook_id = self.notebook.object_id,
+      note_id = self.note.object_id,
+      contents = new_note_contents,
+      startup = startup,
+      previous_revision = previous_revision,
+    ), session_id = self.session_id )
+
+    # assert that the note wasn't actually updated the second time
+    assert result[ "new_revision" ] == None
+    assert result[ "previous_revision" ] == previous_revision
+
+    result = self.http_post( "/notebooks/load_note_by_title/", dict(
+      notebook_id = self.notebook.object_id,
+      note_title = "new title",
+    ), session_id = self.session_id )
+
+    note = result[ "note" ]
+
+    assert note.object_id == self.note.object_id
+    assert note.title == self.note.title
+    assert note.contents == self.note.contents
+    assert note.revision == previous_revision
+
+  def test_save_unchanged_note_with_startup_change( self, startup = False ):
+    self.login()
+
+    # save over an existing note supplying new contents and a new title
+    previous_revision = self.note.revision
+    new_note_contents = u"<h3>new title</h3>new blah"
+    self.http_post( "/notebooks/save_note/", dict(
+      notebook_id = self.notebook.object_id,
+      note_id = self.note.object_id,
+      contents = new_note_contents,
+      startup = startup,
+      previous_revision = previous_revision,
+    ), session_id = self.session_id )
+
+    # now attempt to save over that note again without changing the contents, but with a change
+    # to its startup flag
+    previous_revision = self.note.revision
+    result = self.http_post( "/notebooks/save_note/", dict(
+      notebook_id = self.notebook.object_id,
+      note_id = self.note.object_id,
+      contents = new_note_contents,
+      startup = not startup,
+      previous_revision = previous_revision,
+    ), session_id = self.session_id )
+
+    # assert that the note wasn't actually updated the second time
+    assert result[ "new_revision" ] == None
+    assert result[ "previous_revision" ] == previous_revision
+
+    result = self.http_post( "/notebooks/load_note_by_title/", dict(
+      notebook_id = self.notebook.object_id,
+      note_title = "new title",
+    ), session_id = self.session_id )
+
+    note = result[ "note" ]
+
+    assert note.object_id == self.note.object_id
+    assert note.title == self.note.title
+    assert note.contents == self.note.contents
+    assert note.revision == previous_revision
+
+    # assert that the notebook now has the proper startup status for the note
+    if startup:
+      assert note not in self.notebook.startup_notes
+    else:
+      assert note in self.notebook.startup_notes
+
+  def test_save_note_from_an_older_revision( self ):
+    self.login()
+
+    # save over an existing note supplying new contents and a new title
+    first_revision = self.note.revision
+    new_note_contents = u"<h3>new title</h3>new blah"
+    result = self.http_post( "/notebooks/save_note/", dict(
+      notebook_id = self.notebook.object_id,
+      note_id = self.note.object_id,
+      contents = new_note_contents,
+      startup = False,
+      previous_revision = first_revision,
+    ), session_id = self.session_id )
+
+    # save over that note again with new contents, providing the original
+    # revision as the previous known revision
+    second_revision = self.note.revision
+    new_note_contents = u"<h3>new new title</h3>new new blah"
+    result = self.http_post( "/notebooks/save_note/", dict(
+      notebook_id = self.notebook.object_id,
+      note_id = self.note.object_id,
+      contents = new_note_contents,
+      startup = False,
+      previous_revision = first_revision,
+    ), session_id = self.session_id )
+
+    # make sure the second save actually caused an update
+    assert result[ "new_revision" ]
+    assert result[ "new_revision" ] not in ( first_revision, second_revision )
+    assert result[ "previous_revision" ] == second_revision
+
+    # make sure the first title can no longer be loaded
+    result = self.http_post( "/notebooks/load_note_by_title/", dict(
+      notebook_id = self.notebook.object_id,
+      note_title = "my title",
+    ), session_id = self.session_id )
+
+    note = result[ "note" ]
+    assert note == None
+
+    # make sure the second title can no longer be loaded
+    result = self.http_post( "/notebooks/load_note_by_title/", dict(
+      notebook_id = self.notebook.object_id,
+      note_title = "new title",
+    ), session_id = self.session_id )
+
+    note = result[ "note" ]
+    assert note == None
+
+    # make sure the new title is now loadable
+    result = self.http_post( "/notebooks/load_note_by_title/", dict(
+      notebook_id = self.notebook.object_id,
+      note_title = "new new title",
+    ), session_id = self.session_id )
+
+    note = result[ "note" ]
+
+    assert note.object_id == self.note.object_id
+    assert note.title == self.note.title
+    assert note.contents == self.note.contents
+
   def test_save_note_with_unknown_notebook( self ):
     self.login()
 
@@ -384,9 +534,11 @@ class Test_notebooks( Test_controller ):
       note_id = new_note.object_id,
       contents = new_note.contents,
       startup = startup,
+      previous_revision = None,
     ), session_id = self.session_id )
 
     assert result[ "new_revision" ] and result[ "new_revision" ] != previous_revision
+    assert result[ "previous_revision" ] == None
 
     # make sure the new title is now loadable
     result = self.http_post( "/notebooks/load_note_by_title/", dict(
@@ -424,9 +576,11 @@ class Test_notebooks( Test_controller ):
       note_id = new_note.object_id,
       contents = new_note.contents,
       startup = False,
+      previous_revision = None,
     ), session_id = self.session_id )
 
     assert result[ "new_revision" ] and result[ "new_revision" ] != previous_revision
+    assert result[ "previous_revision" ] == None
 
     # make sure the new title is now loadable
     result = self.http_post( "/notebooks/load_note_by_title/", dict(
@@ -455,9 +609,11 @@ class Test_notebooks( Test_controller ):
       note_id = new_note.object_id,
       contents = new_note.contents,
       startup = False,
+      previous_revision = None,
     ), session_id = self.session_id )
 
     assert result[ "new_revision" ] and result[ "new_revision" ] != previous_revision
+    assert result[ "previous_revision" ] == None
 
     # make sure the new title is now loadable
     result = self.http_post( "/notebooks/load_note_by_title/", dict(
@@ -713,6 +869,36 @@ class Test_notebooks( Test_controller ):
 
     assert result.get( "note" ) == None
 
+  def test_undelete_note_that_is_not_deleted( self ):
+    self.login()
+
+    # "undelete" the note
+    result = self.http_post( "/notebooks/undelete_note/", dict(
+      notebook_id = self.notebook.object_id,
+      note_id = self.note.object_id,
+    ), session_id = self.session_id )
+
+    assert result.get( "error" ) == None
+
+    # test that the "undeleted" is where it should be
+    result = self.http_post( "/notebooks/load_note/", dict(
+      notebook_id = self.notebook.object_id,
+      note_id = self.note.object_id,
+    ), session_id = self.session_id )
+
+    note = result.get( "note" )
+    assert note
+    assert note.object_id == self.note.object_id
+    assert note.deleted_from == None
+
+    # test that the note is not somehow in the trash
+    result = self.http_post( "/notebooks/load_note/", dict(
+      notebook_id = self.notebook.trash.object_id,
+      note_id = self.note.object_id,
+    ), session_id = self.session_id )
+
+    assert result.get( "note" ) == None
+
   def test_undelete_note_without_login( self ):
     result = self.http_post( "/notebooks/undelete_note/", dict(
       notebook_id = self.notebook.object_id,
@@ -789,6 +975,26 @@ class Test_notebooks( Test_controller ):
     assert note.object_id == self.note.object_id
     assert note.deleted_from == self.notebook.object_id
 
+  def test_undelete_note_that_is_not_deleted_from_incorrect_notebook( self ):
+    self.login()
+
+    result = self.http_post( "/notebooks/undelete_note/", dict(
+      notebook_id = self.anon_notebook,
+      note_id = self.note.object_id,
+    ), session_id = self.session_id )
+
+    assert result.get( "error" )
+
+    # test that the note is still in its notebook
+    result = self.http_post( "/notebooks/load_note/", dict(
+      notebook_id = self.notebook.object_id,
+      note_id = self.note.object_id,
+    ), session_id = self.session_id )
+
+    note = result.get( "note" )
+    assert note.object_id == self.note.object_id
+    assert note.deleted_from == None
+
   def test_blank_note( self ):
     result = self.http_get( "/notebooks/blank_note/5" )
     assert result[ u"id" ] == u"5"

model/Notebook.py

@@ -106,9 +106,6 @@ class Notebook( Persistent ):
     if old_note is None:
       raise Notebook.UnknownNoteError( note.object_id )
 
-    if contents == old_note.contents:
-      return
-
     self.update_revision()
     self.__titles.pop( note.title, None )
 

model/test/Test_notebook.py

@@ -99,17 +99,6 @@ class Test_notebook( object ):
     note = self.notebook.lookup_note_by_title( new_title )
     assert note == self.note
 
-  def test_update_unrevised_note( self ):
-    self.notebook.add_note( self.note )
-    old_title = self.note.title
-
-    revision = self.notebook.revision
-    self.notebook.update_note( self.note, self.note.contents )
-    assert self.notebook.revision == revision
-
-    note = self.notebook.lookup_note( self.note.object_id )
-    assert note == self.note
-
   @raises( Notebook.UnknownNoteError )
   def test_update_unknown_note( self ):
     new_contents = u"<h3>new title</h3>new blah"

static/css/style.css

@@ -248,6 +248,7 @@ ol li {
 
 .message_inner {
   padding: 0.5em;
+  line-height: 140%;
   background-color: #ffaa44;
   -moz-border-radius: 0.5em;
   -webkit-border-radius: 0.5em;
@@ -266,6 +267,7 @@ ol li {
 
 .error_inner {
   padding: 0.5em;
+  line-height: 140%;
   color: #ffffff;
   background-color: #dd1111;
   -moz-border-radius: 0.5em;

static/js/Editor.js

@@ -3,7 +3,7 @@ function Editor( id, notebook_id, note_text, deleted_from, revisions_list, inser
   this.notebook_id = notebook_id;
   this.initial_text = note_text;
   this.deleted_from = deleted_from || null;
-  this.revisions_list = revisions_list;
+  this.revisions_list = revisions_list || new Array();
   this.read_write = read_write;
   this.startup = startup || false; // whether this Editor is for a startup note
   this.init_highlight = highlight || false;

static/js/Wiki.js

@@ -568,19 +568,19 @@ Wiki.prototype.delete_editor = function ( event, editor ) {
     if ( editor == this.focused_editor )
       this.focused_editor = null;
 
-    editor.shutdown();
+    if ( this.notebook.trash && !editor.empty() ) {
-  }
+      var undo_button = createDOM( "input", {
+        "type": "button",
+        "class": "message_button",
+        "value": "undo",
+        "title": "undo deletion"
+      } );
+      this.display_message( "The note has been moved to the trash.", [ undo_button ] )
+      var self = this;
+      connect( undo_button, "onclick", function ( event ) { self.undelete_editor_via_undo( event, editor ); } );
+    }
 
-  if ( this.notebook.trash ) {
+    editor.shutdown();
-    var undo_button = createDOM( "input", {
-      "type": "button",
-      "class": "message_button",
-      "value": "undo",
-      "title": "undo deletion"
-    } );
-    this.display_message( "The note has been moved to the trash.", [ undo_button ] )
-    var self = this;
-    connect( undo_button, "onclick", function ( event ) { self.undelete_editor_via_undo( event, editor ); } );
   }
 
   event.stop();
@@ -636,28 +636,60 @@ Wiki.prototype.undelete_editor_via_undo = function( event, editor ) {
   event.stop();
 }
 
+Wiki.prototype.compare_versions = function( event, editor, previous_revision ) {
+  this.clear_pulldowns();
+
+  // display the two revisions for comparison by the user
+  this.load_editor( editor.title, null, editor.id, previous_revision );
+  this.load_editor( editor.title, null, editor.id );
+}
+
 Wiki.prototype.save_editor = function ( editor, fire_and_forget ) {
   if ( !editor )
     editor = this.focused_editor;
 
   var self = this;
   if ( editor && editor.read_write && !editor.empty() ) {
+    var revisions = editor.revisions_list;
     this.invoker.invoke( "/notebooks/save_note", "POST", { 
       "notebook_id": this.notebook_id,
       "note_id": editor.id,
       "contents": editor.contents(),
-      "startup": editor.startup
+      "startup": editor.startup,
+      "previous_revision": revisions.length ? revisions[ revisions.length - 1 ] : "None"
     }, function ( result ) { self.update_editor_revisions( result, editor ); }, null, fire_and_forget );
   }
 }
 
 Wiki.prototype.update_editor_revisions = function ( result, editor ) {
-  if ( result.new_revision ) {
+  // if there's not a newly saved revision, then the contents are unchanged, so bail
-    if ( !editor.revisions_list )
+  if ( !result.new_revision )
-      editor.revisions_list = new Array();
+    return;
 
-    editor.revisions_list.push( result.new_revision );
+  var revisions = editor.revisions_list;
+  var client_previous_revision = revisions.length ? revisions[ revisions.length - 1 ] : null;
+
+  // if the server's idea of the previous revision doesn't match the client's, then someone has
+  // gone behind our back and saved the editor's note from another window
+  if ( result.previous_revision != client_previous_revision ) {
+    var compare_button = createDOM( "input", {
+      "type": "button",
+      "class": "message_button",
+      "value": "compare versions",
+      "title": "compare your version with the modified version"
+    } );
+    this.display_error( 'Your changes to the note titled "' + editor.title + '" have overwritten changes made in another window.', [ compare_button ] );
+
+    var self = this;
+    connect( compare_button, "onclick", function ( event ) {
+      self.compare_versions( event, editor, result.previous_revision );
+    } );
+
+    revisions.push( result.previous_revision );
   }
+
+  // add the new revision to the editor's revisions list
+  revisions.push( result.new_revision );
 }
 
 Wiki.prototype.search = function ( event ) {
@@ -722,9 +754,8 @@ Wiki.prototype.display_message = function ( text, buttons ) {
   this.clear_pulldowns();
 
   var inner_div = DIV( { "class": "message_inner" }, text + " " );
-  for ( var i in buttons ) {
+  for ( var i in buttons )
     appendChildNodes( inner_div, buttons[ i ] );
-  }
 
   var div = DIV( { "class": "message" }, inner_div );
   div.buttons = buttons;
@@ -733,7 +764,7 @@ Wiki.prototype.display_message = function ( text, buttons ) {
   ScrollTo( div );
 }
 
-Wiki.prototype.display_error = function ( text ) {
+Wiki.prototype.display_error = function ( text, buttons ) {
   this.clear_messages();
   this.clear_pulldowns();
 
@@ -745,8 +776,13 @@ Wiki.prototype.display_error = function ( text ) {
       editor.shutdown();
   }
 
-  var inner_div = DIV( { "class": "error_inner" }, text );
+  var inner_div = DIV( { "class": "error_inner" }, text + " " );
+  for ( var i in buttons )
+    appendChildNodes( inner_div, buttons[ i ] );
+
   var div = DIV( { "class": "error" }, inner_div );
+  div.buttons = buttons;
+
   appendChildNodes( "notes", div );
   ScrollTo( div );
 }