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

Fix namespace lookup for collections and has_many #1973

Merged
merged 1 commit into from
Nov 15, 2016

Conversation

groyoh
Copy link
Member

@groyoh groyoh commented Nov 15, 2016

Purpose

Due to the CollectionSerializer not passing the namespace option to the lookup, the namespace lookup doesn't work for collections and has_many relationships. This PR fixes it.

Changes

Pass the namespace option to the serializer lookup from the CollectionSerializer.

Related GitHub issues

#1757 (comment)

@groyoh groyoh added the S: Bug label Nov 15, 2016
@mention-bot
Copy link

@groyoh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @NullVoxPopuli and @bf4 to be potential reviewers.

@groyoh
Copy link
Member Author

groyoh commented Nov 15, 2016

@NullVoxPopuli FYI

expected = {
'title' => 'New Post',
'body' => 'Body', 'writer' => nil,
"chapters"=>[
Copy link
Contributor

Choose a reason for hiding this comment

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

some formatting here. Did you run rubocop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Turned out I ran it without git add'ing it :)

@NullVoxPopuli
Copy link
Contributor

Just the formatting issue, and changelog entry needed. :-) then we can merge

@groyoh
Copy link
Member Author

groyoh commented Nov 15, 2016

Sure thing!

@groyoh groyoh force-pushed the fix_namespace_collection branch from f7db7e2 to 47d1931 Compare November 15, 2016 13:07
@groyoh groyoh force-pushed the fix_namespace_collection branch from 47d1931 to 7bfacf8 Compare November 15, 2016 13:08

class ChapterSerializer < ActiveModel::Serializer
attribute :title do
"Chapter - #{object.title}"
Copy link
Member Author

Choose a reason for hiding this comment

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

When AMS doesn't find the serializer it uses resource.as_json. So I added this "Chapter -" part, to make sure that we use the namespaced serializer instead of resource.as_json.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that -- good thinkin'

@groyoh
Copy link
Member Author

groyoh commented Nov 15, 2016

@NullVoxPopuli fixed formatting and added CHANGELOG.

@NullVoxPopuli NullVoxPopuli merged commit d0de53c into rails-api:master Nov 15, 2016
GregPK pushed a commit to GregPK/active_model_serializers that referenced this pull request Apr 25, 2017
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.

3 participants