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

Modify has_many field show #522

Closed

Conversation

jasontwong
Copy link

Resolves #440

Why:

  • The template is rendering the incorrect partial.

This change addresses the need by:

  • Create field helper collection_partial(namespace).
  • Update has_many/_show.html.erb to use new helper.

Why:

* The template is rendering the incorrect partial.

This change addresses the need by:

* Create field helper `collection_partial`.
Why:

* Tests are failing are changes are not working.

This change addresses the need by:

* The new helper is an instance method on field.
@jasontwong
Copy link
Author

Can anyone rerun the tests on CircleCI? It looks like a non-code issue cause it to fail.

@wjlroe
Copy link

wjlroe commented Apr 7, 2016

I've just hit this bug, is there anything that needs to be done for it to be merged?

@hughfm
Copy link
Contributor

hughfm commented May 16, 2016

Has there been a decision on whether this is the right way to fix this? I'd love to see a solution asap, and would be happy to help work towards one if there are any concerns about what has been proposed here.

@hughfm
Copy link
Contributor

hughfm commented May 19, 2016

@jasontwong thanks for your fix here, I've been using it in my application and all good so far! One thought though...

say for example you have a User class that has_many :posts, and we are rendering a user's posts on their show page... if we have overridden the posts/collection partial, this will now be used correctly 👍

if that partial has not been overridden however, it still defaults to the users/collection partial, if that exists. I'm thinking it may make more sense to default straight back to application/collection. What do you think?

@jasontwong
Copy link
Author

@hughfm I'm glad it's been working well for you. The only reason why I left it to use the users/collection template was to make the default functionality relate as close to the original code as possible. This would make it easier for those already familiar with the usage to add in this override. I agree that the best way would probably be to use application/collection but I would rather leave it to the administrate team to decide what the default functionality should be.

@hughfm
Copy link
Contributor

hughfm commented May 23, 2016

Yep that's fair enough @jasontwong, would love to hear thoughts on this from the team..

woongy added a commit to woongy/routinely that referenced this pull request Aug 28, 2016
Move link to group history inside show partial,
since administate renders incorrect partial if collection is overriden
(see thoughtbot/administrate#522)
@allthesignals
Copy link

allthesignals commented Oct 13, 2016

If you're trying to fix this bug in Rails 5, I've merged both the rails5 compatibility branch and this branch here: https://github.com/MAPC/administrate

Include in your Gemfile like this:
gem 'administrate', github: 'MAPC/administrate', branch: 'rails5'

In order to get the fix applied, you do have to generate the related model's partial with rails g administrate:views:index. Thanks for your work, @jasontwong!

@carlosramireziii
Copy link
Collaborator

@nickcharlton There's a question here about whether or not this is the right approach.

@nickcharlton
Copy link
Member

Hi! I'm just getting around to looking at this now.

I think this is a reasonable approach to the problem. Are we able to test this in both scenarios (using a default collection template, using a custom one)?

@jasontwong
Copy link
Author

It's been a while since I looked at this... I'll dig into the testing.

@nickcharlton
Copy link
Member

Hi @jasontwong! Did you get anywhere with looking at the testing?

@josephchoe
Copy link

Has there been any progress on this?

@nickcharlton
Copy link
Member

@jasontwong Did you get a chance to look at this again?

@pablobm
Copy link
Collaborator

pablobm commented Oct 30, 2019

Closing as there has been no activity for a while. Happy to reopen if someone wants to take over.

@pablobm pablobm closed this Oct 30, 2019
nhippenmeyer added a commit to rinsed-org/administrate that referenced this pull request Apr 27, 2020
nhippenmeyer added a commit to rinsed-org/administrate that referenced this pull request Apr 27, 2020
jordan-brough pushed a commit to rinsed-org/administrate that referenced this pull request Dec 9, 2021
jordan-brough added a commit to rinsed-org/administrate that referenced this pull request Jan 18, 2022
* Remove jquery ujs (#1)

* Update HasMany show collection partial path (#2)

Built on thoughtbot#522

* Enable selectize for select and audience fields (#3 and #5)

* Enable selectize for voucher types field (#6)

* Use and test Ruby 3.0

Co-authored-by: Nic Hippenmeyer <nic@rinsed.co>
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.

Has many field renders (potentially) wrong partial
8 participants