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 RRM module settings #8793

Closed
18 tasks done
nfmohit opened this issue Jun 4, 2024 · 7 comments
Closed
18 tasks done

Add RRM module settings #8793

nfmohit opened this issue Jun 4, 2024 · 7 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 4, 2024

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.


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

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

There's no UI to test the feature as of now.

Testing publicationID setting

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

It should give the following object.

  {
      publicationID: "",
      publicationOnboardingState: "",
      publicationOnboardingStateLastSyncedAtMs: 0
  }
  1. Run following command in browser console.
  googlesitekit.data.dispatch( 'modules/reader-revenue-manager' ).setPublicationID('ABCDEFGH');
  googlesitekit.data.dispatch( 'modules/reader-revenue-manager' ).saveSettings();

This should update the settings on server.

  1. Refresh the page and run following command in browser.
  googlesitekit.data.select( 'modules/reader-revenue-manager' ).getSettings();

It should give following output

  {
      publicationID: "ABCDEFGH",
      publicationOnboardingState: "",
      publicationOnboardingStateLastSyncedAtMs: 0
  }

Testing publicationOnboardingState setting

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

It should give the following object.

  {
      publicationID: "ABCDEFGH",
      publicationOnboardingState: "",
      publicationOnboardingStateLastSyncedAtMs: 0
  }
  1. Run following command in browser console.
  googlesitekit.data.dispatch( 'modules/reader-revenue-manager' ).setPublicationOnboardingState('ONBOARDING_ACTION_REQUIRED');
  googlesitekit.data.dispatch( 'modules/reader-revenue-manager' ).saveSettings();

This should update the settings on server.

  1. Refresh the page and run following command in browser.
  googlesitekit.data.select( 'modules/reader-revenue-manager' ).getSettings();

It should give following output

  {
      publicationID: "ABCDEFGH",
      publicationOnboardingState: "ONBOARDING_ACTION_REQUIRED",
      publicationOnboardingStateLastSyncedAtMs: 0
  }

Testing publicationOnboardingStateLastSyncedAtMs setting

  1. Refresh the page and run the following command in browser console.
  googlesitekit.data.select( 'modules/reader-revenue-manager' ).getSettings();

It should give the following object.

  {
      publicationID: "ABCDEFGH",
      publicationOnboardingState: "ONBOARDING_ACTION_REQUIRED",
      publicationOnboardingStateLastSyncedAtMs: 0
  }
  1. Run following command in browser console.
  googlesitekit.data.dispatch( 'modules/reader-revenue-manager' ).setPublicationOnboardingStateLastSyncedAtMs( Math.floor( Date.now() / 1000 ) );
  googlesitekit.data.dispatch( 'modules/reader-revenue-manager' ).saveSettings();

This should update the settings on server.

  1. Refresh the page and run following command in browser.
  googlesitekit.data.select( 'modules/reader-revenue-manager' ).getSettings();

It should give following output, note that timestamp will vary.

  {
      publicationID: "ABCDEFGH",
      publicationOnboardingState: "",
      publicationOnboardingStateLastSyncedAtMs: 01720542503 // This can vary.
  }

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

  1. Activate the module.

  2. Run the following command in browser console. Remember to replace publicationID value with your publication ID in publisher center. It can be found in publisher center URL.

  const settings = {
    publicationID: "CAow6J6vDA",
    publicationOnboardingState: "ONBOARDING_STATE_UNSPECIFIED",
    publicationOnboardingStateLastSyncedAtMs: Date.now()
  };

  googlesitekit.data.dispatch('modules/reader-revenue-manager').setSettings( settings );
  googlesitekit.data.dispatch('modules/reader-revenue-manager').saveSettings();

Refresh the page.

  1. Go to Site Kit > Settings > Reader Revenue Manager, it should show as conneected. This is because publication ID is set.

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

  1. Once above step for connecting the module is done. Run following command in browser console. It may give you undefined at first run, so run it second time again as request may not have fulfilled for the first time.
 googlesitekit.data.select('modules/reader-revenue-manager').getSettings();

It will give you an object something like

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

Notice that this contains values for all the properties.

  1. Now, disconnect the module and activate it again, but do not run connect steps mentioned above. Run this command again (may have to run twice)
 googlesitekit.data.select('modules/reader-revenue-manager').getSettings();

Note that the values for the properties should be empty string and for publicationOnboardingStateLastSyncedAtMs it shuld be zero.

{
    "publicationID": "",
    "publicationOnboardingState": "",
    "publicationOnboardingStateLastSyncedAtMs": 0
}

Changelog entry

  • Add Reader Revenue Manager module settings infrastructure.
@nfmohit nfmohit self-assigned this Jun 4, 2024
@nfmohit nfmohit added Type: Enhancement Improvement of an existing feature Team M Issues for Squad 2 Module: RRM Reader Revenue Manager module related issues labels Jun 4, 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 and removed Module: RRM Reader Revenue Manager module related issues labels Jun 11, 2024
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label Jun 17, 2024
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Jun 18, 2024
@kuasha420 kuasha420 self-assigned this Jun 19, 2024
@kuasha420 kuasha420 assigned kuasha420 and unassigned kuasha420 Jun 28, 2024
@nfmohit nfmohit self-assigned this Jun 30, 2024
@nfmohit
Copy link
Collaborator Author

nfmohit commented Jun 30, 2024

IB ✅

@nfmohit nfmohit removed their assignment Jun 30, 2024
@eclarke1 eclarke1 added Sp Wk 2 Issues to be completed in the second week of the assigned sprint and removed Next Up Issues to prioritize for definition labels Jul 1, 2024
@ankitrox ankitrox self-assigned this Jul 4, 2024
@ankitrox ankitrox mentioned this issue Jul 4, 2024
18 tasks
@ankitrox ankitrox removed their assignment Jul 8, 2024
@techanvil techanvil self-assigned this Jul 9, 2024
@techanvil
Copy link
Collaborator

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.

@techanvil techanvil assigned ankitrox and unassigned techanvil Jul 9, 2024
@ankitrox ankitrox assigned techanvil and unassigned ankitrox Jul 9, 2024
@techanvil techanvil removed their assignment Jul 10, 2024
@eclarke1 eclarke1 removed the Sp Wk 2 Issues to be completed in the second week of the assigned sprint label Jul 11, 2024
@kelvinballoo
Copy link
Collaborator

kelvinballoo commented Jul 16, 2024

QA Update ⚠️

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

  • Testing publicationID setting

    Screenshot 2024-07-16 at 22 08 57
  • Testing publicationOnboardingState setting

    Screenshot 2024-07-16 at 22 12 23
  • Testing publicationOnboardingStateLastSyncedAtMs setting

    Screenshot 2024-07-16 at 22 18 34
  • 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

    Screenshot 2024-07-16 at 22 19 51

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
Copy link
Collaborator

@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

@ankitrox ankitrox removed their assignment Jul 18, 2024
@kelvinballoo
Copy link
Collaborator

kelvinballoo commented Jul 19, 2024

QA Update ⚠️

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

  • Testing publicationID setting

    Screenshot 2024-07-16 at 22 08 57
  • Testing publicationOnboardingState setting

    Screenshot 2024-07-16 at 22 12 23
  • Testing publicationOnboardingStateLastSyncedAtMs setting

    Screenshot 2024-07-16 at 22 18 34
  • 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 ✅

    Screenshot 2024-07-16 at 22 19 51
  • 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.

    Screenshot 2024-07-19 at 11 28 06

    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.

    Screenshot 2024-07-19 at 11 28 32

    After disconnecting the module, the settings are empty :

    Screenshot 2024-07-19 at 11 30 21

@ankitrox
Copy link
Collaborator

ankitrox commented Jul 19, 2024

@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.

@ankitrox ankitrox removed their assignment Jul 19, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

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

  • Testing publicationID setting

    Screenshot 2024-07-16 at 22 08 57
  • Testing publicationOnboardingState setting

    Screenshot 2024-07-16 at 22 12 23
  • Testing publicationOnboardingStateLastSyncedAtMs setting

    Screenshot 2024-07-16 at 22 18 34
  • 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 ✅

    Screenshot 2024-07-16 at 22 19 51
  • 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.

    Screenshot 2024-07-19 at 11 28 06
  • All module settings should be deleted when the module is disconnected.
    Before disconnecting the module, the property settings are available.

    Screenshot 2024-07-19 at 11 28 32

    After disconnecting the module, the settings are empty :

    Screenshot 2024-07-19 at 11 30 21

    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

8 participants