site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Add JS entry point for RRM

Open nfmohit opened this issue 1 year ago • 13 comments

Feature Description

A JavaScript entry point for Reader Revenue Manager should be added which should include its module and datastore registration. The data store will be named modules/reader-revenue-manager. This should also include adding the submitChanges & validateCanSubmitChanges actions. The module registration should also include a checkRequirements property to ensure the site is on HTTPS as a requirement for module activation.

This issue should also be responsible for enqueuing the JS asset to the Modules\Reader_Revenue_Manager class using the setup_assets method.


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

Acceptance criteria

  • Module and datastore registration should be implemented for the Reader Revenue Manager module in a new JavaScript entry point.
  • The name of the datastore should be modules/reader-revenue-manager.
  • The module icon should be exported from the Figma designs.
  • The module should have placeholder empty components added for setup and settings, such as SettingsEdit, SettingsView, and SetupMain.
  • The data store should have basic actions implemented, such as submitChanges, rollbackChanges, and validateCanSubmitChanges.
  • It should be ensured that the module can be activated only if the site is on HTTPS.
    • The message when the activation is disabled should be: "The site should use HTTPS to set up Reader Revenue Manager".
  • The new JS asset should be enqueued in the module class for Reader Revenue Manager (being implemented in #8785).
  • A "New" badge should be added for the module in Site Kit Settings.

Note: It is not necessary to define module features (that show up in the module disconnection dialog) as part of this issue. Those will be implemented as part of #8845.

Implementation Brief

  • [x] Create a new module reader-revenue-manager inside assets/js/module/.
    • [x] Create assets/js/module/reader-revenue-manager/datastore/base.js. This will be a base store.
      • [ ] Create placeholder components in
      • [x] Call the Modules.createModuleStore.
        • [x] Slug should be reader-revenue-manager.
        • [x] Store name must be reader-revenue-manager.
        • [x] Make sure to pass validateCanSubmitChanges described below.
      • [x] Create placeholder components SettingsEdit and SettingsView inside assets/js/module/reader-revenue-manager/components/settings/
      • [x] Create placeholder components SetupMain inside assets/js/module/reader-revenue-manager/components/setup/
    • [x] Create assets/js/module/reader-revenue-manager/index.js. Use registerModule to register the module. Take this reference on how it can be registered.
      • [x] Pass checkRequirements function to registerModule. In checkRequirements function verify that site is using SSL which can be done by using getHomeURL selector from CORE_SITE store.
      • [x] Throw from checkRequirements with NON_HTTPS_SITE error code and the message defined in the ACs as message when site is not https.
        • [x] This will ensure that the setup is disabled and the correct message is shown when site is not HTTPS.
    • [x] Create assets/js/module/reader-revenue-manager/datastore/settings.js
      • [x] Create a function validateCanSubmitChanges and validate the following.
        • [x] Publication ID is not empty and is string.
        • [x] Publication ID belongs to one of the publication IDs that we received from getPublications selector.
        • [x] Use Invariant in case of error.
      • [x] As of now, we won't need separate submitChanges and rollbackChanges action as those will automatically be created by create-settings-store via Modules.createModuleStore.
  • [x] In webpack/modules.config.js, add following config. We need
    'googlesitekit-modules-reader-revenue-manager':
        './assets/js/googlesitekit-modules-reader-revenue-manager.js',
  • [x] The bundled file (generated assets) needs to be enqueued from within Modules\Reader_Revenue_Manager class using setup_assets function. The class is being implemented in #8785.

  • [x] In a new file assets/js/components/settings/constants.js, create a new constant NEW_MODULES which should be array [ 'ads', 'reader-revenue-manager' ]

    • [x] In assets/js/components/settings/SetupModule.js, update this check to check if module slug is present in NEW_MODULES constant created above, in order to display "New" badge.
    • [x] Do the same step as above in assets/js/components/settings/SettingsActiveModule/Header.js.

Test Coverage

  1. Add tests for the data store modules.
  2. Add tests for the main module file.
  3. Add test for non-HTTPS scheme and ensure that it adds warning.

QA Brief

  • Most of the changes can be verified during the code review process.
  • Enable the rrmModule feature flag via the tester plugin.
  • Go to the Site Kit Settings page -> connect-more-services tab and ensure the "Reader Revenue Manager" module is available in the modules grid.
  • Note that there is no Figma design for the above. We just need to ensure it's available in the Connect More Services.
  • Verify in an HTTPS protocol-enabled site that Set up Reader Revenue Manager is clickable.
  • Go through the setup flow.
  • Click the Cancel button as we haven't implemented the setup form. It will take us to the "Connected Services" screen.
  • Verify the RRM module is available in the "Connected Services".
  • Verify in an HTTP site that Set up Reader Revenue Manager is disabled and shouldn't be able to setup.
  • Verify the The site should use HTTPS to set up Reader Revenue Manager warning should be displayed.
  • Disable the rrmModule feature flag and verify the "Reader Revenue Manager" module is unavailable in the Connect More Services.
  • Go to the Storybook and verify the following stories are available:
    • Modules > ReaderRevenueManager > Setup > SetupMain
    • Modules > ReaderRevenueManager > Settings > SettingsEdit
    • Modules > ReaderRevenueManager > Settings > SettingsView
  • The structure and styles are yet to be implemented for the above stories in the upcoming tickets.

Changelog entry

  • Add Reader Revenue Module setup and settings view foundations.

nfmohit avatar Jun 03 '24 08:06 nfmohit

Hi @nfmohit, AC LGTM, just one part I'm not sure about. In TwG case, we didn't surface the module at all when the site was not HTTPS, but here we're only blocking the user from activating the module. Is this correct? If yes, I'm assuming it will look like the Ads/AdSense module when Ad Blocker is active, and we'll need a warning message to display.

Let me know what you think! Cheers.

kuasha420 avatar Jun 18 '24 09:06 kuasha420

Hi @kuasha420. That is the correct behaviour according to the design doc. I've added a warning message.

nfmohit avatar Jun 18 '24 11:06 nfmohit

@nfmohit Perfect. AC :white_check_mark:

kuasha420 avatar Jun 18 '24 13:06 kuasha420

Hi @ankitrox, IB is looking good. Couple of points.

  1. The new module asset needs to be added to the bundling infra. ie. WebPack in webpack/modules.config.js.
  2. The Asset needs to be enqueued from the Modules\Reader_Revenue_Manager class using setup_assets function.

Cheers.

kuasha420 avatar Jun 20 '24 09:06 kuasha420

Thank you @kuasha420 . I've included those two points along with the missing "New Badge" pointer.

Over to you for re-review.

Thanks again!

ankitrox avatar Jun 20 '24 09:06 ankitrox

Thanks @ankitrox.

Great point about the New badge. Looks like we need to do the same check in assets/js/components/settings/SettingsActiveModule/Header.js as well. Also, I think instead of checking for the new slugs one by one, we should do something similar for how it's done for the EXPERIMENTAL_MODULES, by defining a new constant NEW_MODULES and using that.

Cheers.

kuasha420 avatar Jun 21 '24 11:06 kuasha420

Thank you @kuasha420 . I've updated the IB.

ankitrox avatar Jun 24 '24 16:06 ankitrox

@ankitrox Thanks. I made a small change to the path where the NEW_MODULES constant should be stored, otherwise this is LGTM.

I missed a few points in my previous review. Sorry for that.

  1. Can you make a mention of checkRequirements function on registerModule point? In the AC it is stated that the module should only be able to be activated when the site is an HTTPS site. We can verify that in the checkRequirements function using getHomeURL selector from CORE_SITE store.
  2. When the Site is non-HTTP, the setup will be disabled. We need to show the warning messaage in assets/js/components/notifications/ModuleSettingsWarning.js, there's a message defined in the AC, it should look like the Ad Blocker Warning message with the copy defined in AC.
  3. settingSlugs should be omitted from the IB for now, as those will be handled along with all other module settings in #8793.

Cheers.

kuasha420 avatar Jun 25 '24 10:06 kuasha420

Thanks @kuasha420 . I've updated the IB.

ankitrox avatar Jun 26 '24 14:06 ankitrox

Thank you @ankitrox for all the work here. I've made some slight edits regarding receiveCheckRequirementsError, turns out we can just throw from the checkRequirements function with the correct message, and it will be rendered in the right place. :tada: Everything else is :+1:

IB :green_apple:

kuasha420 avatar Jun 28 '24 10:06 kuasha420

QA Update ⚠️

Hi @hussain-t , I've tested this and here's my main feedback:

QAB: Click the Cancel button as we haven't implemented the setup form. It will take us to the "Connected Services" screen.

It says after cancel, it would take us back to "Connected Services" but currently we go back to the SK Dashboard with an error message. Also, if I then go to the "Connected Services" page, there will be an error there as well.

Error after clicking 'Cancel' Screenshot 2024-07-04 at 17 42 59

Error showing when we are at the "Connected Services' page. Screenshot 2024-07-04 at 17 44 53

kelvinballoo avatar Jul 04 '24 16:07 kelvinballoo

@kelvinballoo, you have canceled the OAuth flow (clicked the OAuth flow cancel button). You should click the setup flow cancel button after completing the OAuth flow. Please refer to the screenshot below:

Screenshot 2024-07-04 at 10 02 37 PM

hussain-t avatar Jul 04 '24 16:07 hussain-t

QA ✅

Thanks @hussain-t . This ticket has been tested good as follows.

  • Under Site Kit Settings page -> connect-more-services tab and ensure the "Reader Revenue Manager" module is available in the modules grid with a 'New' badge. Under HTTPS protocol-enabled site , the Set up Reader Revenue Manager is clickable.

    Screenshot 2024-07-04 at 20 51 52
  • Clicking the Cancel button after the auth setup flow, will take us to the "Connected Services" screen. The RRM module is available in the "Connected Services".

    Screenshot 2024-07-04 at 20 43 30
  • With an HTTP site, the Set up Reader Revenue Manager is disabled and cannot be set up. It would display a warning : "The site should use HTTPS to set up Reader Revenue Manager"

    Screenshot 2024-07-04 at 20 20 29
  • Once the rrmModule feature flag is disabled, the "Reader Revenue Manager" module is unavailable in the Connect More Services.

  • Under Storybook, the following stories are available: Modules > ReaderRevenueManager > Setup > SetupMain Modules > ReaderRevenueManager > Settings > SettingsEdit Modules > ReaderRevenueManager > Settings > SettingsView"

    Screenshot 2024-07-04 at 18 07 06

Moving ticket to Approval.

kelvinballoo avatar Jul 04 '24 16:07 kelvinballoo

The site should use HTTPS to set up Reader Revenue Manager

@nfmohit is this defined somewhere? I think we should be more specific that it is a requirement since the module is also prevented from being activated in this scenario. Not a blocking detail yet, but just thought I'd ask.

aaemnnosttv avatar Jul 12 '24 19:07 aaemnnosttv

The site should use HTTPS to set up Reader Revenue Manager

@nfmohit is this defined somewhere? I think we should be more specific that it is a requirement since the module is also prevented from being activated in this scenario. Not a blocking detail yet, but just thought I'd ask.

Hi @aaemnnosttv ! This is something I added while defining the ACs for this issue with an assumption that this may change later as part of a final language/copy check during the bug bash. I now realise that I should've confirmed this with you and/or Andrey, sorry! Would you like to propose an alternative copy here for which I can make a quick follow-up PR or issue?

nfmohit avatar Jul 12 '24 21:07 nfmohit