Ignore params if blank

[bglusman]

Considered redirecting to updates path, but thought it should function as a search that matches all updates

Refactored updates controller to use a new method in application controller shared in search controller to stay DRY

[wilkie]

Yes, please add a test for this. Thanks. :)

Why can't the helper method you've added be in the model? That would seem more appropriate and consistent with the search method.

[bglusman]

Thanks @wilkie, It probably can and should be in the model, though it is view related since it's about pagination, which feels a little odd perhaps...

Also, I hadn't tested before, but noticed @carols10cents note about google bots hitting https://rstat.us/search?page=742&per_page=20&search= and tested that, and while it doesn't throw an error now, it also doesn't return the same results as search?search= that I'd expect, with or without a search term... Not sure why at a glance, I'll try and look into this a bit too, but in case you or Carol know anything about it or whatnot.

[wilkie]

I'd say that it should be in the model. It's a flavor of a query and it only pertains to Update. The search method does pagination, as well, so it's also a matter of consistency in how they are called.

What are you expecting? What does it do?

[bglusman]

OK, plan to put in model...

I expected it to... Ohhh... I see... it's meant to show the 742nd page of results at 20 results per page? In which case there's no way it should show anything without at least 1500 updates in the system.... I wasn't focused on it's behavior properly, makes sense now....

[wilkie]

Yep. Since you added another pagination route, you'll have to test all of the relevant cases, although it does mean bleeding coverage into acceptance tests. Oh well, the behavior is important. :)

[bglusman]

Moved to model and added 3 unit tests and one acceptance tests, hopefully that's good, though LMK if any suggestions.

[wilkie]

Alright. I quickly reworked this and will push it up soon for your comments. Instead of implementing the correct search behavior based on the query in the controller... I just added that case to the search method itself (and fixed some bugs that we didn't catch with that method in the process!) I think that was what was bugging me before about your fix... I hope you don't mind.

Once my changes pass your tests I'll push it up for review.

[bglusman]

No problem... but isn't that search method only hit when not using elasticsearch? that was main reason I didn't go there, didn't want to monkeypatch elasticsearch/tire's search method, but maybe I misread that...

[wilkie]

https://github.com/wilkie/rstat.us/commits/bugfix/empty_search_query

[bglusman]

OK, yes, didn't think of aliasing the method, I like your way much better now... didn't like define method being behind an else clause anyway, just looks wrong :-) you don't need to check query.nil? || query.blank? though, .blank? covers nil and empty strings.

Do I merge that into my branch so it comes into this pull request? or close and start new PR?

[wilkie]

.blank? covers nil and empty strings.

Ah. Rails magics! Rails gives nil a lot of value, doesn't it. :) Fair enough. I added that change.

The proper thing is for you to clone my branch and review it. You might be able to git reset --hard your master to my branch as well, and it may update this PR when you force a push to your master. That'd be fine. Or create a new PR.

[steveklabnik]

You'll never guess who added it: https://github.com/rails/rails/commit/4ef7bfb0166cb45428861ce9e001838ab9a2796f

[steveklabnik]

You might be able to git reset --hard your master to my branch as well, and it may update this PR when you force a push to your master.

Yep, that's what I'd do.

[bglusman]

OK, done... thanks @wilkie (and thanks @steveklabnik, funny bit of history to know _why did that :-} )

[wilkie]

You'll never guess who added it: rails/rails@4ef7bfb

An individual with no visible stature with dark hair and sunglasses walked into the room and said, 'Even nothingness can have meaning.' The artist handed a self-assured gentleman a slip of paper with a string of letters and numbers only to slip away while that man looked at himself in the mirror. And, the one who drew rabbits, cats, and powerful mice frolicked off shouting 'chunky bacon' and was never to be seen again.

Fantastic.

[wilkie]

I have no idea why the hashes on those commits changed. I must have rebased on an incorrect hash. My apologies. Can you re-reset your master to my branch please, @bglusman?

[bglusman]

I'm not sure which hashes you saw changed, so can't verify/comment, but I fetched your branch, re-reset and pushed again, so I guess it's good now :-)

[wilkie]

It is. Thanks. @carols10cents can review it because she's not the two of us and she's awesome and nice. :)

[carols10cents]

Good teamwork!!! :two_men_holding_hands:

I merged this in, then made the change that was discussed but I didn't see made to switch to only using blank? (a4d312aa31f64b11c4b67371fae726a505967334)

Thennn I tried out the change in the browser, and clicking on the 'Search' link in the top nav bar still wasn't showing all updates because there was still a guard statement if params[:search] in searches_controller. So I went ahead and fixed that too (1a88e27d0f8e1bf41c69b8a599b08ac1626cca5c).

With our powers combined, we are.... CAPTAIN PLANET!!! :earth_africa:

[wilkie]

Weird. 1ade0c8 has that change. But, sweet!