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

Add accepts_nested_attributes_for support for delegated_type #41717

Merged

Conversation

xtr3me
Copy link
Contributor

@xtr3me xtr3me commented Mar 21, 2021

Summary

This PR adds nested_attributes_for support to delegated_type, this allows a developer to create and update records easily without needing to write specific methods like:

class Entry < ApplicationRecord
  delegated_type :entryable, types: %w[ Message Comment ]

  def self.create_with_comment(content, creator: Current.user)
    create! entryable: Comment.new(content: content), creator: creator
  end
end

Using nested_attributes_for allows to execute the following:

class Entry < ApplicationRecord
  delegated_type :entryable, types: %w[ Message Comment ]
  accepts_nested_attributes_for :entryable
end

params = { entry: { entryable_type: 'Comment', entryable_attributes: { content: 'Smiling' } } }
entry = Entry.create(params[:entry])

Why

Nested forms based on accepts_nested_attributes_for are very powerful and this is the last piece missing to also be able to use it on Delegated Types.

Question

Since this is a polymorphic belongs_to relationship, what other tests would we like to have? As it is already tested in TestNestedAttributesOnABelongsToAssociation

@xtr3me xtr3me force-pushed the add_nested_attributes_support_for_delegated_types branch from 4e7082b to a9dfedb Compare March 21, 2021 20:27
@rails-bot
Copy link

rails-bot bot commented Jun 19, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jun 19, 2021
activerecord/CHANGELOG.md Outdated Show resolved Hide resolved
@rails-bot rails-bot bot removed stale labels Jun 20, 2021
@zzak
Copy link
Member

zzak commented Jun 20, 2021

@xtr3me Could you rebase too please? I think this PR could use some more tests, but I also wouldn't put more work into this without getting approval on the base of the feature from another committer 🙏

@rails-bot
Copy link

rails-bot bot commented Sep 20, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Sep 20, 2021
@xtr3me xtr3me marked this pull request as draft September 23, 2021 06:22
@rails-bot rails-bot bot removed the stale label Sep 23, 2021
@xtr3me xtr3me force-pushed the add_nested_attributes_support_for_delegated_types branch from 67dcbdf to c044e86 Compare September 23, 2021 06:37
@xtr3me
Copy link
Contributor Author

xtr3me commented Sep 23, 2021

Hi @zzak i have incorporated your suggestions and rebased with main, thank you for reviewing it 👍
I was wondering if you or anyone else for that matter could guide me in which tests are required extra?

@xtr3me xtr3me marked this pull request as ready for review September 23, 2021 07:27
@xtr3me xtr3me requested a review from zzak September 24, 2021 14:48
@zzak
Copy link
Member

zzak commented Sep 26, 2021

@xtr3me Sorry for the late reply, could you squash your commits too please?

@xtr3me xtr3me force-pushed the add_nested_attributes_support_for_delegated_types branch 3 times, most recently from f3594fc to 7af2cc8 Compare September 26, 2021 08:39
@xtr3me
Copy link
Contributor Author

xtr3me commented Sep 26, 2021

@zzak i've squashed the commits. Thank you very much for reviewing it.

@xtr3me xtr3me force-pushed the add_nested_attributes_support_for_delegated_types branch from efe47cd to f449381 Compare October 31, 2021 10:00
@xtr3me
Copy link
Contributor Author

xtr3me commented Oct 31, 2021

@zzak I have rebased the branch with main. Is there anything else I could do to advance this into merging it in main?

@zzak
Copy link
Member

zzak commented Nov 1, 2021

Thanks for your patience @xtr3me I've pretty limited capacity right now, so hopefully another review can take a look 🙏

@xtr3me
Copy link
Contributor Author

xtr3me commented Nov 2, 2021

@dhh as the original composer of the delegated_types feature could you maybe help out by reviewing this PR and guide me on which changes are needed to get this merged in?

Or do you know somebody who can/has some spare time to review this PR? 👍

Co-Authored-By: Zachary Scott <zzakscott@gmail.com>
@xtr3me xtr3me force-pushed the add_nested_attributes_support_for_delegated_types branch from f449381 to c71a6b0 Compare November 2, 2021 19:46
@dhh
Copy link
Member

dhh commented Nov 3, 2021

This is lovely. Thanks for working on it!

@dhh dhh merged commit 57fe7df into rails:main Nov 3, 2021
@xtr3me xtr3me deleted the add_nested_attributes_support_for_delegated_types branch November 3, 2021 15:18
@ps-ruby
Copy link

ps-ruby commented Nov 8, 2021

Loved it 😍

@Piioo
Copy link

Piioo commented Dec 15, 2023

How to change from Message to Comment? (with removing the Message from DB)

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

Successfully merging this pull request may close these issues.

None yet

5 participants