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. #8965

Merged
merged 22 commits into from
Jul 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions assets/js/modules/reader-revenue-manager/datastore/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,10 @@ import { validateCanSubmitChanges } from './settings';
export default Modules.createModuleStore( 'reader-revenue-manager', {
storeName: MODULES_READER_REVENUE_MANAGER,
validateCanSubmitChanges,
ownedSettingsSlugs: [ 'publicationID' ],
settingSlugs: [
'publicationID',
'publicationOnboardingState',
'publicationOnboardingStateLastSyncedAtMs',
],
} );
22 changes: 19 additions & 3 deletions assets/js/modules/reader-revenue-manager/datastore/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,26 +30,42 @@ import {
INVARIANT_SETTINGS_NOT_CHANGED,
} from '../../../googlesitekit/data/create-settings-store';
import { createStrictSelect } from '../../../googlesitekit/data/utils';
import { isValidPublicationID } from '../utils/validation';
import {
isValidPublicationID,
isValidOnboardingState,
} from '../utils/validation';

// Invariant error messages.
export const INVARIANT_INVALID_PUBLICATION_ID =
'a valid publicationID is required';

export const INVARIANT_INVALID_PUBLICATION_ONBOARDING_STATE =
'a valid publication onboarding state is required';

export function validateCanSubmitChanges( select ) {
const strictSelect = createStrictSelect( select );
// Strict select will cause all selector functions to throw an error
// if `undefined` is returned, otherwise it behaves the same as `select`.
// This ensures that the selector returns `false` until all data dependencies are resolved.
const { haveSettingsChanged, isDoingSubmitChanges, getPublicationID } =
strictSelect( MODULES_READER_REVENUE_MANAGER );
const {
haveSettingsChanged,
isDoingSubmitChanges,
getPublicationID,
getPublicationOnboardingState,
} = strictSelect( MODULES_READER_REVENUE_MANAGER );

invariant( ! isDoingSubmitChanges(), INVARIANT_DOING_SUBMIT_CHANGES );
invariant( haveSettingsChanged(), INVARIANT_SETTINGS_NOT_CHANGED );

const publicationID = getPublicationID();
const onboardingState = getPublicationOnboardingState();
invariant(
isValidPublicationID( publicationID ),
INVARIANT_INVALID_PUBLICATION_ID
);

invariant(
isValidOnboardingState( onboardingState ),
INVARIANT_INVALID_PUBLICATION_ONBOARDING_STATE
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ import {
createTestRegistry,
unsubscribeFromAll,
} from '../../../../../tests/js/utils';
import { MODULES_READER_REVENUE_MANAGER } from './constants';
import {
INVARIANT_INVALID_PUBLICATION_ID,
INVARIANT_INVALID_PUBLICATION_ONBOARDING_STATE,
validateCanSubmitChanges,
} from './settings';

describe( 'modules/reader-revenue-manager settings', () => {
let registry;
Expand All @@ -45,8 +51,52 @@ describe( 'modules/reader-revenue-manager settings', () => {
} );

describe( 'validateCanSubmitChanges', () => {
it( 'it validates', () => {
// TODO: Implement tests as part of #8793.
it( 'should throw invariant error for invalid publication ID of type number', () => {
const settings = {
publicationID: 12345,
publicationOnboardingState: '',
publicationOnboardingStateLastSyncedAtMs: 0,
};

registry
.dispatch( MODULES_READER_REVENUE_MANAGER )
.setSettings( settings );

expect( () => validateCanSubmitChanges( registry.select ) ).toThrow(
INVARIANT_INVALID_PUBLICATION_ID
);
} );

it( 'should throw invariant error for invalid publication ID with special chars', () => {
const settings = {
publicationID: 'ABCD&*12345',
publicationOnboardingState: 'ONBOARDING_ACTION_REQUIRED',
publicationOnboardingStateLastSyncedAtMs: 0,
};

registry
.dispatch( MODULES_READER_REVENUE_MANAGER )
.setSettings( settings );

expect( () => validateCanSubmitChanges( registry.select ) ).toThrow(
INVARIANT_INVALID_PUBLICATION_ID
);
} );

it( 'should throw invariant error for invalid publication onboarding state', () => {
const settings = {
publicationID: 'ABCDEFGH',
publicationOnboardingState: 'invalid_state',
publicationOnboardingStateLastSyncedAtMs: 0,
};

registry
.dispatch( MODULES_READER_REVENUE_MANAGER )
.setSettings( settings );

expect( () => validateCanSubmitChanges( registry.select ) ).toThrow(
INVARIANT_INVALID_PUBLICATION_ONBOARDING_STATE
);
} );
} );
} );
24 changes: 24 additions & 0 deletions assets/js/modules/reader-revenue-manager/utils/validation.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,30 @@ export function isValidPublicationID( publicationID ) {
);
}

/**
* Checks if the given publication onboarding state is valid.
*
* @since n.e.x.t
*
* @param {string} onboardingState Publication onboarding state.
* @return {boolean} `true` if the given publication ID is valid, `false` otherwise.
*/
export function isValidOnboardingState( onboardingState ) {
if ( typeof onboardingState !== 'string' ) {
return false;
}

// List of valid onboarding states.
const validStates = [
'ONBOARDING_STATE_UNSPECIFIED',
'ONBOARDING_ACTION_REQUIRED',
'PENDING_VERIFICATION',
'ONBOARDING_COMPLETE',
];

return validStates.includes( onboardingState );
}

/**
* Checks if a given URL uses HTTPS.
*
Expand Down
50 changes: 49 additions & 1 deletion includes/Modules/Reader_Revenue_Manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,18 @@
use Google\Site_Kit\Core\Modules\Module;
use Google\Site_Kit\Core\Modules\Module_With_Assets;
use Google\Site_Kit\Core\Modules\Module_With_Assets_Trait;
use Google\Site_Kit\Core\Modules\Module_With_Data_Available_State_Trait;
use Google\Site_Kit\Core\Modules\Module_With_Deactivation;
use Google\Site_Kit\Core\Modules\Module_With_Owner;
use Google\Site_Kit\Core\Modules\Module_With_Owner_Trait;
use Google\Site_Kit\Core\Modules\Module_With_Scopes;
use Google\Site_Kit\Core\Modules\Module_With_Scopes_Trait;
use Google\Site_Kit\Core\Modules\Module_With_Service_Entity;
use Google\Site_Kit\Core\Modules\Module_With_Settings;
use Google\Site_Kit\Core\Modules\Module_With_Settings_Trait;
use Google\Site_Kit\Core\REST_API\Data_Request;
use Google\Site_Kit\Core\Util\URL;
use Google\Site_Kit\Modules\Reader_Revenue_Manager\Settings;
use Google\Site_Kit\Modules\Search_Console\Settings as Search_Console_Settings;
use Google\Site_Kit_Dependencies\Google\Service\SubscribewithGoogle as Google_Service_SubscribewithGoogle;

Expand All @@ -31,8 +38,11 @@
* @access private
* @ignore
*/
final class Reader_Revenue_Manager extends Module implements Module_With_Scopes, Module_With_Assets, Module_With_Service_Entity {
final class Reader_Revenue_Manager extends Module implements Module_With_Scopes, Module_With_Assets, Module_With_Service_Entity, Module_With_Deactivation, Module_With_Owner, Module_With_Settings {
use Module_With_Assets_Trait;
use Module_With_Data_Available_State_Trait;
use Module_With_Owner_Trait;
use Module_With_Settings_Trait;
use Module_With_Scopes_Trait;

/**
Expand Down Expand Up @@ -80,6 +90,44 @@ public function setup_services( Google_Site_Kit_Client $client ) {
);
}

/**
* Checks whether the module is connected.
*
* @since n.e.x.t
*
* @return bool True if module is connected, false otherwise.
*/
public function is_connected() {
$options = $this->get_settings()->get();

if ( ! empty( $options['publicationID'] ) ) {
return true;
}

return false;
}

/**
* Sets up the module's settings instance.
*
* @since n.e.x.t
*
* @return Module_Settings
*/
protected function setup_settings() {
return new Settings( $this->options );
}

/**
* Cleans up when the module is deactivated.
*
* @since n.e.x.t
*/
public function on_deactivation() {
$this->get_settings()->delete();
$this->reset_data_available();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Reader Revenue Manager module, as it currently stands, does not have any data to work with, hence, it should neither use the Module_With_Data_Available_State_Trait trait, nor call the reset_data_available function.

Is there a specific reason this was added? If not, I'm happy to remove this as part of one of my upcoming PRs.

Copy link
Collaborator

@techanvil techanvil Jul 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot @nfmohit 🦅 👁️! I missed this in CR. I believe this should indeed be removed, if you can do it via one of your forthcoming PRs that would be great.

}

/**
* Checks if the current user has access to the current configured service entity.
*
Expand Down
126 changes: 126 additions & 0 deletions includes/Modules/Reader_Revenue_Manager/Settings.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
<?php
/**
* Class Google\Site_Kit\Modules\Reader_Revenue_Manager\Settings
*
* @package Google\Site_Kit\Modules\Reader_Revenue_Manager
* @copyright 2021 Google LLC
* @license https://www.apache.org/licenses/LICENSE-2.0 Apache License 2.0
* @link https://sitekit.withgoogle.com
*/

namespace Google\Site_Kit\Modules\Reader_Revenue_Manager;

use Google\Site_Kit\Core\Modules\Module_Settings;
use Google\Site_Kit\Core\Storage\Setting_With_Owned_Keys_Interface;
use Google\Site_Kit\Core\Storage\Setting_With_Owned_Keys_Trait;
use Google\Site_Kit\Core\Storage\Setting_With_ViewOnly_Keys_Interface;
use Google\Site_Kit\Core\Util\Method_Proxy_Trait;

/**
* Class for RRM settings.
*
* @since n.e.x.t
* @access private
* @ignore
*/
class Settings extends Module_Settings implements Setting_With_Owned_Keys_Interface, Setting_With_ViewOnly_Keys_Interface {

use Setting_With_Owned_Keys_Trait;
use Method_Proxy_Trait;

const OPTION = 'googlesitekit_reader-revenue-manager_settings';

/**
* Various Reader Revenue Manager onboarding statuses.
*/
const ONBOARDING_STATE_UNSPECIFIED = 'ONBOARDING_STATE_UNSPECIFIED';
const ONBOARDING_STATE_ACTION_REQUIRED = 'ONBOARDING_ACTION_REQUIRED';
const ONBOARDING_STATE_PENDING_VERIFICATION = 'PENDING_VERIFICATION';
const ONBOARDING_STATE_COMPLETE = 'ONBOARDING_COMPLETE';

/**
* Registers the setting in WordPress.
*
* @since n.e.x.t
*/
public function register() {
parent::register();

$this->register_owned_keys();
}

/**
* Returns keys for owned settings.
*
* @since n.e.x.t
*
* @return array An array of keys for owned settings.
*/
public function get_owned_keys() {
return array( 'publicationID' );
}

/**
* Gets the default value.
*
* @since n.e.x.t
*
* @return array
*/
protected function get_default() {
return array(
'publicationID' => '',
'publicationOnboardingState' => '',
'publicationOnboardingStateLastSyncedAtMs' => 0,
);
}

/**
* Returns keys for view-only settings.
*
* @since n.e.x.t
*
* @return array An array of keys for view-only settings.
*/
public function get_view_only_keys() {
return array();
}

/**
* Gets the callback for sanitizing the setting's value before saving.
*
* @since n.e.x.t
*
* @return callable|null
*/
protected function get_sanitize_callback() {
return function ( $option ) {
if ( isset( $option['publicationID'] ) ) {
if ( ! preg_match( '/^[a-zA-Z0-9]+$/', $option['publicationID'] ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ankitrox This is inconsistent with the related check we have in JS and does not allow for a valid ID that I am trying to test with. See #8948 (comment)
@techanvil

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this @aaemnnosttv, sorry I missed it during CR. As mentioned on Slack, I've got a followup PR ready if we want to reopen this issue, otherwise I'll create a separate issue for the fix.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: The above PR has now been merged.

$option['publicationID'] = '';
}
}

if ( isset( $option['publicationOnboardingStateLastSyncedAtMs'] ) ) {
if ( ! is_int( $option['publicationOnboardingStateLastSyncedAtMs'] ) ) {
$option['publicationOnboardingStateLastSyncedAtMs'] = 0;
}
}

if ( isset( $option['publicationOnboardingState'] ) ) {
$valid_onboarding_states = array(
self::ONBOARDING_STATE_UNSPECIFIED,
self::ONBOARDING_STATE_ACTION_REQUIRED,
self::ONBOARDING_STATE_PENDING_VERIFICATION,
self::ONBOARDING_STATE_COMPLETE,
);

if ( ! in_array( $option['publicationOnboardingState'], $valid_onboarding_states, true ) ) {
$option['publicationOnboardingState'] = '';
}
}

return $option;
};
}
}
Loading
Loading