Closed Bug 1151782 Opened 9 years ago Closed 9 years ago

Inputting 29th Feb as a birthday in the addressbook contact replaces it with 1st Mar.

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
normal

Tracking

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed, seamonkey2.35 fixed, seamonkey2.36 fixed, seamonkey2.37 fixed)

RESOLVED FIXED
Thunderbird 40.0
Tracking Status
thunderbird38 + fixed
thunderbird39 --- fixed
thunderbird40 --- fixed
seamonkey2.35 --- fixed
seamonkey2.36 --- fixed
seamonkey2.37 --- fixed

People

(Reporter: aceman, Assigned: christian)

References

Details

(Keywords: polish)

Attachments

(1 file, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1139167 comment 14 +++

In Thunderbird addressbook, if you enter a birthday of 29/2/2000 (29th Feb), it shows correctly but as soon as you reopen editing the card, it gets modified to 1/3/2000 (1st Mar).

Christian, maybe you can look at this one after bug 1139167 is closed.
Attached patch Patch against comm-central trunk (obsolete) — Splinter Review
The attached patch against comm-central trunk fixes the issue for me.
It also applies to thunderbird-31.5.0 and it also fixes the suite parts.

I only built and tested on thunderbird-31.5.0 though.

The issue is the initialization order of the date picker properties. The relevant code used to init the month, the day of month and only then the year. So I assume the date picker has some default (1900? current year?) which kicks in until overridden.
Moving the year property assignment to be the first of these three, the issue goes away as the datepicker has the necessary information for the target year.
Attachment #8592431 - Flags: review?(mconley)
Attachment #8592431 - Flags: review?(iann_bugzilla)
Yes, this works for me in the contact editor. However, in the contact preview (when you just click the contact row and it renders in the bottom pane), there is still March 3 displayed.
Thanks for the feedback.

The contact preview in the bottom pane has never been broken for me; could you share the steps to reproduce this?
Also, when you said "March 3", I guess you meant "March 1st" instead? Or have you been testing with a different, problematic date?

The only way I can reproduce problems in the bottom pane is by entering 2000-02-29, closing the contact editor, re-opening it (without my patch) and clicking save (it will show and also save the wrong date 2000-03-01). After that, the wrong date seems to be permanently stored in the addressbook and will, of course, also render this way in the preview pane.
Comment on attachment 8592431 [details] [diff] [review]
Patch against comm-central trunk

>+  // get the year first, so that the following month/day
>+  // calculations can take leap years into account.
Nit: If you are ending a comment with a "." then you need to start the first word with an uppercase letter i.e. "Get" rather than "get"

Same for both files.

Otherwise looks good, r=me
Attachment #8592431 - Flags: review?(iann_bugzilla) → review+
Attachment #8592431 - Flags: review?(mconley) → review+
Thanks for the positive reviews, I've attached an updated patch with the comment spelling fixed and will leave it up to the committer which patch to use (which is why I'm not making the previous patch obsolete in this bug).

