Skip to content

fix: belongs_to field should use association primary_key #3665

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

Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
d84d813
fix belongs_to field to use association primary_key, add specs
Nevelito Feb 13, 2025
6694428
fix specs bugs, function did not handle records without primary key
Nevelito Feb 13, 2025
0eea6e0
fix belongs_to_field
Nevelito Feb 13, 2025
3e4c15c
small changes
Nevelito Feb 20, 2025
14ec146
fix fill_field function
Nevelito Feb 20, 2025
7c46787
fix code to old specs work
Nevelito Feb 20, 2025
5840ba5
Add comments, fix code, fix specs
Nevelito Feb 20, 2025
048f19d
optymalize code
Nevelito Feb 20, 2025
fc2342e
fix standardrb spec
Nevelito Feb 20, 2025
e580342
Merge branch 'main' into fix_belongs_to_field
Paul-Bob Feb 28, 2025
22dcaf0
Add migration and change spec
Nevelito Mar 5, 2025
ec5253a
delete uuid from seeds
Nevelito Mar 5, 2025
6b3d1cd
delete debugger from spec
Nevelito Mar 5, 2025
2795894
change price currency bug in schema
Nevelito Mar 5, 2025
7f9840e
undo changes with adding uuid to user
Nevelito Mar 6, 2025
44ec64b
delete unnecessary migrations and relations
Nevelito Mar 6, 2025
ab78711
change spec
Nevelito Mar 6, 2025
ed2870b
Merge branch 'main' into fix_belongs_to_field
Nevelito Mar 6, 2025
c75c9ec
delete changes from Gemfile.lock
Nevelito Mar 6, 2025
5ed4c6d
comment spec
Nevelito Mar 6, 2025
9f636f3
uncomment spec
Nevelito Mar 6, 2025
09d3d89
fix spec
Nevelito Mar 6, 2025
b9643e3
try to fix spec
Nevelito Mar 7, 2025
e0808bc
add new model, add uuid to event, fix spec
Nevelito Mar 10, 2025
dd17f6b
fix standardrb errors
Nevelito Mar 10, 2025
74370e7
fix bugs
Nevelito Mar 10, 2025
6504ea9
return changes from schema
Nevelito Mar 10, 2025
aff6f43
changes
Nevelito Mar 10, 2025
a1b9396
fix bugs
Nevelito Mar 10, 2025
700db5b
change migration version
Nevelito Mar 10, 2025
b4d8dbe
Merge branch 'main' into fix_belongs_to_field
Paul-Bob Mar 13, 2025
7ef911e
request changes
Nevelito Mar 13, 2025
912c9fd
wip
Paul-Bob Apr 12, 2025
d7663e1
Merge branch 'main' into fix_belongs_to_field
Paul-Bob Apr 12, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions db/factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -138,12 +138,18 @@
end

factory :event do
uuid { SecureRandom.uuid }
location { create :location }
name { Faker::Lorem.sentence }
event_time { DateTime.now }
body { Faker::Lorem.paragraphs(number: rand(1...3)).join("\n") }
end

factory :volunteer do
name { Faker::Name.name }
role { Faker::Job.title }
end

factory :store do
name { Faker::Company.name }
size { ["small", "medium", "large"].sample }
Expand Down
16 changes: 13 additions & 3 deletions lib/avo/fields/belongs_to_field.rb
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,13 @@ def options
values_for_type
end

def primary_key
@primary_key ||= reflection.association_primary_key
# Quick fix for "Polymorphic associations do not support computing the class."
rescue
nil
Copy link
Contributor

Choose a reason for hiding this comment

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

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!

end

def values_for_type(model = nil)
resource = target_resource
resource = Avo.resource_manager.get_resource_by_model_class model if model.present?
Expand All @@ -126,6 +133,7 @@ def values_for_type(model = nil)
end

query.all.limit(Avo.configuration.associations_lookup_list_limit).map do |record|
# to_param uses slug so checking primary_key is unnecessary
[resource.new(record: record).record_title, record.to_param]
end.tap do |options|
options << t("avo.more_records_available") if options.size == Avo.configuration.associations_lookup_list_limit
Expand Down Expand Up @@ -212,14 +220,16 @@ def fill_field(record, key, value, params)
if valid_model_class.blank? || id_from_param.blank?
record.send(:"#{polymorphic_as}_id=", nil)
else
record_id = target_resource(record:, polymorphic_model_class: value.safe_constantize).find_record(id_from_param).id
found_record = target_resource(record:, polymorphic_model_class: value.safe_constantize).find_record(id_from_param)

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Addressed here 912c9fd


