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

fix: belongs_to field should use association primary_key #3665

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Nevelito
Copy link
Contributor

@Nevelito Nevelito commented Feb 13, 2025

Description

If record had primary_key set, it was still using :id

Changes:

  • Change fill_field function in belongs_to_firld.rb
  • Add primary_key function
  • Add specs

Fixes # (issue)
Issue: #3413

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

Copy link

codeclimate bot commented Feb 13, 2025

Code Climate has analyzed commit e580342 and detected 0 issues on this pull request.

View more on Code Climate.

@Nevelito Nevelito changed the title fix: belongs_to field should use association primary_key [WIP] fix: belongs_to field should use association primary_key Feb 13, 2025
@Nevelito Nevelito changed the title [WIP] fix: belongs_to field should use association primary_key fix: belongs_to field should use association primary_key Feb 20, 2025
Copy link
Contributor

@Paul-Bob Paul-Bob left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this, @Nevelito!

I tested this PR using the reproduction repo, and it does seem to resolve the issue. However, the test coverage doesn't currently reproduce the problem. Would it be possible to add a belongs_to relationship that uses primary_key in the dummy app and write a test against that scenario?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants