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

Dashboard sharing modal state is preserved after closing #5372

Closed
eclarke1 opened this issue Jun 16, 2022 · 6 comments
Closed

Dashboard sharing modal state is preserved after closing #5372

eclarke1 opened this issue Jun 16, 2022 · 6 comments
Labels
Good First Issue Good first issue for new engineers P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@eclarke1
Copy link
Collaborator

eclarke1 commented Jun 16, 2022

Feature Description

Bug bash issue: https://app.asana.com/0/1202258919887896/1202436389160092 please refer to Asana issue for background

Similar to module settings which rollback to the initial/saved values/states when saved or exiting edit mode, the sharing settings modal should follow a similar pattern and start from a fresh state every time it is opened.

This means the "current" sharing settings should rollback to the saved values (we already have savedSharingSettings in state for this) and any other UI state should be reset as well (whether or not the user role select is in edit mode, no rows should be disabled, scroll position etc).


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

Acceptance criteria

  • When the Dashboard Sharing settings modal is closed, all of its settings should be rolled back to the current saved values and any other component/UI state should be reset to their initial values.

Note: The implementation should follow a similar pattern to the way module settings work (see rollbackSettings)

Implementation Brief

  • Using assets/js/googlesitekit/modules/datastore/sharing-settings.js,
    • Add a new action rollbackSharingSettings with does not have any parameters and returns the type set to a new constant ROLLBACK_SHARING_SETTINGS and an empty payload.
    • The reducer should set sharingSettings to state.savedSharingSettings which are the original saved settings.
  • Using assets/js/components/dashboard-sharing/DashboardSharingSettings/Footer.js,
    • Dispatch the rollbackSharingSettings action within the onCancel function if the sharing settings have changed.
      • To know if the sharing settings have changed, query the haveSharingSettingsChanged selector of the core/modules data store.

Test Coverage

  • Add new tests for the rollbackSharingSettings action.

QA Brief

  • Enable the dashboardSharing feature flag using the tester plugin.
  • Go to the Site Kit Dashboard.
  • Open the Dashboard Sharing settings modal.
  • Make some changes to the roles and management, but do not save.
  • Close the modal.
  • Open the modal back again, make sure the unsaved settings aren't preserved and have been reset to the previously saved settings.

Changelog entry

  • Rollback any unsaved changes to dashboard sharing settings when closing the dialog.
@eclarke1 eclarke1 added P0 High priority Type: Enhancement Improvement of an existing feature labels Jun 16, 2022
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jun 16, 2022
@asvinb asvinb assigned asvinb and unassigned asvinb Jun 17, 2022
@tofumatt tofumatt self-assigned this Jun 20, 2022
@tofumatt
Copy link
Collaborator

IB ✅

@tofumatt tofumatt removed their assignment Jun 21, 2022
@tofumatt tofumatt added the Good First Issue Good first issue for new engineers label Jun 21, 2022
@tofumatt
Copy link
Collaborator

(I'm marking this as a good first issue, but it's probably a bit more complicated than other issues. Feel free to take a look at the code mentioned in the IB first before tackling it and no worries if it's a bit too much and you want to unassign yourself 😄)

@nfmohit nfmohit self-assigned this Jun 22, 2022
@nfmohit nfmohit removed their assignment Jun 22, 2022
@hussain-t hussain-t assigned hussain-t and nfmohit and unassigned hussain-t Jun 22, 2022
@hussain-t
Copy link
Collaborator

@nfmohit, @asvinb, when I reviewed the PR, I realized that the changes aren't working when I click outside of the modal, which closes the modal; however, when I opened it, the changes are preserved. Could you investigate this?

One more concern from my code review, could we add a test case to cover this scenario in assets/js/components/dashboard-sharing/DashboardSharingSettings/index.test.js?

cc: @tofumatt

@nfmohit
Copy link
Collaborator

nfmohit commented Jun 22, 2022

Thank you for the kind review @hussain-t! I have already fixed the first issue that you mentioned in the meantime, and am now working on the test case. I'll follow-up once done.

@hussain-t
Copy link
Collaborator

@nfmohit, The expected scenario tests are supposed to be done in the DashboardSharingSettingsButton component. However, I just realized that 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.

@nfmohit nfmohit assigned hussain-t and unassigned nfmohit Jun 23, 2022
@hussain-t hussain-t assigned nfmohit and unassigned hussain-t and nfmohit Jun 23, 2022
@nfmohit nfmohit assigned aaemnnosttv and unassigned nfmohit Jun 25, 2022
@aaemnnosttv aaemnnosttv removed their assignment Jun 27, 2022
@mohitwp mohitwp self-assigned this Jun 28, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Jun 28, 2022

QA Update ✅

Verified 👍

  • Verified when user close modal by clicking cancel button.
  • Verified when user close modal by clicking outside modal.
  • Verified when user close modal by clicking esc button.
Recording.116.mp4

@mohitwp mohitwp removed their assignment Jun 28, 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 P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants