Page MenuHomePhabricator
Differential D8596

[web] Handle unsaved changes with the UnsavedChangesModal
ClosedPublic
Actions

Authored by rohan on Jul 21 2023, 9:23 AM.
Tags
None
Referenced Files
F2065625: D8596.id.diff
Fri, Jun 21, 10:14 AM
F2064663: D8596.id28975.diff
Fri, Jun 21, 7:26 AM
Unknown Object (File)
Thu, Jun 20, 12:05 AM
Unknown Object (File)
Wed, Jun 19, 5:49 AM
Unknown Object (File)
Thu, Jun 13, 8:55 AM
Unknown Object (File)
Wed, Jun 12, 11:24 PM
Unknown Object (File)
Sat, Jun 8, 3:02 AM
Unknown Object (File)
Mon, Jun 3, 8:46 PM
View All 76 Files
Subscribers
ashoat
tomek

Details

Reviewers
atul
ginsu
Commits
rCOMM45ceb67981c8: [web] Handle unsaved changes with the UnsavedChangesModal
Summary

If there are any changes in the create role screen, we want to push the UnsavedChangesModal if a user tries to click "Back" or the X button on the Modal. I had to check elements individually for the rolePermissions / pendingRolePermissions array since after selecting a permission and then deselecting it, simply using a === would no longer work on the two arrays since they would not be of the same reference.

ENG-4428

Depends on D8595

Test Plan

Tested the modal to confirm that the UnsavedChangesModal appears when it should

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

rohan created this revision.Jul 21 2023, 9:23 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptJul 21 2023, 9:23 AM
rohan edited the summary of this revision. (Show Details)Jul 21 2023, 9:25 AM
rohan edited the test plan for this revision. (Show Details)
rohan added a child revision: D8597: [web] Introduce client-side error handling to prevent duplicate role names.Jul 21 2023, 9:27 AM
Harbormaster completed remote builds in B21124: Diff 28946.Jul 21 2023, 9:47 AM
rohan requested review of this revision.Jul 21 2023, 9:47 AM
rohan updated this revision to Diff 28975.EditedJul 24 2023, 9:49 AM

Update check to short-circuit if array lengths are different (this is to prevent a case in the edit-role flow where a role has 3 permissions and a user deselects one, every element in the the new permissions (array size 2) is still contained in the original permissions (array size 3)

Harbormaster completed remote builds in B21153: Diff 28975.Jul 24 2023, 10:51 AM
atul requested changes to this revision.Jul 26 2023, 10:49 AM

Can we go with O(n) approach instead of O(n^2)?

web/roles/create-roles-modal.react.js
60–62 ↗(On Diff #28975)

The algorithm being used here is O(n^2). Could we go with something like:

const pendingSet = new Set(pendingRolePermissions);
const roleSet = new Set(rolePermissions);
if (pendingSet.size !== roleSet.size) {
    arePermissionsEqual= false;
}
for (let permission of pendingSet) {
    if (!roleSet.has(permission)) return false;
}
arePermissionsEqual = true;

which is O(n) without much more effort? Given the number of permissions at the moment it's probably fine as is, but might as well reduce the complexity right off the bat

This revision now requires changes to proceed.Jul 26 2023, 10:49 AM
rohan updated this revision to Diff 29068.Jul 26 2023, 11:12 AM

Implement O(n) feedback

web/roles/create-roles-modal.react.js
60–62 ↗(On Diff #28975)

Ah yeah that's probably a better idea anyways

atul accepted this revision.Jul 26 2023, 11:13 AM

Sweet, thanks for updating so quickly

This revision is now accepted and ready to land.Jul 26 2023, 11:13 AM
Harbormaster completed remote builds in B21224: Diff 29068.Jul 26 2023, 11:58 AM
rohan updated this revision to Diff 29340.Jul 31 2023, 12:06 PM

Rebase

This revision was landed with ongoing or failed builds.Jul 31 2023, 6:15 PM
Closed by commit rCOMM45ceb67981c8: [web] Handle unsaved changes with the UnsavedChangesModal (authored by rohan). · Explain Why
This revision was automatically updated to reflect the committed changes.
rohan added a commit: rCOMM45ceb67981c8: [web] Handle unsaved changes with the UnsavedChangesModal.
Harbormaster completed remote builds in B21428: Diff 29340.Jul 31 2023, 9:17 PM

Revision Contents
Changeset List

PathSize
web/
roles/
35 lines

Diff 29401

View Options

web/roles/create-roles-modal.react.js

Loading...