Item10281: fix the clumsy handling of the topic in the action editor.…

… An action edit must respect changes that occurred to the topic when the editor is open. git-svn-id: http://svn.foswiki.org/trunk/ActionTrackerPlugin@10656 0b4bb1d4-4e5a-0410-9cc4-b2b747904278
foswiki · Feb 4, 2011 · c1fe05b · c1fe05b
1 parent 4b28557
commit c1fe05b
Show file tree

Hide file tree

Showing 9 changed files with 181 additions and 102 deletions.
diff --git a/data/System/ActionTrackerPlugin.txt b/data/System/ActionTrackerPlugin.txt
@@ -450,8 +450,12 @@ The strings used by the =$button$ switch. A =$closebutton= is a special
 | =ACTIONTRACKERPLUGIN_CLOSEBUTTONNAME= | =Close= |
 | =ACTIONTRACKERPLUGIN_CLOSEBUTTONCLOSED= | =Closed= |
 
----+++ Debugging
+---+++ Known Limitations and Debugging
 
+---++++ Limitations
+The plugin uses the standard Foswiki save method to save the results of action edits. However it doesn't merge parallel edits made to the same action.
+
+---++++ Debugging
 Set to 1 to enable debug features, including the undocumented =%<nop>ACTIONNOTIFICATIONS{}%= and =%<nop>ACTIONTRACKERPREFS%= features.
 
 | *Preference* | *Default* |
@@ -467,7 +471,7 @@ Set to 1 to enable debug features, including the undocumented =%<nop>ACTIONNOTIF
 
 Note that if you want to use the =action= template shipped with the Foswiki:Extensions.CommentPlugin to create actions, then you must put the !CommentPlugin *before* the !ActionTrackerPlugin in the ={PluginsOrder}= configuration option.
 
----++ Plugin Info
+---++ Info
 
 Another great Foswiki extension from the <a style="text-decoration:none" href="http://wikiring.com"><img src="%ATTACHURLPATH%/wikiringlogo20x20.png" alt="" /> *WikiRing* </a> - __Working together to improve your wiki experience__!
 
@@ -483,6 +487,7 @@ Thanks are due to the following sponsors, who have helped make this plugin possi
 |  Copyright | Copyright &copy; 2002-2003 Motorola. All Rights Reserved.%BR% Copyright &copy; 2004-2011 Crawford Currie |
 |  License: | GPL ([[http://www.gnu.org/copyleft/gpl.html][GNU General Public License]]) |
 |  Change History: | |
+|  4 Feb 2011 | Foswiki:Tasks/10281: Correct handling of parallel edits. |
 |  28 Jan 2011 | Foswiki:Tasks/Item8263: Merged Kent Dozier's enhancement for display of select-type fields. Foswiki:Tasks/Item10282: Ensure a link to a missing action gives a meaningful error. Foswiki:Tasks/Item9636: correct parsing of manually-entered dates to default to international standards. Foswiki:Tasks/Item8564: numeric-sort in columns containing numeric data. Foswiki:Tasks/Item10190: support plain-text output from ACTIONSEARCH. Foswiki:Tasks/Item868: Use JQueryPlugin to streamline action editing (including refreshing the topic when an edited action is saved). Foswiki:Tasks/Item1946: The name search is now able to search for orphaned actions. Foswiki:Tasks/Item9687: relative dates in search expressions fixed. |
 |  04 Nov 2010 | Foswiki:Tasks/Item1187: Improved the fix for unwanted date changes when you create or edit an action. Now it should also work for servers west of Greenwich.  |
 |  03 Nov 2010 | Foswiki:Tasks/Item9083: Ensure there's at least one newline at the end of the topic text after an action edit, and always one empty line between topic content and meta; otherwise attachment meta-data may get eaten |
@@ -492,11 +497,11 @@ Thanks are due to the following sponsors, who have helped make this plugin possi
 |  04 Jan 2010 | Foswiki:Tasks/Item8376: doc fix |
 |              | Foswiki:Tasks/Item2467: fix stylesheet. Foswiki:Tasks/Item2474: doc updates |
 |  05 Nov 2009 | Foswiki:Tasks/Item8322: documentation improvements Foswiki:Tasks/Item8092: add CSS classes for different table orientations Foswiki:Tasks/1187: standardised entry fields as accepting GMT times only, fixed the test cases |
-|  15 May 2009 | Foswikitask:Item1627: Add horizontal grid lines to separate tasks as it was too difficult to see where one task ends and the next begins. Foswikitask:Item1187 If the server was east of Greenwich the dates in ActionTrackerPlugin were displayed as one day earlier and each time you edited an action the dates would decrement by one day. Foswikitask:Item1364 ActionTrackerPlugin handles actions without uid very badly. Foswikitask:Item1482 actionnotify refered to old name space. |
+|  15 May 2009 | Foswikitask:Item1627: Add horizontal grid lines to separate tasks as it was too difficult to see where one task ends and the next begins. Foswikitask:Item1187 If the server was east of Greenwich the dates in ActionTrackerPlugin were displayed as one day earlier and each time you edited an action the dates would decrement by one day. Foswikitask:Item1364 ActionTrackerPlugin handles actions without uid very badly. Foswikitask:Item1482 actionnotify referred to old name space. |
 |  29 Jan 2009 | Foswikitask:Item455: port to foswiki Foswikitask:Item5974: correct CSS for search results table Foswikitask:Item5938: load CSS for ACTIONSEARCH Foswikitask:Item5606: Foswikitask:Item5915: respect global settings for time format Foswikitask:Item4312: finished support for header, footer etc. Foswikitask:Item412: added =reverse= Foswikitask:Item5962: removed apparently spurious newline |
 |  09 Jan 2002 | First version |
 |  Dependencies: | %$DEPENDENCIES% |
-|  Plugin Home: | http://foswiki.org/Extensions/ActionTrackerPlugin |
+|  Home page: | http://foswiki.org/Extensions/ActionTrackerPlugin |
 
 %META:FILEATTACHMENT{name="styles.css" attr="h" comment="Stylesheet for actions"}%
 %META:FILEATTACHMENT{name="logo.png" attr="h" comment="Logo"}%

diff --git a/lib/Foswiki/Plugins/ActionTrackerPlugin.pm b/lib/Foswiki/Plugins/ActionTrackerPlugin.pm
@@ -9,7 +9,7 @@ use Foswiki::Func ();
 use Foswiki::Plugins ();
 
 our $VERSION = '$Rev$';
-our $RELEASE = '28 Jan 2011';
+our $RELEASE = '4 Feb 2011';
 our $SHORTDESCRIPTION =
     'Adds support for action tags in topics, and automatic notification of action statuses';
 our $initialised = 0;
@@ -151,7 +151,7 @@ sub _beforeActionEdit {
 
     die unless ($date);
 
-    $tmpl =~ s/%DATE%/$date/go;
+    $tmpl =~ s/%DATE%/$date/g;
     my $user = Foswiki::Func::getWikiUserName();
     $tmpl =~ s/%WIKIUSERNAME%/$user/go;
     $tmpl = Foswiki::Func::expandCommonVariables( $tmpl, $topic, $web );
@@ -181,11 +181,10 @@ sub _beforeActionEdit {
 	return
     };
 
-    my $pretext  = $pre->stringify();
-    my $posttext = $post->stringify();
-
-    $fields .= CGI::hidden( -name => 'pretext',  -value => $pretext );
-    $fields .= CGI::hidden( -name => 'posttext', -value => $posttext );
+    # Add revision info to support merging
+    my $info = $meta->getRevisionInfo();
+    $fields .= CGI::hidden( -name => 'originalrev',
+			    -value => "$info->{version}_$info->{date}" );
 
     $tmpl =~ s/%UID%/$uid/go;
 
@@ -253,36 +252,70 @@ sub _hiddenMeta {
 # actual text.
 # Metadata is handled by the preview script itself.
 sub afterEditHandler {
-    ### my ( $text, $topic, $web ) = @_;
+    my ( $text, $topic, $web ) = @_;
 
     my $query = Foswiki::Func::getCgiQuery();
     return unless ( $query->param('closeactioneditor') );
 
-    return unless lazyInit( $_[2], $_[1] );
+    return unless lazyInit( $web, $topic );
+
+    my ( $ancestorRev, $ancestorDate ) = (0, 0);
+    my $origin = $query->param('originalrev');
+    ASSERT(defined($origin)) if DEBUG;
 
-    my $pretext = $query->param('pretext') || "";
+    if ($origin =~ /^(\d+)_(\d+)$/) {
+	( $ancestorRev, $ancestorDate ) = ( $1, $2 );
+    }
 
-    my $char = chop($pretext);
-    $pretext .= $char if ( $char ne "\n" );
-    $pretext .= "\n";
+    # Get the most recently saved rev
+    (my $meta, $text) = Foswiki::Func::readTopic($web, $topic);
+    my $info = $meta->getRevisionInfo();
+    my $mustMerge = ($ancestorRev ne $info->{version}
+		     || $ancestorDate && $info->{date}
+		     && $ancestorDate ne $info->{date});
 
-    my $posttext = $query->param('posttext') || "";
+    my $latest_as = Foswiki::Plugins::ActionTrackerPlugin::ActionSet::load(
+	$web, $topic, $text, 1);
 
-    # count the previous actions so we get the right action number
-    my $an  = 0;
-    my $tmp = "$pretext";
-    while ( $tmp =~ s/%ACTION{.*?}%//o ) {
-        $an++;
-    }
+    my $uid = $query->param("uid");
+    ASSERT(defined($uid)) if DEBUG;
 
-    my $action =
+    my $latest_act = $latest_as->search(
+	new Foswiki::Attrs('uid="' . $uid . '"'))->first;
+
+    my $new_act =
 	Foswiki::Plugins::ActionTrackerPlugin::Action::createFromQuery(
-	    $_[2], $_[1], $an, $query );
+	    $_[2], $_[1], $latest_act->{ACTION_NUMBER}, $query );
 
-    $action->populateMissingFields();
+    unless (UNIVERSAL::isa($latest_act, 'Foswiki::Plugins::ActionTrackerPlugin::Action')) {
+	# If the edited action was not found in the latest rev, then force it in (it may
+	# have been removed in another parallel edit)
+	$latest_act = $new_act;
+	$latest_as->add($new_act);
+    }
 
-    my $text = $action->stringify();
-    $text = "$pretext$text$posttext";
+    # See if we can get a common ancestor for merging
+    my $old_act;
+    if ($mustMerge) {
+
+	# If we have to merge, we need the ancestor root of the action to
+	# do a three-way merge.
+	# If the previous revision was generated by a reprev,
+	# then the original is lost and we can't 3-way merge
+	unless ($info->{reprev} && $info->{version}
+	    && $info->{reprev} == $info->{version} ) {
+
+	    my ($ances_meta, $ances_text) = Foswiki::Func::readTopic($web, $topic, $ancestorRev);
+	    my $ances = Foswiki::Plugins::ActionTrackerPlugin::ActionSet::load(
+		$web, $topic, $ances_text, $ancestorRev);
+	    $old_act = $ances->search(
+		new Foswiki::Attrs('uid="' . $uid . '"'))->first;
+	}
+    }
+
+    $latest_act->updateFromCopy($new_act, $mustMerge, $info->{version}, $ancestorRev, $old_act);
+    $latest_act->populateMissingFields();
+    $text = $latest_as->stringify();
 
     # take the opportunity to fill in the missing fields in actions
     _addMissingAttributes( $text, $_[1], $_[2] );
@@ -303,7 +336,9 @@ sub beforeSaveHandler {
 
     if ( $query->param('closeactioneditor') ) {
 
-        # this is a save from the action editor
+        # this is a save from the action editor. Text will just be the text of the action - we
+	# must recover the rest from the topic on disc.
+
         # Strip pre and post metadata from the text
         my $premeta  = "";
         my $postmeta = "";

diff --git a/lib/Foswiki/Plugins/ActionTrackerPlugin/Action.pm b/lib/Foswiki/Plugins/ActionTrackerPlugin/Action.pm
@@ -61,79 +61,79 @@ my $dw        = 16;
 my $nw        = 35;
 my %basetypes = (
     changedsince => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 0, 0, undef
+        type => 'noload'
     ),
     closed => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'date', $dw, 1, 0, undef
+        type => 'date', size => $dw, match => 1
     ),
     closer => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'names', $nw, 1, 0, undef
+        type => 'names', size => $nw, match => 1
     ),
     created => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'date', $dw, 1, 0, undef
+        type => 'date', size => $dw, match => 1
     ),
     creator => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'names', $nw, 1, 0, undef
+        type => 'names', size => $nw, match => 1
     ),
     dollar => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 0, 0, undef
+        type => 'noload'
     ),
     due => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'date', $dw, 1, 0, undef
+        type => 'date', size => $dw, match => 1
     ),
     edit => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 0, 0, undef
+        type => 'noload'
     ),
     format => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 0, 0, undef
