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

[#155000779] fix time-based searching for collections #47

Merged
merged 3 commits into from
May 7, 2018

Conversation

MothOnMars
Copy link
Contributor

No description provided.

i14y_params[:query] = search.query
i14y_params.merge! extract_current_search_filter_params(search)
i14y_params.merge! extra_params
search_path i14y_params
url_for i14y_params
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL, wow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Riiiiight? You and me both.

Copy link
Contributor

Choose a reason for hiding this comment

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

@viclim check out this clever solution to the /search and /search/docs context problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@viclim ,thanks for the work you did on this story. I didn't actually mean to steal it, but in the process of poking the code to determine how it should work, I figured out how it could work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nickmarden , the return value is just the path, i.e.:
/search/docs?affiliate=764544198&query=test+query&dc=1&sort_by=date&tbs=m

@MothOnMars
Copy link
Contributor Author

@nickmarden , FYI, I'm doing some final local testing before I assign this (to you...) for official review. I like to open unassigned PRs early to get feedback from Code Climate.

fixtures :affiliates, :i14y_drawers, :i14y_memberships
let(:affiliate) { affiliates(:power_affiliate) }

describe '#path_for_filterable_search' do
describe '#path_for_filterable_search', type: :request do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nickmarden , it feels a little dirty to make this an integration test, but I wasn't able to find a way to stub the current path used by the url_for method (I barked up a number of trees before finding this solution). If you have such a way, I'm all ears.

@nickmarden
Copy link
Contributor

@MothOnMars addresses three issues that I was concerned about:

  • url_for actually returns a path, not a URL, which respects the implied semantics of path_for_i14y_search
  • Added a comment about the weird but valid decision of using a request spec for a controller test
  • Added a missing hash key to a controller spec test

Based on all of that, LGTM 👍

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.

3 participants