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: Adjust access to settings view #4423

Closed
wants to merge 1 commit into from

Conversation

BrittanyIRL
Copy link
Contributor

Summary

Adjusts rules surrounding routes to get to settings so that all users have the ability to see settings when flag is true, hiding parts of settings view when not admin and cannot interact with them (API won't read).

Relevant Technical Choices

  • enabling non admin users to see settings view when enableSettingsView flag is true
  • Hiding toaster errors re settings when canManageSettings is false since that ties to reading settings data and only admin can do that at this moment

To-do

User-facing changes

  • Now when enableSettingsView feature flag is true you can always navigate to settings page regardless of user's role.
  • If you are not admin, you can only see telemetry settings
  • If you are admin you can see google analytics, publisher logos, and telemetry

Testing Instructions

  • Flip enableSettingsView feature flag to 'true' as a logged in admin.
  • Admin should see google analytics, publisher logos, and telemetry on settings view
  • Non admin (any other user role) should only see telemetry on settings view
  • All users should be able to click on the 'settings' nav item (it should show up for everyone) and be able to see settings view.

(Non admin view)
Screen Shot 2020-09-09 at 10 49 11 AM

(Admin view)
Screen Shot 2020-09-09 at 10 49 42 AM


Prep work to merge #4152

… flag is true. Hiding toaster errors re settings when canManageSettings is false since that ties to reading settings data and only admin can do that at this moment
@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2020

Size Change: -15 B (0%)

Total Size: 1.25 MB

ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 296 B 0 B
assets/css/stories-dashboard.css 328 B 0 B
assets/css/web-stories-embed-block.css 515 B 0 B
assets/js/1-bbc6994c3dcd5c5a3380.js 1.25 kB 0 B
assets/js/9-a2ae9e4cb56dda97c498.js 1.25 kB 0 B
assets/js/chunk-web-stories-template-0-********************.js 10.2 kB 0 B
assets/js/chunk-web-stories-template-1-********************.js 10.3 kB 0 B
assets/js/chunk-web-stories-template-2-********************.js 10 kB 0 B
assets/js/chunk-web-stories-template-3-********************.js 10.5 kB 0 B
assets/js/chunk-web-stories-template-4-********************.js 10.6 kB 0 B
assets/js/chunk-web-stories-template-5-********************.js 6.86 kB 0 B
assets/js/chunk-web-stories-template-6-********************.js 9.91 kB 0 B
assets/js/chunk-web-stories-template-7-********************.js 9.78 kB 0 B
assets/js/edit-story.js 498 kB 0 B
assets/js/stories-dashboard.js 588 kB -15 B (0%)
assets/js/web-stories-activation-notice.js 63.6 kB 0 B
assets/js/web-stories-embed-block.js 16.9 kB 0 B

compressed-size-action

@codecov
Copy link

codecov bot commented Sep 9, 2020

Codecov Report

Merging #4423 into main will decrease coverage by 0.13%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4423      +/-   ##
==========================================
- Coverage   83.52%   83.38%   -0.14%     
==========================================
  Files         859      861       +2     
  Lines       15079    15111      +32     
==========================================
+ Hits        12594    12601       +7     
- Misses       2485     2510      +25     
Flag Coverage Δ
#karmatests 71.84% <100.00%> (-0.13%) ⬇️
#unittests 66.39% <40.00%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
assets/src/dashboard/app/index.js 100.00% <100.00%> (ø)
assets/src/dashboard/app/views/toaster/index.js 77.77% <100.00%> (+1.30%) ⬆️
...ts/src/dashboard/components/pageStructure/index.js 90.90% <100.00%> (ø)
...it-story/components/library/panes/text/textPane.js 71.42% <0.00%> (-28.58%) ⬇️
...ets/src/edit-story/components/devTools/devTools.js 41.53% <0.00%> (ø)
...ry/components/library/panes/text/textSets/index.js 0.00% <0.00%> (ø)
...ponents/library/panes/text/textSets/getTextSets.js 0.00% <0.00%> (ø)
...ets/src/edit-story/app/media/pagination/reducer.js 50.00% <0.00%> (+5.00%) ⬆️

Copy link
Contributor

@dmmulroy dmmulroy left a comment

Choose a reason for hiding this comment

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

Works perfectly when switching roles


const enableSettingsView = useFeature('enableSettingsView');
const enableSettingsView =
useFeature('enableSettingsView') && canManageSettings; // we only want to show errors about settings when users have access to managing them, otherwise they can't read settings anyways
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should not be triggering errors in the first place

@swissspidy
Copy link
Collaborator

@BrittanyIRL I'm sorry I didn't see this PR, so I implemented ef06bf4 as part of #4374. I think I prefer that solution over this one as it prevents fetching settings data when the user doesn't have permissions (rather than just hiding the toast messages). PTAL!

@BrittanyIRL
Copy link
Contributor Author

@BrittanyIRL I'm sorry I didn't see this PR, so I implemented ef06bf4 as part of #4374. I think I prefer that solution over this one as it prevents fetching settings data when the user doesn't have permissions (rather than just hiding the toast messages). PTAL!

no worries. that's totally fine. i'll close this one. I was just doing that with the toasts for the time being since i figured the permissions thing would get fixed, purely just a work around to get telemetry and settings in for release.

@swissspidy swissspidy deleted the settings-view-set-by-flag branch September 10, 2020 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants