Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rails upgrade 7.0.4.1 and ruby 3.2.0 #312

Open
wants to merge 26 commits into
base: dev
Choose a base branch
from
Open

Conversation

mestachs
Copy link
Collaborator

@mestachs mestachs commented Jan 17, 2023

companion PR : BLSQ/orbf-rules_engine#91

  • ruby / bundler / rubygem update
  • various gem upgrades (including rails, sprockets,...)
  • dropped system test based on selenium (brittle, hard to setup with matching selenium/chrome driver on laptop)
  • some active record migrations due to upgrade gem upgrades
    • active storage
    • paper trail association
  • fixes
    • a few "keyword vs options" problems
    • validation api changes (errors[:myfield] << "my error message" vs errors.add(:myfield, "my error message")
    • active record update_attributes is gone
    • spec due to changes in dentaku (extra decimal, string vs bigdecimal)

@mestachs mestachs marked this pull request as ready for review January 23, 2023 13:20
@mestachs mestachs changed the title Rails upgrade 6 or 7 Rails upgrade 7.0.4.1 and ruby 3.2.0 Jan 24, 2023
Copy link
Contributor

@pjaspers pjaspers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scorpio moving to another Rails version!

Looks good to me

@@ -394,7 +394,7 @@ def changelog(other_project = original)

diff_symbols = { "+" => :added, "-" => :removed, "~" => :modified }
all_names = to_unified_names.merge(other_project.to_unified_names)
HashDiff.diff(other_project.to_unified_h, to_unified_h).map do |hash_diff|
Hashdiff.diff(other_project.to_unified_h, to_unified_h).map do |hash_diff|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's weird yes, as if the gem changed it afterwards
https://github.com/liufengyun/hashdiff#diff or it's another gem ?

@@ -75,8 +76,8 @@ def package_formula_uniqness
formula_by_codes.each do |code, formulas|
next unless formulas.size > 1

rule.errors[:formulas] << "Formula's code must be unique,"\
" you have #{formulas.size} formulas with '#{code}'"
rule.errors.add(:formulas, "Formula's code must be unique,"\
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll miss the << for errors

@@ -32,12 +32,12 @@
class InvoicingJobSerializer
include FastJsonapi::ObjectSerializer
set_key_transform :camel_lower
attribute :org_unit, &:orgunit_ref
attribute :org_unit do |rec| rec.orgunit_ref end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have gone { } for the single line thing, but this words as well, they don't allow a proc anymore?

@@ -3,6 +3,8 @@
<head>
<title>Open RBF 2.0</title>
<%= csrf_meta_tags %>
<link rel="stylesheet" href="https://use.fontawesome.com/releases/v5.15.4/css/solid.css" integrity="sha384-Tv5i09RULyHKMwX0E8wJUqSOaXlyu3SQxORObAI08iUwIalMmN5L6AvlPX2LMoSE" crossorigin="anonymous"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth it to just download it, and serve it ourselves.

@@ -3,6 +3,7 @@
end

RailsAdmin.config do |config|
config.asset_source = :sprockets
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a line that looks simple but hides a lot of hair pulling.

@pjaspers
Copy link
Contributor

pjaspers commented Jan 24, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants