Erase unsaved Dashboard Sharing settings #5407

nfmohit · 2022-06-22T08:07:45Z

Summary

Addresses issue:

Dashboard sharing modal state is preserved after closing #5372

Relevant technical choices

This PR erases the Dashboard Sharing settings which haven't been saved yet upon closing the Dashboard Sharing settings modal. This:

adds a new action rollbackSharingSettings.
in the reducer, set this action to reset sharingSettings to savedSharingSettings.
upon closing the Dashboard Sharing settings modal, checks if the Dashboard Sharing settings have been changed with haveSharingSettingsChanged and if so, dispatches the new rollbackSharingSettings action.
adds tests for the rollbackSharingSettings action.

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 4.7 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

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

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

hussain-t

Thanks, @nfmohit. The code changes looks good. However there are a couple of concerns to be addressed:

This is not mentioned in the IB; however, can we add a test case to cover this scenario in assets/js/components/dashboard-sharing/DashboardSharingSettings/index.test.js?
This is not working when I click outside of the modal. Could you please investigate on this? Perhaps @asvinb could help on this.

github-actions · 2022-06-22T10:40:23Z

Size Change: +127 B (0%)

Total Size: 1.34 MB

Filename	Size	Change
`./dist/assets/js/googlesitekit-dashboard-********************.js`	61.2 kB	+54 B (0%)
`./dist/assets/js/googlesitekit-dashboard-details-********************.js`	54.5 kB	+46 B (0%)
`./dist/assets/js/googlesitekit-modules-********************.js`	19.2 kB	+27 B (0%)

ℹ️ View Unchanged

Filename	Size
`./dist/assets/css/googlesitekit-admin-css-********************.min.css`	45.5 kB
`./dist/assets/css/googlesitekit-adminbar-css-********************.min.css`	10.9 kB
`./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css`	6.32 kB
`./dist/assets/js/31-********************.js`	51.9 kB
`./dist/assets/js/32-********************.js`	3.12 kB
`./dist/assets/js/analytics-advanced-tracking-********************.js`	769 B
`./dist/assets/js/googlesitekit-activation-********************.js`	27.4 kB
`./dist/assets/js/googlesitekit-adminbar-********************.js`	36.9 kB
`./dist/assets/js/googlesitekit-api-********************.js`	9.15 kB
`./dist/assets/js/googlesitekit-base-********************.js`	1.13 kB
`./dist/assets/js/googlesitekit-dashboard-splash-********************.js`	70.4 kB
`./dist/assets/js/googlesitekit-data-********************.js`	2.09 kB
`./dist/assets/js/googlesitekit-datastore-forms-********************.js`	8.78 kB
`./dist/assets/js/googlesitekit-datastore-location-********************.js`	2.09 kB
`./dist/assets/js/googlesitekit-datastore-site-********************.js`	14.5 kB
`./dist/assets/js/googlesitekit-datastore-ui-********************.js`	8.88 kB
`./dist/assets/js/googlesitekit-datastore-user-********************.js`	21.3 kB
`./dist/assets/js/googlesitekit-i18n-********************.js`	3.92 kB
`./dist/assets/js/googlesitekit-idea-hub-notice-********************.js`	45.1 kB
`./dist/assets/js/googlesitekit-idea-hub-post-list-********************.js`	25.5 kB
`./dist/assets/js/googlesitekit-modules-adsense-********************.js`	66.1 kB
`./dist/assets/js/googlesitekit-modules-analytics-4-********************.js`	19 kB
`./dist/assets/js/googlesitekit-modules-analytics-********************.js`	68.6 kB
`./dist/assets/js/googlesitekit-modules-idea-hub-********************.js`	27.8 kB
`./dist/assets/js/googlesitekit-modules-optimize-********************.js`	19.3 kB
`./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js`	18.3 kB
`./dist/assets/js/googlesitekit-modules-search-console-********************.js`	38.8 kB
`./dist/assets/js/googlesitekit-modules-tagmanager-********************.js`	30.5 kB
`./dist/assets/js/googlesitekit-modules-thank-with-google-********************.js`	17.7 kB
`./dist/assets/js/googlesitekit-polyfills-********************.js`	379 B
`./dist/assets/js/googlesitekit-settings-********************.js`	50.5 kB
`./dist/assets/js/googlesitekit-user-input-********************.js`	44.8 kB
`./dist/assets/js/googlesitekit-vendor-********************.js`	323 kB
`./dist/assets/js/googlesitekit-widgets-********************.js`	18.7 kB
`./dist/assets/js/googlesitekit-wp-dashboard-********************.js`	60.2 kB
`./dist/assets/js/runtime-********************.js`	1.22 kB

_{compressed-size-action}

nfmohit · 2022-06-22T17:15:13Z

Thank you for the review @hussain-t!

Working on it.
Pushed a fix for it.

I'll circle around once I'm done with the test case. Thanks!

hussain-t · 2022-06-23T12:06:40Z

@nfmohit, The expected scenario tests are supposed to be done in the DashboardSharingSettingsButton component. However, I just realized we don't have a test file for that component. So, I think it's out of the scope to add tests for the DashboardSharingSettingsButton component. My point for adding a test is to have bulletproof testing coverage.

We can proceed without adding the test.

hussain-t

Thanks, @nfmohit. I don't think we need to have a param in the closeDialog callback. Otherwise it's good to go.

assets/js/components/dashboard-sharing/DashboardSharingSettingsButton.js

hussain-t

Thanks, @nfmohit. FYI, the closeDialog callback is being used in the onApply callback. We are waiting for await saveSharingSettings() before we call the closeDialog. So by that point of time the new savedSharingSettings will be fetched and set in the state. Anyways this is not a big problem. I will leave it to the merge reviewer to take a call 👍

aaemnnosttv

Thanks @nfmohit, this is a great start just a few small things to address then this should be good to go 👍

assets/js/components/dashboard-sharing/DashboardSharingSettingsButton.js

assets/js/googlesitekit/modules/datastore/sharing-settings.test.js

nfmohit · 2022-06-24T15:44:21Z

Thanks @nfmohit, this is a great start just a few small things to address then this should be good to go 👍

Thank you so much for the detailed and kind review @aaemnnosttv! I'm working on it and will circle back once done.

aaemnnosttv · 2022-06-27T18:29:39Z

@nfmohit would you please resolve the merge conflict?

aaemnnosttv

Great, thanks @nfmohit !

nfmohit · 2022-06-28T01:15:09Z

Thank you so much @aaemnnosttv! 🙏

nfmohit added 4 commits June 22, 2022 13:15

Add action to rollback sharing settings.

683c538

Update reducer to restore sharing settings.

e881fd2

Dispatch rollback action if settings have changed.

595bd44

Add tests for the rollbackSharingSettings action.

82ef4b5

nfmohit marked this pull request as ready for review June 22, 2022 08:10

hussain-t requested changes Jun 22, 2022

View reviewed changes

Move the rollback functionality to the parent component.

6eaa841

nfmohit requested a review from hussain-t June 23, 2022 12:38

hussain-t requested changes Jun 23, 2022

View reviewed changes

hussain-t approved these changes Jun 23, 2022

View reviewed changes

aaemnnosttv requested changes Jun 23, 2022

View reviewed changes

nfmohit added 2 commits June 25, 2022 14:13

Use useEffect instead of closeDialog to rollback settings.

9f029a2

Update assertions in test case to make it more explicit.

ebaf019

nfmohit force-pushed the enhancement/5372-erase-unsaved-ds-settings branch from e836e4d to ebaf019 Compare June 25, 2022 08:47

Update test case to use appropriate wording.

828ee6c

nfmohit requested a review from aaemnnosttv June 25, 2022 08:56

aaemnnosttv approved these changes Jun 27, 2022

View reviewed changes

Merge branch 'develop' into enhancement/5372-erase-unsaved-ds-settings.

2bca490

aaemnnosttv merged commit 5d0397b into develop Jun 27, 2022

aaemnnosttv deleted the enhancement/5372-erase-unsaved-ds-settings branch June 27, 2022 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Erase unsaved Dashboard Sharing settings #5407

Erase unsaved Dashboard Sharing settings #5407

nfmohit commented Jun 22, 2022

hussain-t left a comment

github-actions bot commented Jun 22, 2022 •

edited

Loading

nfmohit commented Jun 22, 2022

hussain-t commented Jun 23, 2022

hussain-t left a comment

hussain-t left a comment

aaemnnosttv left a comment

nfmohit commented Jun 24, 2022

aaemnnosttv commented Jun 27, 2022

aaemnnosttv left a comment

nfmohit commented Jun 28, 2022

Erase unsaved Dashboard Sharing settings #5407

Erase unsaved Dashboard Sharing settings #5407

Conversation

nfmohit commented Jun 22, 2022

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

hussain-t left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 22, 2022 • edited Loading

nfmohit commented Jun 22, 2022

hussain-t commented Jun 23, 2022

hussain-t left a comment

Choose a reason for hiding this comment

hussain-t left a comment

Choose a reason for hiding this comment

aaemnnosttv left a comment

Choose a reason for hiding this comment

nfmohit commented Jun 24, 2022

aaemnnosttv commented Jun 27, 2022

aaemnnosttv left a comment

Choose a reason for hiding this comment

nfmohit commented Jun 28, 2022

github-actions bot commented Jun 22, 2022 •

edited

Loading