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

Feature: Run callbacks on through model when detaching association #3004

Merged
merged 9 commits into from
Jul 27, 2024

Conversation

binarygit
Copy link
Contributor

@binarygit binarygit commented Jul 17, 2024

Description

Fixes #2512

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

Manual review steps

  1. Step 1
  2. Step 2

Manual reviewer: please leave a comment with output from the test if that's the case.

Copy link

codeclimate bot commented Jul 17, 2024

Code Climate has analyzed commit 14b3980 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1

View more on Code Climate.

@binarygit
Copy link
Contributor Author

binarygit commented Jul 17, 2024

@Paul-Bob

# team.rb
  has_many :team_members, through: :memberships, class_name: "User", source: :user, inverse_of: :teams

# user.rb
  has_and_belongs_to_many :teams, join_table: :team_memberships, inverse_of: :admin

Here team defines it's team_members assoc using a through table however, User defines the same association as a HABTM.

Detaching a user from the team show page will call team_membership.destroy but because user has setup this assoc as HABTM I don't think when detaching a team from the user's show page should call team_membership.destroy. Am I correct?

Update: Adrian and I talked about this. user.rb should be declaring the relationship as a through relationship. It's unusual to declare a relationship as a through at one end and a HABTM on the other end.

@binarygit binarygit force-pushed the detach-through-associations branch from d9fda8f to e7b26af Compare July 17, 2024 19:26
@Paul-Bob
Copy link
Contributor

Update: Adrian and I talked about this. user.rb should be declaring the relationship as a through relationship. It's unusual to declare a relationship as a through at one end and a HABTM on the other end.

Agree, I also noticed that particularity on our dummy recently.

Does this PR work well from both sides if the associations are correctly defined?

@binarygit binarygit force-pushed the detach-through-associations branch from 2f60cb4 to 1196118 Compare July 23, 2024 08:15
@binarygit binarygit marked this pull request as ready for review July 23, 2024 08:29
@binarygit binarygit requested a review from Paul-Bob July 23, 2024 08:29
@binarygit binarygit changed the title Call destroy when detaching through associations Feature: Run callbacks on through model when detaching association Jul 23, 2024
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.

Looking good! Thanks @binarygit, let's please remove the duplicated association and also solve the conflicts.

@binarygit
Copy link
Contributor Author

Wait. I’m working on displaying the messages when associations are misconfigured

@binarygit binarygit force-pushed the detach-through-associations branch from 1cab170 to 7d6d0e9 Compare July 25, 2024 14:16
@binarygit binarygit force-pushed the detach-through-associations branch 2 times, most recently from e608c8f to 03b0466 Compare July 25, 2024 15:54
@@ -93,6 +96,18 @@

private

def redirect_with_error_message

Check failure

Code scanning / CodeQL

Code injection Critical

This code execution depends on a
user-provided value
.
@binarygit binarygit force-pushed the detach-through-associations branch 3 times, most recently from 714e695 to bd7251a Compare July 25, 2024 16:39
@binarygit
Copy link
Contributor Author

@Paul-Bob I couldn't find a straightforward way to check if associations are properly configured. Sorry for holding this up, maybe merge this now and I'll pick this up again later.

@binarygit
Copy link
Contributor Author

binarygit commented Jul 25, 2024

I actually talked about this here: https://rubyonrails-link.slack.com/archives/C05054QPL/p1721928807967489 and I don't think verifying associations are setup correctly should be our job.

I did try and it's too much work and also feels like it should be upto the user (like the devs point out in the link) to set it up correctly. WDYT?

@Paul-Bob
Copy link
Contributor

It's ok without the association check @binarygit thanks for looking into it

Copy link
Collaborator

@adrianthedev adrianthedev left a comment

Choose a reason for hiding this comment

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

Looks good.

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.

It's looking good, thanks @binarygit!

@Paul-Bob Paul-Bob merged commit fb235fd into avo-hq:main Jul 27, 2024
19 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detaching a record via an association calls delete_all, skipping ActiveRecord callbacks, instead of destroy.
3 participants