Closed Bug 463030 Opened 11 years ago Closed 9 years ago

Until date of recurrence rule can set before start date of event

Categories

(Calendar :: General, defect, minor)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: martinschroeder, Assigned: bv1578)

References

Details

(Whiteboard: [needed beta][has l10n impact])

Attachments

(1 file, 1 obsolete file)

You can create an event, and specify a recurrence rule with an until date which is before the start date of the event.
Duplicate of this bug: 491603
Attached patch patch - v1 (obsolete) β€” β€” Splinter Review
This patch seems working fine. It restores the previous (correct) date and shows an alert when the user selects or types a date earlier than the start date in the same way as it has been done for the end date.

Since the function called by "onchange" property for the datepicker, is called twice (I don't exactly understand why, but doesn't seem an event captured twice in the DOM chain), I've inserted a control that prevents the second call if the warning-alert must be showed.
Without that, the "Edit Recurrence" dialog disappears when the user types a  until date earlier than the start date then clicks the OK button or press the enter key.

The only case with a possible wrong behavior is when the user types something that isn't a date in the datepicker (e.g. random characters) then press enter key or OK button, because in this case the dialog is being closed, although the date gets the old correct value. Don't know if this is tolerable or should be avoided (if yes, how?).

The first time that |checkUntilDate.warning| is checked (inside "checkUntilDate" and "onAccept" functions), it is undefined, but his behavior is as a "false" boolean that's what is needed. Does it also need a check for an undefined value?
Assignee: nobody → bv1578
Status: NEW → ASSIGNED
Attachment #440506 - Flags: review?(philipp)
Flags: blocking-calendar1.0+
Whiteboard: [not needed beta][has l10n impact]
Comment on attachment 440506 [details] [diff] [review]
patch - v1

>         } else {
>             untilDate = untilDate.getInTimezone(gStartTime.timezone); // calIRecurrenceRule::untilDate is always UTC or floating
>+            gUntilDate = untilDate;
>             setElementValue("recurrence-duration", "until");
>             setElementValue("repeat-until-date", untilDate.getInTimezone(floating()).jsDate);
>         }
Go ahead and just assign to gUntilDate and then use it in the setElementValue call.


> }
> 
>+function checkUntilDate() {
This function has some magic to it, please document it well (i.e function header)

>+    if (checkUntilDate.warning) {
>+        checkUntilDate.warning = false;
>+        return;
>+    }
So if there is a warning, then just set it false and return? Why? Please document inline

>+    let untilDate = jsDateToDateTime(getElementValue("repeat-until-date"), gStartTime.timezone);
>+    let startDate = gStartTime.clone();
>+    startDate.isDate = true;
>+    checkUntilDate.warning = false;
The last line is not needed, if warning is true then this code will never be reached.


Thanks for the patch! With comments fixed, r=philipp
Please upload a new patch for checkin.
Attachment #440506 - Flags: review?(philipp) → review+
Whiteboard: [not needed beta][has l10n impact] → [needed beta][has l10n impact]
Whiteboard: [needed beta][has l10n impact] → [needed beta][has l10n impact][needs updated patch]
Attached patch patch -v2 β€” β€” Splinter Review
Unfortunately, apart from errors you pointed out, there are others malfunctioning in the patch :(
1. creating a new event (hence without an until date already set), and _typing_ a wrong date in the datepicker then pressing the "OK" button causes an error in the console "gUntilDate" is null;
2. modifying an event which has been built with a wrong until date (earlier than the start date), the patch doesn't allow to change the date because the warning dialog appears just after a click on the datepicker occurs, so it's impossible doing another click to select a correct date;
3. when the user _types_ a wrong date in the datepicker, then press the "Cancel" button the warning dialog appears, instead it shouldn't.

I solved case 2 by immediately changing the until date read from the rule to the value of the start date. Not sure if it's right because in this way the user doesn't see the original (wrong) date in the datepicker (he can see it in the recurrence summary though), but, on the other hand, I think it's useless showing a forbidden until date even if the the original event has a such date.

For the case 3 I've added the function "onCancel" that sets |checkUntilDate.warning| to false in order to prevent the warning from appearing.
Here I have a doubt: since the function checkUntilDate is called before onCancel, could there be a timing problem? Could callback function activate the warning before the checkUntilDate.warning is set to false by the onCancel function? On my system doesn't happen but I wouldn't know if it might depend on the CPU clock, system activity, etc. Maybe the timeout for the callback function could be increased to e.g. 100-200 ms to make the patch safer for this particular case.
Attachment #440506 - Attachment is obsolete: true
Attachment #467728 - Flags: review?(philipp)
Comment on attachment 467728 [details] [diff] [review]
patch -v2

About the timing issue, I think thats fine for now. If we run into problems in some situations we can change the behavior.

r=philipp
Attachment #467728 - Flags: review?(philipp) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/a4b8a47cac5f>
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b3
Whiteboard: [needed beta][has l10n impact][needs updated patch] → [needed beta][has l10n impact]
You need to log in before you can comment on or make changes to this bug.