search

RefugeRestrooms / refugerestrooms

REFUGE restrooms indexes and maps safe restroom locations for trans, intersex, and gender nonconforming individuals.
http://www.refugerestrooms.org
GNU Affero General Public License v3.0
888 stars 262 forks source link

Rails7 upgrade #689

Closed ice1080 closed 7 months ago

ice1080 commented 7 months ago

Context

Summary of Changes

Checklist

ice1080 commented 7 months ago

@DeeDeeG once #688 gets merged I can merge or rebase. I also see the same single unit test failure on this, but it works manually.

DeeDeeG commented 7 months ago

Thanks for this!

I've read the diff once over, and it all generally looks good (and a mostly small functional diff, considering a major version upgrade of our main framework), considering the tests are passing in CI. (Other than the one known issue from before this PR as you mentioned).

I hope to get a chance to manually test it myself soon, take a look around at the finer details, just look through more thoroughly.

But this will be great to have, with this we'll pretty much have all our fundamental dependencies like Node, Ruby and Rails up to date! (JS and Ruby dependencies not doing too badly in some senses either.) [EDIT: Efforts to move away from Rails' Webpacker would still be an improvement as well, of course! And any possibility of a JS full app re-write, etc. But keeping the current stuff up to date is important!]

Once again, thanks, and I'll hope to review this one properly soon! (Open to input from other maintainers, but this seems like a small enough change and with CI looking good that I'm hopeful this is a fairly easy upgrade.)

DeeDeeG commented 7 months ago

Hi again, just checking in / providing a status update:

I have another project that I have been busy with for the past few days (that project gets especially busy leading up to/around the 15th.)

I hope to make some time for this after the other project settles down for the month. Ideally I'd like to get my personal Heroku account set up for testing, if the other maintainers haven't set up testing environments on the main account before then. Especially for the NodeJS bump. I may merge this one first if this one proves easier to test locally.

Best Regards.

DeeDeeG commented 7 months ago

I have the chance to test this manually now.

Local/manual testing is working out very well for basically the whole app, barring one part.

The API docs page /api/docs (like https://refugerestrooms.org/api/docs/) is not loading its content. So, I suppose something about how JavaScript is loaded may have changed?

We have some files related to loading these API docs:

-- One can see the PR where it was implemented in this form here: https://github.com/RefugeRestrooms/refugerestrooms/pull/506

If it is possible to get this working again, that would be very good. No specs test the docs page working at the moment, so it is tested by manual testing only for now.

Best Regards.

ice1080 commented 7 months ago

Ah interesting @DeeDeeG , I'll look into this. I definitely tested them manually at one point but something else I did must have broken them. Will get a fix out!

ice1080 commented 7 months ago

Hmm, I see /api/docs working when I run it locally using docker-compose up. If you run this branch locally are you able to see it working? Looking into switching to the prod config locally if possible now to see if I can reproduce

ice1080 commented 7 months ago

@DeeDeeG I wasn't able to reproduce locally since it seems to be a production only issue and I don't have the necessary config vars to run prod locally. I pushed one change blind that seems like it could be related, from the rails docs, but I can't test it so need you to try that one more time when you get some time

DeeDeeG commented 7 months ago

My apologies, it appears the issue was just on my Docker setup. I deleted all volumes and images built for Refuge and rebuilt them (docker compose build and docker compose up) and then it worked.

Extra details about the error I was seeing (now resolved) (click to expand if curious): For what it's worth, the error was something to do with Webpacker not finding some function it wanted to call. There was some sort of bundle JS file it was calling into, and I'm not sure what was wrong there exactly, but as mentioned it did go away after I rebuilt the image, regardless of with/without the last commit in this branch. ``` Uncaught TypeError: __webpack_require__(...).start is not a function js application.js:2 Webpack 3 __webpack_require__ ```

The latest commit did not seem to make the difference, so given it seems unrelated to the issue, I'd like to ask that it be reverted, or I can certainly do that if you prefer, and I would be ready to merge this PR at that point.

Thank you very much once again!

ice1080 commented 7 months ago

So glad to hear it worked! Reverting the commit now

ice1080 commented 7 months ago

@DeeDeeG just reverted the latest commit (f465bd46c1d900cd9e642954fd07cd384b0289f0). Should be ready now!

Please note that there are a few configs that are a bit more advanced that should be reviewed as well. These configs live in the following files:

For longer term maintenance, someone with more rails experience should review those and work on getting the project updated with those files using these guides:

I noticed in application.rb that we're still using defaults from rails 5 so each future rails version will get harder to update.

Anyway though, this rails7 upgrade should help with some of the other issues in the repo and we can tackle those later. Thanks!

DeeDeeG commented 7 months ago

just reverted the latest commit

Thank you!

a few configs that are a bit more advanced that should be reviewed as well [ . . . ] new_framework_defaults*.rb

Yes, I suppose we need to go through the backlog of all these, flip them one by one, test one by one, and so on. Should be doable. I think we've been reluctant to put a chunk of time into verifying something that might maybe break the app in an obscure way if we don't catch it, but it'd be good to enable defaults and drop these configs that effectively put us at some risk of having the old behavior deprecated/removed for us by a newer Rails version that comes out... I shall have it in the back of my mind on my to-do list, at minimum. Maybe I can carve out time to work through them all (or a good chunk of them every so often) some time soon... In any case, progress is progress!

I'm confident in merging this one. A big thanks for this upgrade PR.

And then, since https://github.com/RefugeRestrooms/refugerestrooms/pull/688 needs some testing with a Heroku account to confirm the config it pushes to Heroku is adequate to have the app build successfully, I'll work that out on a test dyno on a real Heroku account. If that all goes well, I can push all of these updates out to production together. If the Node update hits a snag, I'll probably push the rest out first and troubleshoot that one if it takes a lot of doing.

Anyway, future plans aside, this one is good to go! Much appreciated.