1688708 - Unable to delete repeating tasks: Uncaught TypeError: can't access property "clone", aItem.parentItem.startDate is undefined

Status: RESOLVED FIXED

(Calendar :: Tasks, defect)

Description

[Lasana Murray]

Steps To Reproduce:

1. Go to the tasks tab.
2. Create a new task that repeats for 3 days.
3. Click on any occurrence of the event and attempt to delete it.

Expected Result:
Task is deleted successfully.

Actual Result:
Task is not deleted. The following is logged to console: "Uncaught TypeError: can't access property "clone", aItem.parentItem.startDate is undefined"

The countOccurences() function assumes the passed item is a calIEvent despite the documentation indicating it handles calITodo as well.

Comment 1

[Lasana Murray]

Attachment: bug1688708.patch

Fix for this. The included test times out sometimes, I have not been able to figure out why yet. Trunk is busted right now so I'll do a try run for this later.

Comment 2

[Lasana Murray]

Attachment: bug1688708.patch

Had the wrong bug number in the commit message.

Comment 3

[Geoff Lankow (:darktrojan)]

Comment on attachment 9199897 [details] [diff] [review]
bug1688708.patch

Review of attachment 9199897 [details] [diff] [review]:
-----------------------------------------------------------------

Not bad but a few work-ons.

::: calendar/base/modules/calRecurrenceUtils.jsm
@@ +474,5 @@
>            byCount = true;
>          } else {
>            // the rule is limited by as an until date
> +          let parentItem = aItem.parentItem;
> +          let startDate = parentItem.startDate ?? parentItem.entryDate;

Use cal.item.isEvent or cal.item.isToDo to determine which field should be used here and below. There may be other reasons that startDate is falsy.

Also can you turn that comment into a real sentence while you're here?

::: calendar/test/browser/browser.ini
@@ +18,5 @@
>  [browser_dragEventItem.js]
>  [browser_eventDisplay.js]
>  [browser_import.js]
>  [browser_localICS.js]
> +[browser_taskDelete.js]

This test would be better in the views folder, as it tests the tasks view.

::: calendar/test/browser/browser_taskDelete.js
@@ +49,5 @@
> +  let tree = window.document.querySelector("#calendar-task-tree");
> +  let radio = window.document.querySelector("#opt_next7days_filter");
> +  EventUtils.synthesizeMouseAtCenter(radio, {});
> +
> +  await BrowserTestUtils.waitForEvent(tree, "refresh");

waitForEvent should go before the click and await after it. Just in case the event happens before we begin waiting, which can happen despite it not making a lot of sense.

Comment 4

[Lasana Murray]

Use cal.item.isEvent or cal.item.isToDo to determine which field should be used here and below. There may be other reasons that startDate is falsy.

I avoided doing this because of how those functions work underneath (re: bug 1687329). Also, this code previously assumed startDate is always set, hence the error. If for any reason startDate and entryDate is nullable then we are going to run into an error.

This test would be better in the views folder, as it tests the tasks view.

Are you sure about that? That folder looks like tests specific to the display of data, this tests specific functionality. There is also a browser_taskDisplay.js in here too. Looks to me that tasks needs its own folder.

Comment 5

[Lasana Murray]

Attachment: bug1688708v2.patch

Made the waitForEvent change. Also got around the occasional timeouts by manually refreshing the tree view.

Comment 6

[Lasana Murray]

Try run in progress:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=d3fea9625ef3c77c2ca96b96d06144635334f614

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/17f59cbc7b73
do not assume items are events in countOccurrences(). r=darktrojan

Comment 8

[Magnus Melin [:mkmelin]]

I guess 78 affected. Should we uplift?

Comment 9

[Lasana Murray]

Comment on attachment 9200707 [details] [diff] [review]
bug1688708v2.patch

[Approval Request Comment]
Regression caused by (bug #): Unknown
User impact if declined: Users will be unable to delete repeating tasks which is kind of annoying.
Testing completed (on c-c, etc.): c-c trunk
Risk to taking this patch (and alternatives if risky): Not too much but I was unable to run the tests on 78

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9200707 [details] [diff] [review]
bug1688708v2.patch

[Triage Comment]
Approved for esr78
(and is backed by automated test)

Comment 11

[Rob Lemley [:rjl]]

Thunderbird 78.9.0:
https://hg.mozilla.org/releases/comm-esr78/rev/4f0d70c522b5

Comment 12

[Geoff Lankow (:darktrojan)]

Test fixed for 78:
https://hg.mozilla.org/releases/comm-esr78/rev/1a5cd2aa11de