clio / polymorphic_integer_type

MIT License
37 stars 35 forks source link

Support for Rails 6.1.4.1 and onward (Until 7) + Supporting CI Changes. #47

Closed Aelphaeis closed 2 years ago

Aelphaeis commented 3 years ago

Presently we do not have support for rails 6.1.4.1.

The objective of this pull request is to fix tests and ensure that the polymorphic integer type works with rails 6.1.4.1 as well as update CI to test this functionality.

Several tests are broken because https://github.com/rails/rails/pull/40969 & https://github.com/rails/rails/issues/40943 were fixed/closed by https://github.com/rails/rails/commit/b52b8040ff8170118bc8ada43413e5a6d97a778f. The net effect that the original replace_keys has been updated to have a new argument called force

I've made some changes to better support continuous integration.

fixes #38

Aelphaeis commented 3 years ago

Several tests are broken because https://github.com/rails/rails/pull/40969 & https://github.com/rails/rails/issues/40943 were fixed/closed by https://github.com/rails/rails/commit/b52b8040ff8170118bc8ada43413e5a6d97a778f.

The net effect that the original replace_keys has been updated to have a new argument called force

The stack trace comes from the BelongsToPolymorphicAssociation.replace_keys and looks as follows:

  1) PolymorphicIntegerType when the source is nil should have the no id/type for the source
     Failure/Error:
       private def replace_keys(record)
         super
         unless record.nil?
           owner[reflection.foreign_type] = record.class.base_class
         end

     ArgumentError:
       wrong number of arguments (given 2, expected 1)
     # ./lib/polymorphic_integer_type/belongs_to_polymorphic_association_extension.rb:4:in `replace_keys'
     # ./lib/polymorphic_integer_type/module_generator.rb:24:in `block (2 levels) in generate_and_include'

To fix while preserving existing behaviour we should set the force variable to true; however, this has the negative side effect of re-introducing the bugs this fixes. Will investigate other solutions.

Aelphaeis commented 3 years ago

I've made some changes to better support continuous integration.

Previously bundler cache was set to true. This had caused CI to run the following command

  /opt/hostedtoolcache/Ruby/2.6.8/x64/bin/bundle config --local deployment true

When you used bundle config to set the deployment to true the gemfile must match the gemfile.lock.

This meant that if you changed a dependency in the gemspec you were forced to run bundle install --gemfile path/to/gemfile on each of the gemfiles and check in the resulting lock files.

I felt that this was unintuitive and a lot of work, I also felt that the setting deployment to true was unnecessary and decided to do the bundle install manually. This results in us no longer needing to update the lock files. Since all of the required libraries are publicly available in ruby gems and on github I think this is safe.

This allowed us to delete the lock files in d9e7c8d.

Aelphaeis commented 3 years ago

https://github.com/rails/rails/commit/ccb13cb3bc002bc03c669d504bebdfcf3db5d6c1 renames association to reflection. This causes our patch to fail. I've updated it as a result.