Rails 5.2

[btyy77c]

Context

• Fixes [Gemfile] Update to (at least) Rails 5.1.6 #497
• Update to Rails 5.2

Summary of Changes

• Put Gems in alphabetical order
• Updated Gems: activeadmin, grape, grape-swagger, grape-kaminari, rails, rspec-rails, simple_form
• Added Gems: bootsnap, listen
• Updated test: changed expect(response).to be_success to assert_response :success
• Updated config files based on rails app:update recommendations

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

After

[DeeDeeG]

@btyy77c, Thanks very much for this pull request!

As per usual for Rails updates, we'll want to test this a bunch before merging, and give it a pretty thorough review. I'll start by just making sure I can run it in Docker without errors (which I expect will be no problem).

In any case this looks great! 🎉

Happy Hacktoberfest to you! 🎃 💻 🍂 ✨

[DeeDeeG]

As an aside: I notice some of these commits are made with a different git "name" and email set than your official GitHub name/email combo. So not all of these commits would get attributed to your GitHub account properly when we merge.

(This wouldn't make a difference if we squash and merge, since it would produce just the one commit, hopefully with your correct info on it. We tend to decide on whether to squash or not at merge time.)
Edit: Whoops, I see all the commits have the same author info, so squash-and-merge won't help.

If you want to go back and fix this (or have us fix it), it should be doable.

Here's two methods:
https://www.git-tower.com/learn/git/faq/change-author-name-email#chapter_using+interactive+rebase
https://www.git-tower.com/learn/git/faq/change-author-name-email#chapter_using+git+filter-branch

(Edit to add: If something were to go wrong with fixing name/email in these commits, I have a backup branch I intend to leave in the same state this PR started with, at least for the time being: https://github.com/DeeDeeG/RefugeRestrooms/tree/rails52-backup)

Trying this pull request out in Docker as soon as I get Docker installed on my new Ubuntu install.

[DeeDeeG]

I was able to test this branch in Docker. It mostly works as intended, but I noticed a few problems.

• The API page isn't working. (This seems to be a canary in the coal mine; It breaks a lot.)
• Restroom entries from db/export.csv aren't showing up. (usually when you search "San Francisco" from the splash page, you will see about a page or two of results. When I submitted a new restroom, it came up as restroom 1, indicating the list of restrooms was empty. Sometimes this goes away by starting over with a fresh Docker container, depending on the root cause. So it might be just my Docker container not working.)
  • I had this happen running develop branch in Docker... so probably not related to this pull request.
  • Edit: This is now fixed in pull request Fix empty restrooms db during development #508
• Maps aren't showing correctly on restroom pages. (They just show up as gray rectangles. If you click on the map rectangle, you get instructions about Ctrl + scroll to zoom, so the Google Maps script is loaded, it just isn't displaying quite right.)
  • Edit to add: This is happening for at least one restroom at https://refugerestrooms.org as well, so it's likely not caused by this Pull Request. (For example: https://refugerestrooms.org/restrooms/44920)
  • Edit 2: This appears to be because we are over our Maps API quota, and entries are getting submitted with lat/long. Having no or null coordinates apparently breaks the map view.
• http://127.0.0.1:3000/restrooms/ shows up strangely, like a blank/null results screen trying to show a map view.
  • Also happening on develop branch in Docker
  • Edit: Also fixed by Fix empty restrooms db during development #508
• the li element near the top of a restroom entry's page has a bullet point next to it, should be updated to list-style-type: none from effectively list-style-type: disc
  • This also happens in develop. Possibly to do with Rework restroom-list view to be mobile responsive #483 or something like that. But not likely due to this pull request.

I may have missed some things, but anyways, that's enough things to start with. Thanks again, I do think this is a good place to start from. Should be fixable. 👍

[btyy77c]

• The API page isn't working. (This seems to be a canary in the coal mine; It breaks a lot.)

When I visit production, the api is not working: (https://www.refugerestrooms.org/api or https://www.refugerestrooms.org/api.json) Is there a different route or something else I should be checking? Thanks!

[DeeDeeG]

The API page can be visited at [base-url]/api/docs/, e.g.: 127.0.0.1:3000/api/docs/.

Edit: Trailing forward-slash is important!

[DeeDeeG]

I think there is something off with our Docker setup, because none of the dummy entries from db/export.csv are loaded, even on the develop branch.

On develop: The API page does work. And if I submit a new restroom, then ask for all restrooms by date on the API page, the one I submitted shows up in response.

Screenshot of the API page, showing just one result

Edit: This is fixed with #508

[btyy77c]

The API page can be visited at [base-url]/api/docs/, e.g.: 127.0.0.1:3000/api/docs.

So, I'm not sure how long you've worked on this application, but do you know the history of why the /api/docs app was placed in the public folder (https://github.com/RefugeRestrooms/refugerestrooms/tree/develop/public/api/docs)? Is there a reason it couldn't be part of the rails application?

[btyy77c]

Sorry! I didn't mean to close the pull request. I normally use gitlab and I'm apparently terrible with GitHub.

[DeeDeeG]

So, I'm not sure how long you've worked on this application, but do you know the history of why the /api/docs app was placed in the public folder . . .[link]. . . ?

This was before I came in. But I guess it's sort of a hack/kludge that lets this page be viewable without doing a lot of route configuration and so on.

Is there a reason it couldn't be part of the rails application?

No, I think that is just how the contributor who added this ended up doing it. Ideally, in my opinion, we would go in and install this as a proper nodejs package via our package.json. But "part of the app" sounds like the right direction to go in 👍 .

Edit: Technical typos.

Sorry! I didn't mean to close the pull request. I normally use gitlab and I'm apparently terrible with GitHub.

No worries! (It's nice to hear not everyone is on closed platforms, and totally open-source workflows are viable.)

(P.S. I think GitHub is fine, for the record, but it is always good to have options than no options. And GitLab is pretty neat.)

[DeeDeeG]

From #492 (comment):

As @tkwidmer mentioned in Slack, we might want to also consider installing swagger-ui as a proper node package.

Our public/api directory is mostly (entirely?) a copy of the swagger-ui project from the 2.0 series: https://github.com/swagger-api/swagger-ui/tree/2.x/dist

I suspect we could install (the 2.0 version?) of the nodejs package and then update as needed.

Edit: This is PR #506. Thanks @btyy77c!

[DeeDeeG]

Sorry to bother you @btyy77c. (These pull requests you have put in are really helpful!)

To clarify about the commit author deatils: Does it bother you if these commits aren't linked to your GitHub account?

If so: Please let us know what e-mail should be in these commits to properly link to your account. I'm prepared to rebase the pull requests with the correct author info if that works for you. You can check what e-mails are already linked to your GitHub account (or even add another email address to your account) here: https://github.com/settings/emails

Edit: As a reminder, there is a totally anonymous email option that still links these commits to your GitHub account. That would be the username@users.noreply.github.com format.

If not, or if you'd actually prefer these commits not be linked to your account (better anonymity, etc), please let us know. That is no problem, but I'd hate to use someone's work without crediting them, or at least confirming how they do or don't want to be attributed.

Thanks again!

- DeeDeeG

[btyy77c]

Sorry to bother you @btyy77c. (These pull requests you have put in are really helpful!)

To clarify about the commit author deatils: Does it bother you if these commits aren't linked to your GitHub account?

If so: Please let us know what e-mail should be in these commits to properly link to your account. I'm prepared to rebase the pull requests with the correct author info if that works for you. You can check what e-mails are already linked to your GitHub account (or even add another email address to your account) here: https://github.com/settings/emails

Edit: As a reminder, there is a totally anonymous email option that still links these commits to your GitHub account. That would be the username@users.noreply.github.com format.

If not, or if you'd actually prefer these commits not be linked to your account (better anonymity, etc), please let us know. That is no problem, but I'd hate to use someone's work without crediting them, or at least confirming how they do or don't want to be attributed.

Thanks again!

• DeeDeeG

I don't have a preference either way. I use gitlab mostly and I don't want to change my email to anything like noreply.github.com. Thanks for checking first!

[DeeDeeG]

Edit: I think this comment is incorrect, actually. See next comment.

Hi again,

I did get to take another look at this. The primary issue left with this pull request is the API documentation page not working.

I am pretty confident that has to do with our API being built against an older API spec, and the new grape and grape-swagger gems not being compatible with our somewhat old API.

We would need to update our actual API code to fix this; I made issue #516 to address it.

For now I'd recommend rolling back grape and grape-swagger to the versions they have in the current develop branch. That should make the API page functional again.

I'm also taking a look at the way the new config files are merged, but everything generally works as expected when I boot this up in Docker, so the config files are not likely to need changes, or else they would just be small tweaks.

Best,

- DeeDeeG

[DeeDeeG]

Well, the updated grape and grape-swagger gems are working in #506, so clearly our API is fundamentally compatible with the new gem versions.

I will try to see if there is any way to get the new swagger setup working on Rails 5.2.1, and if so I will update here. Then we can probably merge this and #506 once we get them working together.

[DeeDeeG]

For some reason, on this branch I get two requests for "/api/swagger_doc.json" every time I visit the API docs page:

Started GET "/api/swagger_doc.json" for 10.0.2.2 at 2018-10-16 02:42:03 +0000
Started GET "/api/swagger_doc.json" for 10.0.2.2 at 2018-10-16 02:42:03 +0000

Edit: This happens in develop as well.

[DeeDeeG]

In the process of troubleshooting this pull request, I went and performed a Rails 5.2.1 upgrade over again from scratch in this branch: https://github.com/DeeDeeG/refugerestrooms/tree/rails-5.2.1

It's a bit more minimal in terms of changes, and it does work, including the API documentation page.

I only updated the minimum amount of Gems required, didn't enable the listen Gem, and tried to keep config changes to a minimum and preserve existing custom configs, while merging the new ones from 5.2.1.

I still can't figure out why this pull request broke the API page. After all of this, I might lean toward just merging that branch. But the proof that this was easy enough to do in an afternoon, and some impetus to actually get this done, is still owed to this pull request, regardless. So thanks again.

Best regards,

- DeeDeeG

[DeeDeeG]

I (finally) figured out what was going on with the API page.

This branch still has the old 2.x version of swagger-ui, which only supports APIs made conforming to the Swagger 1.2 API specification...

Upgrading the grape and grape-swagger gems causes our app to serve an OpenAPI 2.0 API... which the old swagger-ui code doesn't recognize and assumes is a "deprecated" API format, ultimately failing to load.

So that match-up is what breaks the docs page. (Evidently the API itself did not break, just the page explaining the API to end-users. That's why our tests all passed anyway. I think we should add some RSpec tests to the API docs page at some point, so we test that page, and not just the raw API calls.)

---

In short: We can fix the API docs page for this pull request by using older grape and grape-swagger. (See this branch at my fork.)

[DeeDeeG]

Now that the API docs page is working, I'm good to merge this.

Thanks!

(Aside: I did end up changing the author/committer email to one that GitHub would recognize.)