clio / polymorphic_integer_type

MIT License
37 stars 35 forks source link

Write attribute with polymorphic integer type #61

Closed wendy-clio closed 10 months ago

wendy-clio commented 10 months ago

Resolved https://github.com/clio/polymorphic_integer_type/issues/14 Resolved https://github.com/clio/polymorphic_integer_type/issues/17

This is a PR for fixing the problem of below example, by manually write to record when there's integer_type options exist for either has_many or has_one association/reflection and existing proper value for the foreign_type and integer_mapping_type.

Class A
  Belongs_to :b, polymorphic: true, integer_type: true
  Belongs_to :c, polymorphic: true, integer_type: true
End
Class B
  Has_one :a, integer_type: true
End
Class C
  Has_many :as, integer_type: true
End

C.as =  [ A.new] # fails
B.a = A.new #fails
tudorbertiean commented 10 months ago

Curious as to why this is a "temp" PR??

apalmblad commented 10 months ago

ONe more case, which I think we should consider if we want to do this correctly: Class A Belongs_to :b, polymorphic: true, integer_type: true End Class B Has_one :a, integer_type: true End class D < B end

What should the type column be in this case? I think it should match D but we should double-check how current polymorphism works.

wendy-clio commented 10 months ago

changes LGTM, if you can send over some QA steps I can help test this out as well and do a bit of regression testing too

you can easier clone the repo and checkout my branch to set to your local project Gemfile gem "polymorphic_integer_type", path: '../polymorphic_integer_type' to test out.

wendy-clio commented 10 months ago

ONe more case, which I think we should consider if we want to do this correctly: Class A Belongs_to :b, polymorphic: true, integer_type: true End Class B Has_one :a, integer_type: true End class D < B end

What should the type column be in this case? I think it should match D but we should double-check how current polymorphism works.

The native polymorphic is assigning type as B. Basically the class that has the actual reflection setup.

apalmblad commented 10 months ago

The native polymorphic is assigning type as B. Basically the class that has the actual reflection setup.

Cool -> let's add a spec for that, and then I'm good,

wendy-clio commented 10 months ago

The native polymorphic is assigning type as B. Basically the class that has the actual reflection setup.

Cool -> let's add a spec for that, and then I'm good,

there is a test that exist previously already done the job, when dog is inherent from Animal.rb and the expect is assert against "Animal" type at this line expect(link.source_type).to eq("Animal")

apalmblad commented 10 months ago

Suggested a few other reviewers; this seems fine to me and fixes an issue we'd have to have worked around, but I'd love at least another consideration of wtf could go wrong when this ships to manage.

wendy-clio commented 10 months ago

Suggested a few other reviewers; this seems fine to me and fixes an issue we'd have to have worked around, but I'd love at least another consideration of wtf could go wrong when this ships to manage.

Nice! I will keep it open for couple more days.