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

Fixes #28182 - fixing broken search bar in facts #7554

Merged
merged 1 commit into from
May 18, 2020

Conversation

amirfefer
Copy link
Member

@amirfefer amirfefer commented Mar 31, 2020

In Host -> Facts page the search bar contains host = $fqdn, and after cleaning it up and search for another query this happens:
unspaced-search

it should add a separator " and ":
fixed-search

@theforeman-bot
Copy link
Member

Issues: #28182

@amirfefer
Copy link
Member Author

amirfefer commented Apr 5, 2020

  • Fixing Tests failures

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I ran into this bug last Friday so I can confirm it's annoying. However, I wonder if a separate parameter really the correct way. Shouldn't it check if params[:search] is not blank? The search may also be a complex expression. Some cases I can think of that might fail, but not sure if they will:

  • name = myfact or name = my_other_fact should be (name = myfact or name = my_other_fact) and ...
  • name = myfact and fqdn = host.example.com will result in name = myfact and fqdn = host.example.com and fqdn = host.example.com (note the double space)

It would be good to have test cases for this.

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

I believe Ewoud is right and we could simplify this.

app/controllers/application_controller.rb Outdated Show resolved Hide resolved
@ezr-ondrej
Copy link
Member

@amirfefer ?

test/controllers/fact_values_controller_test.rb Outdated Show resolved Hide resolved
test/controllers/fact_values_controller_test.rb Outdated Show resolved Hide resolved
app/controllers/application_controller.rb Outdated Show resolved Hide resolved
@amirfefer
Copy link
Member Author

Thanks @ezr-ondrej , @ekohl ! I've added also a test

Comment on lines 26 to 28
def has_search_param?
params[:search].blank?
end
Copy link
Member

Choose a reason for hiding this comment

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

This appears unused and also reversed the logic.

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Tested, seems to work properly. Just the left-over has_search_param? method needs cleaning.

@amirfefer
Copy link
Member Author

thanks @tbrisker , @ekohl !
updated

Copy link
Member

@tbrisker tbrisker left a comment

Choose a reason for hiding this comment

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

Thanks @amirfefer !

@tbrisker tbrisker merged commit f3cfe40 into theforeman:develop May 18, 2020
@tbrisker
Copy link
Member

2.1 - 84d0f49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants