855334 - (gaia-caldav2) [meta] Add support for Google Calendar CalDAV v2 prior to September 16th (they shutdown the old entrypoint!!!)

Status: RESOLVED FIXED

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

Description

[James Lal [:lightsofapollo]]

Google is not shutting down CalDAV support but they are adding a new authentication layer which we don't support yet.

We must fill out the form (see herehttps://developers.google.com/google-apps/calendar/caldav) (we are in the process of this now) and once we obtain the documentation we can implement changes required for the new Google entrypoint.

Comment 1

[James Lal [:lightsofapollo]]

To be clear... the calendar will absolutely break on google if we don't fix this...

Comment 2

[Jason Smith [:jsmith]]

For context to those triaging - read http://googleblog.blogspot.co.uk/2013/03/a-second-spring-of-cleaning.html for clarity. We don't fix this, we lose google calendar import support for v1 after Sept 16th on FF OS devices.

Comment 3

[Chris Lee [:clee]]

We need to block on this per comment 1.  

Triage team, please mark blocking.  Thanks.

Comment 4

[Daniel Coloma:dcoloma]

Blocking, who should take care of this one?

Comment 5

[James Lal [:lightsofapollo]]

I will take this one...

We need to signup with Google first ( sent a email to :clee with the details and someone in product will follow up). The work mostly involves integrating their oauth2 layer (or so I was told)... I suspect this is a very small amount of work but we need to get the new entrypoint & docs to start.

Comment 6

[Alex Keybl [:akeybl]]

Amelie - can you help us understand the latest we could still make this small change, given it's dependent upon an external process with Google?

Kevin - can you add our other partner contacts to verify the same?

Comment 7

[Andrew Overholt [:overholt]]

(In reply to James Lal [:lightsofapollo] from comment #5)
> We need to signup with Google first ( sent a email to :clee with the details
> and someone in product will follow up)

For those playing along at home, I spoke with clee and James is going to do the follow up here :)

Comment 8

[Amelie Kong]

(In reply to Alex Keybl [:akeybl] from comment #6)
> Amelie - can you help us understand the latest we could still make this
> small change, given it's dependent upon an external process with Google?
> 
> Kevin - can you add our other partner contacts to verify the same?

Better to have it before June 20th.

Comment 9

[Kai-Chih Hu [:khu]]

[e-mail], could we have your comments here? Thanks!

Comment 10

[Jason Smith [:jsmith]]

Just noting to those that are tracking stuff - we're blocked here until we get an update from Google. Which likely isn't happening this week.

Comment 11

[James Lal [:lightsofapollo]]

OK, I have the Google document detailing how to setup the caldav account. The issue is we must supply a Google API Key which provides likely need to supply themselves.

My working theory is to create a build step where a JSON file must be provided that contains the API Key details. We will need to figure out the best way to get QA testing with our mozilla keys for now. I am not 100% sure yet but we may also need UX input depending on how we authenticate. I will start implementing the technical side now and post more details soon.

Comment 12

[James Lal [:lightsofapollo]]

Sorry for the misspelling.

"...is we must supply a Google API Key which __partners__ likely need to supply themselves."

Comment 13

[James Lal [:lightsofapollo]]

We will likely need an oauth2 flow for this (which is unfortunate) as it may require user input to re-authenticate more frequently (not sure how often) then just the first login =/.

We can likely avoid new strings/UX issues but this is going to be fairly jarring for the end-user I suspect. I will post some screenshots soon.

Comment 14

[James Lal [:lightsofapollo]]

Another concern is the "refresh" tokens... periodic sync requires no user input in the typical CalDAV case but we _may_ need to prompt the user if we are offline for some period of time (looks like an hour). To avoid this we can hit the google oauth server for token refreshes while online but that only mitigates part of the problem...

Comment 15

[Jason Smith [:jsmith]]

The more discussion goes on this about the complexity, the more I'm thinking that the testing requirements on this bug is almost going to require a full blown test run with the google calendar integration again. Am I right?

Comment 16

[Alex Keybl [:akeybl]]

(In reply to Jason Smith [:jsmith] from comment #15)
> The more discussion goes on this about the complexity, the more I'm thinking
> that the testing requirements on this bug is almost going to require a full
> blown test run with the google calendar integration again. Am I right?

I'd say so.

We're milestoning this for 5/10 since this work needs to be frontloaded to accommodate for the above test run and cert cycles.

Comment 17

[James Lal [:lightsofapollo]]

OK, so we have a number of different issues... I think it makes sense to turn this into a metabug so we can show more progress here and attempt to front-load the part we need to test heavily first.

-  oAuth 2.0 authentication flow to get api key / allowing CalDAV to interact over 
oAuth 2

-  handling cases where our refresh tokens expire after the user signs up (we need to have them re-authenticate again)

-  we need to add a build step so third-parties can declare their own google API keys.

---

What we can do is land the initial work with a temporary API key so we can immediately start testing with partners while they get their own API keys and then delete the temp one prior to launch. I am still operating under the assumption everyone will need their own API key.

Comment 18

[Rob MacDonald [:robmac]]

Quick update on UX status - James, Casey and I met this afternoon and here's the summary:

- The flow for happy cases is similar to gmail contacts import and, as a result, involves minimal new UI

- If the calendar fails to sync, due to a token expiry, password change or otherwise, we need to design exception cases. We discussed showing error notifications in the utility tray and within the app itself. Within the app, we would have a one time notification and an error icon indicator within the drawer and settings. Tapping on this would bring the user back to the third party website where they can re-authenticate.

- If the user re-authenticates using another account with the same service, the existing calendar data will be replaced with the data for the new account. 

- James is targeting this for completion on May 10 -  This means the UX spec and visuals are required by May 3.

I've created a needsinfo for Przemek to confirm his availability next week.

James, let me know if I've missed anything.

- Rob

Comment 19

[Alex Keybl [:akeybl]]

(In reply to Rob MacDonald [:robmac] from comment #18)
> Quick update on UX status - James, Casey and I met this afternoon and here's
> the summary:
> 
> - The flow for happy cases is similar to gmail contacts import and, as a
> result, involves minimal new UI

Marking with late-l10n so that others are aware of the new UX. We should be looking for the *absolute minimum* code/UX changes and string additions here. Make this as simple as possible.

Comment 20

[Jason Smith [:jsmith]]

(In reply to Alex Keybl [:akeybl] from comment #19)
> (In reply to Rob MacDonald [:robmac] from comment #18)
> > Quick update on UX status - James, Casey and I met this afternoon and here's
> > the summary:
> > 
> > - The flow for happy cases is similar to gmail contacts import and, as a
> > result, involves minimal new UI
> 
> Marking with late-l10n so that others are aware of the new UX. We should be
> looking for the *absolute minimum* code/UX changes and string additions
> here. Make this as simple as possible.

Completely agree. When UX gets an initial spec up, let's get a spec review here by a l10n reviewer to check for if they are okay with the strings being introduced.

Comment 21

[Stephany Wilkes]

So next steps for UX are:
* Create new UX (and VxD?) specs;
* There's a possibility of a new flow here, and hence new strings; let's get those to 110n ASAP;
* Confirm Przemek's availability to work on this with the May 3 deadline in mind. Let me know if this is a Rob+Przemek or if Przemek can take point on this (more solo).

Comment 22

[James Lal [:lightsofapollo]]

Keep in mind the code timelines do _not_ include the time it may take for partners to get their own API keys... We can reasonably say that if it works for our API keys it will work for theirs but that is not a guarantee.

Comment 23

[James Lal [:lightsofapollo]]

Attachment: Begining of OAuth 2 flow for calendar

Comment 24

[James Lal [:lightsofapollo]]

Turning this into a metabug we have multiple steps here.

Comment 25

[James Lal [:lightsofapollo]]

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

Pointer to Github pull-request

Comment 26

[James Lal [:lightsofapollo]]

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

targeted wrong bug #

Comment 27

[Stephany Wilkes]

Stephany Wilkes changed story state to started in Pivotal Tracker

Comment 28

[Rob MacDonald [:robmac]]

Attachment: Draft - Calendar oauth exception handling (for discussion)

Posting draft flows for team feedback. I look forward to everyone's comments.

Also, can we link from the utility tray to the account settings view for the account that is not working?

Comment 29

[Stephany Wilkes]

Rob's spec it attached; please review.

Clearing needinfo for Przemek: per Rob and Przemek, VxD is no longer needed. The icon required already existed and Rob has it.

Comment 30

[Jason Smith [:jsmith]]

Comment on attachment 745245 [details]
Draft - Calendar oauth exception handling (for discussion)

Putting feedback on myself, James, and Kevin to provide feedback on the spec.

Comment 31

[Jason Smith [:jsmith]]

Comment on attachment 745245 [details]
Draft - Calendar oauth exception handling (for discussion)

My feedback comments:

Generally makes sense to me with some scope and l10n refinement. We need to be really careful about scope here vs. balancing UX concerns vs. balancing l10n risk. Some of these UX concepts explored in the spec are great ideas, but I am concerned about the quality & l10n risk this poses given how late we're making changes here in 1.01. I'm going go into more detail below, but my general suggestion is that we should only introduce new UX with new strings when it's absolutely necessary and reuse existing UX in other cases to reduce schedule vs. quality vs. l10n risk.

Page 5 Comments

I think the sync error notification is a good idea in it's concept, but I think we should cut it from the 1.01 scope since 1) it's new UX with l10n strings and a custom button, so there's l10n and quality risk there 2) there's existing UX we can use here for sync failures to reduce change risk.

For the service auth view, I believe that the window.open UI design states that we should use an X, not a back button there for doing off-origin authentication. One thing I'm concerned about here that's outside of the UX scope is that we're relying on consuming Google web-based content in this workflow, which has been known to have a large amount of web compatibility problems on FF OS. If there's ways we can reduce our exposure to Google's web content, then we should do that if possible, as that avoid getting hit by the large array of compatibility problems Google's sites have on FF OS.

Page 6 Comments

I think the accounts drawer warning icon concept is a good idea, but not blocking for 1.01 in terms of UX needed here. We might need to keep this out of scope for 1.01 as a result.

The calendar settings warning icon concept is a good idea, but not blocking for 1.01 in terms of uX needed here. We might need to keep this out of scope for 1.01 as a result.

The account settings screen appears to be an entirely new UX. Specific comments on this design are:

	* I think to avoid introducing a new string, let's use "Remove local data" instead of "Remove account"
	* The manage account, display name, and the associated text box feels awkward and a forced feature to take up space. See the page 7 comments for more discussion on this.

Page 7 Comments

The account settings UI feels awkward anyways for including the display name concept - it's almost like we're forcing a feature here with low value to take up space. I would rather suggest cutting that feature and keeping around a status for if we're synced or not and allowing for sign in if need be (remember, the changed password use case can happen, so we shouldn't force the user to enter an error state to allow them to resign in if they already know they need to sign in again).

Page 11 Comments

I'd keep the concepts of this page out of scope for 1.01.

Comment 32

[Axel Hecht [:Pike]]

Comment on attachment 745245 [details]
Draft - Calendar oauth exception handling (for discussion)

I'd prefer to comment on the final patch, once there is one and it does actually need new strings. Cancelling feedback request for now.

Comment 33

[Rob MacDonald [:robmac]]

Attachment: Draft - Calendar oauth exception handling Version 0.2

Updated based on current feedback - details to be added within the bug comments.

Comment 34

[Rob MacDonald [:robmac]]

(In reply to Jason Smith [:jsmith] from comment #31)
> Comment on attachment 745245 [details]
> Draft - Calendar oauth exception handling (for discussion)
> 
> Page 5 Comments
> 
> For the service auth view, I believe that the window.open UI design states
> that we should use an X, not a back button there for doing off-origin
> authentication. 

Thanks! Fixed in spec.

> 	* I think to avoid introducing a new string, let's use "Remove local data"
> instead of "Remove account"

"Remove local data" isn't clear and suggests that the account will be retained but the data will be removed. The term "Remove account" is used in the title of the dialog that is triggered when tapping this button. If possible, I'd like to use this string in the button as it's much clearer.

> 	* The manage account, display name, and the associated text box feels
> awkward and a forced feature to take up space. See the page 7 comments for
> more discussion on this.

Agreed. Replaced with sync status as per your suggestion.

I've posted a revised spec based on this feedback (attachment 746260 [details]). The updated version also includes proposed utility tray notifications. Thanks again and let me know if you have any additional comments.

Comment 35

[Stephany Wilkes]

Rob Macdonald changed story state to delivered in Pivotal Tracker

Comment 36

[Jason Smith [:jsmith]]

(In reply to Rob MacDonald [:robmac] from comment #34)
> (In reply to Jason Smith [:jsmith] from comment #31)
> 
> > 	* I think to avoid introducing a new string, let's use "Remove local data"
> > instead of "Remove account"
> 
> "Remove local data" isn't clear and suggests that the account will be
> retained but the data will be removed. The term "Remove account" is used in
> the title of the dialog that is triggered when tapping this button. If
> possible, I'd like to use this string in the button as it's much clearer.

Right, I agree here in it's concept. I'm just wondering if we need to be careful switching the string here, given that it's going to cause us to deliver another translated string as a result.

> 
> > 	* The manage account, display name, and the associated text box feels
> > awkward and a forced feature to take up space. See the page 7 comments for
> > more discussion on this.
> 
> Agreed. Replaced with sync status as per your suggestion.
> 
> I've posted a revised spec based on this feedback (attachment 746260 [details]
> [details]). The updated version also includes proposed utility tray
> notifications. Thanks again and let me know if you have any additional
> comments.

Looks fine overall.

Comment 37

[James Lal [:lightsofapollo]]

Jason- we don't use window.open here yet... once/if we can redirect we will use that functionality. We basically re-implement basic browser functionality (using mozbrowser).

Comment 38

[James Lal [:lightsofapollo]]

@robmac:

on page 7 the steps 6,7 don't seem particularly useful? (and add strings) Can we simply reuse the existing "we are setting up your account" flow and then redirect them back to the account list?

^ same with page 8 #4 #5

Icons + unable to sync should make it fairly clear when things fail... There is also the possibility of rolling failures ( basically we can never authenticate ) which is the worst case... Not sure how we would handle that or if we should invest for that particular error.

Comment 39

[Rob MacDonald [:robmac]]

Attachment: Draft - Calendar oauth exception handling Version 0.3

Updated based on feedback from James and others.

- Modified final two views of calendar settings and flow to remove sync status and maintain consistency with current implementation.

Comment 40

[Rob MacDonald [:robmac]]

The warning icon that was used for email is available here:

https://bug813404.bugzilla.mozilla.org/attachment.cgi?id=690871

(It's part of bug 813404.)

- Rob

Comment 41

[Stephany Wilkes]

Stephany Wilkes changed story state to accepted in Pivotal Tracker

Comment 42

[Rob MacDonald [:robmac]]

Please note use of string "Unable to sync". Flagging Stas for L10N.

- Rob

Comment 43

[Axel Hecht [:Pike]]

Please use needinfo if you needinfo, or if there's something else actionable. Which also implies that once there is something actionable, you should explicitly tell stas or I, as otherwise it may very well get lost in our bugmail.

Comment 44

[James Lal [:lightsofapollo]]

Update-

Integrated up to first error flow (which landed in master) on my testing version of v1.0.1 https://github.com/lightsofapollo/gaia/tree/calendar-integration-v1.0.1

We did early testing for success flows and that part looks good... I still would like to spend some time re-testing everything for the error flow on v1.0.1 (tomorrow morning). I have just gotten it to the point where the conflicts are resolved and tests pass.

Comment 45

[James Lal [:lightsofapollo]]

OK- I tested manually (in addition to the passing unit test suite for v1.0.1) throughout the day... All the basic operations worked for me (in combination) so I uplifted so others can begin testing (on v1.0.1)... Its important to note that we have the following new strings:

modify-account-unable-to-sync=Unable to sync
modify-account-sign-in=Sign in
notification-error-sync-title=Calendar sync error
notification-error-sync-description=Tap for more info

@Pike- We tried to keep this to a minimum and given the new flows I think this is fairly decent (but obviously > 0 ;) ).

I will leave this open so we can add any new bugs we find and for any final UX/string changes we may need as a result of partner testing.

Comment 46

[Jason Smith [:jsmith]]

James - Can you do the remaining uplifts in the bug dependencies here for b2g18? I'd like to make sure our 1.1 test run captures testing on this feature as well.

Comment 47

[James Lal [:lightsofapollo]]

Yep- I will uplift tomorrow morning

Comment 48

[Jason Smith [:jsmith]]

Bug bash notes for reference for testing here - https://etherpad.mozilla.org/gaia-calendar-oauth-bug-bash

Comment 49

[Jason Smith [:jsmith]]

Testing and development work is finished here. Closing.