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

Use new_resource in new action #1097

Merged
merged 1 commit into from
Mar 26, 2020

Conversation

EugeneGrigorenko
Copy link
Contributor

@EugeneGrigorenko EugeneGrigorenko commented Feb 15, 2018

P.S. Though, it works as expected on has_one association, when trying to build has_many association it doesn't work. The reason is that select is used on has_many form and only ids get send with the request. And because the record doesn't have an id it gets lost. I'm not sure if it's a bug and should be addressed or not, please let me know what you think.

@nickcharlton
Copy link
Member

Hi @EugeneGrigorenko!

Thanks for this! It sounds like we should make sure it still works with a has_many in this PR.

Do you also think you'd be able to add some tests for the feature this gives us, too?

@EugeneGrigorenko
Copy link
Contributor Author

hi @nickcharlton, sorry for delay
i've investigated a bit and I can say, that has_many works with ids. Like e.g. overriding new_resource like this would initialize customer form with first and last orders selected.

      resource_class.new.tap do |resource|
        resource.order_ids = [::Order.first.id, ::Order.last.id]
      end

To supported creation of has_many via nested forms, I believe, we need to change https://github.com/nickcharlton/administrate-field-nested_has_many

As for tests, I'm not sure what I can test exactly, as, basically that's a one line refactoring so far. It's supposed to being overriden and then it gives new functionality

@nickcharlton
Copy link
Member

Okay, cool!

Could you try and confirm if we do indeed to change administrate-field-nested_has_many? If we do, I'll want to coordinate the releases.

@pablobm
Copy link
Collaborator

pablobm commented Jan 16, 2020

On one hand, this PR looks like a slam dunk to me (provided there's a rebase first). I can't see how it would interfere with anything, and I can see how it's beneficial to have this API.

On the other hand, I'm looking into administrate-field-nested_has_many and I'm having issues making it work at all (unrelated to this PR). Specifically, I'm getting an error when trying to create a School resource in the dummy app... I'm not feeling quite like looking into that one now 😓

@nickcharlton, do you have any thoughts?

- Remove code duplication
- Simplify making new form with prefilled associations
@nickcharlton nickcharlton merged commit 8af6ce8 into thoughtbot:master Mar 26, 2020
@nickcharlton
Copy link
Member

I rebased and then merged. We'll worry about nested has many when it's a problem (assuming it is!).

@sedubois
Copy link
Contributor

Is this new_resource method documented somewhere?

@nickcharlton
Copy link
Member

I don't believe so, but I might be wrong. Would you be able to do that?

If it doesn't fit anywhere as is, we might want to create an "Administrate Internals" documentation page.

@sedubois
Copy link
Contributor

sedubois commented Apr 4, 2020

Sorry @nickcharlton I'm not familiar with this method, maybe someone else knows a bit more?

@nickcharlton
Copy link
Member

I don't either! I'll open a new issue to keep track of it.

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

Successfully merging this pull request may close these issues.

4 participants