-
Notifications
You must be signed in to change notification settings - Fork 278
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
Comments
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 ✅ |
Hi @ankitrox, IB is looking good. Couple of points.
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! |
Thank you @ankitrox for all the work here. I've made some slight edits regarding IB 🍏 |
…gle/site-kit-wp into enhance/#8786-add-rrm-js-entry-point.
…oint Enhance/#8786 - Implement JS entry point for RRM
QA Update
|
@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.
Moving ticket to Approval. |
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 thesubmitChanges
&validateCanSubmitChanges
actions. The module registration should also include acheckRequirements
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 thesetup_assets
method.Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
modules/reader-revenue-manager
.SettingsEdit
,SettingsView
, andSetupMain
.submitChanges
,rollbackChanges
, andvalidateCanSubmitChanges
.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
reader-revenue-manager
insideassets/js/module/
.assets/js/module/reader-revenue-manager/datastore/base.js
. This will be a base store.Modules.createModuleStore
.reader-revenue-manager
.reader-revenue-manager
.validateCanSubmitChanges
described below.SettingsEdit
andSettingsView
insideassets/js/module/reader-revenue-manager/components/settings/
SetupMain
insideassets/js/module/reader-revenue-manager/components/setup/
assets/js/module/reader-revenue-manager/index.js
. UseregisterModule
to register the module. Take this reference on how it can be registered.checkRequirements
function toregisterModule
. IncheckRequirements
function verify that site is using SSL which can be done by usinggetHomeURL
selector fromCORE_SITE
store.checkRequirements
withNON_HTTPS_SITE
error code and the message defined in the ACs as message when site is not https.assets/js/module/reader-revenue-manager/datastore/settings.js
validateCanSubmitChanges
and validate the following.getPublications
selector.Invariant
in case of error.submitChanges
androllbackChanges
action as those will automatically be created bycreate-settings-store
viaModules.createModuleStore
.webpack/modules.config.js
, add following config. We needThe bundled file (generated assets) needs to be enqueued from within
Modules\Reader_Revenue_Manager
class usingsetup_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 constantNEW_MODULES
which should be array[ 'ads', 'reader-revenue-manager' ]
assets/js/components/settings/SetupModule.js
, update this check to check if module slug is present inNEW_MODULES
constant created above, in order to display "New" badge.assets/js/components/settings/SettingsActiveModule/Header.js
.Test Coverage
QA Brief
rrmModule
feature flag via the tester plugin.HTTPS
protocol-enabled site thatSet up Reader Revenue Manager
is clickable.Cancel
button as we haven't implemented the setup form. It will take us to the "Connected Services" screen.HTTP
site thatSet up Reader Revenue Manager
is disabled and shouldn't be able to setup.The site should use HTTPS to set up Reader Revenue Manager
warning should be displayed.rrmModule
feature flag and verify the "Reader Revenue Manager" module is unavailable in the Connect More Services.Modules > ReaderRevenueManager > Setup > SetupMain
Modules > ReaderRevenueManager > Settings > SettingsEdit
Modules > ReaderRevenueManager > Settings > SettingsView
Changelog entry
The text was updated successfully, but these errors were encountered: