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

User: Support sort query param for user and org user, search endpoints #75229

Merged
merged 17 commits into from
Sep 28, 2023

Conversation

gamab
Copy link
Contributor

@gamab gamab commented Sep 21, 2023

What is this feature?

Support sort query param for user and org user, search endpoints.
This scope param will allow to order (asc or desc) from the backend, the user list based on login, email, name, last_seen_at fields.

Required by: #75228

Why do we need this feature?

[Add a description of the problem the feature is trying to solve.]

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.2.x milestone Sep 21, 2023
)

// FilterWhere limits the set of dashboard IDs to the dashboards for
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved all interface here to prevent a cyclic import situation

@gamab gamab changed the title Provide backend for user sorting User: Support sort query param for user and org user, search endpoints Sep 21, 2023
Copy link
Contributor

@linoman linoman left a comment

Choose a reason for hiding this comment

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

Looks really good already! Do you think we should update the endpoints comments to include the new optional query parameter?

pkg/api/org_users.go Show resolved Hide resolved
@gamab gamab changed the base branch from interactive-table/controlled-sort to main September 22, 2023 15:19
@gamab gamab marked this pull request as ready for review September 22, 2023 15:20
@gamab gamab requested review from a team as code owners September 22, 2023 15:20
@gamab gamab requested review from alexanderzobnin, danielkenlee, zserge, mildwonkey and nikimanoledaki and removed request for a team September 22, 2023 15:20
@gamab gamab added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Sep 25, 2023
@eleijonmarck
Copy link
Contributor

eleijonmarck commented Sep 25, 2023

nits

  • nit: should we add benchmarks to see how the impact is for big users bases? (i created about 100 users locally when i tested for retrieving the sorted users)
  • nit: perhaps we can add a comment above this line about not surfacing the err if we use the wrong sorting option. and if so perhaps log it.
  • nit: add documentation about using sort options

testing locally works fine ⚡
searching with correct sorting works as expected 👍
http://admin:admin@localhost:3000/api/users/search?query=a&sort=login-asc


notes:
sorting using the wrong sorting option asc yields a "unreasonable" error:

http://admin:admin@localhost:3000/api/users/search?query=a&sort=asc

response:
{
"message": "Failed to fetch users",
"traceID": ""
}

but the error is:

MessageID:
"unknown sorting option"
LogMessage:
"asc option unknown"

this line https://github.com/grafana/grafana/blob/gamab/user-sort-option/pkg/services/searchusers/searchusers.go#L65

does not provide the error in the message. most likely because we want to hide user info.

@gamab
Copy link
Contributor Author

gamab commented Sep 27, 2023

nit: should we add benchmarks to see how the impact is for big users bases? (i created about 100 users locally when i tested for retrieving the sorted users)

I think I'm going to skip on that, we were already sorting by name, email. By default the behavior remains the same.

@gamab
Copy link
Contributor Author

gamab commented Sep 27, 2023

nit: add documentation about using sort options

Thanks for pointing that out.
The docs on the subject seem pretty poor, I explained the sort query param in the two places we were mentioning user search ✍️

@gamab
Copy link
Contributor Author

gamab commented Sep 27, 2023

nit: perhaps we can add a comment above this line about not surfacing the err if we use the wrong sorting option. and if so perhaps log it.

I addressed this, the error is now properly surfaced.

@@ -49,6 +49,8 @@ Authorization: Basic YWRtaW46YWRtaW4=

Default value for the `perpage` parameter is `1000` and for the `page` parameter is `1`. Requires basic authentication and that the authenticated user is a Grafana Admin.

`sort` is a comma separated list of options to order the search result. Accepted values for the sort filter are: `login-asc`, `login-desc`, `email-asc`, `email-desc`, `name-asc`, `name-desc`, `lastSeenAtAge-asc`, `lastSeenAtAge-desc`. By default value if `sort` is not specified, the user list will be ordered by `login`, `email` in ascending order.
Copy link
Contributor

Choose a reason for hiding this comment

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

🥇

Copy link
Contributor

@eleijonmarck eleijonmarck left a comment

Choose a reason for hiding this comment

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

Left a comment on ID enumeration, which we should consider for user endpoints.

Otherwise this looks great!

Copy link
Contributor

@eleijonmarck eleijonmarck left a comment

Choose a reason for hiding this comment

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

Tested and error pops up!

@gamab gamab merged commit 96cbe70 into main Sep 28, 2023
14 checks passed
@gamab gamab deleted the gamab/user-sort-option branch September 28, 2023 08:16
@gamab gamab added add to changelog and removed no-changelog Skip including change in changelog/release notes labels Sep 28, 2023
otilor pushed a commit to otilor/grafana that referenced this pull request Sep 28, 2023
…nts (grafana#75229)

* User: Add sort option to user search

* Switch to an approach that uses the dashboard search options

* Cable user sort on the org endpoint

* Alias user table with u in org store

* Add test and cover orgs/:orgID/users/search endpoint

* Add test to userimpl store

* Simplify the store_test with sortopts.ParseSortQueryParam

* Account for PR feedback

* Positive check

* Update docs

* Update docs

* Switch to ErrOrFallback

Co-authored-by: Karl Persson <kalle.persson@grafana.com>

---------

Co-authored-by: Karl Persson <kalle.persson@grafana.com>
rwwiv pushed a commit that referenced this pull request Oct 2, 2023
…nts (#75229)

* User: Add sort option to user search

* Switch to an approach that uses the dashboard search options

* Cable user sort on the org endpoint

* Alias user table with u in org store

* Add test and cover orgs/:orgID/users/search endpoint

* Add test to userimpl store

* Simplify the store_test with sortopts.ParseSortQueryParam

* Account for PR feedback

* Positive check

* Update docs

* Update docs

* Switch to ErrOrFallback

Co-authored-by: Karl Persson <kalle.persson@grafana.com>

---------

Co-authored-by: Karl Persson <kalle.persson@grafana.com>
mildwonkey pushed a commit that referenced this pull request Oct 4, 2023
…nts (#75229)

* User: Add sort option to user search

* Switch to an approach that uses the dashboard search options

* Cable user sort on the org endpoint

* Alias user table with u in org store

* Add test and cover orgs/:orgID/users/search endpoint

* Add test to userimpl store

* Simplify the store_test with sortopts.ParseSortQueryParam

* Account for PR feedback

* Positive check

* Update docs

* Update docs

* Switch to ErrOrFallback

Co-authored-by: Karl Persson <kalle.persson@grafana.com>

---------

Co-authored-by: Karl Persson <kalle.persson@grafana.com>
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
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

5 participants