Google & Microsoft calendar API integration #3340

damencho · 2018-08-02T23:54:16Z

No description provided.

For the web part it just adds new property to enable/disable calendar web integration, disabled by default.

virtuacoplenny · 2018-08-07T15:53:26Z

package.json

    "jwt-decode": "2.2.0",
    "lib-jitsi-meet": "github:jitsi/lib-jitsi-meet#2be752fc88ff71e454c6b9178b21a33b59c53f41",
    "lodash": "4.17.4",
+    "@microsoft/microsoft-graph-client": "1.1.0",


Did you manually add this dependency? I think if you added it using npm's --save it would have been added with the other packages that start with @.

Yes, manually. I see. I will move it.

virtuacoplenny · 2018-08-07T15:54:41Z

react/features/calendar-sync/actions.js

+            });
+
+        if (!api) {
+            return Promise.resolve();


Should this be Promise.reject instead of resolving?

virtuacoplenny · 2018-08-07T16:00:21Z

react/features/calendar-sync/functions.web.js

    }
+
+    return calendarIntegrationInstance;


I'm unsure how I feel about the calendarIntegration Instance being stored in this file. I think it's fine but I also feel it might be something that could be stored in the redux state instead or held onto by some singleton.

virtuacoplenny · 2018-08-07T16:01:57Z

react/features/calendar-sync/middleware.js

@@ -79,6 +80,11 @@ StateListenerRegistry.register(
    /* listener */ (googleAPIState, { dispatch }) => {
        dispatch(setCalendarAPIState(googleAPIState));
    });
