876794 - Editing an all-day Google calendar event sends the wrong alarm time information to Google

Status: RESOLVED FIXED

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

Description

[Jason Smith [:jsmith]]

Build: B2G 18 5/28
Device: Unagi

STR

1. Import a google calendar with some events (including an all-day event)
2. Edit the alarms on the all-day event & save
3. Check the result on the Google Calendar app on the web

Expected

The edits made should be seen on the server.

Actual

The edits on the server do show changes, but they are completely incorrect. Here's one example of changes I saw:

Before edit:

All-day event with a 10 minute, 20 minute email alarm notification

Edit on Client Side:

All-day event with a 1 day alarm

After edit on server:

All-day event has a 15 hour pop-up alarm, 440 minute email alarm, and 430 min email alarm.

Comment 1

[James Lal [:lightsofapollo]]

I think the confusion / bug here is we don't actually set an alarm for 1 day before (which would be the previous midnight) we set it for 9am in the previous day...

My stance has been whatever information we receive from the server is the absolute truth... Meaning if we get an alarm that should fire at midnight we will fire it then... In the same vein while we display "1 day before" what we really mean is 9am the day before so that is what is sent to the server (google).

Google then displays it "correctly" meaning what we sent it.

---

The only real fix is to violate the "server is the thuth" rule which I would not want as a client user.

Comment 2

[Jason Smith [:jsmith]]

(In reply to James Lal [:lightsofapollo] ( Back June 3rd ) from comment #1)
> I think the confusion / bug here is we don't actually set an alarm for 1 day
> before (which would be the previous midnight) we set it for 9am in the
> previous day...
> 
> My stance has been whatever information we receive from the server is the
> absolute truth... Meaning if we get an alarm that should fire at midnight we
> will fire it then... In the same vein while we display "1 day before" what
> we really mean is 9am the day before so that is what is sent to the server
> (google).
> 
> Google then displays it "correctly" meaning what we sent it.
> 
> ---
> 
> The only real fix is to violate the "server is the thuth" rule which I would
> not want as a client user.

That however doesn't address why the email alarm times changed.

9am might be okay then given that analysis.

Let me do some more testing here to investigate here. If I'm still able to reproduce the email problem, then that's a big problem here - we shouldn't flip around email alarm times. I'll also check the theory above too.

Comment 3

[Jason Smith [:jsmith]]

So I tested the following case:

1. Import a google calendar with an all-day event with a 10 min popup alarm, 10 min email alarm
2. Edit it to one day for the alarm
3. Check google calendar

Here's what I noticed:

* The event has it's timezone converted to GMT +00:00
* The email reminder changed from 10 min to 430 minutes
* The pop-up reminder changed from 10 min to 15 hours
* The timeframe of the event is now 7am of the current day to 7am of the following day

As such, I don't think I agree with the analysis given above. The problem here looks like when you are writing to Google's caldav API, you aren't including timezone information, which is likely throwing off all of the numbers we are seeing here.

Comment 4

[Jason Smith [:jsmith]]

More analysis btw on comment 3:

Notice that the timeframe changed 7 hours ahead with the timezone change to GMT +00:00. That also explains why we changed 10 minutes to 430 minutes - that's also an increase of 7 hours to reflect the timezone change.

Comment 5

[Jason Smith [:jsmith]]

This appears to be happening in the general case btw event even if you edit the title of the event.

Comment 6

[James Lal [:lightsofapollo]]

OK- I figured this out

- We are missing https://bugzilla.mozilla.org/show_bug.cgi?id=836496 which makes this problem less of an issue

- When google creates an "all day" event it uses absolute alarm times (a specific point in UTC time).

- We change the timezone of the event when editing all day events to UTC (in v1.1 bug 836496 fixes this)

This means when we change the date of the event (or at all in v1.1) the relative distance between the alarm (which is at a fixed time) is different then it was prior to the edit.
This results in the Google's UI changing from 10 minutes to something else.

This also means when we change the date of an event the "email alarm" times would not be altered (we assume relative time right now).

For example:

- Create all day event in google's UI
- event's email alarm will fire at day X
- edit the event's date in our UI

events date has changed but email alarm will still fire at the original date (X) rather then being relative / updated.

Comment 7

[James Lal [:lightsofapollo]]

To be clear- this is a case where the server is telling is one thing but meaning another... We can't apply a fix to all servers but we can detect when google (and only google) sends us a email alarm with a fixed time and update it accordingly.

Comment 8

[James Lal [:lightsofapollo]]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10308

Pointer to Github pull-request

Comment 9

[James Lal [:lightsofapollo]]

Comment on attachment 760736 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10308

This patch fixes the google but and allows us to support updating absolute alarm times in other cases. In addition this updates ICAL.js so we can leverage some minor new functionality / bug fixes.

Comment 10

[Kevin Grandon :kgrandon]

Comment on attachment 760736 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10308

Nice fix!

Comment 11

[Kevin Grandon :kgrandon]

Note to QA: If we follow the STR exactly, the alarm in google will now say "15 hours before", instead of 1 day before. This is because we fire our alarms the morning of (instead of at midnight).

Comment 12

[Kevin Grandon :kgrandon]

Landed in master: https://github.com/mozilla-b2g/gaia/commit/1ec8a103933b084b31be5fbb1d457fafc41d681b

Comment 13

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

I was not able to uplift this bug to v1-train.  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-train, 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-train
  git cherry-pick -x -m1 1ec8a103933b084b31be5fbb1d457fafc41d681b
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 14

[James Lal [:lightsofapollo]]

in v1-train: 10fb3219dd2502b1a1467dc900c5230363a0371c

Comment 15

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

1.1hd: 10fb3219dd2502b1a1467dc900c5230363a0371c

Comment 16

[Jason Smith [:jsmith]]

Tested this against the target STR & regression tested, but I guess I'm puzzled what this patch actually fixed. What I understand from comment 11 does make sense. However, I'm seeing two regressions that I think might have came from this patch (or if not, the other leo+ patch).

James - Can you clarify the patch intent here? And did this patch cause the regressions cited in bug 884627 and bug 884636?

Comment 17

[James Lal [:lightsofapollo]]

This patch fixes an issue where events have absolute alarm times (one specific date/time) rather then being relative to the event.

Previously when we updated the time of events with those absolute alarms we didn't update the absolute alarm times to be relative to the new date (which results in really weird email alarm times). This patch updates those absolute alarm times to be relative to the new start date.

Comment 18

[Montoya Clemmons (MClemmons)]

Four new test cases created to cover this bug.

https://moztrap.mozilla.org/manage/cases/?filter-id=9281

https://moztrap.mozilla.org/manage/cases/?filter-id=9282

https://moztrap.mozilla.org/manage/cases/?filter-id=9283

https://moztrap.mozilla.org/manage/cases/?filter-id=9284