You are here: Foswiki>Tasks Web>Item727 (23 Feb 2009, KennethLavrsen)Edit Attach

Item727: Comment plugin loses data

pencil
Priority: Urgent
Current State: Closed
Released In: 1.0.1
Target Release: patch
Applies To: Extension
Component: CommentPlugin
Branches:
Reported By: TimotheLitt
Waiting For:
Last Change By: KennethLavrsen
I discovered that the comment plugin can lose posted comments.

Consider this case:
%COMMENT{type="above" cols="100" target="%INCLUDINGTOPIC%#LatestComment"}%
The specified anchor does not exist - in this case, it was corrupted by the WYSIWYG plugin. Comment.pm tries a s//, but doesn't notice that it fails.

In any case, an error setting up the page should not cause user data to be lost. I marked this report Urgent because it results in user data loss.

Here's a (somewhat ugly) patch (against TWiki, but foswiki code is identical):
--- lib/TWiki/Plugins/CommentPlugin/Comment.pm~ 2008-09-11 23:41:58.000000000 -0400
+++ lib/TWiki/Plugins/CommentPlugin/Comment.pm  2009-01-10 13:45:46.000000000 -0500
@@ -309,32 +309,35 @@
             $text .= "\n" unless $output =~ m/^\n/s;
             $text .= $output;
             $text .= "\n" unless $text =~ m/\n$/s;
         } else {
             if ( $location ) {
                 if ( $position eq 'BEFORE' ) {
-                    $text =~ s/(?<!location\=\")($location)/$output$1/m;
+                    $text .= $output unless( $text =~ s/(?<!location\=\")($location)/$output$1/m );
                 } else { # AFTER
-                    $text =~ s/(?<!location\=\")($location)/$1$output/m;
+                    $text .= $output unless( $text =~ s/(?<!location\=\")($location)/$1$output/m );
                 }
+               $text .= "\n" unless $text =~ m/\n$/s;
             } elsif ( $anchor ) {
                 # position relative to anchor
                 if ( $position eq 'BEFORE' ) {
-                    $text =~ s/^($anchor\s)/$output$1/m;
+                    $text .= $output unless( $text =~ s/^($anchor\s)/$output$1/m );
                 } else { # AFTER
-                    $text =~ s/^($anchor\s)/$1$output/m;
+                    $text .= $output unless( $text =~ s/^($anchor\s)/$1$output/m );
                 }
+               $text .= "\n" unless $text =~ m/\n$/s;
             } else {
                 # Position relative to index'th comment
                 my $idx = 0;
                 unless( $text =~ s((%COMMENT({.*?})?%.*\n))
                           (&_nth($1,\$idx,$position,$index,$output))eg ) {
                     # If there was a problem adding relative to the comment,
                     # add to the end of the topic
                     $text .= $output;
                 };
+               $text .= "\n" unless $text =~ m/\n$/s;
             }
         }
     }
     if (defined $remove) {
         # remove the index'th comment box

done - slightly differently - so that above and below work as though no anchor/location was specified - with unit test smile

-- SvenDowideit - 15 Jan 2009

Thanks. I followed the example of the "position after the i'th comment fails" code.

Perhaps these failures should create a warning (or debug) log entry? We're not doing what the user expected, and it could be quite frustrating for someone to debug.

-- TimotheLitt - 15 Jan 2009

need to fix the unit test

debug and error logs won't help users much (unless the admin notices in a timely way :/)

i guess this is another one of those things where we need a user error status bad - like broadcastmsg where IF parse failures, eval errors, superceeded syntax detection and the like would go .

hard :/ -- SvenDowideit - 16 Jan 2009

Agree it's a general problem. Maybe not too hard.

Here's a possible approach:
  • Reserve a topic in each web - say 'WebErrors'.
  • Errors are written to this topic in a standard format:
    • #ErrorNumber<seq>
    • ---++ date-timestamp: Web.Topic:Rev: Component: Error: 1-line-summary
    • any error detail lines (traceback, parameters, etc)
    • date-timestamp: Error-end
      • This is a delimiter so anything can be logged
    • Component is core, plugin name, etc; Error is severity
  • Standard error (possible warn, debug) routines that handle locking, sequence, format
    • Special cases:
      • always lock topic with error, then WebErrors to prevent deadlock
      • Errors on the WebErrors topic
      • Hard limit on topic size to prevent runaway errors/denial of service. Configurable - perhaps 10_000 lines to start?
    • Foswiki::userError("Summary", <<"Detail", {web=>, topic=>, rev=>, component=>}); hash params default to current topic, caller
  • Error routine inserts [[WebErrors#ErrorNumber<seq>][ %RED%Error in <Component>%ENDCOLOR% ]] in topic with problem
    • Just after macro that causes problem, or foot of page if we can't do any better or aren't sure about nested macro calls
    • Provides link to data while maximizing chance that page is still useful
    • Do not update topic if no CHANGE access for viewer, or same Component has an error marker on page at this point.
  • For bonus points, put a checkbox 'remove this error' on each heading under WebErrors & a 'submit' at bottom - makes it easy to remove when fixed
  • Cron script (like statistics) to prune 'old' errors
  • Note that an admin can subscribe to WebErrors with MailerContrib, or a maintainer/developer can put a SEARCH on his home page that looks for errors on a component or topic that he supports.
-- TimotheLitt - 16 Jan 2009

Correct me if I'm wrong, but this looks like it's closed. Timothe, great ideas on the error reporting, but shouldn't that be a feature request, rather than an addendum to a plugin bug?

-- CrawfordCurrie - 21 Jan 2009

Yes, I thought about that when I wrote it -- but it was incidental to this instance, so I replied where Sven's comment was made.

I'll open a separate feature request so it doesn't get lost.

-- TimotheLitt - 23 Jan 2009

ItemTemplate edit

Summary Comment plugin loses data
ReportedBy TimotheLitt
Codebase 1.0.0, 1.0.0 beta3, 1.0.0 beta2, 1.0.0 beta1, trunk
SVN Range Foswiki-1.0.0, Thu, 08 Jan 2009, build 1878
AppliesTo Extension
Component CommentPlugin
Priority Urgent
CurrentState Closed
WaitingFor
Checkins distro:950afb1c911f distro:e5d124396fbe
TargetRelease patch
ReleasedIn 1.0.1
Topic revision: r14 - 23 Feb 2009, KennethLavrsen
The copyright of the content on this website is held by the contributing authors, except where stated elsewhere. See Copyright Statement. Creative Commons License    Legal Imprint    Privacy Policy