Add RRM module settings

[nfmohit]

Feature Description

The module settings required for Reader Revenue Manager should be added both in the server-side and datastore, including their validation. The module setting values should also be made available in the Site Kit debug information.

This issue should also be responsible for adding the is_connected and on_deactivation methods for the Modules\Reader_Revenue_Manager class, where the module is considered connected when the publicationID module setting is set, and all the module settings are deleted on deactivation.

• See relevant section in the design doc.

---

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

Acceptance criteria

• Module settings for the Reader Revenue Manager module should be added in both the server and client sides.
• The module settings that should be added are:
  • publicationID (string); Default: Empty string; Owned setting.
  • publicationOnboardingState (string); Default: Empty string.
  • publicationOnboardingStateLastSyncedAtMs (integer); Default: 0 (zero).
• Necessary validation should be implemented for the added module settings.
• Connection logic should be implemented for the module so that it is considered connected with the publicationID module setting set.
• All module settings should be deleted when the module is disconnected.

Implementation Brief

• [x] Create Google\Site_Kit\Modules\Reader_Revenue_Manager\Settings in includes/Modules/Reader_Revenue_Manager/Settings.php file.
  • [x] The class should extend from Module_Settings and implement Setting_With_Owned_Keys_Interface
  • [x] This class should use the Setting_With_Owned_Keys_Trait trait.
  • [x] Define OPTION constant with the value googlesitekit_reader-revenue-manager_settings
  • [x] Define register function which calls parent::register() and $this->register_owned_keys() (provided by the trait) to register the owned keys.
  • [x] Define get_owned_keys function that returns an array with the owned key defined in the ACs. ie. publicationID.
  • [x] Define get_default function with the keys and default values defined in the ACs.
  • [x] Define get_sanitize_callback to ensure that the types of the settings fields are verified before saving.
• [x] In includes/Modules/Reader_Revenue_Manager.php:
  • [x] In the Reader_Revenue_Manager class implement Module_With_Deactivation, Module_With_Owner and Module_With_Settings interfaces.
    • [x] Use Module_With_Owner_Trait and Module_With_Settings_Trait traits.
    • [x] Define setup_settings function that returns a new instance of Google\Site_Kit\Modules\Reader_Revenue_Manager\Settings.
  • [x] Define is_connected function that returns false if publicationID is not set (ie. the default empty string). Otherwise return parent::is_connected().
  • [x] Define on_deactivation function that deletes the module settings.
• [x] In assets/js/module/reader-revenue-manager/datastore/base.js:
  • [x] Pass the newly added settings slugs in the createModuleStore call.
• [x] In assets/js/module/reader-revenue-manager/datastore/settings.js:
  • [x] Add/update validation logic for the newly introduced settings fields.

Test Coverage

• Implement tests for newly added settings class using SettingsTestCase in tests/phpunit/integration/Modules/Reader_Revenue_Manager/SettingsTest.php
• In tests/phpunit/integration/Modules/Reader_Revenue_ManagerTest.php:
  • Implement Module_With_Settings_ContractTests.
  • Add test coverage for connection and deactivation logic.
• Add Jest test coverage for newly added settings and validation.

QA Brief

Changelog entry

[nfmohit]

IB ✅

[techanvil]

Hi @ankitrox, nice work so far on the QAB here too. I'd suggest that it states to "repeat the above for the publicationOnboardingState and publicationOnboardingStateLastSyncedAtMs settings", or something along those lines, and also touches on the expected valid value ranges for these settings.

[kelvinballoo]

QA Update ⚠️

Tested this as per the ACs and these are working as expected:

• Testing publicationID setting

  

• Testing publicationOnboardingState setting

  

• Testing publicationOnboardingStateLastSyncedAtMs setting

  

• The above tests were done without SK being set up but with the feature flag for RRM on. I set up SK and the results were consistent.

• I also tested with the feature flag off and the scripts would have errors and this is expected

  

QUERY ⚠️ The main things I wanted to double confirm are whether we should do any other particular tests to validate those points from the ACs:

• Necessary validation should be implemented for the added module settings.
• Connection logic should be implemented for the module so that it is considered connected with the publicationID module setting set.
• All module settings should be deleted when the module is disconnected.

Based on my understanding, the QAB steps do not address those AC points but I could be wrong.

[ankitrox]

@kelvinballoo

Necessary validation should be implemented for the added module settings.

We have got it covered via the test case here

Connection logic should be implemented for the module so that it is considered connected with the publicationID module setting set.

Added in QAB

All module settings should be deleted when the module is disconnected.

Added in QAB

[kelvinballoo]

QA Update ⚠️

Tested this as per the ACs and these are working as expected:

• Testing publicationID setting ✅

  

• Testing publicationOnboardingState setting ✅

  

• Testing publicationOnboardingStateLastSyncedAtMs setting ✅

  

• The above tests were done without SK being set up but with the feature flag for RRM on. I set up SK and the results were consistent. ✅

• I also tested with the feature flag off and the scripts would have errors and this is expected ✅

  

• Connection logic should be implemented for the module so that it is considered connected with the publicationID module setting set. ⚠️ After running the script and reloading the page, the RRM module is now connected.

  

  One thing to clarify is the order of the Owner ID. ⚠️ Currently, it's returning as the first one, compared to the QAB where it is in the last position. Do we think that's an issue @ankitrox

  QAB Reference:

   {
       "publicationID": "CAow6J6vDA",
       "publicationOnboardingState": "ONBOARDING_STATE_UNSPECIFIED",
       "publicationOnboardingStateLastSyncedAtMs": 1721316244959,
       "ownerID": 1
   }

• All module settings should be deleted when the module is disconnected. ✅ Before disconnecting the module, the property settings are available.

  

  After disconnecting the module, the settings are empty :

  

[ankitrox]

@kelvinballoo Property positioning in the object does not matter.

   {
       "publicationID": "CAow6J6vDA",
       "publicationOnboardingState": "ONBOARDING_STATE_UNSPECIFIED",
       "publicationOnboardingStateLastSyncedAtMs": 1721316244959,
       "ownerID": 1
   }

OR

   {
       "ownerID": 1,
        "publicationOnboardingStateLastSyncedAtMs": 1721316244959,
         "publicationOnboardingState": "ONBOARDING_STATE_UNSPECIFIED",
       "publicationID": "CAow6J6vDA",
   }

Both are okay provided that values are same.

[kelvinballoo]

QA Update ✅

Tested this as per the ACs and these are working as expected:

• Testing publicationID setting ✅

  

• Testing publicationOnboardingState setting ✅

  

• Testing publicationOnboardingStateLastSyncedAtMs setting ✅

  

• The above tests were done without SK being set up but with the feature flag for RRM on. I set up SK and the results were consistent. ✅

• I also tested with the feature flag off and the scripts would have errors and this is expected ✅

  

• Connection logic should be implemented for the module so that it is considered connected with the publicationID module setting set. ✅
  After running the script and reloading the page, the RRM module is now connected.

  

• All module settings should be deleted when the module is disconnected. ✅ Before disconnecting the module, the property settings are available.

  

  After disconnecting the module, the settings are empty :

  

  Moving ticket to Approval.