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

Empty search correct #1195

Merged
merged 2 commits into from
Nov 5, 2021
Merged

Conversation

MilanPospisil
Copy link
Contributor

@MilanPospisil MilanPospisil commented Nov 4, 2021

Fixes AAH-820

Compound filter had problem that it accepted blank string (string with only empty spaces). This PR disables filter button in this situation. This corrects the issue for many containers that uses compound filter.

Copy link
Member

@ZitaNemeckova ZitaNemeckova left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ZitaNemeckova ZitaNemeckova merged commit ee0a93d into ansible:master Nov 5, 2021
@newswangerd newswangerd added the backport-4.4 This PR should be backported to stable-4.4 (2.1) label Dec 10, 2021
himdel pushed a commit to himdel/ansible-hub-ui that referenced this pull request Dec 14, 2021
* repair compound filter to not accept blank string

* prettier

(cherry picked from commit ee0a93d)
@himdel
Copy link
Collaborator

himdel commented Dec 14, 2021

4.4 backport: #1376

@himdel
Copy link
Collaborator

himdel commented Dec 15, 2021

@MilanPospisil can you please re-check if this works on master?

I think it doesn't:
20211215030403

(or am I testing the wrong thing?)

.. More specifically, I think the only difference on master is not being able to click the search button .. but it can still come from searching by pressing enter, and then from the url.

I think we need to handle at least:

  • (done) searching by clicking should be disabled for empty search field (or one full of spaces)
  • searching by enter should not work in empty search field (or one full of spaces)
  • AppliedFilters should never show the empty keywords - it should act as if no filter was present, no Clear all filters either
  • it should not affect the actual search results (I think this only works in collections now because every collection has a space somewhere in description)

Ideally we should clear ?keywords= and ?keywords=%20 from params (not just keywords, any such param),
and it should not be necessary to change AppliedFilters - we don't hide empty params there, we remove them before AppliedFilters renders them (but if it turns out you'd have to change every single container, hiding it in AppliedFilters might be better),
search params are expanded from params in every controller, so we also need to change that before a space lands there,
so I think the right place to fix this is ParamHelper.

@MilanPospisil
Copy link
Contributor Author

@MilanPospisil can you please re-check if this works on master?

I think it doesn't: 20211215030403

(or am I testing the wrong thing?)

.. More specifically, I think the only difference on master is not being able to click the search button .. but it can still come from searching by pressing enter, and then from the url.

I think we need to handle at least:

* (done) searching by clicking should be disabled for empty search field (or one full of spaces)

* searching by enter should not work in empty search field (or one full of spaces)

* `AppliedFilters` should never show the empty keywords - it should act as if no filter was present, no Clear all filters either

* it should not affect the actual search results (I think this only works in collections now because every collection has a space somewhere in description)

Ideally we should clear ?keywords= and ?keywords=%20 from params (not just keywords, any such param), and it should not be necessary to change AppliedFilters - we don't hide empty params there, we remove them before AppliedFilters renders them (but if it turns out you'd have to change every single container, hiding it in AppliedFilters might be better), search params are expanded from params in every controller, so we also need to change that before a space lands there, so I think the right place to fix this is ParamHelper.

Ok, I did only button disable, I did forgot about enter and URL never come into my mind. I will create new PR and I will do it.

@MilanPospisil
Copy link
Contributor Author

MilanPospisil commented Dec 21, 2021

Solving this in:
#1402

himdel pushed a commit to himdel/ansible-hub-ui that referenced this pull request Jan 4, 2022
* repair compound filter to not accept blank string

* prettier

(cherry picked from commit ee0a93d)
@ansible ansible deleted a comment from patchback bot Jan 4, 2022
@himdel himdel added backport-4.4 This PR should be backported to stable-4.4 (2.1) and removed backport-4.4 This PR should be backported to stable-4.4 (2.1) labels Jan 6, 2022
@patchback
Copy link

patchback bot commented Jan 6, 2022

Backport to stable-4.4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4.4/ee0a93d1959343eae8e8b6f615ef3ba72e773bef/pr-1195

Backported as #1466

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jan 6, 2022
* repair compound filter to not accept blank string

* prettier

(cherry picked from commit ee0a93d)
himdel pushed a commit that referenced this pull request Jan 6, 2022

Verified

This commit was created on github.com and signed with GitHub’s verified signature. The key has expired.
* repair compound filter to not accept blank string

* prettier

(cherry picked from commit ee0a93d)

Co-authored-by: MilanPospisil <arkanus@seznam.cz>
@github-actions github-actions bot added the backported-4.4 This PR has been backported to stable-4.4 (2.1) label Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-4.4 This PR should be backported to stable-4.4 (2.1) backported-4.4 This PR has been backported to stable-4.4 (2.1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants