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

Set Correct Owned Keys For Thank With Google Settings #5587

Closed
kuasha420 opened this issue Jul 21, 2022 · 5 comments
Closed

Set Correct Owned Keys For Thank With Google Settings #5587

kuasha420 opened this issue Jul 21, 2022 · 5 comments
Labels
Good First Issue Good first issue for new engineers Module: Thank with Google Thank with Google module related issues P0 High priority PHP Type: Enhancement Improvement of an existing feature

Comments

@kuasha420
Copy link
Contributor

kuasha420 commented Jul 21, 2022

Feature Description

When converting old SwG Settings Class to TwG Class, we accidentally made All the settings keys owned keys which is not correct. Only the publicationID should be an owned key. Let's remove the others from the owned keys and add tests accordingly.


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

Acceptance criteria

  • publicationID should be the only owned setting for Thank with Google
  • The Google\Site_Kit\Tests\Modules\Thank_With_Google\SettingsTest (added in Add enhanced Thank with Google setting validation #5461) test case should implement the Setting_With_Owned_Keys_Trait for consistency with others

Implementation Brief

  • In the get_owned_keys method of Google\Site_Kit\Modules\Thank_With_Google\Settings class:
    • Only keep publicationID and remove others from the return array.
  • In Google\Site_Kit\Tests\Modules\Thank_With_Google\SettingsTest class:
    • Import and use Google\Site_Kit\Tests\Core\Storage\Setting_With_Owned_Keys_ContractTests.
    • Implement the get_setting_with_owned_keys protected method.
    • See Google\Site_Kit\Tests\Modules\Tag_Manager\SettingsTest for an example.

Test Coverage

  • The existing and newly added TwG tests should pass.

QA Brief

This is maybe best QA'd as a QA:Eng as we don't have the UI to connect TwG module or change settings.

  • Complete TwG Setup by pasting the following snippet in browser console after clicking Connect Service on Thank with Google.
( async () => {
	await googlesitekit.data
		.dispatch( 'modules/thank-with-google' )
		.setPublicationID( 'Test-Pub_id' );
	await googlesitekit.data
		.dispatch( 'modules/thank-with-google' )
		.setColorTheme( 'blue' );
	await googlesitekit.data
		.dispatch( 'modules/thank-with-google' )
		.setButtonPlacement( 'dynamic_low' );
	await googlesitekit.data
		.dispatch( 'modules/thank-with-google' )
		.setButtonPostTypes( [ 'post' ] );
	await googlesitekit.data
		.dispatch( 'modules/thank-with-google' )
		.saveSettings();
	window.location.href = '/wp-admin/admin.php?page=googlesitekit-settings#/connected-services';
} )();
  • Check the current TwG owner ID with the following snippet.
googlesitekit.data
		.select( 'modules/thank-with-google' ).getOwnerID()
  • Switch to another admin account and connect site kit.
  • Change a non-owned key such as buttonPostType using the following snippet:
( async () => {
	await googlesitekit.data
		.dispatch( 'modules/thank-with-google' )
		.setButtonPostTypes( [ 'post', 'page' ] );
	await googlesitekit.data
		.dispatch( 'modules/thank-with-google' )
		.saveSettings();
	window.location.href = '/wp-admin/admin.php?page=googlesitekit-settings#/connected-services';
} )();
  • Make sure the change is saved.
googlesitekit.data
		.select( 'modules/thank-with-google' ).getButtonPostTypes()
  • Check that the ownerID has not been changed.
googlesitekit.data
		.select( 'modules/thank-with-google' ).getOwnerID()
  • Now change the publicationID.
( async () => {
	await googlesitekit.data
		.dispatch( 'modules/thank-with-google' )
		.setPublicationID( 'Test-Pub_NewId' );
	await googlesitekit.data
		.dispatch( 'modules/thank-with-google' )
		.saveSettings();
	window.location.href = '/wp-admin/admin.php?page=googlesitekit-settings#/connected-services';
} )();
  • The ownerID should now change to the new admins ID.
googlesitekit.data
		.select( 'modules/thank-with-google' ).getOwnerID()
  • Rerun the get snippets if first time they return undefined.

Changelog entry

  • Update the Thank with Google module's owned settings.
@kuasha420 kuasha420 added P0 High priority Type: Enhancement Improvement of an existing feature PHP Module: Thank with Google Thank with Google module related issues labels Jul 21, 2022
@aaemnnosttv aaemnnosttv added the QA: Eng Requires specialized QA by an engineer label Jul 21, 2022
@aaemnnosttv aaemnnosttv removed their assignment Jul 21, 2022
@kuasha420 kuasha420 self-assigned this Jul 21, 2022
@kuasha420 kuasha420 added the Good First Issue Good first issue for new engineers label Jul 21, 2022
@kuasha420 kuasha420 removed their assignment Jul 21, 2022
@techanvil techanvil self-assigned this Jul 22, 2022
@techanvil
Copy link
Collaborator

IB ✅

@techanvil techanvil removed their assignment Jul 22, 2022
@kuasha420 kuasha420 removed their assignment Jul 22, 2022
@jimmymadon jimmymadon assigned jimmymadon and unassigned jimmymadon Jul 24, 2022
@eugene-manuilov
Copy link
Collaborator

  • No need to QA this regularly as long as the tests pass.

@kuasha420 we need to make sure that the owner ID is not getting changed when other settings are changed. Could you please update QAB with instructions how to change TwG settings and how to verify that the owner ID hasn't changed?

@eugene-manuilov eugene-manuilov removed the QA: Eng Requires specialized QA by an engineer label Jul 25, 2022
@kuasha420 kuasha420 removed their assignment Jul 26, 2022
@wpdarren wpdarren self-assigned this Jul 26, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Jul 26, 2022

QA Update: ⚠️

@kuasha420 thank you so much for the detailed QAB, this helped a lot! I just have one observation.

I pasted the code to change a non-owned key such as buttonPostType using the snippet. That ran with no errors.

When I ran this code I received an error:

googlesitekit.data
		.select( 'modules/thank-with-google' ).setButtonPostTypes()

Error message:

VM1397:2 Uncaught TypeError: googlesitekit.data.select(...).setButtonPostTypes is not a function
    at <anonymous>:2:42

All of the other steps worked perfect, even the steps after running this code. Any thoughts?

@kuasha420
Copy link
Contributor Author

@wpdarren I made a copy paste error here as the snippet would be the following:

googlesitekit.data
		.select( 'modules/thank-with-google' ).getButtonPostTypes()

Also rerun the snippets to get if it returns undefined first time.

Cheers.

@kuasha420 kuasha420 removed their assignment Jul 26, 2022
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • Ran through the steps in the QAB:
    • Checked the current TwG owner ID with the following snippet. (it appeared as '1')
    • Switched to another admin account and connected site kit
    • Changed a non-owned key buttonPostType using the snippet.
    • Confirmed that the change had saved to the buttonPostType
    • Confirmed that the ownerID has not been changed. (was still appearing as '1')
    • Changed the publicationID and confirmed that the ownerID has changed (it appeared as '2')
Screenshots

image
image

@wpdarren wpdarren removed their assignment Jul 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers Module: Thank with Google Thank with Google module related issues P0 High priority PHP Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants