Add JS entry point for RRM
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.
- See relevant section in the design doc.
- See #5365 which was a similar effort for the former Thank with Google epic.
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, andSetupMain. - The data store should have basic actions implemented, such as
submitChanges,rollbackChanges, andvalidateCanSubmitChanges. - 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-managerinsideassets/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
validateCanSubmitChangesdescribed below.
- [x] Slug should be
- [x] Create placeholder components
SettingsEditandSettingsViewinsideassets/js/module/reader-revenue-manager/components/settings/ - [x] Create placeholder components
SetupMaininsideassets/js/module/reader-revenue-manager/components/setup/
- [x] Create
assets/js/module/reader-revenue-manager/index.js. UseregisterModuleto register the module. Take this reference on how it can be registered.- [x] Pass
checkRequirementsfunction toregisterModule. IncheckRequirementsfunction verify that site is using SSL which can be done by usinggetHomeURLselector fromCORE_SITEstore. - [x] Throw from
checkRequirementswithNON_HTTPS_SITEerror 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] Pass
- [x] Create
assets/js/module/reader-revenue-manager/datastore/settings.js- [x] Create a function
validateCanSubmitChangesand 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
getPublicationsselector. - [x] Use
Invariantin case of error.
- [x] As of now, we won't need separate
submitChangesandrollbackChangesaction as those will automatically be created bycreate-settings-storeviaModules.createModuleStore.
- [x] Create a function
- [x] Create
- [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_Managerclass usingsetup_assetsfunction. The class is being implemented in #8785. -
[x] In a new file
assets/js/components/settings/constants.js, create a new constantNEW_MODULESwhich 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 inNEW_MODULESconstant created above, in order to display "New" badge. - [x] Do the same step as above in
assets/js/components/settings/SettingsActiveModule/Header.js.
- [x] In
Test Coverage
- Add tests for the data store modules.
- Add tests for the main module file.
- 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
rrmModulefeature 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
HTTPSprotocol-enabled site thatSet up Reader Revenue Manageris clickable. - Go through the setup flow.
- Click the
Cancelbutton 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
HTTPsite thatSet up Reader Revenue Manageris disabled and shouldn't be able to setup. - Verify the
The site should use HTTPS to set up Reader Revenue Managerwarning should be displayed. - Disable the
rrmModulefeature 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 > SetupMainModules > ReaderRevenueManager > Settings > SettingsEditModules > 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.
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.
Hi @kuasha420. That is the correct behaviour according to the design doc. I've added a warning message.
@nfmohit Perfect. AC :white_check_mark:
Hi @ankitrox, IB is looking good. Couple of points.
- The new module asset needs to be added to the bundling infra. ie. WebPack in
webpack/modules.config.js. - The Asset needs to be enqueued from the
Modules\Reader_Revenue_Managerclass usingsetup_assetsfunction.
Cheers.
Thank you @kuasha420 . I've included those two points along with the missing "New Badge" pointer.
Over to you for re-review.
Thanks again!
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.
Thank you @kuasha420 . I've updated the IB.
@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.
- Can you make a mention of
checkRequirementsfunction onregisterModulepoint? 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 thecheckRequirementsfunction usinggetHomeURLselector fromCORE_SITEstore. - 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. settingSlugsshould be omitted from the IB for now, as those will be handled along with all other module settings in #8793.
Cheers.
Thanks @kuasha420 . I've updated the IB.
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:
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 showing when we are at the "Connected Services' page.
@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:
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.
-
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".
-
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"
-
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"
Moving ticket to Approval.
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.
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?