-
-
Notifications
You must be signed in to change notification settings - Fork 469
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
[WIP] 3728 - Add ability to search by user email to superuser user view #3811
Conversation
@Kernapillar this is marked as WIP - is it ready for review? |
Yeah It is, I set it as WIP because I wasn't sure if the "clear" button fits in the project well or not, I meant to leave a note about it but didnt get a chance. |
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.
@Kernapillar I love this -- how we can type into both the name and the email, and it filters by both ! It's going to make things a lot easier when we need to work with a particular user.
The clear button, however, is not playing well with the pagination -- if I go to page 2 and hit clear, nothing seems to happen.
I'm not sure we need the clear function -- we've only got the two fields that we can filter on at this time, so it's no great burden to manually clear them. So -- if you want to take a look at what's happening with clear and the pagination, that's great -- but I'd also be fine with removing the clear button.
Whichever way you go, we'll also get @dorner to do a technical review of it
@cielf Thanks for the feedback, I just pushed a version without the clear button and its tests, might as well keep it simple! |
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.
Looks good - I had just one question!
@@ -5,7 +5,8 @@ class Admin::UsersController < AdminController | |||
def index | |||
@filterrific = initialize_filterrific( | |||
User.includes(:organization).alphabetized, | |||
params[:filterrific] | |||
params[:filterrific], | |||
available_filters: [:search_query, :search_name, :search_email] |
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.
What's search_query
here?
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.
That is a very good question, looking at it now its most likely a relic from when I was trying to figure out how filterrific worked, that I didn't realize was still in there. I just tested it locally and it does not seem to be doing anything, ill push an updated version without the :search_query shortly!
Thanks for the contribution! |
Just checked this out on staging. So nice! |
Resolves #3728
Description
I have added a second search bar for filtering by email as requested in issue #3728, as well as a clear button that will clear both search inputs. I chose to add the clear button because the two search inputs update the filters when any letter is added or removed to either search bar, and there is no dedicated search button.
I have added Rspec tests for the email search and clear button functionality, in spec/system/admin/users_system_spec.rb, to keep it with the user name search tests.
Type of change
How Has This Been Tested?
I have manually tested the changes, and added additional tests to the users_system_spec.rb file as mentioned above.
To find the new search bar:
sign in as superadmin@example.com
select Users > All Users in the sidebar