+        type => 'noload'
     ),
     header => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 0, 0, undef
+        type => 'noload'
     ),
     late => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 0, 0, undef
+        type => 'noload'
     ),
     n => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 0, 0, undef
+        type => 'noload'
     ),
     nop => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 0, 0, undef
+        type => 'noload'
     ),
     notify => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'names', $nw, 1, 0, undef
+        type => 'names', size => $nw, match => 1
     ),
     percnt => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 0, 0, undef
+        type => 'noload'
     ),
     quot => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 0, 0, undef
+        type => 'noload'
     ),
     sort => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 0, 0, undef
+        type => 'noload'
     ),
     state => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'select', 1, 1, 1, [ 'open', 'closed' ]
+        type => 'select', size => 1, match => 1, defineable => 1, values => [ 'open', 'closed' ]
     ),
     text => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 1, 0, undef
+        type => 'noload', match => 1, merge => 1
     ),
     topic => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 1, 0, undef
+        type => 'noload', match => 1
     ),
     uid => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'text', $nw, 1, 0, undef
+        type => 'text', size => $nw, match => 1
     ),
     web => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 1, 0, undef
+        type => 'noload', match => 1
     ),
     who => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'names', $nw, 1, 0, undef
+        type => 'names', size => $nw, match => 1
     ),
     within => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 1, 0, undef
