Activity summary for incoming POP messages filtered by "before junk" rules doesn't work

RESOLVED FIXED in Thunderbird 45.0

Status

Thunderbird
Filters
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jorg K (GMT+2), Assigned: arai)

Tracking

({regression})

Trunk
Thunderbird 45.0
regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Activity summary for incoming pop messages filtered by "before junk" rules doesn't work.

In bug 1211261 I implemented the "UnincorporatedMessageMoved" event, so that POP messages that got moved by a "before junk" rule would get shown in the activity manager.

This worked nicely. Today I noticed that on Daily of 2015-12-10 the summarising doesn't work any more. I believe it worked a couple of days ago, I will get a regression window.

The summarising is done here:
https://hg.mozilla.org/comm-central/annotate/63a4dcada36a2181c1ba6457f58fa0f0476f4f06/mail/components/activity/modules/moveCopy.js#l316

Did something change with the message ID?

I tried "after junk" rules, which are summarised here:
https://hg.mozilla.org/comm-central/annotate/63a4dcada36a2181c1ba6457f58fa0f0476f4f06/mail/components/activity/modules/moveCopy.js#l126
and they still get summaries properly.

How did that break?
(Reporter)

Comment 1

2 years ago
Any idea why that broke?
Flags: needinfo?(rkent)
Keywords: regression
(Reporter)

Comment 2

2 years ago
Created attachment 8697559 [details] [diff] [review]
Patch showing the debug I added

Something has gone bust with activities.

Debug statements are:
dump("+++++++++++++++ |" + activities[activities.length-1].id + "|\n");
dump("+++++++++++++++ |" + this.lastMessage.id + "|\n");

The debug shows:
Incorporate message begin:
uidl string: UID139-1444039152
GetDiskSpaceAvailable returned: 18012839936 bytes
+++++++++++++++ |0|
+++++++++++++++ |undefined|
Incorporate message complete.
Incorporate message begin:
uidl string: UID140-1444039152
GetDiskSpaceAvailable returned: 18012831744 bytes
+++++++++++++++ |0|
+++++++++++++++ |4|
Incorporate message complete.

So two messages were received and incorporated.

Sadly, the last retrieved activity always has id=0 so it never matches.

I also noticed that clicking "Clear List" in the Activity Manager doesn't work and returns this in the console:
JavaScript error: chrome://messenger/content/activity.js, line 263: TypeError: item.detachFromActivity is not a function

The list isn't cleared. On a non-debug Daily the list is cleared. So I don't understand this either.
(Reporter)

Comment 3

2 years ago
OK, regression range:

Worked on 2015-12-04
Busted on 2015-12-05.
http://hg.mozilla.org/comm-central/pushloghtml?startdate=2015-12-04&enddate=2015-12-05+12%3A00

Most likely bug 824104.
Flags: needinfo?(arai.unmht)
(Assignee)

Comment 4

2 years ago
Created attachment 8697587 [details] [diff] [review]
Fix broken mail activity after for-each replacement.

Sorry for poor quality patch :P
I think this will fix something.

Jorg, can you test this?  or is there any way to test it locally?
Assignee: nobody → arai.unmht
Flags: needinfo?(arai.unmht) → needinfo?(mozilla)
Attachment #8697587 - Flags: review?(rkent)
(Reporter)

Comment 5

2 years ago
Comment on attachment 8697587 [details] [diff] [review]
Fix broken mail activity after for-each replacement.

Thanks, that fixes both problems:
- Activities combine/summarise again as expected.
- Clear List works.

Please review/land this quickly. I'm using Daily and I need this function.
Flags: needinfo?(rkent)
Flags: needinfo?(mozilla)
Attachment #8697587 - Flags: feedback+
(Reporter)

Updated

2 years ago
Status: NEW → ASSIGNED

Comment 6

2 years ago
Comment on attachment 8697587 [details] [diff] [review]
Fix broken mail activity after for-each replacement.

Looks good to me.
Attachment #8697587 - Flags: review?(rkent) → review+
(Assignee)

Comment 7

2 years ago
https://hg.mozilla.org/comm-central/rev/b65465b757bbc3c5632c145893f640d1e0718de9
Bug 1231887 - Fix broken mail activity after for-each replacement. r=rkent
(Reporter)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 45.0
(fixing dependency)
Blocks: 824104

Comment 9

2 years ago
Wouldn't for..of work here?
(Assignee)

Comment 10

2 years ago
for-of requires the object to be iterable.
but _activities property is just an object.
we could replace _activities and related implementation with Map or something to use for-of there.
You need to log in before you can comment on or make changes to this bug.