Item10558: MailerContrib change emails are incorrect on 1.1.3beta2

There appears to be something wrong with the change emails on Foswiki.org 1.1.3-beta2.

• Revisions are reversed in the links
• Some revision ranges are unusual. PublishPlugin was changed - Rev 23 created. Link in the change notice was http://foswiki.org/bin/rdiff/Extensions/PublishPlugin?rev2=5&rev1=23. However the prior revision Rev 22 was saved: Topic revision: r22 - 01 Nov 2010. Why was the change reported as r5 -> r23
• The diff is backwards. ExtensionNews was reported as change rev 140 -> 142. But prior save was rev 141, and the diff had TOPICINFO for rev 142 in strikeout compared to rev 141.
• TocPlugin only has a Rev 1 and 2, but the change reports Rev 1 -> Rev 3

This is an automated e-mail from Foswiki.

New or changed topics in Foswiki.Extensions, since 28 Mar 2011 - 06:12:

Topics in Extensions web:	Changed: (now 20:15)	Changed by:
PublishPlugin	28 Mar 2011 - 18:32 - r5->r23	CrawfordCurrie
nop PublishPlugin Generates a static view of a web, as HTML files on disc, or as a PDF, or as a zip or tgz archive file, or by uploading directly to an FTP server ...
TocPlugin	28 Mar 2011 - 18:34 - r1->r3	CrawfordCurrie
nop TocPlugin Supports the generation of tables of contents, together with symbolic cross references that operate within and between topics. Adds a number of new macros ...
BookmakerPlugin	28 Mar 2011 - 19:04 - r1->r2	CrawfordCurrie
BookmakerPlugin The Bookmaker plugin provides support for interactive capture and sorting of ordered lists of topics. The lists are held in topics, and may be manually ...
ExtensionNews	28 Mar 2011 - 19:08 - r140->r141	CrawfordCurrie
You can include this feed with % nop INCLUDE{"SimpleNewsFeedData" pattern=".*?(( \*.*?\n){1,5}).*"}% to show the last 5 news stories Extensions.ExtensionNews 142 Extensions.ExtensionNews 141 %META:TOPICINFO{author="ArthurClemens" comment="save topic" date="1301343167" format="1.1" reprev="139" version="142"}% %META:TOPICINFO{author="CrawfordCurrie" comment="save topic" date="1301339291" format="1.1" reprev="139" version="141"}% ---+ Extension News

Review recent changes in: http://foswiki.org/Extensions/WebChanges

Subscribe / Unsubscribe in: http://foswiki.org/Extensions/WebNotify

-- GeorgeClark - 28 Mar 2011

It appears that part of the problem is in Foswiki::Meta::summariseChanges $orev isn't actually referenced when loading the topic objects, so the old rev is whatever is pulled into $oldTopicObject = $this->new( $session, $this->web, $this->topic );

There are no unit tests for either Foswiki::Func::summariseChanges or Foswiki::Meta::summariseChanges

Setting to urgent to draw some attention. Not sure if this should really block 1.1.3, but clearly something is broken.

-- GeorgeClark - 28 Mar 2011

I've committed a fix. This late in release it needs more eyes. Foswiki::Meta::summariseChanges failed to load the "old rev" of the topic it was summarizing. I plan to patch this into Foswiki.org so we can review the results on the topic notification emails.

-- GeorgeClark - 29 Mar 2011

Revised Meta.pm has been applied to foswiki.org.

-- GeorgeClark - 29 Mar 2011

Your analysis and fix look correct to me. I tracked back through the history of that code, and I think it came about because back in the dim-and-distant, it was only ever called when the "current" object was already the $orev object. Since $this->new cloned that rev, the code worked - but of course, perfectly reasonable changes in the way it is called made that assumption invalid. So forcing $orev is the right thing to do.

BTW $this->new is perfectly valid; it clones an existing meta (though the normal call would be $this->new() with no parameters)

-- CrawfordCurrie - 29 Mar 2011

Good that we caught this. Shows that eating our own dogfood on f.o. was an important investment.

-- KennethLavrsen - 29 Mar 2011

