Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronize Google profile data (name, avatar) on a regular basis #6003

Closed
tofumatt opened this issue Oct 14, 2022 · 10 comments
Closed

Synchronize Google profile data (name, avatar) on a regular basis #6003

tofumatt opened this issue Oct 14, 2022 · 10 comments
Labels
Exp: SP P1 Medium priority PHP Type: Enhancement Improvement of an existing feature

Comments

@tofumatt
Copy link
Collaborator

tofumatt commented Oct 14, 2022

Bug Description

Currently we don't refresh a user's profile data in the background unless there was an error with a profile data request (see: #6002 (comment)). This has two issues:

  1. If the user updates any of their profile data, like their photo or name, Site Kit won't ever reflect those changes.
  2. If we add new fields to the info we want to get from their profile (like in User menu does not show which account is connected #5724 where we added the full_name property), they won't ever be added to the profile info.

To fix this, we should schedule a task that updates the user's profile info, perhaps every few hours so it doesn't feel stale after a Google profile change or new feature being added.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The user's profile data (eg. the data stored in wp_googlesitekit_profile) should be updated whenever the googlesitekit_reauthorize_user action runs and succeeds, if their profile data hasn't been updated in the past 24 hours.

Implementation Brief

  • In includes/Core/Authentication/Clients/OAuth_Client.php:
    • Update the refresh_profile_data method:
      • In the array with which $this->profile->set() method is called, add a last_updated key with the value of the current UNIX timestamp (using the time() function).
  • In includes/Core/Authentication/Authentication.php:
    • Update the register method:
      • Add an action with the googlesitekit_reauthorize_user with a callback function doing the following:
        • In the profile data (obtainable by calling the $this->profile->get() method), check the last_updated key value.
        • Compare that with the current timestamp (e.g. time() - $profile_data['last_updated']) and see if the diffence is more than 24 hours (DAY_IN_SECONDS).
        • If so, call the $this->get_oauth_client()->refresh_profile_data() method with a 30 minutes $retry_after value.

Test Coverage

  • In tests/phpunit/integration/Core/Authentication/Clients/OAuth_ClientTest.php:
    • Update the test case for refresh_profile_data to reflect the above changes.
  • In tests/phpunit/integration/Core/Authentication/AuthenticationTest.php:
    • Update the test case for register to reflect the above changes.

QA Brief

  • Currently, after you set up Site Kit with your Google account, if you make any changes to your Google account, for example, change your profile picture or name, these changes will not be reflected in Site Kit.
  • After the PR for this issue is merged, this should no longer be the case. Any such changes made to your Google account will be reflected in Site Kit.
  • The refresh will take place once Site Kit re-authorises the user (which happens once every hour). Once one refresh takes place, the next one will take place only after 24 hours.
  • Make changes to your Google profile and verify that the changes show up in Site Kit, keeping the above information in consideration.

Changelog entry

  • Implement Google profile data synchronisation.
@tofumatt tofumatt added P0 High priority Type: Enhancement Improvement of an existing feature PHP labels Oct 14, 2022
@tofumatt tofumatt self-assigned this Oct 14, 2022
@tofumatt tofumatt assigned aaemnnosttv and unassigned tofumatt Oct 14, 2022
@tofumatt
Copy link
Collaborator Author

tofumatt commented Oct 14, 2022

@aaemnnosttv Do you think two hours is too aggressive for profile data refreshes? Or not aggressive enough? I don't want to make too many requests, but having up-to-date info relatively close to when the user updates it is nice UX…

@aaemnnosttv
Copy link
Collaborator

As a regular cron, we're limited to the intervals that are defined in wp_get_schedules. This is extendable and we could add our own custom schedule here but I think this is probably unnecessary. I have a few ideas about this though.

  • This hasn't been called very much in the past so calling it on a regular/daily basis on every site going forward could raise quota problems as it would likely be a huge increase compared to the current
  • I also wouldn't expect the user's profile data to change very often at all so I wonder if this is something we should poll in the background at all
  • Polling this in the background would also increase the number of token refreshes which otherwise wouldn't happen so this would be an added load on the SK service as well
  • I'm not even sure we need the retry mechanism via cron anymore now that requests are automatically retried since Retry failed requests: implement exponential backoff #1998 (probably fine to keep, just thinking out loud)

A few ideas about how we might do this:

  • Request it on the dashboard – wouldn't invoke any additional token refresh, and would be on-demand. If different than what we have saved, this could update automatically or we could make a request back to WP to update it. Could be checked once/hour as part of the normal cache TTL (could be longer though) but would still be on-demand so not as much of a concern.
  • Request/queue it when the user signs into WP – doesn't happen very often, but on demand. Could be queued as a one-off scheduled event to run in the background on the next cron. This is probably the simplest option
  • I also thought about hooking it on googlesitekit_reauthorize_user which fires right after our token is refreshed, (should be no more than once/hr) but that seems a bit aggressive, but would/should be on-demand as well and should never cause an additional token refresh because it only runs after we have a new token.

@aaemnnosttv aaemnnosttv assigned tofumatt and unassigned aaemnnosttv Oct 14, 2022
@tofumatt
Copy link
Collaborator Author

That makes sense… I think requesting it on the dashboard (eg. choice 1) makes the most sense, as it would be nice if this data was reflected the UI without too much delay—my concern with the WP sign-in or even the token refresh is that a user might update their info and then not see the effects in the Site Kit dashboard for hours or even days.

Let's go with the Dashboard Refresh; I've updated the ACs—does that sound good for now?

@tofumatt tofumatt assigned aaemnnosttv and unassigned tofumatt Oct 28, 2022
@aaemnnosttv
Copy link
Collaborator

  • The user's profile data (eg. the data stored in wp_googlesitekit_profile) should be updated whenever a user with Google credentials visits the Site Kit dashboard.

I don't think we should request this on every visit to the dashboard, and we also shouldn't make the request during the page load. Maybe that's not what you had in mind either, but my idea for that strategy was more to refresh the profile data from a request invoked by the client (perhaps throttled by a cached request TTL).

my concern with the WP sign-in or even the token refresh is that a user might update their info and then not see the effects in the Site Kit dashboard for hours or even days.

Access tokens only last for an hour after which they're refreshed on-demand so it wouldn't be out of date for longer than an hour.

I don't think we should be overly concerned with this being out of sync as the most critical part (the email address) can't be changed without first disconnecting, so that part would always be accurate. The rest of the profile data is non-critical so keeping it up to date in a timely manner is more of a nice-to-have. We've never updated it after the initial connection before so this is a big enhancement in that regard already 😄

I still feel WP sign-in would be a good trigger for this as everyone needs to sign-in before they can get to the SK dashboard and this should be infrequent enough to avoid hitting the API too often.

Details

In this case the action should only schedule the one-off background event though just like the fallback case in refresh_profile_data today, although without a recurring retry. If we schedule the job to run immediately (i.e. on the next cron), there's a good chance it would already be populated in the background by the time the user gets the the SK dashboard because a cron should be spawned at the same time.

The benefit of using a client side request is that the profile data would update and the user would see the latest data more/less right away. The downside is that I think this data would rarely change to make use of that benefit and so 99% of the requests would be unnecessary. The latter holds true for the WP login strategy too, there would just be much fewer.

@aaemnnosttv aaemnnosttv assigned tofumatt and unassigned aaemnnosttv Nov 2, 2022
@bethanylang
Copy link
Collaborator

@tofumatt Following up on this one, thank you!

@tofumatt
Copy link
Collaborator Author

If we tie the event to WordPress login, the user data effectively won't ever change for a user who has the "Remember me" checkbox selected though. I definitely have some WordPress sites I basically log into once when I'm on a new device and then that's it 😅

Of the options, I think "Options 3", eg hooking it on googlesitekit_reauthorize_user would make sense. But maybe even only doing it every X token refreshes or only if X amount of time has passed. It's a bit aggressive, but otherwise I'm thinking of a support request where a user wonders why their profile is out-of-date and we tell them to "sign out and sign in again". 😉

hello-it-have-you-tried

Ad

I've written ACs with that in mind, let me know what you think @aaemnnosttv.

@tofumatt tofumatt assigned aaemnnosttv and unassigned tofumatt Jan 17, 2023
@aaemnnosttv
Copy link
Collaborator

If we tie the event to WordPress login, the user data effectively won't ever change for a user who has the "Remember me" checkbox selected though. I definitely have some WordPress sites I basically log into once when I'm on a new device and then that's it 😅

This choice only extends the login session validity from 2 days to 14 (ref).

Of the options, I think "Options 3", eg hooking it on googlesitekit_reauthorize_user would make sense. But maybe even only doing it every X token refreshes or only if X amount of time has passed.

Sounds good to me, I like the idea of adding a rate limit to it, that makes it sound a bit more sensible 👍

AC 👍

I'm going to downgrade this from a P0 though since the display name updates have already been released for a while now and this hasn't raised any support issues AFAIK.

@aaemnnosttv aaemnnosttv removed their assignment Mar 31, 2023
@aaemnnosttv aaemnnosttv added P1 Medium priority and removed P0 High priority labels Mar 31, 2023
@aaemnnosttv aaemnnosttv changed the title Schedule a cron_refresh_profile_data function to run occasionally so we have up-to-date profile data Synchronize Google profile data (name, avatar) on a regular basis Mar 31, 2023
@nfmohit nfmohit self-assigned this Apr 4, 2023
@nfmohit
Copy link
Collaborator

nfmohit commented Apr 4, 2023

@aaemnnosttv @tofumatt What do you folks think of storing when the profile was last updated in the profile data itself, e.g. with a last_updated property? That way, we could compare this option value property with the current time in googlesitekit_reauthorize_user, and refresh the profile data if it has been more than 24 hours (draft IB added).

The other alternative could be using a transient to store the time, but I think the former is simpler.

Thoughts?

@nfmohit nfmohit added the Exp: SP label Apr 4, 2023
@nfmohit nfmohit assigned aaemnnosttv and unassigned nfmohit Apr 5, 2023
@aaemnnosttv
Copy link
Collaborator

Great suggestion, thanks @nfmohit!

IB ✅

@aaemnnosttv aaemnnosttv removed their assignment Apr 5, 2023
@nfmohit nfmohit self-assigned this May 28, 2023
@nfmohit nfmohit removed their assignment May 29, 2023
@nfmohit nfmohit assigned eugene-manuilov and unassigned nfmohit May 31, 2023
eugene-manuilov added a commit that referenced this issue Jun 1, 2023
…ofile-data

Regularly sync Google profile data
@eugene-manuilov eugene-manuilov removed their assignment Jun 1, 2023
@mohitwp mohitwp self-assigned this Jun 6, 2023
@mohitwp
Copy link
Collaborator

mohitwp commented Jun 13, 2023

QA Update ✅

  • Verified on dev environment.
  • Verified that Google profile data (name,avatar) is gets update in 24 hours when googlesitekit_reauthorize_user action run and succeeds.
  • Tested by updating the name and avatar under google profile and verified after 24 hours both name and avatar gets update under Site kit.

image

Recording.396.mp4

@mohitwp mohitwp removed their assignment Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP P1 Medium priority PHP Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants