966516 - [B2G][Calender]Changing the month in an event doesn't update to the number associated with that month

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Calendar, defect)

Description

[Andrew Stole [:astole]]

Attachment: Video

Certain months update to the incorrect number that is associated with it when creating an event.

Repro Steps:
1) Updated Buri to BuildID: 20140131095418
2) Open calender app
3) Create a new event
4) Change the month of the event to February and press 'OK'

Actual:
The number representing the month is incorrect.

Expected:
The number corresponds to the correct month.

Environmental Variables:
Device: Buri v1.4 M-C Mozilla RIL
BuildID: 20140131095418
Gaia: aedd5c9636f305d4433491056a0ca984dfb859b1
Gecko: 735a648bca0d
Version: 29.0a1
Firmware Version: V1.2-device.cfg

Notes:
1. This does not occur for all months but it does occur for multiple months, not just February.

Repro frequency: 3/3, 100%
See attached: Video, Screenshot

Comment 1

[Andrew Stole [:astole]]

Attachment: Screenshot

Comment 2

[Andrew Stole [:astole]]

This issue also occurs on the latest v1.3, v1.2, and v1.1

v1.3
Environmental Variables:
Device: Buri v1.3 Mozilla RIL)
BuildID: 20140131004001
Gaia: 0ddcd8da5bfe1b48c73502ef29220e92f2db6b73
Gecko: 32e45047b663
Version: 28.0a2
Firmware Version: V1.2-device.cfg

v1.2
Environmental Variables:
Device: Buri v1.2 Mozilla RIL
BuildID: 20140131004004
Gaia: 539a25e1887b902b8b25038c547048e691bd97f6
Gecko: f9f469d5d1e1
Version: 26.0
Firmware Version: V1.2-device.cfg

v1.1
Environmental Variables:
Device: Buri v1.1 Mozilla RIL
BuildID: 20140130041201
Gaia: c434fe9a0e823029796805e141cfa983cda2d246
Gecko: 1b24a4178317
Version: 18.0
Firmware Version: V1.2-device.cfg

Comment 3

[Jason Smith [:jsmith]]

Probably a date picker bug.

Comment 4

[Dylan Oliver [:doliver]]

(In reply to Andrew Stole from comment #2)
> This issue also occurs on the latest v1.3, v1.2, and v1.1
> 

It's hard to believe it's been around for very long -- has there been recent date picker work that got ported back to those branches? This is very easy to reproduce on 1.4. And if this is on 1.3 too, we should figure out what's happening. Miller, can you start the investigation?

Comment 5

[Jason Smith [:jsmith]]

(In reply to Dylan Oliver [:doliver] from comment #4)
> (In reply to Andrew Stole from comment #2)
> > This issue also occurs on the latest v1.3, v1.2, and v1.1
> > 
> 
> It's hard to believe it's been around for very long -- has there been recent
> date picker work that got ported back to those branches? This is very easy
> to reproduce on 1.4. And if this is on 1.3 too, we should figure out what's
> happening. Miller, can you start the investigation?

I think what's likely causing this is from the fact that the user scrolled part way to March & then the picker scrolled back to February. The user then submitted the date picker result, which ended up registering March, instead of February.

Comment 6

[Dylan Oliver [:doliver]]

(In reply to Jason Smith [:jsmith] from comment #5)
> I think what's likely causing this is from the fact that the user scrolled
> part way to March & then the picker scrolled back to February. The user then
> submitted the date picker result, which ended up registering March, instead
> of February.

I think it's less complicated than that. If I pick any date in June, it displays July in the event window. Seems to affect Feb, Apr, Jun, Sep & Nov.

Comment 7

[Jason Smith [:jsmith]]

I'm puzzled why this isn't a regression then. Can clarify exacting what's been on 1.1?

Comment 8

[Miller Medeiros [:millermedeiros]]

I think nobody noticed this before because I was only able to reproduce it when the system date is Jan 29, 30 & 31.

So the steps to reproduce are:

 1. set the system date to be January 29, 30 or 31 of 2014.
 2. open calendar app and create a new event.
 3. tap the date picker (doesn't matter if it's the start or end date).
 4. slide the month picker to "February" (which will snap the day to 28).
 5. press "ok".
 6. the date will be displayed as "03/28/2014" instead of "02/28/2014"

I will investigate it further, write tests for it, understand why this happen and try to come up with a fix.

PS: this happens every time the current day is higher than the maximum day of next month (current day is 31, next month have 30 days).

Comment 9

[Miller Medeiros [:millermedeiros]]

I spent whole day trying to write an integration test for this bug (failing test, before starting development) but it was not possible. Can't set the system date through the settings app on marionette (it is tied to the host date). I also tried to access the `navigator.mozTime.set()` directly but `mozTime` is only available under the "chrome" scope (`client.scope({context:'chrome'})`) and calling it doesn't actually update the value (does not throw error, but have no effect).

I guess the solution is to mock the Date object and check how the date picker is implemented, and only write unit tests for it; and do integration tests manually during development (and hope for the better).

I did not had time to check what is causing the problem (if it's on Calendar app or SpinDatePicker), did not want to fix the problem before having a failing test.

Comment 10

[Matthew Vaughan]

(In reply to Jason Smith [:jsmith] from comment #7)
> I'm puzzled why this isn't a regression then. Can clarify exacting what's
> been on 1.1?

In v1.1, the result is what is seen in 1.2, 1.3, and 1.4, and what is seen in the attached screenshot. If I select February as the new date for example, it will show '3' instead of '2'; if I pick April as the new date, it will show '5' instead of '4'; etc.

- Buri 1.1 Build -
Device: Buri v1.1 MOZ RIL
BuildID: 20140130041201
Gaia: c434fe9a0e823029796805e141cfa983cda2d246
Gecko: 1b24a4178317
Version: 18.0
Firmware Version: V1.2-device.cfg

Comment 11

[Preeti Raghunath(:Preeti)]

1.3+ for incorrect date

Comment 12

[Miller Medeiros [:millermedeiros]]

I'm 100% sure the bug is on the Calendar itself, in fact I just fixed it locally. I should provide a patch later today or early tomorrow, just need to clean my tests first. It was caused by some premature optimization on the "calendar/js/views/modify_event.js" specifically on the `_updateDateTimeLocale` method (on the `input` event listener).

The previous code was calling `Date#setDate` and `Date#setMonth` individually, which causes the date to "overflow" to next month. Eg:

  var d = new Date(2013,0,31);
  d.setMonth(1);
  // > 2014-03-03
  d.setDate(30);
  // > 2014-03-30
  d.setMonth(1);
  // > 2014-03-02

so as you can see, setting the month and date individually is very error prone, unless you reset the day to be smaller than 28 before making the changes (ideally you just set it to 1). Or you can simply pass the whole date to the constructor (which is way simpler). Both options will fix the bug.

The main issue is that there is no way to write integration tests for it. So I'm planning to land only with unit tests.

Comment 13

[Miller Medeiros [:millermedeiros]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15957

I only wrote unit tests, since marionette tests won't allow us to set the system date. The test suite is disabled because of Bug 917537; but I executed unit tests locally and did manual test on the device and the patch should be working properly.

Comment 14

[Kai-Chih Hu [:khu]]

Can we get ETA to fix this bug? Thank you.

Comment 15

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8370490 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15957

addin :kgrandon as additional reviewer since :gaye is at a conference. But both are busy and might not have enough time to do it today.

Comment 16

[Kevin Grandon :kgrandon]

Comment on attachment 8370490 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15957

I am adding James to the review here as well. I think this is mostly fine, but I don't want to R+ right now because I'm not really comfortable with mocking the entire Date object. If one of James/Gareth is ok with it - then we can go with it.

I think it would be much better to use sinon and create a stub for window.Date, or do sinon.useFakeTimers which is supposed to work fairly well for the date object. I'll see if I can provide some example code and attach it to github.

Comment 17

[Kevin Grandon :kgrandon]

Comment on attachment 8370490 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15957

Clearing review for now. More info:

The function you are changing, _updateDateTimeLocale, appears to already have a test case here: https://github.com/mozilla-b2g/gaia/blob/master/apps/calendar/test/unit/views/modify_event_test.js#L354

My recommendation would be to simply copy or add additional test cases to the existing test, utilizing the problematic dates. If you want to abstract into smaller helper functions, that's fine too - but I would suggest leveraging the same style of testing that we already are for this piece of code.

Comment 18

[Miller Medeiros [:millermedeiros]]

(In reply to Kevin Grandon :kgrandon from comment #16)
> Comment on attachment 8370490 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15957
> 
> I am adding James to the review here as well. I think this is mostly fine,
> but I don't want to R+ right now because I'm not really comfortable with
> mocking the entire Date object. If one of James/Gareth is ok with it - then
> we can go with it.
> 
> I think it would be much better to use sinon and create a stub for
> window.Date, or do sinon.useFakeTimers which is supposed to work fairly well
> for the date object. I'll see if I can provide some example code and attach
> it to github.

the code for `sinon.useFakeTimers` is very similar to what I did: https://github.com/cjohansen/Sinon.JS/blob/master/lib/sinon/util/fake_timers.js#L238-L301 - since project was not using sinon anywhere, and I saw some other "mocks" on the tests I decided to follow same style and just create a helper for what I needed.

Comment 19

[Miller Medeiros [:millermedeiros]]

(In reply to Kevin Grandon :kgrandon from comment #17)
> Comment on attachment 8370490 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15957
> 
> Clearing review for now. More info:
> 
> The function you are changing, _updateDateTimeLocale, appears to already
> have a test case here:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/calendar/test/unit/
> views/modify_event_test.js#L354
> 
> My recommendation would be to simply copy or add additional test cases to
> the existing test, utilizing the problematic dates. If you want to abstract
> into smaller helper functions, that's fine too - but I would suggest
> leveraging the same style of testing that we already are for this piece of
> code.

the existing test won't work. - that's what I tried initially, but I wasn't able to reproduce the error (since the previous test only checks the "setup" and not the event listener). The old test is only good to confirm that the refactoring did not break the initialization behavior (before user changes the value).

Comment 20

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8370490 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15957

LGTM but let's use sinon.useFakeTimers so we don't have to maintain this mock date.

Comment 21

[Miller Medeiros [:millermedeiros]]

Comment on attachment 8370490 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15957

was not aware that Sinon was already available on the tests, otherwise I wouldn't create the mock_date.js file :D, and I agree that it's a better idea. Just updated the pull request, thanks for the reviews!

Comment 22

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8370490 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/15957

Thanks Miller! Nice job!

Comment 23

[Miller Medeiros [:millermedeiros]]

merged into master, thanks for the reviews! https://github.com/mozilla-b2g/gaia/commit/7dd74e8ca3c444daa8ad724c22aa212cacbe8fb9

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x -m1 7dd74e8ca3c444daa8ad724c22aa212cacbe8fb9
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 25

[Miller Medeiros [:millermedeiros]]

(In reply to John Ford [:jhford] -- please use 'needinfo?' instead of a CC from comment #24)
> I was not able to uplift this bug to v1.3.  If this bug has dependencies
> which are not marked in this bug, please comment on this bug.  If this bug
> depends on patches that aren't approved for v1.3, we need to re-evaluate the
> approval.  Otherwise, if this is just a merge conflict, you might be able to
> resolve it with:

Hi John, it was a simple merge conflict, just needed to `git rm apps/calendar/test/unit/.jshintrc`. Pushed to v1.3 branch: https://github.com/mozilla-b2g/gaia/commit/464c279118985455d3c8af2f63feaa9ec96fe6e5