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

[WIP] 3728 - Add ability to search by user email to superuser user view #3811

Merged
merged 27 commits into from
Aug 22, 2023

Conversation

Kernapillar
Copy link
Contributor

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

  • New feature (non-breaking change which adds functionality)

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

@dorner
Copy link
Collaborator

dorner commented Aug 4, 2023

@Kernapillar this is marked as WIP - is it ready for review?

@Kernapillar
Copy link
Contributor Author

@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.

@cielf cielf self-requested a review August 4, 2023 23:23
cielf
cielf previously requested changes Aug 4, 2023
Copy link
Collaborator

@cielf cielf left a 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

@Kernapillar
Copy link
Contributor Author

@cielf Thanks for the feedback, I just pushed a version without the clear button and its tests, might as well keep it simple!

Copy link
Collaborator

@dorner dorner left a 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]
Copy link
Collaborator

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?

Copy link
Contributor Author

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!

@dorner dorner dismissed cielf’s stale review August 22, 2023 13:00

addressed by commits

@dorner dorner merged commit 3e0041b into rubyforgood:main Aug 22, 2023
11 checks passed
@dorner
Copy link
Collaborator

dorner commented Aug 22, 2023

Thanks for the contribution!

@cielf
Copy link
Collaborator

cielf commented Aug 26, 2023

Just checked this out on staging. So nice!

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.

Add ability to search by user email to superuser user view
3 participants