Add support for foreign_type #1425

mswiszcz · 2021-03-05T23:02:08Z

Allows to create tests checking set foreign_type

class Visitor < ActiveRecord::Base
  belongs_to :location, foreign_type: :facility_type, polymorphic: true
end

class Hotel < Location
  has_many :visitors, inverse_of: :location, foreign_type: :facility_type, as: :location
end

class PrisonCell < Location
  has_one :visitor, inverse_of: :location, foreign_type: :facility_type, as: :location
end

expect(Visitor.new).to belong_to(:location).with_foreign_type(:facility_type)
expect(Hotel.new).to have_many(:visitors).with_foreign_type(:facility_type)
expect(PrisonCell.new).to have_one(:visitor).with_foreign_type(:facility_type)

mswiszcz · 2021-03-06T00:01:34Z

Can someone rerun those checks? Seems like a dependency / gems server was temporarily unavailable

mcmire · 2021-03-06T00:17:43Z

@mswiszcz Yup! Re-running now.

mcmire

Hey @mswiszcz, sorry for the delay in reviewing this. I've left some comments below, would you mind taking another look?

mcmire · 2021-03-23T16:28:36Z

lib/shoulda/matchers/active_record/association_matcher.rb

@@ -1267,6 +1331,12 @@ def foreign_key_exists?
          !(belongs_foreign_key_missing? || has_foreign_key_missing?)
        end

+        def foreign_type_matches?
+          return true unless options.key?(:foreign_type)


We don't typically return early or use the modifier form of unless. Since the end result is a boolean, what do you think about combining these into one conditional?

!options.key?(:foreign_type) || ( !belongs_foreign_type_missing? && !has_foreign_type_missing? )

mcmire · 2021-03-23T16:33:17Z

lib/shoulda/matchers/active_record/association_matcher.rb

@@ -1472,6 +1579,16 @@ def foreign_key_reflection
          end
        end

+        def foreign_type
+          type = if [:has_one, :has_many].include?(macro)


To simplify this expression and remove the temporary assignment, what do you think about moving the .to_s to has_column? at line 1509?

mcmire · 2021-03-23T16:34:00Z

spec/unit/shoulda/matchers/active_record/association_matcher_spec.rb

      define_model :parent do
        has_many :children, foreign_key: :ancestor_id, inverse_of: false
      end

      expect(Parent.new).to have_many(:children)
    end

+    it 'accepts an association with a nonstandard type, with reverse association turned off' do
+      define_model :visitor, location_id: :integer, facitility_type: :string


Should this be facility_type?

matsales28 · 2024-01-12T14:42:53Z

I think this would be a nice addition, @mswiszcz do you still plan to address the feedback? If not I can address it and give you credits.

mswiszcz · 2024-01-12T17:02:58Z

hey @matsales28, I totally forgot about that PR after changing project, sorry about that. Please feel free to take that over as I don't have much time at the moment to finish it

matsales28 · 2024-02-02T16:00:11Z

I'm closing this PR in favor of #1609

mswiszcz added 3 commits March 6, 2021 00:00

Add support for foreign_type

ed00c46

Rubocop fix

7484513

Fix documentation

1cf931c

mcmire reviewed Mar 23, 2021

View reviewed changes

matsales28 mentioned this pull request Feb 2, 2024

feat: Add support for foreign_type qualifier on AssociationMatcher #1609

Merged

matsales28 closed this Feb 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for foreign_type #1425

Add support for foreign_type #1425

mswiszcz commented Mar 5, 2021

mswiszcz commented Mar 6, 2021

mcmire commented Mar 6, 2021

mcmire left a comment

mcmire Mar 23, 2021

mcmire Mar 23, 2021

mcmire Mar 23, 2021

matsales28 commented Jan 12, 2024

mswiszcz commented Jan 12, 2024 •

edited

Loading

matsales28 commented Feb 2, 2024

Add support for foreign_type #1425

Add support for foreign_type #1425

Conversation

mswiszcz commented Mar 5, 2021

mswiszcz commented Mar 6, 2021

mcmire commented Mar 6, 2021

mcmire left a comment

Choose a reason for hiding this comment

mcmire Mar 23, 2021

Choose a reason for hiding this comment

mcmire Mar 23, 2021

Choose a reason for hiding this comment

mcmire Mar 23, 2021

Choose a reason for hiding this comment

matsales28 commented Jan 12, 2024

mswiszcz commented Jan 12, 2024 • edited Loading

matsales28 commented Feb 2, 2024

mswiszcz commented Jan 12, 2024 •

edited

Loading