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 JS entry point for RRM #8786

Open
23 of 24 tasks
nfmohit opened this issue Jun 3, 2024 · 13 comments
Open
23 of 24 tasks

Add JS entry point for RRM #8786

nfmohit opened this issue Jun 3, 2024 · 13 comments
Labels
Module: RRM Reader Revenue Manager module related issues P0 High priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Jun 3, 2024

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

  • Create a new module reader-revenue-manager inside assets/js/module/.
    • Create assets/js/module/reader-revenue-manager/datastore/base.js. This will be a base store.
      • Create placeholder components in
      • Call the Modules.createModuleStore.
        • Slug should be reader-revenue-manager.
        • Store name must be reader-revenue-manager.
        • Make sure to pass validateCanSubmitChanges described below.
      • Create placeholder components SettingsEdit and SettingsView inside assets/js/module/reader-revenue-manager/components/settings/
      • Create placeholder components SetupMain inside assets/js/module/reader-revenue-manager/components/setup/
    • Create assets/js/module/reader-revenue-manager/index.js. Use registerModule to register the module. Take this reference on how it can be registered.
      • 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.
      • Throw from checkRequirements with NON_HTTPS_SITE error code and the message defined in the ACs as message when site is not https.
        • This will ensure that the setup is disabled and the correct message is shown when site is not HTTPS.
    • Create assets/js/module/reader-revenue-manager/datastore/settings.js
      • Create a function validateCanSubmitChanges and validate the following.
        • Publication ID is not empty and is string.
        • Publication ID belongs to one of the publication IDs that we received from getPublications selector.
        • Use Invariant in case of error.
      • 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.
  • In webpack/modules.config.js, add following config. We need
    'googlesitekit-modules-reader-revenue-manager':
        './assets/js/googlesitekit-modules-reader-revenue-manager.js',
  • 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 Implement RRM PHP module foundation #8785.

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

    • 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.
    • 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 nfmohit self-assigned this Jun 3, 2024
@nfmohit nfmohit added Type: Enhancement Improvement of an existing feature javascript Pull requests that update Javascript code Module: RRM Reader Revenue Manager module related issues Team M Issues for Squad 2 and removed javascript Pull requests that update Javascript code labels Jun 3, 2024
@ivonac4 ivonac4 added the P0 High priority label Jun 5, 2024
@nfmohit nfmohit removed their assignment Jun 8, 2024
@ivonac4 ivonac4 added Module: RRM Reader Revenue Manager module related issues Next Up Issues to prioritize for definition and removed Module: RRM Reader Revenue Manager module related issues labels Jun 11, 2024
@kuasha420 kuasha420 self-assigned this Jun 18, 2024
@kuasha420
Copy link
Collaborator

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 kuasha420 assigned nfmohit and unassigned kuasha420 Jun 18, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jun 18, 2024

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

@nfmohit nfmohit assigned kuasha420 and unassigned nfmohit Jun 18, 2024
@kuasha420
Copy link
Collaborator

@nfmohit Perfect. AC ✅

@kuasha420 kuasha420 removed their assignment Jun 18, 2024
@ankitrox ankitrox self-assigned this Jun 19, 2024
@ankitrox ankitrox removed their assignment Jun 19, 2024
@kuasha420 kuasha420 self-assigned this Jun 19, 2024
@kuasha420
Copy link
Collaborator

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 kuasha420 assigned ankitrox and unassigned kuasha420 Jun 20, 2024
@ankitrox
Copy link
Collaborator

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 ankitrox assigned kuasha420 and unassigned ankitrox Jun 20, 2024
@kuasha420
Copy link
Collaborator

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. 🎉 Everything else is 👍

IB 🍏

@kuasha420 kuasha420 removed their assignment Jun 28, 2024
@ivonac4 ivonac4 removed the Next Up Issues to prioritize for definition label Jun 28, 2024
@hussain-t hussain-t self-assigned this Jun 30, 2024
ankitrox added a commit that referenced this issue Jul 2, 2024
hussain-t added a commit that referenced this issue Jul 2, 2024
…gle/site-kit-wp into enhance/#8786-add-rrm-js-entry-point.
@hussain-t hussain-t removed their assignment Jul 2, 2024
@techanvil techanvil assigned techanvil and hussain-t and unassigned techanvil Jul 2, 2024
@hussain-t hussain-t assigned techanvil and unassigned hussain-t Jul 2, 2024
@techanvil techanvil assigned hussain-t and unassigned techanvil Jul 3, 2024
@hussain-t hussain-t assigned techanvil and unassigned hussain-t Jul 3, 2024
ankitrox added a commit that referenced this issue Jul 3, 2024
techanvil added a commit that referenced this issue Jul 3, 2024
…oint

Enhance/#8786 - Implement JS entry point for RRM
@techanvil techanvil removed their assignment Jul 3, 2024
@kelvinballoo kelvinballoo self-assigned this Jul 4, 2024
@kelvinballoo
Copy link
Collaborator

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

@hussain-t
Copy link
Collaborator

@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

@kelvinballoo
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: RRM Reader Revenue Manager module related issues P0 High priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants