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

Store initial setup time for Site Kit and modules as meta. #6443

Open
derweili opened this issue Jan 23, 2023 · 8 comments
Open

Store initial setup time for Site Kit and modules as meta. #6443

derweili opened this issue Jan 23, 2023 · 8 comments
Assignees
Labels
Exp: SP Module: Analytics Google Analytics module related issues Module: Search Console Search Console module related issues Module: Tag Manager Google Tag Manager module related issues P2 Low priority Type: Enhancement Improvement of an existing feature

Comments

@derweili
Copy link
Collaborator

derweili commented Jan 23, 2023

Feature Description

This is a followup for #5853 where showing the auto-update banner is delayed after the initial plugin setup for 10 minutes.
Currently there is no way to get the site kit setup time to calculate the time delta. Therefore the auto-update banner (#5853) tries to recognize the initial setup by verifying URL query params. A cacheItem with a 10 minutes lifetime is used to recognize the initial setup for the following 10 minutes.

There are many assumptions in this process, which are unrelated to the to the actual component.

Therefore it would be better, if the setup time was stored in options and available through the datastore.
The same applies not only to Site Kit setup but also to the setup time of each module.


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

Acceptance criteria

  • After successfull Site Kit setup, the current time should be stored in options table.
  • This setup time should be available through a new selector on the core/site datastore.
  • For each module: Adsense, Analytics, Analytics-4, Optimize, Search Console, Tag Manager:
    • After successfull setup of each module, the current time should be stored in the module settings.
    • The modules setup time should be available through a new selector on the modules datastore.
  • The Auto-update banner approach used for Encourage users to enable auto-updates for Site Kit #5853 should be updated to use this new data.

Implementation Brief

  • Using includes/Core/Modules/Modules.php,
    • Within activate_module, update the newly activated module settings by adding setupTime with the value set to the current timestamp.
      • To set the module settings, use $module->get_settings()->merge() function.
  • Using each module's settings class, for e.g includes/Modules/AdSense/Settings.php, add the setupTime to the array of settings within the get_default function.
  • Add the setupTime setting for the module on the JS side as well for each module when creating the module store. For e.g in assets/js/modules/adsense/datastore/base.js.
    • The getSetupTime selector will automatically be available.
  • Similar to includes/Core/Authentication/Verification.php, create includes/Core/Authentication/SetupTime.php to add a new option googlesitekit_setup_time and extending the Setting class instead of User_Setting.
  • Within includes/Core/Authentication/Clients/OAuth_Client.php,
    • Set the googlesitekit_setup_time option value to the current timestamp using the new SetupTime class, before the user is redirected to the splash page, with the authentication_success URL param set and there is no redirection url set.
  • Using includes/Core/Authentication/Authentication.php,
    • Within the inline_js_base_data method, add setupTime to the returned data, with the value set to a new option googlesitekit_setup_time, which can be obtained by calling $this->options->get method, passing the option name as parameter.
  • Using assets/js/googlesitekit/datastore/site/info.js,
    • In the RECEIVE_SITE_INFO reducer, add the new setupTime property to siteInfo.
    • Add a new selector getSetupTime which returns the above value.
  • Using assets/js/components/notifications/EnableAutoUpdateBannerNotification.js,
    • Update the logic to set isInitialPluginSetup to use the getSetupTime selector from the core/site datastore so that the component is rendered 10 mins after the setup time.

Test Coverage

  • Add JS tests for the new getSetupTime for both, modules data store and the core/site data store.
  • Add PHP tests for the new setupTime options for both, modules and core site options.

QA Brief

Changelog entry

@derweili derweili added Type: Enhancement Improvement of an existing feature Module: Site Verification Google Site Verification module related issues Module: Analytics Google Analytics module related issues Module: Search Console Search Console module related issues Module: Optimize Google Optimize module related issues Module: Tag Manager Google Tag Manager module related issues and removed Module: Site Verification Google Site Verification module related issues labels Jan 23, 2023
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jan 23, 2023
@tofumatt
Copy link
Collaborator

ACs here are good, thanks for including the module setup dates as well 👍🏻

I did add a note to the ACs to use this new selector in the Auto-update logic; I don't think it warrants a new issue 😄

Moved to IB.

@mxbclang
Copy link

@derweili Can you please add a priority to this issue? Thanks!

@derweili derweili added the P2 Low priority label Jan 27, 2023
@asvinb asvinb self-assigned this Feb 8, 2023
@asvinb asvinb added the Exp: SP label Feb 9, 2023
@asvinb asvinb removed their assignment Feb 9, 2023
@techanvil techanvil self-assigned this Feb 16, 2023
@techanvil
Copy link
Collaborator

techanvil commented Feb 16, 2023

Hey @asvinb, the IB is looking good for the AC, but I have some reservations about the AC itself.

  • Seeing as the intention is to show the auto-update reminder banner 10 minutes after the user has set up Site Kit, I don't think we should be storing the setup time as a site-wide setting, but rather a user setting. Otherwise, if one user sets up SK, and then a second user does, the second user will reset the timing for the first user as well., Or, if the setting is only stored on the very first setup, the second user may be shown the banner straight after they set up the plugin.
  • It's not clear to me why we're adding the timestamp for modules as well. I don't see any plans to use it, and unless there are concrete plans to do so, we shouldn't be adding code that may never be used.

I think this might need to have the AC updated. What's your take on this?

@techanvil techanvil assigned asvinb and unassigned techanvil Feb 16, 2023
@asvinb
Copy link
Collaborator

asvinb commented Feb 17, 2023

@techanvil

  • I think it makes sense to have the setup time stored as a user setting instead of a site setting.
  • Here also I fully agree with you, if we are not going to use the code, we shouldn't be adding them.

Do you want me to update the IB or curious to hear @aaemnnosttv 's thoughts about this.

@asvinb asvinb assigned aaemnnosttv and unassigned asvinb Feb 17, 2023
@techanvil
Copy link
Collaborator

Thanks @asvinb. I think it might be worth updating the AC, as well as the IB, seeing as we've caught it at this stage. Interested to hear @aaemnnosttv's thoughts too.

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Mar 27, 2023

Anything user-specific which isn't critical and 1 hr or less is probably better suited as client-side cache rather than a persistent value in the DB. I wouldn't say this is critical, so if the user were to install SK via WP CLI and this doesn't get set and the user sees it right away instead, that seems fine to me.

Moving this back to AC for potential revision.

@aaemnnosttv aaemnnosttv assigned techanvil and unassigned aaemnnosttv Mar 27, 2023
@techanvil
Copy link
Collaborator

Anything user-specific which isn't critical and 1 hr or less is probably better suited as client-side cache rather than a persistent value in the DB. I wouldn't say this is critical, so if the user were to install SK via WP CLI and this doesn't get set and the user sees it right away instead, that seems fine to me.

Moving this back to AC for potential revision.

@aaemnnosttv, while I think this is a perfectly reasonable statement, when reading the Feature Description for this one it's evident that we currently do make use of the client-side cache for this functionality, but we don't have a very good way of identifying when the site has just been setup on the client side. I think this issue is really about adding some server-side state to make this new-site status more reliable, and we might still be relying on client-side caching to determine whether we'd actually actioned the notification.

With that in mind, how do you feel about this one?

@techanvil techanvil assigned aaemnnosttv and unassigned techanvil Apr 3, 2023
@mxbclang mxbclang removed the Module: Optimize Google Optimize module related issues label Nov 27, 2023
@ivonac4
Copy link
Collaborator

ivonac4 commented Jan 8, 2024

@aaemnnosttv Seems like this one requires your opinion. Do you have time to take a look anytime soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP Module: Analytics Google Analytics module related issues Module: Search Console Search Console module related issues Module: Tag Manager Google Tag Manager module related issues P2 Low priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants