Improve publication onboarding state synchronization. #9469
+420
−393
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Build files for d17f5bd are ready:
|
|
Size Change: +178 B (+0.01%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
nfmohit
requested changes
Oct 12, 2024
There was a problem hiding this comment.
Brilliant work on this, thank you @ankitrox !
- I have left a number of comments in the PR. Could you please go through my comments and update the PR accordingly?
- The major issue that I've noticed is that the "publication approved overlay notification" does not appear when the cron event runs and the publication onboarding state changes to complete. Could you investigate?
- I see that the
publicationOnboardingStateLastSyncedAtMssetting is still used across the codebase, mostly in tests and stories. Can they be removed?
Please let me know if you have any questions. Thank you!
There was a problem hiding this comment.
We should no longer set publicationOnboardingStateLastSyncedAtMs in the selectPublication action.
Comment on lines
68
to
98
| /** | ||
| * User options instance. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @var User_Options | ||
| */ | ||
| protected $user_options; | ||
|
|
||
| /** | ||
| * Constructor. | ||
| * | ||
| * @since n.e.x.t | ||
| * | ||
| * @param Context $context Plugin context. | ||
| * @param Options $options Optional. Option API instance. Default is a new instance. | ||
| * @param User_Options $user_options Optional. User Option API instance. Default is a new instance. | ||
| * @param Authentication $authentication Optional. Authentication instance. Default is a new instance. | ||
| * @param Assets $assets Optional. Assets API instance. Default is a new instance. | ||
| */ | ||
| public function __construct( | ||
| Context $context, | ||
| Options $options = null, | ||
| User_Options $user_options = null, | ||
| Authentication $authentication = null, | ||
| Assets $assets = null | ||
| ) { | ||
| parent::__construct( $context, $options, $user_options, $authentication, $assets ); | ||
| $this->user_options = $user_options; | ||
| } | ||
|
|
There was a problem hiding this comment.
It is not necessary to re-declare the $user_options class property because the parent class, Module, already declares the $user_options class.
Please take a look at other module classes, such as Analytics_4, on how they use the $user_options property.
Suggested change
| /** | |
| * User options instance. | |
| * | |
| * @since n.e.x.t | |
| * | |
| * @var User_Options | |
| */ | |
| protected $user_options; | |
| /** | |
| * Constructor. | |
| * | |
| * @since n.e.x.t | |
| * | |
| * @param Context $context Plugin context. | |
| * @param Options $options Optional. Option API instance. Default is a new instance. | |
| * @param User_Options $user_options Optional. User Option API instance. Default is a new instance. | |
| * @param Authentication $authentication Optional. Authentication instance. Default is a new instance. | |
| * @param Assets $assets Optional. Assets API instance. Default is a new instance. | |
| */ | |
| public function __construct( | |
| Context $context, | |
| Options $options = null, | |
| User_Options $user_options = null, | |
| Authentication $authentication = null, | |
| Assets $assets = null | |
| ) { | |
| parent::__construct( $context, $options, $user_options, $authentication, $assets ); | |
| $this->user_options = $user_options; | |
| } |
| @@ -38,6 +42,8 @@ | |||
| use Google\Site_Kit\Modules\Reader_Revenue_Manager\Web_Tag; | |||
| use Google\Site_Kit\Modules\Search_Console\Settings as Search_Console_Settings; | |||
| use Google\Site_Kit_Dependencies\Google\Service\SubscribewithGoogle as Google_Service_SubscribewithGoogle; | |||
| use Google\Site_Kit\Modules\Reader_Revenue_Manager\Synchronize_OnboardingState; | |||
There was a problem hiding this comment.
While we're at it, let's re-order this import alphabetically. Also, if we do remove the constructor as described above, we'll also need to remove a few unused imports.
There was a problem hiding this comment.
Two points:
- Sanitization should be added for the
publicationOnboardingStateChangedsetting. - Existing sanitization should be removed for the legacy
publicationOnboardingStateLastSyncedAtMssetting.
Comment on lines
2
to
7
| /** | ||
| * Class Google\Site_Kit\Modules\Analytics_4\Synchronize_OnboardingState. | ||
| * | ||
| * @since n.e.x.t | ||
| * @package Google\Site_Kit\Modules\Reader_Revenue_Manager | ||
| */ |
There was a problem hiding this comment.
The header comment for this file appears to be quite different than other classes.
Suggested change
| /** | |
| * Class Google\Site_Kit\Modules\Analytics_4\Synchronize_OnboardingState. | |
| * | |
| * @since n.e.x.t | |
| * @package Google\Site_Kit\Modules\Reader_Revenue_Manager | |
| */ | |
| /** | |
| * Class Google\Site_Kit\Modules\Reader_Revenue_Manager\Synchronize_OnboardingState | |
| * | |
| * @package Google\Site_Kit\Modules\Reader_Revenue_Manager | |
| * @copyright 2024 Google LLC | |
| * @license https://www.apache.org/licenses/LICENSE-2.0 Apache License 2.0 | |
| * @link https://sitekit.withgoogle.com | |
| */ |
| @@ -293,7 +295,6 @@ public function test_get_debug_fields() { | |||
| array( | |||
| 'reader_revenue_manager_publication_id', | |||
| 'reader_revenue_manager_publication_onboarding_state', | |||
| 'reader_revenue_manager_publication_onboarding_state_last_synced_at', | |||
There was a problem hiding this comment.
According to my observation, I don't think any other change apart from this one is needed in this test suite.
| $this->assertFalse( $settings['publicationOnboardingStateChanged'] ); | ||
|
|
||
| do_action( Synchronize_OnboardingState::CRON_SYNCHRONIZE_ONBOARDING_STATE ); | ||
| $test_synced_at = time(); |
There was a problem hiding this comment.
Suggested change
| $test_synced_at = time(); |
Comment on lines
152
to
165
| // Get cron list and check if the cron is scheduled. | ||
| $crons = _get_cron_array(); | ||
| $cron_exists = false; | ||
|
|
||
| foreach ( $crons as $timestamp => $cron_events ) { | ||
| foreach ( $cron_events as $event_key => $cron_event ) { | ||
| if ( Synchronize_OnboardingState::CRON_SYNCHRONIZE_ONBOARDING_STATE === $event_key ) { | ||
| $cron_exists = true; | ||
| break 2; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| $this->assertTrue( $cron_exists ); |
There was a problem hiding this comment.
I believe we can simplify this.
Suggested change
| // Get cron list and check if the cron is scheduled. | |
| $crons = _get_cron_array(); | |
| $cron_exists = false; | |
| foreach ( $crons as $timestamp => $cron_events ) { | |
| foreach ( $cron_events as $event_key => $cron_event ) { | |
| if ( Synchronize_OnboardingState::CRON_SYNCHRONIZE_ONBOARDING_STATE === $event_key ) { | |
| $cron_exists = true; | |
| break 2; | |
| } | |
| } | |
| } | |
| $this->assertTrue( $cron_exists ); | |
| $this->assertTrue( | |
| (bool) wp_next_scheduled( Synchronize_OnboardingState::CRON_SYNCHRONIZE_ONBOARDING_STATE ) | |
| ); |
| @@ -170,7 +177,7 @@ export default { | |||
| .dispatch( MODULES_ANALYTICS_4 ) | |||
| .receiveGetAccountSummaries( { | |||
| accountSummaries, | |||
| nextPageToken: null, | |||
| nextPageToken: '', | |||
There was a problem hiding this comment.
I don't think we need to change this.
Suggested change
| nextPageToken: '', | |
| nextPageToken: null, |
Comment on lines
94
to
100
| const shouldShowNotification = | ||
| isDismissed === false && | ||
| showApprovedNotificationUI === true && | ||
| ! isViewOnly && | ||
| dashboardType === VIEW_CONTEXT_MAIN_DASHBOARD; | ||
| dashboardType === VIEW_CONTEXT_MAIN_DASHBOARD && | ||
| initialPublicationOnboardingStateChanged.current === true && | ||
| publicationOnboardingState === ONBOARDING_COMPLETE; |
There was a problem hiding this comment.
Currently, the overlay notification no longer appears for me, because no one sets the value for UI_KEY_READER_REVENUE_MANAGER_SHOW_PUBLICATION_APPROVED_NOTIFICATION in the CORE_UI store, which is only done when the sync is run on the client-side.
The logic here should be changed so that the notification is displayed when either the UI store value is set, OR, the new conditions are met.
Somewhat like the following:
const shouldShowNotification =
isDismissed === false &&
! isViewOnly &&
dashboardType === VIEW_CONTEXT_MAIN_DASHBOARD &&
( showApprovedNotificationUI === true ||
( initialPublicationOnboardingStateChanged.current === true &&
publicationOnboardingState === ONBOARDING_COMPLETE ) );Could you please investigate and ensure that the notification appears?
|
Thank you @nfmohit for reviewing the PR and adding the feedback. I've addressed the feedback and left response for couple of comments. Over to you for another round of review. Thanks again. |
This was referenced Oct 15, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist