fix: belongs_to field should use association primary_key

[Nevelito]

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

[codeclimate]

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

View more on Code Climate.

[Paul-Bob]

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?

[Nevelito]

FactoryBot.define do
  factory :user do
    uuid { SecureRandom.uuid }
    first_name { Faker::Name.first_name }
    last_name { Faker::Name.last_name }
    email { Faker::Internet.email }
    password { Faker::Internet.password }
    roles { {admin: false, manager: [true, false].sample, writer: [true, false].sample} }
    birthday { Faker::Date.birthday(min_age: 18, max_age: 65) }
    custom_css { ".header {\n  color: red;\n}" }
  end
  

schema:

create_table "users", force: :cascade do |t|
  t.string "email", default: "", null: false
  t.string "first_name"
  t.string "last_name"
  t.json "roles"
  t.date "birthday"
  t.text "custom_css"
  t.bigint "team_id"
  t.string "encrypted_password", default: "", null: false
  t.string "reset_password_token"
  t.datetime "reset_password_sent_at", precision: nil
  t.datetime "remember_created_at", precision: nil
  t.datetime "created_at", null: false
  t.datetime "updated_at", null: false
  t.boolean "active", default: true
  t.string "slug"
  t.string "uuid", null: false
  t.index ["email"], name: "index_users_on_email", unique: true
  t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true
  t.index ["slug"], name: "index_users_on_slug", unique: true
  t.index ["team_id"], name: "index_users_on_team_id"
  t.index ["uuid"], name: "index_users_on_uuid", unique: true
end

Do you have any ideas why it is happening ? I could not find any info in Internet.

[Nevelito]

I pushed changes, have a look in free time. I will try to fix it tomorrow one more time.

[Paul-Bob]

I pushed changes, have a look in free time. I will try to fix it tomorrow one more time.

The User and Post models are heavily used in many tests. If you notice that this change breaks existing tests, I suggest creating the test scenario with other models that are less burdened, such as Event, or even a new model if necessary.

Let's ensure that all the tests run successfully.

[Nevelito]

I pushed changes, have a look in free time. I will try to fix it tomorrow one more time.

The User and Post models are heavily used in many tests. If you notice that this change breaks existing tests, I suggest creating the test scenario with other models that are less burdened, such as Event, or even a new model if necessary.

Let's ensure that all the tests run successfully.

Hi, I’m still having some issues with testing. Could you take a look and maybe suggest something I could try?

[Paul-Bob]

Hei @Nevelito

I suggest setting up the test scenario using the Event model or another simpler model and set up a new association that uses a custom primary_key. If needed, you could even create a new model instead of dynamically adding columns and associations during tests. That should make things more manageable and reliable.

[Paul-Bob]

Thanks for looking into this, @Nevelito !

Everything looks clean, and the setup appears to be correct. I’ve left some comments on the code, let’s continue the discussion there.

[Paul-Bob]

This was a temporary fix for an error encountered during development. Could you remove this rescue and verify if the error still occurs? If it does, let's investigate the root cause and implement a more robust solution if possible. Thanks!

[Paul-Bob]

Suggested change

	record_id = found_record&.send(primary_key.presence || :id)
	record_id = found_record.send(primary_key.presence || :id)

This line must necessarily have a found_record. If no record is found, it indicates an issue that needs to be addressed. Let's remove the safe navigator here, as well as on line 232, and check if anything breaks. If it does, we should investigate the root cause.

[Paul-Bob]

Addressed here 912c9fd

[Paul-Bob]

This test is not effectively validating the changes. If we use the same setup on main without the logic changes to the belongs_to field, it still runs successfully without any failures.

Let's correctly set the volunteer with field :event, as: :belongs_to and write a test that ensures a volunteer is created, during creation it selects an event, and is correctly associated with that event after creation.

[github-actions]

This PR has been marked as stale because there was no activity for the past 15 days.