This is fixed and can be set to Waiting for release. Leaving it open for now - I'd like to add some additional unit tests for the summariseChanges function.

-- GeorgeClark - 29 Mar 2011

The underlying issue is fixed, but the actual summary is very unusual, so I think something might be wrong deeper in Meta::summariseChanges I've checked in a unit test that passes, but I don't understand the output at all. The topic contains:

Rev 2 text

Line 1

Line 3

Rev 3 Text

Line 1
<nop>SomeOtherData
Line 3

output from summariseChanges

 Line 3
-<nop>TemporaryMetaTestsTestWebMetaTests.RevIt 2
+<nop>TemporaryMetaTestsTestWebMetaTests.RevIt 3
-<nop>%<nop><nop>META:<nop>TOPICINFO{author="<nop>BaseUserMapping_666" comment="" date="1301524...
+<nop>%<nop><nop>META:<nop>TOPICINFO{author="<nop>BaseUserMapping_666" comment="" date="1301524...
 Line 1

-- GeorgeClark - 30 Mar 2011

Unit test with the above bogus results checked in to release11 and trunk. Note that trunk output is slightly different - eliminating the empty comment from the TOPICINFO

-- GeorgeClark - 30 Mar 2011

distro:0775d1e4ad34 has some fixes for trunk that seems to clean up a few issues with summariseChanges. Needs careful review before considering for 1.1.3

-- GeorgeClark - 31 Mar 2011

Where is Release branch on this one at the moment.

Can I build a release candidate or do I need to wait for this to be fully fixed?

What is still broken?

-- KennethLavrsen - 04 Apr 2011

The release branch has the initial fix but not the follow-up changes that rework the Foswiki::Merge::simpleMerge, and Foswiki::Meta::summariseChanges, and Foswiki::Render::TML2PlainText. The changes were extensive enough that I'm uneasy making the change without a 2nd set of eyes.

So Release branch does generate a summary of changes, but they are a bit nonsense because of some duplicated text. See distro:0775d1e4ad34 and distro:8d41f4aa48ba for the details on the changes.

-- GeorgeClark - 05 Apr 2011

Oh - I was thinking of patching these changes into Foswiki.org, since we use the WebNotify mailing for Tasks, it would get them a bit of a workout.

-- GeorgeClark - 05 Apr 2011

I have read through the changes and nothing screams out at me; the existing unit tests seem to still pass, so I don't have any additional comments except to suggest that the more extensive changes are held back to minimise risk to 1.1.3 - I don't think 1.1.3 is any worse in this than 1.1.2 (correct me if I'm wrong, in which case the changes should be merged).

-- CrawfordCurrie - 05 Apr 2011

No 1.1.3 is no worse that 1.1.2 that I can figure - it appears that the emails have been broken since 1.1.0. There is another task about garbage in the HTML emails, so there has been some notice. Item10014 - I suspect the %META in the data is because of changes to Meta::stringify with the refactored Meta.

• 1.0.9 - ObjectMethod stringify( $types ) -> $string
• 1.1.2 - ObjectMethod stringify( $debug ) -> $string

So summariseChanges in 1.1 was still passing a type selector regex to stringify, using the 1.0.9 API definition, and getting back extraneous information in the summary. The debug flag caused extra Meta to be included. And the text was attached twice, both by the caller and by stringify.

I guess other than the changes to the Merge routine, I'm feeling a bit better about this fix.

-- GeorgeClark - 06 Apr 2011

They just got dcommitted to release 1.1.3 - thought I had the fixes stashed.

-- GeorgeClark - 07 Apr 2011

Kenneth, please release!

-- FranzJosefGigler - 07 Apr 2011

The patches are applied to foswiki.org.

-- GeorgeClark - 08 Apr 2011

One last change and ready for release. Foswiki.org is running this and the change emails appear okay to me.

-- GeorgeClark - 09 Apr 2011

There was double-encoding of <nop>'s when summarising a rev without changes. This also has changed the Search $changes format a bit - fixed unit tests, but needs further verification.

-- GeorgeClark - 13 Apr 2011

Commenting is disabled while not logged in