Closed Bug 1688708 Opened 4 years ago Closed 4 years ago

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

Categories

(Calendar :: Tasks, defect)

defect

Tracking

(thunderbird_esr78 fixed)

RESOLVED FIXED
87 Branch
Tracking Status
thunderbird_esr78 --- fixed

People

(Reporter: lasana, Assigned: lasana)

Details

Attachments

(1 file, 2 obsolete files)

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.

Depends on: 1686466
Status: NEW → ASSIGNED
Attached patch bug1688708.patch (obsolete) — Splinter Review

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.

Attachment #9199849 - Flags: review?(geoff)
No longer depends on: 1686466
Attached patch bug1688708.patch (obsolete) — Splinter Review

Had the wrong bug number in the commit message.

Attachment #9199849 - Attachment is obsolete: true
Attachment #9199849 - Flags: review?(geoff)
Attachment #9199897 - Flags: review?(geoff)
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.
Attachment #9199897 - Flags: review?(geoff)

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.

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

Attachment #9199897 - Attachment is obsolete: true
Attachment #9200707 - Flags: review?(geoff)
Attachment #9200707 - Flags: review?(geoff) → review+
Target Milestone: --- → 87 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/17f59cbc7b73
do not assume items are events in countOccurrences(). r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED

I guess 78 affected. Should we uplift?

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

Attachment #9200707 - Flags: approval-comm-esr78?

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

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

Attachment #9200707 - Flags: approval-comm-esr78? → approval-comm-esr78+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: