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

Add OSM users by username #438

Merged
merged 7 commits into from
May 5, 2023
Merged

Conversation

willemarcel
Copy link
Contributor

Contributes to #97

It first searches in the osm teams database. If it doesn't find a user, then we search in OSM.

It's missing to modify the frontend yet.

@vercel
Copy link

vercel bot commented Apr 4, 2023

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

Name Status Preview Comments Updated (UTC)
osm-teams ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 20, 2023 10:24pm

@willemarcel
Copy link
Contributor Author

@LanesGood I've added the frontend to allow adding users to teams by username.
@kamicut The only thing that is missing is to update the members' table after adding a member. Could you take a look?

@LanesGood
Copy link
Member

@willemarcel this is looking great!

  • I think we should have a toast saying "member {{NAME}} successfully added" Would you be able to add this?
  • Do you think we should we close the modal after success? Or leave open?
    • It's nice to have it open if a user wants to continue adding mutliple users. In this case, we could rename the button to "Add Members" and then this sets us up nicely to later on do a multiple-add-at-once feature

@willemarcel willemarcel changed the title Add /api/users to allow search OSM users by username Add OSM users by username Apr 13, 2023
@willemarcel
Copy link
Contributor Author

@LanesGood thanks for the feedback! I'll make the changes you suggested. I prefer to keep the modal open.

@willemarcel
Copy link
Contributor Author

@LanesGood I made the changes and also increased the size of the input fields.

@LanesGood
Copy link
Member

@willemarcel I'm still seeing the "adding" loading spinner when I find a user and hit "search" - but I haven't hit "add" yet. I'm working in Chrome.

I also realized that I was expecting "fuzzy" search and the platform requires exact search.

@LanesGood
Copy link
Member

Thanks for debugging with me @willemarcel! As you saw on the slack, i don't see the issue when I reload... though I did see it again a little, I'm not sure how to reproduce but will let you know if it continues to pop up.

I pushed up some styling changes for the form field layout:
image

Finally, I noticed that there are no error notifications/toasts when a user adds a member twice, or when they try to add themselves. I think this is restricted at the API level, and I see no console or network errors, so I'm guessing that we should make some UI changes.

Copy link
Member

@LanesGood LanesGood left a comment

Choose a reason for hiding this comment

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

I think this works great and is a much welcomed addition! I'll defer to @kamicut on model/data review.

To confirm, does this cache searches for the OSM database? Are these usernames cached in the osm teams database and now available without hitting OSM for all OSM Teams users?

@willemarcel
Copy link
Contributor Author

To confirm, does this cache searches for the OSM database? Are these usernames cached in the osm teams database and now available without hitting OSM for all OSM Teams users?

I didn't implement cache. When the user search, we try first to find the user in the OSM Teams database, if it's not found, we fetch the OSM API.

Copy link
Member

@kamicut kamicut left a comment

Choose a reason for hiding this comment

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

LGTM!

There was a small issue where at some point the "+ Add" button "froze", but after refreshing the page I couldn't replicate the bug. I'm not sure what it could be. If we see this happen again we can open an issue.

@willemarcel willemarcel merged commit 8a5d760 into develop May 5, 2023
@willemarcel willemarcel deleted the feature/search-by-username branch May 5, 2023 14:10
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.

3 participants