+StateListenerRegistry.register(
+    /* selector */ state => state['features/google-api'].profileEmail,


It feels off to me that there is now duplicated state. I think you mentioned you weren't sure about the whole profileEmail thing. Why does the same state need to be in calendar and google api? Could the components needing the email look in multiple places for it instead, maybe through a selector/helper that knows where to look in redux?

Or maybe they're separate enough concerns to keep the two separate state entries?

virtuacoplenny · 2018-08-07T16:09:58Z

react/features/calendar-sync/web/googleCalendar.js

-    init(dispatch: Function, getState: Function): Promise<void> {
-        if (getState()['features/calendar-sync'].apiState !== 0) {
+    init(): Promise<void> {
+        if (this._getState()['features/calendar-sync'].apiState !== 0) {


btw, the 0 should probably be the constant for the google api state.

virtuacoplenny · 2018-08-07T16:12:55Z

react/features/calendar-sync/web/microsoftCalendar.js

+
+            // not authorized, skip
+            if (!getState()['features/calendar-sync'].msAuthState) {
+                return Promise.resolve();


Would a reject be better here?

virtuacoplenny · 2018-08-07T16:27:41Z

react/features/calendar-sync/actions.js

+}
+
+/**
+ * Prompts the participant to sign in..


Double period. I'm blocking this PR no matter what now. I dream of a world where there are no more double periods.

virtuacoplenny · 2018-08-07T16:28:58Z

react/features/calendar-sync/actions.js

+ */
+export function signIn() {
+    return (dispatch: Dispatch<*>, getState: Function): Promise<*> => {
+        const calendarType = getState()['features/calendar-sync'].calendarType;


nit: there's a preference to declare variables with destructuring where preferable. So const { calendarType } = getState()['features/calendar-sync']. I guess there's no linter enforcement for that.

virtuacoplenny · 2018-08-07T16:30:20Z

react/features/calendar-sync/constants.js

+/**
+ * The max number of events to fetch from the calendar.
+ */
+export const MAX_LIST_LENGTH = 10;


Maybe grouping these constants together in an object would be consistent with the other constants? Also, what does ther -1 mean for FETCH_START_DAYS?

Nevermind, these were always separated. I'd prefer not changing existing code as much as possible for now. Any comment requesting change to existing logic you should just say no.

virtuacoplenny · 2018-08-07T16:34:52Z

react/features/calendar-sync/functions.any.js

+    }
+
+    return null;
+}


All these helpers were pre-existing but have been moved around with no changes?

Yes, there was only one change I made. 7f8a1e2#diff-e626ccf6b0437daab4a769da852e453dR24
It was executing one unnecessary fetch every time because of comparing arrays, at least that what I saw on mobile, not sure whether that array === array is working differently on mobile.

I see. So instead of comparing array values it was comparing if they're pointers to the same array.

virtuacoplenny · 2018-08-07T16:38:52Z

react/features/calendar-sync/functions.web.js

+ * otherwise, {@code false}.
+ */
+export function _isCalendarEnabled() {
+    return config.enableCalendarIntegration === true;


One thing to look out for, if possible, is it is preferred to get config from redux.

I was not sure how to do that in the constants, as this was the way it was implemented for mobile, so I didn't want to change that logic.

virtuacoplenny · 2018-08-07T16:54:07Z

react/features/calendar-sync/web/microsoftCalendar.js

+        /* eslint-enable max-params */
+
+        if (this._isSignedIn()) {
+            this._dispatch(setCalendarAPIState(CALENDAR_API_STATES.SIGNED_IN));


Why does a getCalendarEntries function dispatch being signed in?

If you have token that is not expired and you reload the page, this will set the correct state of the calendar-sync that it is signed in, have a correct token ... Maybe this state should be done in one place when we init the api or when we construct the MicrosoftCalendarApi object ..., WDYT?

The fact is that currently when you load the welcome page it calls _fetchCalendarEntries which end up calling getCalendarEntries and I was using that to set the correct state of the feature ...

virtuacoplenny · 2018-08-07T16:56:45Z

react/features/calendar-sync/web/microsoftCalendar.js

+            tokenExpires: expires.getTime(),
+            idToken
+        }));
+        this._dispatch(setCalendarProfileEmail(payload.preferred_username));


I think this function could be more concise by not doing any dispatching and instead having the thing that uses the result of the function doing the dispatching. Right now this token not only gets valid token parts but also updates calendar auth state and username, when I think it doesn't have to.

Ok, I will change it, thanks. The caller already dispatches signed_id state ... so it should also dispatches and the rest ....

virtuacoplenny · 2018-08-07T16:58:49Z

react/features/calendar-sync/web/microsoftCalendar.js

@@ -0,0 +1,492 @@
+/* @flow */


Are token expiration and renewal handled yet?

Nope, this is not handled yet.

virtuacoplenny · 2018-08-07T17:00:09Z

react/features/google-api/constants.js

 *
 * @type {Array<string>}
 */
 export const GOOGLE_API_SCOPES = [
-    'https://www.googleapis.com/auth/youtube.readonly'
+    'https://www.googleapis.com/auth/youtube.readonly',
+    'https://www.googleapis.com/auth/calendar'


Are there any changes needed to deployed applications to get calendar permissions?

I think so, you need to just enable that API. We should take care of that about meet.jit.si. I was also thinking of dropping a simple README with links to Google and Microsoft dashboard how to enable these.

virtuacoplenny · 2018-08-07T17:03:46Z

react/features/google-api/googleApi.js

+                    endDate.setDate(endDate.getDate() + fetchEndDays);
+
+                    return this._getGoogleApiClient()
+                        .client.calendar.events.list({


Did you look at the google api request batching? I'm wondering if it's super easy.

You mean to get rid of some of the Promises part?

No, to make it so there aren't multiple requests going out at once and instead are batched into one request. Now I think we should punt on it considering the breadth of this PR.

virtuacoplenny · 2018-08-07T17:28:14Z

react/features/calendar-sync/web/microsoftCalendar.js

+
+                const query = client.api('/me/calendars');
+
+                query.get((error, response) => {


I read this in the microsoft-graph-client readme: The actions(.get(), .put(), etc.) accept a callback or don't pass in a function to get back a Promise. That might mean you don't need the wrapper.

virtuacoplenny · 2018-08-07T17:48:33Z

static/msredirect.html

@@ -0,0 +1,11 @@
+<html>


I'm wondering if it's best to include the doctype. According to W3C it is required but I don't think omitting it actually causes much harm for this case.

The case where there is a calendar type google selected, but not logged in, trying to login on loading welcome page will show a warning that it tried to open a popup, which was denied by browser.

virtuacoplenny · 2018-08-08T18:31:33Z

react/features/calendar-sync/actions.js

+            });
+
+        if (!api) {
+            return Promise.reject('No calendar type selected!');


Would it make sense to do this check even before trying to get the api?

Nevermind, I get what you're doing. In case some unexpected type is set you want to error here.

virtuacoplenny · 2018-08-08T18:36:15Z

react/features/calendar-sync/web/microsoftCalendar.js

+        this._currentRequest = {};
+
+        // checks post messages for events from the popup
+        window.addEventListener('message', this._onPostMessage.bind(this));


If switching between microsoft and google, would this listener cause any problems? Do you need to do any cleanup in a deconstructor/destroy method?

I don't see how it can cause any problems, but cleaning is something we should take care. I will check it now.

virtuacoplenny · 2018-08-14T16:13:00Z

config.js

@@ -256,6 +256,10 @@ var config = {
    // maintenance at 01:00 AM GMT,
    // noticeMessage: '',

+    // Enables calendar integration, depends on googleApiApplicationClientID
+    // and microsoftApiApplicationClientID
+    // enableCalendarIntegration: false,


Maybe microsoftApiApplicationClientID needs to be added as a separate entry in config. googleApiApplicationClientID is listed under undocumented config values and is also present in the overrides whitelist.

virtuacoplenny · 2018-08-14T16:17:46Z

react/features/calendar-sync/actionTypes.js

+ *     type: CLEAR_CALENDAR_INTEGRATION
+ * }
+ */
+export const CLEAR_CALENDAR_INTEGRATION = Symbol('CLEAR_CALENDAR_INTEGRATION');


Do you want to rename these to something? I had used the word "integration" to avoid conflicting with existing names.

We agreed to leave it as integration.

virtuacoplenny · 2018-08-14T16:18:23Z

react/features/calendar-sync/actionTypes.js

+ *     calendarType: ?string
+ * }
+ */
+export const SET_CALENDAR_TYPE = Symbol('SET_CALENDAR_TYPE');


I think SET_CALENDAR_TYPE can be removed. Probably have to do a pass of all actions/actionTypes to make sure any unused ones are cleaned up.

virtuacoplenny · 2018-08-14T16:23:18Z

react/features/calendar-sync/actions.js

+            integrationType
+        } = getState()['features/calendar-sync'];
+
+        if (!enableCalendarIntegration) {


Maybe this should use the function for checking if calendar integration is enabled.

I'm also second-guessing the usefulness of this for the welcome page. Will the welcome page be calling this?

virtuacoplenny · 2018-08-14T16:32:33Z

react/features/calendar-sync/actions.js

+            return Promise.reject('No integration found');
+        }
+
+        // FIXME BEFORE MERGE: Where is the actual setting happening?


One issue here is the google-api doesn't call setCalendarProfileEmail but microsoftCalendar does. There should be consistency on where setCalendarProfileEmail is being called.

virtuacoplenny · 2018-08-14T17:25:23Z

react/features/google-api/googleApi.js

+                });
+
+                return Promise.all(promises)
+                    .then(result =>


I think naming this "results" might make it more obvious, if it isn't already, that there are multiple results in an array that need to be flattened.

virtuacoplenny · 2018-08-14T17:29:05Z

react/features/settings/components/web/CalendarTab.js

+    /**
+     * The current calendar integration in use, if any.
+     */
+    _isConnectedToCalendar: ?Object,


this is a boolean now.

virtuacoplenny · 2018-08-14T17:29:34Z

react/features/settings/components/web/CalendarTab.js

+     */
+    componentDidMount() {
+        this.props.dispatch(bootstrapCalendarIntegration())
+            .catch(() => console.warn('this error right here...'))


Gotta remove this debugging code.

virtuacoplenny · 2018-08-14T17:30:15Z

react/features/settings/components/web/CalendarTab.js

+ * @param {Object} state - The Redux state.
+ * @private
+ * @returns {{
+ *     _enableGoogleIntegration: boolean,


Have to update this jsdoc for the proper return.

virtuacoplenny · 2018-08-14T17:30:33Z

react/features/settings/components/web/SettingsDialog.js

+import {
+    getMoreTabProps,
+    getProfileTabProps
+} from '../../functions';


Undo this change.

List microsoftApiApplicationClientID in undocument config. remove unused SET_CALENDAR_TYPE action use helper for calendar enabled in bootstrap reorder actions reorder imports change order of signin -> set type -> update profile add logging for signout error reword setting dialog desc to avoid redundancy add jsdoc to microsoft button props reorder calendar constants move default state to reducer (not reused anywhere) update comment about calendar-sync due to removal of getCalendarState update comment for getCalendarIntegration remove vague comment alpha order reducer, return default state on reset alpha order persistence registry remove unnecessary getType from apis update comments in microsoftCalendar alpha order google-api exports, use api.get in loadGoogleAPI set jsdoc for google signin props alpha order googleapi methods fix calendartab docs

virtuacoplenny · 2018-08-14T21:08:11Z

react/features/calendar-sync/web/microsoftCalendar.js

+ * @returns {Object}
+ */
+function getParamsFromHash(hash) {
+    const params = new URLSearchParams(hash.slice(1, hash.length));


Looks like "parseURLParams" exists somewhere and we should use that.

The web part needs configuration in order to refresh tokens (Microsoft).

updateProfile changed to getCurrentEmail rename result to results stop storing integration in redux, store if ready for use use existing helpers to parse redirect url

damencho requested a review from virtuacoplenny August 2, 2018 23:54

yanas assigned virtuacoplenny Aug 6, 2018

damencho added 3 commits August 7, 2018 11:14

Refactor calendar-sync feature to be loaded on web.

7f8a1e2

For the web part it just adds new property to enable/disable calendar web integration, disabled by default.

Initial implementation of retrieving google calendar events.

89ebc2f

Initial implementation of retrieving microsoft calendar events.

2e13d53

damencho force-pushed the calendar-web branch from 5313ff5 to 2e13d53 Compare August 7, 2018 16:21

virtuacoplenny reviewed Aug 7, 2018

View reviewed changes

damencho added 8 commits August 7, 2018 13:48

Fixes comments.

eacea85

Rework to use the promise part of microsoft-graph-client api.

58362ea

Moves dispatching some actions, fixing comments.

63a83df

Makes sure we do not initializeClient google-api client multiple times.

4e5b76a

Do not try to login when fetching calendar entries.

ddf7fba

The case where there is a calendar type google selected, but not logged in, trying to login on loading welcome page will show a warning that it tried to open a popup, which was denied by browser.

Updates profile display data on sign in.

92f07c3

Propagate google-api state to calendar-sync only if we use google cal.

99e7af0

Adds sign out action.

aece063

virtuacoplenny reviewed Aug 8, 2018

View reviewed changes

damencho and others added 7 commits August 8, 2018 14:10

Clears the event listener when the popup closes.

ac1f410

Clears calendarIntegrationInstance on signOut.

f05cb30

WIP: UI for calendar settings, refactor auth flows

5b251a2

Clean up some unused constants, functions and exports.

e554667

break circular dependency of function and constant

b00a5dd

Exports only isCalendarEnabled from functions.

340a4d5

Checks isSignedIn when doing fetchCalendarEntries on web.

d132c4f

virtuacoplenny reviewed Aug 14, 2018

View reviewed changes

damencho and others added 4 commits August 14, 2018 16:12

Moves fetching calendar from APP_WILL_MOUNT to SET_CONFIG.

7ade6d0

The web part needs configuration in order to refresh tokens (Microsoft).

Fixes storing token expire time and refreshing tokens in Microsoft impl.

940b42e

Address comments

d103f40

updateProfile changed to getCurrentEmail rename result to results stop storing integration in redux, store if ready for use use existing helpers to parse redirect url

update jsdocs, get google app id from redux

4ca5eff

virtuacoplenny previously approved these changes Aug 15, 2018

View reviewed changes

clear integration instead of actual sign out

3ec0b88

virtuacoplenny dismissed their stale review via 3ec0b88 August 15, 2018 17:55

virtuacoplenny approved these changes Aug 15, 2018

View reviewed changes

virtuacoplenny merged commit 7eda313 into master Aug 15, 2018

damencho deleted the calendar-web branch August 17, 2018 21:45


		const query = client.api('/me/calendars');

		query.get((error, response) => {

Google & Microsoft calendar API integration #3340

Google & Microsoft calendar API integration #3340

Conversation

damencho commented Aug 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment