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

Issues with has_many relation for STI model #3406

Closed
4 of 11 tasks
sandraa-nestor opened this issue Nov 11, 2024 · 7 comments · Fixed by #3414
Closed
4 of 11 tasks

Issues with has_many relation for STI model #3406

sandraa-nestor opened this issue Nov 11, 2024 · 7 comments · Fixed by #3414
Assignees
Labels
Bug Something isn't working

Comments

@sandraa-nestor
Copy link

Describe the bug

STI model
class Entities::System < Entity with relation has_many :bank_accounts
Bank account model
class BankAccount < ApplicationRecord with belongs_to :system_entity, class_name: "Entities::System"

  1. Association policies on STI models are ignored. Cannot hide Attach bank account button in tab in SystemEntityPolicy.
    Image

  2. When creating Bank account through parent resource belongs_to selector of System Entity is not set automatically:
    avo/resources/bank_accounts/new?via_record_id=2&via_relation=bank_accounts&via_relation_class=Entities::System&via_resource_class=Avo::Resources::SystemEntity
    Image

Models and resource files

class Avo::Resources::BankAccount < Avo::BaseResource
  def fields
    field :system_entity, as: :belongs_to, only_on: [:new, :show, :index]
  end
end
class Avo::Resources::SystemEntity < Avo::Resources::Entity
  def fields
    super

    tabs do
      field :bank_accounts, as: :has_many
    end
  end
end
class SystemEntityPolicy < EntityPolicy
  def create_bank_accounts?
    BankAccountPolicy.new(user, record).create?
  end

  def edit_bank_accounts?
    BankAccountPolicy.new(user, record).edit?
  end

  def attach_bank_accounts?
    false
  end

  def detach_bank_accounts?
    false
  end
end

Expected behavior & Actual behavior

Expected behavior: bank accounts can be managed through SystemEntityPolicy
Actual behavior: SystemEntityPolicy has no impact on Bank accounts tab

Expected behavior: bank accounts can be created through parent resource with correctly selected System Entity
Actual behavior: the selector is blank

System configuration

Avo version: 3.13.7

Rails version: 7.2.2

Ruby version: 3.3.4

License type:

  • Community
  • Pro
  • Advanced

Are you using Avo monkey patches, overriding views or view components?

  • Yes. If so, please post code samples.
  • No

Impact

  • High impact (It makes my app un-usable.)
  • Medium impact (I'm annoyed, but I'll live.)
  • Low impact (It's really a tiny thing that I could live with.)

Urgency

  • High urgency (I can't continue development without it.)
  • Medium urgency (I found a workaround, but I'd love to have it fixed.)
  • Low urgency (It can wait. I just wanted you to know about it.)
@adrianthedev
Copy link
Collaborator

Hey @sandraa-nestor.
Thanks for opening this issue.
Can you please create a reproduction repo that we can run on our side?

Here's a quick command that will help you generate a new Avo app in no-time.
https://docs.avohq.io/3.0/technical-support.html#reproduction-repository

Thank you!

@Paul-Bob
Copy link
Contributor

I'll address point 1 from the issue. No need to include the policies in the reproduction repository, but please do it with the point 2.


By default, each resource has a one-to-one mapping with a policy, with that policy being determined based on the model class tied to the resource.

For example:

  • Avo::Resources::SystemEntity uses the SystemEntity model, and thus defaults to SystemEntityPolicy.
  • Similarly, Avo::Resources::BankAccount would use the BankAccount model and map to BankAccountPolicy.

When Avo determines which policy class to use, it simply appends Policy to the model class name. This default behavior works the same whether the models involve single-table inheritance (STI) or not.

In your scenario, it seems logical to manage both resources with a single policy file. You can accomplish this by specifying the self.authorization_policy (docs here):

class Avo::Resources::BankAccount < Avo::BaseResource
  self.authorization_policy = SystemEntityPolicy
  def fields
    field :system_entity, as: :belongs_to, only_on: [:new, :show, :index]
  end
end

@sandraa-nestor
Copy link
Author

Ahh, I see that... I had an self.model_class option set for SystemEntity resource. That's why I need to set authorization_policy explicitly.

class Avo::Resources::SystemEntity < Avo::Resources::Entity
   self.model_class = ::Entities::System
   self.authorization_policy = SystemEntityPolicy
 ...
end

Now policy works fine, thank you @Paul-Bob!

@sandraa-nestor
Copy link
Author

Hey @adrianthedev!
I've managed to reproduce the issue. It's because of foreign_key option on has_many relation

class Users::Student < User
  has_many :marks, foreign_key: :student_id
end

Here is my reproduction repo

Screen.Recording.2024-11-12.at.13.47.31.mov

@Paul-Bob Paul-Bob added the Bug Something isn't working label Nov 12, 2024
@Paul-Bob Paul-Bob moved this to Next up in Issues Nov 12, 2024
@Paul-Bob Paul-Bob self-assigned this Nov 12, 2024
@Paul-Bob
Copy link
Contributor

Thank you for the reproduction repository @sandraa-nestor

I'll have a look

@Paul-Bob
Copy link
Contributor

Setting the inverse_of on the model association fixes this issue.

class Users::Student < User
  has_many :marks, foreign_key: :student_id, inverse_of: :student
end

Avo occasionally leans on model reflections, particularly when it comes to association fields. Rails usually handles inverse_of automatically, but it seems in this setup, Rails isn't quite catching onto inverse_of unless you configure it explicitly.

My recommendation? Just go ahead and declare inverse_of on all associations to play it safe.

Marking this as done, but feel free to reopen or keep chatting if needed!

@github-project-automation github-project-automation bot moved this from Next up to Done in Issues Nov 12, 2024
@Paul-Bob
Copy link
Contributor

I'm reopening this issue to explore the possibility of identifying this use case and raising a developer-friendly error to prompt the setting of inverse_of.

@Paul-Bob Paul-Bob reopened this Nov 13, 2024
@Paul-Bob Paul-Bob moved this from Done to Next up in Issues Nov 13, 2024
@github-project-automation github-project-automation bot moved this from In Review to Done in Issues Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants