Add rails 7.0 support and drop rails 6.1

[kbrock]

Fixed includes and select issues

we're good to go for rails 7.0 with this

[jrafanie]

EDIT: where is call being called originally? What's the entrypoint?

[kbrock]

Great question.

Sure, we do explicitly call it in our specs: https://github.com/kbrock/activerecord-virtual_attributes/blob/allow_rails_7/spec/virtual_includes_spec.rb#L542

But most of the time, it is called directly by rails.

From what I can tell, this is mostly called directly from preload_associations via exec_query.

https://github.com/rails/rails/blob/7-0-stable/activerecord/lib/active_record/relation.rb#L825-L832

I did try patching in that caller, but when we ran the preloader directly, this had issues - as you know since you had to skip those 3 tests before (we were since able to unskip).

So this fixes 18 specs (probably 50+ uses) in virtual_includes_specs like:

expect(Author.includes(:nick_or_name)).to preload_values(:nick_or_name, author_name)

This includes value ends up in a Preloader. As you know, it is not valid. So we need to translate from the old includes values to the proper ones.

Now I did try and just update @associations value directly, but that only worked if there were no additional levels, so many tests were still failing.

This fix called allows us to get more complicated with our associations.
Coincidently, this recursive-ish solution is similar to the v6 changes where we call into self if the value changed. That was purely coincidental but brought me comfort.

[jrafanie]

@kbrock in https://github.com/ManageIQ/activerecord-virtual_attributes/compare/master...jrafanie:activerecord-virtual_attributes:allow_rails_7_backup?expand=1 (the backup for the old PR), I was also able to delete without_virtual_includes, apply_join_dependency, and calculate from virtual_fields.rb. Can you see if we can remove them? See: c4a3491

My bad. I don't know why but I didn't think it made it into #146, but it did. Sorry.

[kbrock]

@jrafanie good catch. We can definitely deleted them. I thought I had.
they must have snuck back in during rebasing.

update: where are you seeing them?
I went through all monkey patched methods and commented each of them out to ensure they wre required.

We are only patching the following methods (and Preloader.call):

# Expect these methods to exist. (Otherwise we are patching the wrong methods)
%w[
  grouped_records
  preloaders_for_reflection
].each { |method| assert_klass_has_instance_method(ActiveRecord::Associations::Preloader::Branch, method) }

%w[
  build_select
  arel_column
  construct_join_dependency
].each { |method| assert_klass_has_instance_method(ActiveRecord::Relation, method) }

[kbrock]

update:

• rebased
• added comments
• extracted a few PRs that are not directly responsible

update:

• removed extra changes to grouped_records
  • It had extra changes that were unnecessary.
  • Think the changes to includes fixed this stuff as well.
  • This reduces the complexity on this method and hopefully makes code climate happier.

[jrafanie]

@jrafanie good catch. We can definitely deleted them. I thought I had. they must have snuck back in during rebasing.

update: where are you seeing them? I went through all monkey patched methods and commented each of them out to ensure they wre required.

We are only patching the following methods (and Preloader.call):

Sorry, it landed in #146, I forgot I was able to get that moved to the mvp PR

[jrafanie]

ManageIQ/manageiq#22873 is updated to use this branch for rails 7.0 support, and cross repo is being run against it: ManageIQ/manageiq-cross_repo-tests#871. It's 💚 other than consumption but I'll take a look at that. It's not a new problem. It has been failing but because it always fails, I've looked at the other failures first.

    1) ManageIQ::Showback::DataRollup#engine update data_rollups should return value of the group with field
       Failure/Error: expect(data_rollup.get_group('CPU', 'average')).to eq([data_rollup.resource.metrics.for_time_range(data_rollup.start_time, data_rollup.end_time).average(:cpu_usage_rate_average).to_s, "percent"])
  
         expected: ["50.5", "percent"]
              got: [50.5, "percent"]
  
         (compared using ==)
       # ./spec/models/data_rollup_spec.rb:226:in `block (4 levels) in <top (required)>'

[jrafanie]

@kbrock I think we're good to go on moving this forward. I think the test failure in consumption is unrelated to virtual attributes. Looking at this PR one more time.

[kbrock]

update (lj):

• rebased master to get 2 PRs in there

update:

• rebase master
• updated comments in Association::Preloader - grouping by association rather than class (to avoid return from within the loop

(not sure if the preloader change was from here or a previous push)

[kbrock]

This has changelog in it.
Do we think we will get in #150, or hold off? (I want to update the CHANGELOG to reflect)

[miq-bot]

Some comments on commits kbrock/activerecord-virtual_attributes@0595547~...2905c1c

spec/spec_helper.rb

• ⚠️ - 46 - Detected puts. Remove all debugging statements.
• ⚠️ - 47 - Detected puts. Remove all debugging statements.

[miq-bot]

Checked commits kbrock/activerecord-virtual_attributes@0595547~...2905c1c with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
5 files checked, 3 offenses detected

lib/active_record/virtual_attributes/virtual_fields.rb

• ⚠️ - Line 182, Col 23 - Lint/AmbiguousOperatorPrecedence - Wrap expressions with varying precedence with parentheses to avoid ambiguity.
• ❗ - Line 182, Col 15 - Layout/EmptyLineAfterGuardClause - Add empty line after guard clause.
• ❗ - Line 205, Col 13 - Style/MultilineBlockChain - Avoid multi-line chains of blocks.

[jrafanie]

We did a cross repo on this branch and it all passed 💚

ManageIQ/manageiq-cross_repo-tests#871