I guess it makes sense to wait on aceman's feedback in order to see if any other fixes are needed.
Flags: needinfo?(acelists)
Let's consider this simple fix for TB 38 (note removed FF flags as not relevant).
(In reply to Christian Hoffmann from comment #3)
> Thanks for the feedback.
> 
> The contact preview in the bottom pane has never been broken for me; could
> you share the steps to reproduce this?
> Also, when you said "March 3", I guess you meant "March 1st" instead? Or
> have you been testing with a different, problematic date?
Sorry, yes it is March 1st.

> The only way I can reproduce problems in the bottom pane is by entering
> 2000-02-29, closing the contact editor, re-opening it (without my patch) and
> clicking save (it will show and also save the wrong date 2000-03-01). After
> that, the wrong date seems to be permanently stored in the addressbook and
> will, of course, also render this way in the preview pane.
STR:
1. create a contact with date of birth set to 29th Feb 2000. Notice the year is not filled in. Close the card.
2. The bottom preview pane shows "Birthday: March 1st".
3. Edit the contact again, check birthday field. It is still at 29th Feb. (thanks to the patch, otherwise it would be 1st Mar here too).

My timezone on Linux is CEST (GMT+2).

(In reply to Christian Hoffmann from comment #5)
> Created attachment 8592454 [details] [diff] [review]
> Patch against comm-central trunk (with comment spelling fix)
> 
> Thanks for the positive reviews, I've attached an updated patch with the
> comment spelling fixed and will leave it up to the committer which patch to
> use (which is why I'm not making the previous patch obsolete in this bug).
It is usually not the committer's job to decide about patches. When the reviewer said what to do and you have done it, mark the older patch obsolete. Only the new one is valid and passed review.
Flags: needinfo?(acelists)
Attached patch Patch against comm-central (obsolete) — Splinter Review
aceman, Archaeopteryx -
Thanks for yesterday's explanations and the suggestions on IRC. Based on these information I have worked on an updated patch which should also catch these edge cases.

To avoid duplicate code between the preview pane and the contact editor, I had to move some code to abCommon.js.
Attachment #8592431 - Attachment is obsolete: true
Attachment #8592454 - Attachment is obsolete: true
Attachment #8593468 - Flags: review?(mconley)
Attachment #8593468 - Flags: review?(iann_bugzilla)
Assignee: nobody → christian
Status: NEW → ASSIGNED
Comment on attachment 8593468 [details] [diff] [review]
Patch against comm-central

>+/**
>+ * Validates the given year and returns it, if it looks sane.
>+ * Returns kDefaultYear (a leap year), if no valid date is given.
>+ * This ensures that month/day calculations still work.
>+ */
>+function sanitizeBirthdateYear(year) {
functions usually have arguments of the format aVariable, so in this case it should be aYear. Function name could be shortened to something like saneBirthYear or santizeBirthYear.
>+  return year && year < 10000 && year > 0 ? year : kDefaultYear;
>+}


>     /// setup the birthday information
>     var day = card.getProperty("BirthDay", null);
>     var month = card.getProperty("BirthMonth", null);
>     var year = card.getProperty("BirthYear", null);
>     var dateStr;
>     if (day > 0 && day < 32 && month > 0 && month < 13) {
>+      var saneYear = sanitizeBirthdateYear(year);
>+      var date = new Date(saneYear, month - 1, day);
As this is overwritten in the first part of the if statement, is it worth just having var date; here and moving the setting into the else part of the if statement? Also, as saneYear is only used the once, can it be inlined?

>       // if the year exists, just use Date.toLocaleString
>       if (year) {
>         // use UTC-based calculations to avoid off-by-one day
>         // due to time zone/dst discontinuity
>         date = new Date(Date.UTC(year, month - 1, day));
>         date.setUTCFullYear(year); // to handle two-digit years properly
>         dateStr = date.toLocaleDateString([], {timeZone: "UTC"});
>       }

Only f+ for the moment, as I would want to review the new patch.
Attachment #8593468 - Flags: review?(iann_bugzilla) → feedback+
Tested against Thunderbird comm-central trunk.
Attachment #8593468 - Attachment is obsolete: true
Attachment #8593468 - Flags: review?(mconley)
Attachment #8595161 - Flags: review?(mconley)
Attachment #8595161 - Flags: review?(iann_bugzilla)
Thanks for your review.

(In reply to Ian Neal from comment #9)
> functions usually have arguments of the format aVariable, so in this case it
> should be aYear. Function name could be shortened to something like
> saneBirthYear or santizeBirthYear.
Alright, changed these. I was used to longer names, but this is ok for me as well.

> As this is overwritten in the first part of the if statement, is it worth
> just having var date; here and moving the setting into the else part of the
> if statement?
Yes, did so now. Thought I'd not start refactoring code which was not directly related to the fix but rather "just sorrounding".

> Also, as saneYear is only used the once, can it be inlined?
I guess you meant the function call / eliminating the variable. Inlined now.
Comment on attachment 8595161 [details] [diff] [review]
Patch against comm-central

Magnus, if you get a chance could you look at this? We'd like to get this in a TB 38 beta soon.
Attachment #8595161 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8595161 [details] [diff] [review]
Patch against comm-central

Looks good to me
Attachment #8595161 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8595161 [details] [diff] [review]
Patch against comm-central

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

Looks good, thx! r=mkmelin
Attachment #8595161 - Flags: review?(mkmelin+mozilla)
Attachment #8595161 - Flags: review?(mconley)
Attachment #8595161 - Flags: review+
Comment on attachment 8595161 [details] [diff] [review]
Patch against comm-central

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

Perfect, works for me. No more 1st March when not intended. Thanks!
Attachment #8595161 - Flags: feedback+
Please format the commit message in the standard way in the future:
Bug NNN - Description. r=reviewer,reviewer
Thanks!
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 40.0
Comment on attachment 8595161 [details] [diff] [review]
Patch against comm-central

I probably won't take this in beta 4, but in beta 5.
Attachment #8595161 - Flags: approval-comm-beta?
Attachment #8595161 - Flags: approval-comm-aurora?
Comment on attachment 8595161 [details] [diff] [review]
Patch against comm-central

https://hg.mozilla.org/releases/comm-beta/rev/257c59adc0ec
https://hg.mozilla.org/releases/comm-aurora/rev/9cb911a699c9
Attachment #8595161 - Flags: approval-comm-beta?
Attachment #8595161 - Flags: approval-comm-beta+
Attachment #8595161 - Flags: approval-comm-aurora?
Attachment #8595161 - Flags: approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.