Closed Bug 876794 Opened 7 years ago Closed 7 years ago
Editing an all-day Google calendar event sends the wrong alarm time information to Google
358 bytes, text/html
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.
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?
(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.
QA Contact: 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.
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.
This appears to be happening in the general case btw event even if you edit the title of the event.
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
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.
Pointer to Github pull-request
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
Closed: 7 years ago
Resolution: --- → FIXED
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
in v1-train: 10fb3219dd2502b1a1467dc900c5230363a0371c
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?
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.
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
Flags: in-moztrap? → in-moztrap+
You need to log in before you can comment on or make changes to this bug.