+        type => 'noload', match => 1
     ),
     ACTION_NUMBER => new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
-        'noload', 0, 0, 0, undef
+        type => 'noload'
     ),
 );
 
@@ -241,8 +241,9 @@ sub extendTypes {
                 }
             }
             $types{$name} =
-              new Foswiki::Plugins::ActionTrackerPlugin::AttrDef( $type, $size,
-                1, 1, \@values );
+              new Foswiki::Plugins::ActionTrackerPlugin::AttrDef(
+		  type => $type, size => $size,
+		  match => 1, defineable => 1, values => \@values );
         }
         else {
             return 'Bad EXTRAS definition \'' . $def . '\' in EXTRAS';
@@ -1040,6 +1041,37 @@ sub createFromQuery {
         $attrs, $desc );
 }
 
+# Copy noload attributes from a copy of the action
+sub updateFromCopy {
+    my ( $this, $copy, $mustMerge, $curRev, $origRev, $ancestoraction ) = @_;
+    foreach my $attrname ( keys %types ) {
+        if ( defined $copy->{$attrname}
+	    && (!defined($this->{attrname}) || $copy->{attrname} ne $this->{$attrname})) {
+	    if ($mustMerge && $ancestoraction && $types{$attrname}->{merge}) {
+		# must attempt to merge
+		require Foswiki::Merge;
+		if ($ancestoraction) {
+		    $this->{$attrname} =
+			Foswiki::Merge::merge3(
+			    $origRev, $ancestoraction->{$attrname},
+			    $curRev, $this->{$attrname},
+			    'new', $copy->{$attrname},
+			    '.*?\n', $Foswiki::Plugins::SESSION );
+		} else {
+		    $this->{$attrname} =
+			Foswiki::Merge::merge2(
+			    $curRev, $this->{$attrname},
+			    'new', $copy->{$attrname},
+			    '.*?\n', $Foswiki::Plugins::SESSION );
+		}
+	    } else {
+		# Take the value from the action editor
+		$this->{$attrname} = $copy->{$attrname};
+	    }
+	}
+    }
+}
+
 sub formatForEdit {
     my ( $this, $format ) = @_;
 

diff --git a/lib/Foswiki/Plugins/ActionTrackerPlugin/ActionSet.pm b/lib/Foswiki/Plugins/ActionTrackerPlugin/ActionSet.pm
@@ -27,6 +27,12 @@ sub add {
     push @{ $this->{ACTIONS} }, $action;
 }
 
+sub first {
+    my $this = shift;
+    return undef unless scalar(@{$this->{ACTIONS}});
+    return $this->{ACTIONS}->[0];
+}
+
 # PUBLIC STATIC load an action set from a block of text
 sub load {
     my ( $web, $topic, $text, $keepText ) = @_;

diff --git a/lib/Foswiki/Plugins/ActionTrackerPlugin/AttrDef.pm b/lib/Foswiki/Plugins/ActionTrackerPlugin/AttrDef.pm
@@ -6,14 +6,11 @@ use integer;
 package Foswiki::Plugins::ActionTrackerPlugin::AttrDef;
 
 sub new {
-    my ( $class, $type, $size, $match, $candef, $values ) = @_;
-    my $this = ();
+    my ( $class, %options) = @_;
+    my $this = { %options };
 
-    $this->{type}       = $type;
-    $this->{size}       = $size || 1;
-    $this->{match}      = $match || 1;
-    $this->{defineable} = $candef;
-    $this->{values}     = $values;
+    $this->{size}       ||= 1;
+    $this->{match}      ||= 1;
 
     return bless( $this, $class );
 }
@@ -41,7 +38,7 @@ sub firstSelect {
 sub isRedefinable {
     my $this = shift;
 
-    return $this->{defineable} == 1;
+    return $this->{defineable} ? 1 : 0;
 }
 
 1;

diff --git a/test/unit/ActionTrackerPlugin/ActionTests.pm b/test/unit/ActionTrackerPlugin/ActionTests.pm
@@ -472,7 +472,7 @@ sub test_HTMLFormattingOpen {
     $fmt =
       new Foswiki::Plugins::ActionTrackerPlugin::Format( "", "| \$edit |", "" );
     my $url =
-"$Foswiki::cfg{DefaultUrlHost}$Foswiki::cfg{ScriptUrlPath}/edit$Foswiki::cfg{ScriptSuffix}/Test/Topic\\?skin=action%2cpattern;atp_action=AcTion0;.*";
+"$Foswiki::cfg{DefaultUrlHost}$Foswiki::cfg{ScriptUrlPath}/edit$Foswiki::cfg{ScriptSuffix}/Test/Topic\\?skin=action,pattern;atp_action=AcTion0;.*";
     $s = $fmt->formatHTMLTable( [$action], "href" );
     $this->assert( $s =~ m(<td> <a href="(.*?)">edit</a> </td>), $s );
     $this->assert( $1, $s );