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

Persist the state of the reporting table so that viewers can return to their previous view easily #4853

Merged
merged 6 commits into from
May 2, 2024

Conversation

gilluminate
Copy link
Contributor

@gilluminate gilluminate commented May 1, 2024

Closes PROD-1907

Description Of Changes

This modifies the existing Data Map Reporting page to keep track of the various states in Local Storage in addition to the react state. This will allow the user to reload the page, or close the page and come back to it, and not need to adjust these settings again.

This includes storing

  • The overall "Group by" status
  • Each column’s “Group/Display” status
  • Each column’s width
  • Column visibility
  • Column order

Code Changes

  • created a new hook useLocalStorage in AdminUI client that is a modified version of the one in FidesJS.
  • converted or created state management over to local storage for the following:
export enum DATAMAP_LOCAL_STORAGE_KEYS {
  GROUP_BY = "datamap-group-by",
  COLUMN_ORDER = "datamap-column-order",
  TABLE_GROUPING = "datamap-table-grouping",
  TABLE_STATE = "datamap-report-table-state",
  DISPLAY_ALL_COLUMNS = "datamap-display-all-columns",
}

Some things needed to be updated to accept initialized values from the storage, since many were just using hard coded defaults:

  • Column ordering modal now respects column ordering when initializing
  • The tableInstance is now initialized using the stored state if there is one.

Steps to Confirm

  1. Visit Data map report page in Admin UI at /reporting/datamap
  2. Resize a column or two
  3. Change the "Group by..." to something different than the default (eg. "Data Use")
  4. Open the "Edit Columns" modal and hide a few columns as well as reording a few columns
  5. Change one or two of the columns to "Display All" from the heading dropdown menu.
  6. Now reload the page and ensure that all of the above changes have been remembered

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Issue Requirements are Met
  • Update CHANGELOG.md

Copy link

vercel bot commented May 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fides-plus-nightly ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 2, 2024 10:02pm

Copy link

cypress bot commented May 1, 2024

Passing run #7592 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge af3a9d9 into d1f6a0a...
Project: fides Commit: 8dccd8f3bf ℹ️
Status: Passed Duration: 00:34 💡
Started: May 2, 2024 10:12 PM Ended: May 2, 2024 10:13 PM

Review all test suite changes for PR #4853 ↗︎

@gilluminate gilluminate force-pushed the PROD-1907-persist-the-state-of-the branch 2 times, most recently from 1fcda67 to c745ba3 Compare May 1, 2024 23:19
@gilluminate gilluminate marked this pull request as ready for review May 2, 2024 02:43
@gilluminate gilluminate requested a review from jpople May 2, 2024 18:54
Copy link
Contributor

@jpople jpople left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good! UAT'd:

  • Changes to column visibility, width, and order are working as expected
  • The "group by" state for the whole table is stored, but not "group/display" for individual columns (e.g. the data categories column), which I think is meant to be part of the AC:

Components to include in the state

Each column’s “Group/Display” status

Also, is this something we can add a Cypress test for?

@gilluminate
Copy link
Contributor Author

  • The "group by" state for the whole table is stored, but not "group/display" for individual columns (e.g. the data categories column), which I think is meant to be part of the AC:

Ah, good catch! Yes, I misunderstood the AC there. I'll add or update to include this.

@gilluminate
Copy link
Contributor Author

Also, is this something we can add a Cypress test for?

I can look into that. Might need to figure out how to persist the Cypress environment's local storage between page loads.

@gilluminate
Copy link
Contributor Author

@jpople updated to address your comments. Thanks for those!

I verified that we can go ahead and keep my accidental inclusion of the "Group by" state, in addition to the rest. I've update the AC to reflect that.

I added some Cypress tests for verifying persistence on page reload. I didn't include column sizing or column reordering because a) they weren't included in the original tests, b) I think they were left off originally because it would be difficult to simulate dragging items to specific spots and check that they are in the correct spot using Cypress alone. Neither task can be accomplished without a mouse at the moment. We should probably fix that for accessibility in the future, which will in turn enable better testing.

@gilluminate gilluminate requested a review from jpople May 2, 2024 22:16
@gilluminate gilluminate merged commit 8751e3e into main May 2, 2024
13 checks passed
@gilluminate gilluminate deleted the PROD-1907-persist-the-state-of-the branch May 2, 2024 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants