-
Notifications
You must be signed in to change notification settings - Fork 6
Make users searchable #121
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
Conversation
4f7b7c2 to
526939c
Compare
| <div class="form-group"> | ||
| <%= f.label :provider %> | ||
| <%= f.select :id, options_from_collection_for_select(providers, :id, :name), { prompt: "Change provider" }, class: "form-select", data: { action: "change->topics#chooseProvider" } %> | ||
| <%= f.select :id, options_from_collection_for_select(providers, :id, :name), { prompt: "Change provider" }, class: "form-select", data: { action: "change->search#chooseOption" } %> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we add another select box like this, we will have to change naming again because this one will become too general.
I would suggest to support different targets for search controller or even to have two controllers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed... I'm unsure what to do about namings for now since we don't know what the searches (here and on other pages) are ultimately going to look like... 🤔 Maybe it'll be easier when we have more clarity on what we need for all searchable models?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to use non-generalized names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've undone the first commit that was replacing topics_controller.js with the more generic search_controller.js and removed #chooseOption from search_controller.js since it's not currently used. For Users, I left search_controller.js generic because I think it may be re-used on other pages that have no behaviour specific to them. Let me know what you think
526939c to
e194a77
Compare
e194a77 to
64730d4
Compare
There used to be a search functionality in the users index page that was using Mazer's datatable stylesheet (PR#52), but it was then removed in PR#86 as we discussed doing it our way instead. This commit enables searching Users by using a similar logic to what Dmitry did for searching Topics. For the JS controller, I re-used what we had in topics_controller.js but did not replace topics_controller.js with the more generic "search_controller.js" after discussion with Dmitry during PR review. This is because the chooseProvider function is specific to the topics controller and replacing it with a generic name such as chooseOption may be too generic if we then need to add other choosing actions on Topics.
64730d4 to
af9eb9a
Compare
What Issue Does This PR Cover, If Any?
It's a follow-up to PR #86 that removed the search functionality that was using Mazer's datatable stylesheet to allow us to search users.
What Changed? And Why Did It Change?
It adds search functionality to the Users page.
I'm re-using @dmitrytrager's work to apply the same logic to searching users, so I have renamed the topics_controller.js controller to a more generic name.
I've also added system tests for the search functionality for Topics and fixed an issue with sorting Topics in ascending order.
How Has This Been Tested?
I've added configuration to use selenium_chrome_headless driver for system tests and system tests for the search functionalities of the Topics and Users page.
I've also added a SystemHelpers file with a login_as method for system tests.
Please Provide Screenshots
Users.search.mp4