Closed Bug 1633772 Opened 5 months ago Closed 5 months ago

Recurring task with recurrence until date raises an exception when listed

Categories

(Calendar :: Provider: Local Storage, defect, P3)

Thunderbird 76

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 78.0

People

(Reporter: chris, Assigned: chris)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:75.0) Gecko/20100101 Firefox/75.0

Steps to reproduce:

Create a recurring daily task UNTIL a date (i.e. 2020-04-28)
Restart Thunderbird

Actual results:

The task is no longer listed among tasks
An exception is raised in the console:

TypeError: lastRecurrence.endDate is undefined
CalRecurrenceInfo.jsm:158:11

NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "lastRecurrence.endDate is undefined" {file: "resource:///modules/CalRecurrenceInfo.jsm" line: 158}]'[JavaScript Error: "lastRecurrence.endDate is undefined" {file: "resource:///modules/CalRecurrenceInfo.jsm" line: 158}]' when calling method: [calIRecurrenceInfo::recurrenceEndDate]
CalStorageCalendar.jsm:665

Expected results:

The task should be displayed in task list and no exception should be raised.

I guess this problem occurred after bug 1590665 was implemented. There is no endDate in calITodo, contrary to calIEvent.

Are you able to provide a patch?

Keywords: regression
Regressed by: 1590665

Same error is reported in Bug 1610990 for Thunderbird 73.

Yes I could work on a patch. I could use the entryDate of the last task occurrence instead of the event endDate.
But the question is if the optional task dueDate should be considered too.

Here is a patch proposal to prevent an exception when computing the recurrence end date for tasks.
The entry date and the due date are now considered (the end date is used for events). Since a task can extend on several days if the optional due date is set, and can be displayed in this way in the agenda view, the due date is also taken into account.

Since an occurrence of the task could possibly have its due date modified to a date positioned after the last occurrence of the recurrence, all modified occurrences are examined as well. This process is generic and now also apply to events.

As previously (event endDate) this code introduces a dependency on todo and event definition (event endDate, task entryDate and dueDate properties). It may have been possible to use a "virtual" function getItemEndingDate() in ItemBase class and implement it in CalEvent.jsm and CalTodo.jsm but its usage and semantic would have seem very specific to CalRecurrenceInfo.jsm. So a unique getItemEndingDate() was implemented in this file. If you prefer this alternative spliting in CalEvent.jsm and CalTodo.jsm let me know.

Assignee: nobody → chris
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #9146440 - Flags: review?(paul)

Sorry, the wrong version of the file was committed. A new version of the patch is attached.

Attachment #9146440 - Attachment is obsolete: true
Attachment #9146440 - Flags: review?(paul)
Attachment #9147193 - Flags: review?(paul)
Comment on attachment 9147193 [details] [diff] [review]
Bug 1633772 - End date, entry date and due date of last occurrence and exceptions are now considered when computing recurrence end date

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

Thanks for catching this, and especially for writing this patch that fixes it.  I confirmed the issue and that the patch fixes it.  Just a couple of minor requests on code comments, if you could address those and then upload an updated patch, and this will be ready to land.

::: calendar/base/src/CalRecurrenceInfo.jsm
@@ +136,5 @@
>      }
>      return true;
>    },
>  
> +  getItemEndingDate(item) {

Can you please add a docstring for this new function.  We're working on adding them to existing code and any new code added.  Something like:

  /**
   * Get the item ending date.
   *
   * @param {calIEvent | calITodo} item - The item.
   * @return {calIDateTime | null} The ending date or null.
   */
   getItemEndingDate(item) {

@@ +142,5 @@
> +      if (item.endDate) {
> +        return item.endDate;
> +      }
> +    } else if (cal.item.isToDo(item)) {
> +      // Due date must be considered since it is used when displaying the task in agenda view

Nit: add a period (".") at end of comments like this.
Attachment #9147193 - Flags: review?(paul) → review+

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is -- (non,) indicating it has has not been previously triaged, the bug's Severity is being updated to -- (default, untriaged.)

Severity: normal → --

I'm happy to help. Thank you for the review ! I followed your recommendations and uploaded a new version of the patch.

Attachment #9147193 - Attachment is obsolete: true
Attachment #9147605 - Flags: review?(paul)
Comment on attachment 9147605 [details] [diff] [review]
Bug 1633772 - End date, entry date and due date of last occurrence and exceptions are now considered when computing recurrence end date

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

Looks good.  I'll set the flag and the patch will be landed soon.  Thanks again!
Attachment #9147605 - Flags: review?(paul) → review+
Priority: -- → P3

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/d57944ea052e
End date, entry date and due date of last occurrence and exceptions are now considered when computing recurrence end date. r=pmorris

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → 78
Duplicate of this bug: 1610990
You need to log in before you can comment on or make changes to this bug.