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

217 new admin endpoints: search user #233

Merged
merged 5 commits into from
Jan 18, 2024

Conversation

HubertWojcik10
Copy link
Contributor

  • create an endpoint to search for user(s) based on their id, name, or email.

@ghost
Copy link

ghost commented Dec 7, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@HubertWojcik10 HubertWojcik10 force-pushed the 217-new-admin-endpoints-search-user branch from 45b4c45 to 3ab7b21 Compare December 10, 2023 08:59
@A-Guldborg
Copy link
Contributor

A-Guldborg commented Dec 18, 2023

I tried to look a bit into the frontend for this :) I found a few things to consider:

  • if the "search engine" is good enough, it probably does not need to be able to take multiple / different parameters like we discussed last time. I think the current version is adequate, also not to overload the UI. Might need a discussion once the frontend is added with Omid though.
  • If I send an empty string as a parameter, it will cause an error (It changes from empty string to null).
  • The API right now both has two contradicting authorization headers (AllowAnonymous and AuthorizeRoles(UserGroup.Board) - probably only should have the latter).
  • The ProducesResponseType header for 200 OK is ApiError and not some sort of List/Enumerable/Collection of UserSearchResponse.
  • The frontend needs to know the total number of items in order for the pagination to work properly, otherwise it will think that there is only pageNum elements in total and disables pagination.
  • Also - is there a reason why the search gives a UserSearchResponse with a UserRole whereas the update usergroup takes a usergroup as input?

Sorry for the many comments 😅 I just learned/found a couple things when messing around with the frontend framework 😄 Merry Xmas 🎄

@jonasanker jonasanker linked an issue Dec 29, 2023 that may be closed by this pull request
6 tasks
Copy link
Member

@jonasanker jonasanker 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 added various comments and added my thoughts to the implementation of the search query. Hope it makes sense else let's start a discussion on Slack :)

Also, we should start implementing tests, also for this API endpoint. I can help to create an example to get you started @HubertWojcik10 :)

Co-authored-by: Hubert <hubert.wojcik002@gmail.com>
@jonasanker jonasanker dismissed marfavi’s stale review January 18, 2024 17:53

Reviewed by Jonas

@jonasanker jonasanker enabled auto-merge (squash) January 18, 2024 17:53
Copy link

sonarcloud bot commented Jan 18, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@jonasanker jonasanker merged commit 46a9e66 into main Jan 18, 2024
5 of 6 checks passed
@jonasanker jonasanker deleted the 217-new-admin-endpoints-search-user branch January 18, 2024 17:58
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.

New Admin Endpoints
4 participants