Skip to content

Reset RRM module settings when publication ID is changed #9953

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

Closed
4 tasks done
nfmohit opened this issue Jan 2, 2025 · 8 comments
Closed
4 tasks done

Reset RRM module settings when publication ID is changed #9953

nfmohit opened this issue Jan 2, 2025 · 8 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 Jan 2, 2025

Feature Description

When the RRM publication is changed, all the module setting values should be reset to their defaults.


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

Acceptance criteria

  • If the selected publication in the Reader Revenue Manager settings edit screen is changed, the following module settings should be reset to their default values:
    • publicationOnboardingStateChanged
    • productID (only if the rrmModuleV2 feature flag is enabled)

Implementation Brief

  • In assets/js/modules/reader-revenue-manager/datastore/publications.js:
    • Update the selectPublication action:
      • Update the settings object when defined to include a publicationOnboardingStateChanged property with value false.
      • Inside the check for the rrmModuleV2 feature flag, append a productID property to the settings object with a value of openaccess.

Test Coverage

  • In assets/js/modules/reader-revenue-manager/datastore/publications.test.js:
    • Update the test coverage for selectPublication to cover the above changes.

QA Brief

Note: Make sure that you have two publications available for the site in publisher center, else create two.

  1. Install the cron manager plugin. Enable the rrrmModuleV2 flag.

  2. In test plugin settings, "Reader Revenue Manager > Force Reader Revenue Manager publication onboarding state", set this to ONBOARDING_ACTION_REQUIRED.

  3. Setup RRM and connect to the publication which has status ONBOARDING_COMPLETE.

  4. In test plugin settings, "Reader Revenue Manager > Force Reader Revenue Manager publication onboarding state", set this to No onboarding state enforced (default).

  5. Go to Tools > Cron manager, execute the cron googlesitekit_cron_synchronize_publication.

  6. Go to Site Kit > Settings > Reader Revenue Manager, click "Edit".

  7. Run the following command in broweser console.

  googlesitekit.data.dispatch( 'modules/reader-revenue-manager' ).setProductID( 'test' );
  1. Run the following command in browser console.
  googlesitekit.data.select( 'modules/reader-revenue-manager' ).getSettings();

It should give output like (but not same):

{
    "ownerID": 1,
    "publicationID": "CAow8pTZCw",
    "publicationOnboardingState": "ONBOARDING_COMPLETE",
    "publicationOnboardingStateChanged": true,
    "snippetMode": "post_types",
    "postTypes": [
        "post"
    ],
    "productID": "test",
    "productIDs": [
        "Privilege First",
        "supercharge",
        "sample",
        "advanced",
        "basic",
        "intermediate"
    ],
    "paymentOption": "noPayment"
}
  1. Select another publication in the dropdown and the command given above once again. publicationOnboardingStateChanged should be false and productID should be openaccess.

Changelog entry

  • Add mechanism to reset publication-specific information when the publication is changed.
@nfmohit nfmohit added P0 High priority Type: Enhancement Improvement of an existing feature Team M Issues for Squad 2 labels Jan 2, 2025
@hussain-t hussain-t self-assigned this Jan 6, 2025
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label Jan 13, 2025
@hussain-t hussain-t assigned nfmohit and unassigned hussain-t Jan 15, 2025
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jan 16, 2025

Note for AC reviewer

The AC has been updated to reset only the publicationOnboardingStateChanged and productID module settings instead of all module settings. I'll explain the reason in the following.

We have largely two types of module settings in Reader Revenue Manager:

  1. User preferences, such as:
    1. snippetMode
    2. postTypes
  2. A "cached" value of information received from the API, such as:
    1. publicationOnboardingState
    2. productIDs
    3. paymentOption

We do not want to reset the user preference module settings because they may still be relevant after changing the publication.

We also do not want to reset the publication information because they are set automatically when the user selects a publication, thanks to the selectPublication action.

The only outliers are:

  1. productID: The set default product ID will likely not be available in the newly selected publication.
  2. publicationOnboardingStateChanged: This is a module setting that is essentially used as a UI-aid. It should be reset as well.

Thanks!

@nfmohit nfmohit removed their assignment Jan 16, 2025
@nfmohit nfmohit added the Module: RRM Reader Revenue Manager module related issues label Jan 18, 2025
@techanvil techanvil self-assigned this Jan 22, 2025
@techanvil
Copy link
Collaborator

techanvil commented Jan 22, 2025

Hi @nfmohit, thanks for drafting the AC, as well as the clarifying note.

I'd suggest the AC could be a bit more specific about what's meant by the connected publication being changed. Technically a publication is only really "connected" when the new publicationID is persisted to the DB, but it seems we might want to do a reset on the client-side when selecting a new publication from the dropdown prior to saving it, so the product ID selection resets to the default selection, as the product ID might otherwise refer to a value which is not present in the newly selected publication.

We could flesh this out in the IB, but it feels like more of a requirement than an implementation detail and it would help the IB author to include this in the AC.

@techanvil techanvil assigned nfmohit and unassigned techanvil Jan 22, 2025
@nfmohit nfmohit assigned techanvil and unassigned nfmohit Jan 23, 2025
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jan 23, 2025

@techanvil As discussed, I've updated the AC according to your feedback. While I was at it, I also added an IB, which, if you think looks good, this can be moved directly to EB. Thank you!

@techanvil
Copy link
Collaborator

Thanks @nfmohit! Sorry, though - I should have phrased my previous suggestion like so:

we might also want to do a reset on the client-side

I.e. maybe we do still want to do a server-side reset in the change handler too. Maybe not - it depends if we expect the publication ID to change via a route other than updating it via the selectPublication() selector. Can you think of a scenario where that would apply?

@techanvil techanvil assigned nfmohit and unassigned techanvil Jan 24, 2025
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jan 25, 2025

Thank you @techanvil. I did think about that and at this point, I don't think the publication would change without the help of selectPublication.

@nfmohit nfmohit assigned techanvil and unassigned nfmohit Jan 25, 2025
@techanvil
Copy link
Collaborator

Thanks @nfmohit, good to know! In that case, these AC LGTM.

AC ✅

@techanvil techanvil removed their assignment Jan 27, 2025
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Jan 27, 2025
@techanvil techanvil self-assigned this Jan 28, 2025
@techanvil
Copy link
Collaborator

techanvil commented Jan 28, 2025

Thanks @nfmohit, this IB LGTM. I did make a small tweak to remove the "towards the end" specification for appending productID, as it seems this could reasonably be done toward the start of the conditional block where the other properties are being set on the settings object.

IB ✅

@techanvil techanvil removed their assignment Jan 28, 2025
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Jan 28, 2025
@ivonac4 ivonac4 removed the Next Up Issues to prioritize for definition label Jan 29, 2025
@ankitrox ankitrox self-assigned this Jan 29, 2025
@ankitrox ankitrox removed their assignment Jan 30, 2025
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Jan 31, 2025
@mohitwp mohitwp self-assigned this Feb 3, 2025
@mohitwp
Copy link
Collaborator

mohitwp commented Feb 3, 2025

QA Update ✅

  • Tested on dev environment.
  • Tested when rrmModuleV2 feature flag is enabled.
  • Tested by following the steps mentioned under QAB.
  • After executing cron ran the command googlesitekit.data.select( 'modules/reader-revenue-manager' ).getSettings(); and it's gives output where "productID": "test" and publicationOnboardingStateChanged : true

Image

  • Verified that after changing the publication id publicationOnboardingStateChanged set to false and productID set to openaccess.

Image

Recording.1762.mp4

@mohitwp mohitwp removed their assignment Feb 3, 2025
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