foodcoops / foodsoft

Web-based software to manage a non-profit food coop (product catalog, ordering, accounting, job scheduling).
https://foodcoops.net/
Other
326 stars 146 forks source link

ruby 2.7.2 and rails 7 upgrade #979

Closed yksflip closed 1 year ago

yksflip commented 1 year ago

This PR has the required changes for the ruby and rails upgrade, see #956

it'd make sense to first merge these:

  1. [x] rswag api tests #969
  2. [x] rebase master
  3. [x] make ci run again
  4. [x] run rubocop autofixes
  5. [x] create issue for ruby 3 upgrade
  6. [x] fix order matrix pdf generation error (sql injection possible?)
    Dangerous query method (method whose arguments are used as raw SQL) called with non-attribute argument(s): "SUM(COALESCE(group_order_articles.result, group_order_articles.quantity))".This method should not be called with user-provided values, such as request parameters or model attributes. Known-safe values can be passed by wrapping them in Arel.sql().
  7. [x] check flaky tests
  8. [x] do some more manual acceptance testing (real life scenarios)
  9. [x] test production (docker) setup

  1. [ ] controller tests #970 #972 this isn't a blocker actually. would be nice for better coverage, but let's get the upgrade done first.

happy for any comments or ideas how we can proceed with this! :)

lentschi commented 1 year ago

@yksflip First of all: Thanks for all your work on this!! :blush:

Then one question related to the reviewing process: Would it be possible to do the rubocop autofixes (changed due to the rubocop upgrade) in a separate PR? (That new PR could be easily merged without much review as rubocop's autofixes are fairly stable afaik. It would also make this PR - the actual ruby & rails upgrade - easier to review.) Or is that simply not possible due to ruby syntax changes? (I didn't check)

kidhab commented 1 year ago

How can we move forward with this and all related MR? I think it is more difficult to merge this if the development continues and the MRs and the master branch diverge more and more. Maybe it's fine to solve the merge conflicts and just merge it? After all the opener worked in a team for about 6 months on the Foodsoft code and got support from the folks at Pragma Shift who work on a daily basis with RoR. Also there's a Foodsoft instance out there running for with the RoR update for some months now, isn't it?

Maybe we can ask some Foodcoops which are using the global hosting to use a testing instance with these MR to get more Feedback?

And maybe we can fund some work to solve all related conflicts if it's a lack of time?

yksflip commented 1 year ago

hey! thanks kidhab for the ping! You're absolutely right, and this is totally waiting on me right now. It's so great to see activity on the main branch and we should definetly get this merged soon! I'll get back on this next week!

kidhab commented 1 year ago

With Ruby 2.7.8 the 2.7 series reached EOL. Maybe this is a good topic for the next community call.

yksflip commented 1 year ago

one question related to the reviewing process: Would it be possible to do the rubocop autofixes (changed due to the rubocop upgrade) in a separate PR? (That new PR could be easily merged without much review as rubocop's autofixes are fairly stable afaik. It would also make this PR - the actual ruby & rails upgrade - easier to review.) Or is that simply not possible due to ruby syntax changes? (I didn't check)

not sure if i understand correctly .. so there are still a lot rubocop violations that need to be autofixed .. I think it would make sense to do it in this PR, but in a seperate commit. I think for reviewing you can select what commits to display, so one could disable the rubocop commit ... The changes that are already in here are needed for syntax reasons I think.

yksflip commented 1 year ago

With Ruby 2.7.8 the 2.7 series reached EOL. Maybe this is a good topic for the next community call.

why is software aging so fast? :sweat_smile: Should talk about it ... but hopefully going to ruby 3.1 shouldn't be a big deal i guess? (But I'd seperate it from this here ...)

yksflip commented 1 year ago

After some more testing I feel pretty confident about this changes. If nobody has any concerns, I'd go on with a optimistic merge ... @wvengen @paroga @lentschi