record.send(:"#{polymorphic_as}_id=", record_id)
end
else
record_id = value.blank? ? value : target_resource(record:).find_record(value).id
found_record = value.present? ? target_resource(record:).find_record(value) : nil

record.send(:"#{key}=", record_id)
record.send(:"#{key}=", found_record&.send(primary_key.presence || :id))
end

record
Expand Down
13 changes: 13 additions & 0 deletions spec/dummy/app/avo/resources/volunteer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class Avo::Resources::Volunteer < Avo::BaseResource
# self.includes = []
# self.attachments = []
# self.search = {
# query: -> { query.ransack(id_eq: params[:q], m: "or").result(distinct: false) }
# }
def fields
field :id, as: :id
field :name, as: :text
field :role, as: :text
field :event_uuid, as: :text
end
end
4 changes: 4 additions & 0 deletions spec/dummy/app/controllers/avo/volunteers_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# This controller has been generated to enable Rails' resource routes.
# More information on https://docs.avohq.io/3.0/controllers.html
class Avo::VolunteersController < Avo::ResourcesController
end
2 changes: 1 addition & 1 deletion spec/dummy/app/models/event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class Event < ApplicationRecord
has_rich_text :body

belongs_to :location, optional: true

has_many :volunteers, foreign_key: "event_uuid"
has_one_attached :profile_photo
has_one_attached :cover_photo

Expand Down
3 changes: 3 additions & 0 deletions spec/dummy/app/models/volunteer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class Volunteer < ApplicationRecord
belongs_to :event, foreign_key: "event_uuid", primary_key: "uuid"
end
6 changes: 6 additions & 0 deletions spec/dummy/db/migrate/20250310175239_add_uuid_to_events.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
class AddUuidToEvents < ActiveRecord::Migration[6.1]
def change
add_column :events, :uuid, :uuid
add_index :events, :uuid, unique: true
end
end
12 changes: 12 additions & 0 deletions spec/dummy/db/migrate/20250310175357_create_volunteers.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
class CreateVolunteers < ActiveRecord::Migration[6.1]
def change
create_table :volunteers do |t|
t.string :name
t.string :role
t.uuid :event_uuid, null: false

t.timestamps
end
add_index :volunteers, :event_uuid
end
end
13 changes: 12 additions & 1 deletion spec/dummy/db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[8.0].define(version: 2025_01_03_192051) do
ActiveRecord::Schema[8.0].define(version: 2025_03_10_175357) do
# These are extensions that must be enabled in order to support this database
enable_extension "pg_catalog.plpgsql"

Expand Down Expand Up @@ -114,7 +114,9 @@
t.datetime "updated_at", null: false
t.text "body"
t.bigint "location_id"
t.uuid "uuid"
t.index ["location_id"], name: "index_events_on_location_id"
t.index ["uuid"], name: "index_events_on_uuid", unique: true
end

create_table "fish", force: :cascade do |t|
Expand Down Expand Up @@ -297,6 +299,15 @@
t.index ["team_id"], name: "index_users_on_team_id"
end

create_table "volunteers", force: :cascade do |t|
t.string "name"
t.string "role"
t.uuid "event_uuid", null: false
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.index ["event_uuid"], name: "index_volunteers_on_event_uuid"
end

add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id"
add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id"
add_foreign_key "comments", "users"
Expand Down
10 changes: 10 additions & 0 deletions spec/features/avo/belongs_to_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,16 @@
end
end

describe "with custom primary key set" do
let(:event) { create(:event, name: "Sample Event") }

let!(:volunteer) { create(:volunteer, event: event) }

it "displays the event link using the UUID and stores the correct foreign key" do
expect(volunteer.event_uuid).to eq(event.uuid)
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


describe "hidden columns if current polymorphic association" do
let!(:user) { create :user }
let!(:project) { create :project, name: "Haha project" }
Expand Down
Loading