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

Configuration: Add an option to hide certain users in the UI #28942

Merged
merged 34 commits into from Nov 24, 2020

Conversation

AgnesToulet
Copy link
Contributor

@AgnesToulet AgnesToulet commented Nov 9, 2020

What this PR does / why we need it:
Add an option in the configuration to hide certain users in the UI.
Hidden users are not hidden for themselves and for Grafana admins.

Special notes for your reviewer:
As Grafana admins will continue to see all users, admin routes are not impacted by this change (except for /api/orgs/:orgId/users that uses a helper function shared with non-admin routes - but the behavior will not change)

Copy link
Contributor

@sakjur sakjur left a comment

Choose a reason for hiding this comment

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

This looks really nice 👍

I left some Go specific feedback. I'll have to spend some more time examining the SQL to see if I can find something there.

pkg/setting/setting.go Outdated Show resolved Hide resolved
pkg/setting/setting.go Outdated Show resolved Hide resolved
pkg/api/dtos/models.go Outdated Show resolved Hide resolved
pkg/api/org_users.go Outdated Show resolved Hide resolved
pkg/api/team_members.go Outdated Show resolved Hide resolved
pkg/api/user.go Outdated Show resolved Hide resolved
pkg/services/sqlstore/user.go Outdated Show resolved Hide resolved
AgnesToulet and others added 2 commits November 10, 2020 08:24
Co-authored-by: Emil Tullstedt <emil.tullstedt@grafana.com>
@AgnesToulet AgnesToulet marked this pull request as ready for review November 10, 2020 07:43
@AgnesToulet AgnesToulet requested a review from a team as a code owner November 10, 2020 07:43
@AgnesToulet AgnesToulet requested review from wbrowne, jessabe and marefr and removed request for a team November 10, 2020 07:43
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Looks good!

but there are some issues adding more global variables (something we are trying to stop with), and the extra query to get hidden user ids seems unnecessary

pkg/api/dtos/models.go Outdated Show resolved Hide resolved
pkg/setting/setting.go Outdated Show resolved Hide resolved
pkg/services/sqlstore/team.go Outdated Show resolved Hide resolved
conf/defaults.ini Outdated Show resolved Hide resolved
Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Looks good now, but I will leave @sakjur to leave final ok, as I did not go through the SQL, sounded like he wanted to take a close look at that

Copy link
Contributor

@sakjur sakjur left a comment

Choose a reason for hiding this comment

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

I have one question about the order of params in one of the queries, other than that it looks good

pkg/services/sqlstore/team.go Outdated Show resolved Hide resolved
pkg/services/sqlstore/team.go Outdated Show resolved Hide resolved
pkg/services/sqlstore/team.go Show resolved Hide resolved
pkg/services/sqlstore/team.go Outdated Show resolved Hide resolved
pkg/services/sqlstore/team.go Show resolved Hide resolved
Copy link
Contributor

@sakjur sakjur left a comment

Choose a reason for hiding this comment

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

LGTM, we should add tests for this (but I get that we want this out asap, so we can take that in a separate PR)

@xlson xlson added the old backport v7.3.x Mark PR to be automatically backported to v7.3.x label Nov 10, 2020
@xlson xlson added this to the 7.3.2 milestone Nov 10, 2020
@AgnesToulet AgnesToulet marked this pull request as ready for review November 18, 2020 08:15
@AgnesToulet
Copy link
Contributor Author

I added automated tests and refactored what you suggested, thanks for the feedback! It's ready for a final review.

@torkelo torkelo modified the milestones: 7.3.4, 7.3.5 Nov 24, 2020
Copy link
Contributor

@xlson xlson left a comment

Choose a reason for hiding this comment

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

Great work on adding tests! Looks good to go. I'm going to test it manually now.

xlson and others added 3 commits November 24, 2020 11:00
* master: (71 commits)
  Security: Fixes minor security issue with alert notification webhooks that allowed GET & DELETE requests grafana#29330
  Chore: Bump storybook to v6 (grafana#28926)
  ReleaseNotes: Updates release notes link in package.json (master) (grafana#29329)
  Docs: Accurately reflecting available variables (grafana#29302)
  Heatmap: Fixes issue introduced by new eventbus (grafana#29322)
  Dashboard Schemas (grafana#28793)
  devenv: Add docker load test which authenticates with API key (grafana#28905)
  Login: Fixes redirect url encoding issues of # %23 being unencoded after login (grafana#29299)
  InfluxDB: update flux library and support boolean label values (grafana#29310)
  Explore/Logs: Update Parsed fields to Detected fields (grafana#28881)
  GraphNG: Init refactorings and fixes (grafana#29275)
  fixing a broken relref link (grafana#29312)
  Drone: Upgrade build pipeline tool (grafana#29308)
  decreasing frontend docs threshold. (grafana#29304)
  Docker: update docker root group docs and docker image (grafana#29222)
  WebhookNotifier: Convert tests away from goconvey (grafana#29291)
  Annotations: fixing so when changing annotations query links submenu will be updated. (grafana#28990)
  [graph-ng] add temporal DataFrame alignment/outerJoin & move null-asZero pass inside (grafana#29250)
  Dashboard: Fixes kiosk state after being redirected to login page and back (grafana#29273)
  make it possible to hide change password link in profile menu (grafana#29246)
  ...
* master:
  Guardian: Rewrite tests from goconvey (grafana#29292)
  Docs: Fix editor role and alert notification channel description (grafana#29301)
  Docs: Improve custom Docker image instructions (grafana#29263)
@xlson xlson modified the milestones: 7.3.5, 7.3.4 Nov 24, 2020
@xlson xlson added the old backport v7.3.x Mark PR to be automatically backported to v7.3.x label Nov 24, 2020
@xlson xlson merged commit 22788d1 into grafana:master Nov 24, 2020
@grafanabot
Copy link
Contributor

The backport to v7.3.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-28942-to-v7.3.x origin/v7.3.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 22788d1d861094170b993d3f1a659d2a79876826
# Push it to GitHub
git push --set-upstream origin backport-28942-to-v7.3.x
git switch master
# Remove the local backport branch
git branch -D backport-28942-to-v7.3.x

Then, create a pull request where the base branch is v7.3.x and the compare/head branch is backport-28942-to-v7.3.x.

@xlson xlson modified the milestones: 7.3.4, 7.4.0 Nov 24, 2020
@xlson xlson removed the old backport v7.3.x Mark PR to be automatically backported to v7.3.x label Nov 24, 2020
@mjseaman mjseaman added this to the 7.4.0 milestone Jan 8, 2021
@ivanahuckova ivanahuckova changed the title Add an option to hide certain users in the UI Config: Add an option to hide certain users in the UI Jan 20, 2021
@ivanahuckova ivanahuckova changed the title Config: Add an option to hide certain users in the UI Configuration: Add an option to hide certain users in the UI Jan 20, 2021
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.

None yet

6 participants