Keeping filters state on pagination

[GPrimola]

Context

• Fixes #226
• When the user selects a filter on the search page, the filter wasn't being kept by the pagination.

Summary of Changes

• Frontend:
  • Added functions to manipulate the URL on apps/javascript/packs/views/restrooms/restrooms.js
  • The URL is built with filters query parameter and sent to the backend.
• Backend:
  • Added a before action restrooms_filters to whitelist the given filters;
  • Apply the whitelisted filters to query the database
  • Builds the restroom index view reflecting the filters on the state of the buttons.

Checklist

• [ ] Tested Mobile Responsiveness
• [ ] Added Unit Tests
• [ ] CI Passes
• [ ] Deploys to Heroku on test Correctly (Maintainers will handle)
• [ ] Added Documentation (Service and Code when required)

Screenshots

Before

Page 1

Page 2

After

Page 1

Page 2

[DeeDeeG]

@GPrimola thank you for this pull request!

I will be taking a look at this closely when I get the time. I'll be looking at the code, loading it in Docker, and seeing if the CI can be made to pass testing. I might not be able to get to this right away, but this is a really great addition, so I hope to be able to merge it soon. Thanks again.

Update: I see the CI is "not passing" due to CodeClimate. I personally don't mind CodeClimate warnings very much. It may have helpful suggestions, but what is more important to me is the app working as well as it can. Coding style is a distant second or third priority, in my opinion.

[DeeDeeG]

I can say as an initial reaction: I am happy to see the filters are part of the URL parameters! Thank you for that!

[GPrimola]

@DeeDeeG I'm glad to contribute to this project, specially because I identify with the community as a gay man. :)

About Code Climate's complain, the thing is that I touch a js that has no tests, and as I saw in the codebase there's no tests for JS files, that's why the coverage dropped.

Regarding code style, I totally understand your feeling, because they don't add value to the end-user. However, as this is a OSS, I strongly recommend to have some linters, like Rubocop, due to the amount of developers that write for this code base. With a linter you can have a standardized code base.

I'd like to ask you to add Hacktoberfest topic on the About section of this repo. Due to recent policies of the event, to prevent spammers, they're only counting repos with this topic.

Thank you!! :)

[DeeDeeG]

Regarding code style, I totally understand your feeling, because they don't add value to the end-user. However, as this is a OSS, I strongly recommend to have some linters, like Rubocop, due to the amount of developers that write for this code base. With a linter you can have a standardized code base.

Doesn't sound difficult to add. Hopefully it will not disrupt things too much for me -- I'm the most-active maintainer, but I didn't write most of the code. I'm probably the least-expert programmer who has been a maintainer, heh. I'm best at non-code things, like setting up Docker, handling merges with Git, and researching/updating dependencies. So reviewing a lot of small code changes might be a bit much for me to handle personally. I do my best, though! Maybe it would work out fine. It does sound like somewhat of a good idea.

I see there was some intent to do this way back in #165.

I'd like to ask you to add Hacktoberfest topic on the About section of this repo. Due to recent policies of the event, to prevent spammers, they're only counting repos with this topic.

Ah, I see. Can do.

I found this tweet https://twitter.com/hacktoberfest/status/1312221208667185153 and article https://hacktoberfest.digitalocean.com/hacktoberfest-update explaining it in more detail. (That honestly doesn't surprise me, as handling the increased volume of Pull Requests well is a lot of work! Sad to hear about the actual spam, though.) Anyhow, thanks for letting me know about this. Adding that topic for a month is no problem.

[DeeDeeG]

Hi again. I was able to take a look at this in Docker, and in a Heroku instance (Heroku is the hosted web app platform we use). This looks pretty good overall.

I am seeing a bug, though. Sometimes, clicking a filter button fails to toggle that filter. The page refreshes, but the same filters are used as from before the refresh. (Tested in Firefox and Chrome, Ubuntu and macOS, from Docker and on Heroku).

(This is not 100% of the time. It often gets "stuck" with the same set of filters enabled, unable to change which ones are on or off through multiple clicks and page refreshes. Sometimes, waiting about 10 seconds seems to "un-stick" the filters and make them changeable again, but this also doesn't work 100% of the time.)

I don't see an obvious cause, so at the moment I am writing this comment, I can't be of much help figuring out why this happens. I noticed it happens fairly often when: all three filters are enabled, and I'm trying to toggle one of them off. (But again, this is not 100% consistent).

I hosted a separate Heroku app on my account for testing purposes. In case that makes it more convenient to test this, I will link it here: https://refuge-deedeeg-test.herokuapp.com/restrooms?utf8=%E2%9C%93&lat=37.7749295&long=-122.4194155&search=san+francisco

[GPrimola]

@DeeDeeG I'll take a look on that! I just tried at the link you sent and could notice it too. Sure it's something related to JS, I'll try to do it in another way. :)

[DeeDeeG]

Hi @GPrimola, is there any update on this? No worries either way, but there is another potential contributor looking into this problem as well. They posted an issue at #645.

Thanks again.

- DeeDeeG

[GPrimola]

Hey @DeeDeeG I'll push updates on this by tomorrow! Sorry to be off in the last days!

[GPrimola]

@DeeDeeG could you test it again? I just pushed new code.

[DeeDeeG]

@GPrimola very nice! This looks like it is working 100%! Sorry it took me so long to look at this updated code.

You can try it again at the updated test Heroku instance: https://refuge-deedeeg-test.herokuapp.com/restrooms?utf8=%E2%9C%93&lat=37.7749295&long=-122.4194155&search=san+francisco

---

I re-ran the Travis CI tests (automated functionality testing) and it's passing now.

There are some warnings that are cluttering the test output, though.

jQuery.Deferred exception: undefined is not a function (evaluating 'Object.entries(filters)') http://127.0.0.1:43067/packs-test/js/application-742f5a6fbd25fb94965c.js:864:17

(From: https://travis-ci.com/github/RefugeRestrooms/refugerestrooms/builds/189750688#L940)

Is there any way to make sure the typical operation of the site doesn't generate this warning in the console?

I think we could live with that, but ideally there should be a way to not hit that warning.

Thanks again for your efforts on this.

- DeeDeeG

[DeeDeeG]

@GPrimola I don't know if you saw in my comment above, but there is a warning in the console with this Pull Request.

jQuery.Deferred exception: undefined is not a function (evaluating 'Object.entries(filters)') http://127.0.0.1:43067/packs-test/js/application-742f5a6fbd25fb94965c.js:864:17

Do you have any thoughts on how to fix it?

[GPrimola]

Hey @DeeDeeG ! I just pushed a fix for that! I think the version of JS run by rspec is old and doesn't have Object.entries function. Anyway, I refactored and everything is fine now!

Thanks and sorry for that!