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

Add getAdminScreenURL and getAdminReauthURL selectors to base module store #1559

Closed
felixarntz opened this issue May 14, 2020 · 5 comments
Closed
Labels
Good First Issue Good first issue for new engineers P0 High priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature
Milestone

Comments

@felixarntz
Copy link
Member

felixarntz commented May 14, 2020

Feature Description

stores created by createModuleStore should include new selectors for module-specific admin URLs. These can be built on top of the enhanced version of getAdminURL from the core/site store, which was implemented as part of #1500.


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

Acceptance criteria

  • The core/user/data/authentication REST route should also return needsReauthentication, which uses Authentication::need_reauthenticate() (will be migrated to OAuth_Client class via Request write scopes only when needed #1566, just fyi - depending on timeline this may overlap).
  • The core/user store should receive an additional selector needsReauthentication() that exposes the above.
  • createModuleStore should receive a new options argument adminPage. The default should be "googlesitekit-dashboard". Modules with their own screens (Search Console, Analytics, AdSense) need to be amended so that they provide the slugs for their own screens when calling createModuleStore.
  • createModuleStore should receive a new options argument requiresSetup. The default should be a boolean true. Modules that do not require setup (only PageSpeed Insights for now) need to be amended so that they provide false for this argument.
  • Store objects returned by createModuleStore should have two additional selectors:
    • getAdminScreenURL( queryArgs ): Should use getAdminURL from core/site passing adminPage as the selector's page parameter, and queryArgs can be passed through.
    • getAdminReauthURL(): Should mirror the exact behavior of the legacy getReAuthURL utility function, based on getAdminScreenURL, whether the module requiresSetup, and whether the user needs to re-authenticate (requested scopes and granted scopes are available via core/user score, let's introduce a new selector to simplify this check, see below).
  • Refactored module code based on the datastore that currently still relies on one of the old utility functions (e.g. getReAuthURL, used at least by refactored AdSense code) should be modified to use the new selectors.

Implementation Brief

  • Update the core/user/data/authentication REST endpoint in includes/Core/Authentication/Authentication.php to also return needsReauthentication which will get its value from the OAuth_Client::needs_reauthentication() function.

  • Add needsReauthentication() selector to assets/js/googlesitekit/datastore/user/authentication.js which connects to the needsReauthentication value returned from the above endpoint.

  • Add these arguments to options in createModuleStore() in assets/js/googlesitekit/modules/create-module-store.js

    • adminPage (string) default value of googlesitekit-dashboard
    • requiresSetup (boolean) default value of true
  • For modules with their own screens, add the adminPage argument with the appropriate page name (ie: googlesitekit-module-analytics for the analytics module) when createModuleStore() is called for them.

    • assets/js/modules/search-console/datastore/index.js
    • assets/js/modules/analytics/datastore/index.js
    • assets/js/modules/adsense/datastore/index.js
  • For PageSpeed Insights module, pass false as the options.requiresSetup argument for createModuleStore() at assets/js/modules/pagespeed-insights/datastore/index.js

  • Create a new file assets/js/googlesitekit/modules/datastore/info.js that creates a store with selectors for getAdminScreenURL() and getAdminReauthURL() which will use getAdminURL() from the core/site store with the proper adminPage parameter which is passed to createModuleStore().

  • Ensure the above info store is combined with the other stores in assets/js/googlesitekit/modules/create-module-store.js to add to the base module store.

  • Write tests in assets/js/googlesitekit/modules/datastore/info.js to ensure the selectors properly call getAdminURL() with the correct parameters.

  • Update tests in assets/js/googlesitekit/modules/create-module-store.test.js to ensure the new selectors are included.

  • Refactor code in modules to use the new selector instead of utilities like getReAuthURL, for example:

QA Brief

Changelog entry

  • Add selectors to get module-specific admin screen URLs to every module datastore.
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature QA: Eng Requires specialized QA by an engineer Good First Issue Good first issue for new engineers Next Up labels May 14, 2020
@aaemnnosttv
Copy link
Collaborator

AC's mostly look good to me @felixarntz.

probably worth introducing a needsReauthentication() selector to that store and handle the logic there

Should this issue include the needsReauthentication selector to core/user as well?

@felixarntz
Copy link
Member Author

@aaemnnosttv Yeah that's what I was thinking. Will clarify a bit more.

@aaemnnosttv
Copy link
Collaborator

IB ✅

@aaemnnosttv aaemnnosttv removed their assignment May 29, 2020
@ShahAaron ShahAaron self-assigned this May 29, 2020
@ShahAaron ShahAaron removed their assignment Jun 2, 2020
@eclarke1 eclarke1 removed the Next Up label Jun 2, 2020
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Jun 4, 2020
@ShahAaron ShahAaron assigned felixarntz and unassigned ShahAaron Jun 5, 2020
@eclarke1 eclarke1 modified the milestones: Sprint 24, Sprint 25 Jun 8, 2020
@felixarntz felixarntz assigned ShahAaron and unassigned felixarntz Jun 10, 2020
@ShahAaron ShahAaron assigned felixarntz and unassigned ShahAaron Jun 15, 2020
@felixarntz felixarntz removed their assignment Jun 15, 2020
@eugene-manuilov eugene-manuilov self-assigned this Jun 17, 2020
@eugene-manuilov
Copy link
Collaborator

eugene-manuilov commented Jun 17, 2020

Overall, everything looks good. Found a few minor things:

  1. There is a new redundancy introduced in the PR for this ticket. The createInfoStore store has getAdminReauthURL selector that calls select( CORE_USER ).getAuthentication() to determine whether or not user needs reauthenticaion:

    getAdminReauthURL: createRegistrySelector( ( select ) => ( state, reAuth = true ) => {
    const { needsReauthentication } = select( CORE_USER ).getAuthentication() || {};

    But there is a special selector for it in the user authentication store:

    needsReauthentication: createRegistrySelector( ( select ) => () => {
    const { needsReauthentication } = select( STORE_NAME ).getAuthentication() || {};
    return needsReauthentication;
    } ),

    Even though it is not a critical issue, it violates DRY principal and makes it slightly harder to maintain in the future, if we need to adjust reauthentication logic. So, my recommendation is to update getAdminReauthURL selector to use needsReauthentication selector of the user authentication store.

  2. As part of this ticket we need to refactor getReAuthURL function use to rely on the datastore methods. I still see that the dashboard cta component of the page speed insights module uses it:

    global.location = getReAuthURL( 'pagespeed-insights' );

    I think it needs to be reworked to use getAdminReauthURL selector of the createInfoStore store since it violates the following acceptance criteria:

    Refactored module code based on the datastore that currently still relies on one of the old utility functions (e.g. getReAuthURL, used at least by refactored AdSense code) should be modified to use the new selectors.

@ShahAaron can you look at it and fix?

@felixarntz
Copy link
Member Author

@ShahAaron @eugene-manuilov Good catch, can you address point 1. specifically?

Point 2. was maybe due to a slightly unclear ACs, the dashboard CTA you're referring to is technically not a refactored component, so we don't need to update that yet. No harm in doing that, but I'd rather not make this change now this late in the release cycle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers P0 High priority QA: Eng Requires specialized QA by an engineer Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants