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

Write attribute with polymorphic integer type #61

Merged
merged 2 commits into from
Dec 15, 2023

Conversation

wendy-clio
Copy link
Contributor

@wendy-clio wendy-clio commented Dec 12, 2023

Resolved #14
Resolved #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

@wendy-clio wendy-clio self-assigned this Dec 12, 2023
@wendy-clio wendy-clio requested review from a team, MrJakeReid and tudorbertiean and removed request for a team December 12, 2023 19:20
@wendy-clio wendy-clio assigned apalmblad and unassigned apalmblad Dec 12, 2023
@wendy-clio wendy-clio force-pushed the fix_has_many_has_one_assign_logic branch from dd95d02 to c480a89 Compare December 12, 2023 19:29
@wendy-clio wendy-clio force-pushed the fix_has_many_has_one_assign_logic branch from c480a89 to 504bca4 Compare December 12, 2023 19:59
@wendy-clio wendy-clio force-pushed the fix_has_many_has_one_assign_logic branch from 504bca4 to f77fc1e Compare December 12, 2023 20:02
@wendy-clio wendy-clio requested a review from Vtown121 December 12, 2023 20:04
@tudorbertiean
Copy link

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

Copy link

@tudorbertiean tudorbertiean left a comment

Choose a reason for hiding this comment

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

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

@apalmblad
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

MrJakeReid
MrJakeReid previously approved these changes Dec 13, 2023
Copy link

@MrJakeReid MrJakeReid left a comment

Choose a reason for hiding this comment

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

We went through the QA and the explanation of the team, thank you for the walkthrough and jumping on the fix!

tudorbertiean
tudorbertiean previously approved these changes Dec 13, 2023
Vtown121
Vtown121 previously approved these changes Dec 13, 2023
@wendy-clio wendy-clio force-pushed the fix_has_many_has_one_assign_logic branch from bbcdc49 to 8f727b1 Compare December 13, 2023 20:14
@apalmblad
Copy link
Member

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
Copy link
Contributor Author

wendy-clio commented Dec 13, 2023

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")

@wendy-clio wendy-clio force-pushed the fix_has_many_has_one_assign_logic branch from 8f727b1 to 5d258ed Compare December 13, 2023 20:38
apalmblad
apalmblad previously approved these changes Dec 13, 2023
@apalmblad
Copy link
Member

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
Copy link
Contributor Author

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.

@@ -64,8 +68,10 @@ def remove_type_and_establish_mapping(name, options, scope)
condition = instance_exec(&scope).merge(condition) if scope.is_a?(Proc)
condition
}
return foreign_type, klass_mapping.to_i

Choose a reason for hiding this comment

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

For readability sake, what do you think about adding a comment with an example of what we're expecting to return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like source_type, 2? but I doubt it will meant much for people without the full context of polymorphic setting (activeRecord, reflections and mapping)

Choose a reason for hiding this comment

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

That's true! Never mind then!

@wendy-clio wendy-clio merged commit 148a2cc into master Dec 15, 2023
8 checks passed
@wendy-clio wendy-clio deleted the fix_has_many_has_one_assign_logic branch December 15, 2023 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants