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

Fix bug with host search where search text would be ignored. #695

Merged

Conversation

bcipriano
Copy link
Collaborator

Link the Issue(s) this Pull Request is related to.
#694

Summarize your change.
I think the if in this handler is a bit off. From my reading, it's trying to accomplish two things:

  1. If the regex is blank, set a special "blank" value into the search object -- in this case an empty array.
  2. If the search term has changed, refresh the widget so the host filtering can occur.

But there are two problems with the way it's currently written:

  1. If the search term has not changed, a blank value will be stored in search object, regardless of the contents of the search box. This causes Host widget is not honoring the filter option when the screen refreshes #694.
  2. The widget is outside the if/else block, so it's always refreshed, even if the search term hasn't changed, so (2) above is not accomplished.

The reason this doesn't crop up more often is because it's kind of difficult to trigger this handler without also changing the contents of the search box. (Hence the alt+tab in the reproduction steps in the issue.)

@bcipriano
Copy link
Collaborator Author

@gregdenton Looks like you changed some of this code way back in #209, am I understanding this correctly?

Copy link
Collaborator

@gregdenton gregdenton left a comment

Choose a reason for hiding this comment

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

LGTM. This makes a lot more sense and adheres to what I believe was the original intention of the __filterByHostName variable.

@bcipriano bcipriano merged commit 9cf2873 into AcademySoftwareFoundation:master May 27, 2020
@bcipriano bcipriano deleted the fix-host-search branch May 27, 2020 18:14
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.

2 participants