Add query params to advanced_search_path if it is present. Fixes first requisite issue #2724

[KarlHeitmann]

This is a follow up for #2833 . #2833 Adds the second feature request for #2724 . The current PR adds the first feature request in #2724 . I didn't understand the third feature request on that issue. If someone can write an example I can do the rest of the work.

I've tried to build a test for this branch, but it is my first time working with mini test, I've got an annoying error that the param is not permitted.... but it is a query params! It should be accepted. Here is the code snippet:

# test/integration/search_test.rb

  test "have query params in advanced_search_path link" do
    query_params = { description: "rubygem_x" }
    visit search_path(query: query_params)
    
    assert page.has_link?("Advanced Search", href: search_path(query: query_params))
  end

And the error:

I couldn't find any similar test in the codebase to take as an example.

[simi]

Would be advanced_search_path((query: params[:query]).compact) enough?

[KarlHeitmann]

You're right! {query: params[:query]} does the job. I've amended the commit and force-pushed it into my branch.

[codecov]

Codecov Report

Merging #3508 (52e1b71) into master (f7235ca) will not change coverage.
The diff coverage is n/a.

❗ Current head 52e1b71 differs from pull request most recent head 3a11d1e. Consider uploading reports for the commit 3a11d1e to get more accurate results

@@           Coverage Diff           @@
##           master    #3508   +/-   ##
=======================================
  Coverage   97.98%   97.98%           
=======================================
  Files         180      180           
  Lines        4357     4357           
=======================================
  Hits         4269     4269           
  Misses         88       88           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[KarlHeitmann]

Thanks @simi for your review.

While fixing the issue, I found out I've missed something important... this PR couldn't be so easy 😅

After clicking the link with {query: params[:query]}.compact I get to the "results page" searches#show and I can see that the main input box has the right input text ✔️

However, if you pay attention to the individual search inputs for each search term, you can see they are blank ❌

The problem is that if you attempt to modify one of the small inputs, the main input query gets erased and you lose all the other search terms 😮

I can create a new method that matches the query string with something like (name: \w+)|(description: \w+)|...., and if it matches, create 5 new variables to use in app/views/searches/advanced:14 and in lines 17, 20, 23, 26 of that same file. I think you've got this rough idea.

But I think a better solution would be to leverage the file lib/elastic_searcher.rb to get the "n" separated query params (0 <= n <= 5). But I don't know if it is possible to leverage that file, I have this idea because the elastic searcher should separate the query params internally to do the advanced search. But I don't know if it is possible to expose the separated query params that are in the guts of elastic searcher. Also, I have not worked before with elastic search, so this API is strange to me.

I'd like to know your feedback, or somebody else to fix the issue. Should I go with the regex approach? Try to leverage elastic searcher? Or is there something else I may be missing?

Best,