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

RESOLVED FIXED in 1.1 QE3 (26jun)

Status

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jsmith, Assigned: jlal)

Tracking

unspecified
1.1 QE3 (26jun)
ARM
Gonk (Firefox OS)
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(blocking-b2g:leo+, b2g18 verified, b2g-v1.1hd fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
blocking-b2g: --- → leo?

Updated

6 years ago
blocking-b2g: leo? → leo+
Target Milestone: --- → 1.1 QE3
(Assignee)

Comment 1

6 years ago
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.
blocking-b2g: leo+ → leo?
(Reporter)

Comment 2

6 years ago
(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.
Keywords: qawanted
QA Contact: jsmith
(Reporter)

Comment 3

6 years ago
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.
Keywords: qawanted
(Reporter)

Comment 4

6 years ago
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.
(Reporter)

Comment 5

6 years ago
This appears to be happening in the general case btw event even if you edit the title of the event.
(Assignee)

Comment 6

6 years ago
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.
Depends on: 836496
(Assignee)

Comment 7

6 years ago
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.
blocking-b2g: leo? → leo+
(Assignee)

Comment 8

6 years ago
Created attachment 760736 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10308

Pointer to Github pull-request
(Assignee)

Comment 9

6 years ago
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.
Attachment #760736 - Flags: review?(kgrandon)
Comment on attachment 760736 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/10308

Nice fix!
Attachment #760736 - Flags: review?(kgrandon) → review+
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).
Landed in master: https://github.com/mozilla-b2g/gaia/commit/1ec8a103933b084b31be5fbb1d457fafc41d681b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Reporter)

Updated

6 years ago
Keywords: verifyme
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
Flags: needinfo?(nobody)
(Reporter)

Updated

6 years ago
Flags: needinfo?(nobody) → needinfo?(jlal)
(Reporter)

Updated

6 years ago
Flags: in-moztrap?
(Assignee)

Comment 14

6 years ago
in v1-train: 10fb3219dd2502b1a1467dc900c5230363a0371c
status-b2g18: --- → fixed
Flags: needinfo?(jlal)
1.1hd: 10fb3219dd2502b1a1467dc900c5230363a0371c
status-b2g-v1.1hd: --- → fixed
(Reporter)

Updated

6 years ago
Depends on: 884627
(Reporter)

Updated

6 years ago
Depends on: 884636
(Reporter)

Comment 16

6 years ago
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?
status-b2g18: fixed → verified
Flags: needinfo?(jlal)
(Reporter)

Updated

6 years ago
Keywords: verifyme
Assignee: nobody → jlal
(Reporter)

Updated

6 years ago
No longer depends on: 884627
(Assignee)

Comment 17

6 years ago
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.
(Reporter)

Updated

6 years ago
No longer depends on: 884636
(Reporter)

Updated

6 years ago
Flags: needinfo?(jlal)
You need to log in before you can comment on or make changes to this bug.