-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Adds tests and documentation for polymorphism #1420
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
require 'test_helper' | ||
|
||
module ActiveModel | ||
class Serializer | ||
module Adapter | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO we should split this into different files under the adapters namespace for that sake of organization There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the feedback @joaomdmoura. My first implementation was split out like you suggested, but I followed @bf4's suggestion to combine these, which has a benefit of being able to compare the payloads side by side. If you'd like, I'll revert the changes. Let me know the final verdict. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. np, you can keep it as it is, it a non blocking comment for sure, I can talk with @bf4 later to know if there any other reason to keep it this way, but it's just a minor detail There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I hope to work with @remear to do some of these in cucumber so they're easier to compare, in general. I suggested one file since yhe differences are subtle '. I'd be for separate files if we were more consistent in test file organization B mobile phone
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the tests are currently a bloody mess. We really need to do something about it. |
||
class PolymorphicTest < ActiveSupport::TestCase | ||
setup do | ||
@employee = Employee.new(id: 42, name: 'Zoop Zoopler', email: 'zoop@example.com') | ||
@picture = @employee.pictures.new(id: 1, title: 'headshot-1.jpg') | ||
@picture.imageable = @employee | ||
|
||
@attributes_serialization = serializable(@picture, serializer: PolymorphicBelongsToSerializer) # uses default adapter: attributes | ||
@json_serialization = serializable(@picture, adapter: :json, serializer: PolymorphicBelongsToSerializer) | ||
@json_api_serialization = serializable(@picture, adapter: :json_api, serializer: PolymorphicBelongsToSerializer) | ||
end | ||
|
||
def test_attributes_serialization | ||
expected = | ||
{ | ||
id: 1, | ||
title: 'headshot-1.jpg', | ||
imageable: { | ||
id: 42, | ||
name: 'Zoop Zoopler' | ||
} | ||
} | ||
|
||
assert_equal(expected, @attributes_serialization.as_json) | ||
end | ||
|
||
def test_json_serializer | ||
expected = | ||
{ | ||
picture: { | ||
id: 1, | ||
title: 'headshot-1.jpg', | ||
imageable: { | ||
id: 42, | ||
name: 'Zoop Zoopler' | ||
} | ||
} | ||
} | ||
|
||
assert_equal(expected, @json_serialization.as_json) | ||
end | ||
|
||
def test_json_api_serializer | ||
expected = | ||
{ | ||
data: { | ||
id: '1', | ||
type: 'pictures', | ||
attributes: { | ||
title: 'headshot-1.jpg' | ||
}, | ||
relationships: { | ||
imageable: { | ||
data: { | ||
id: '42', | ||
type: 'employees' | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
||
assert_equal(expected, @json_api_serialization.as_json) | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,6 +72,14 @@ def cache_key | |
end | ||
end | ||
|
||
class Employee < ActiveRecord::Base | ||
has_many :pictures, as: :imageable | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe seializer name can describe what it's for testing? Polymorhipichasmany polymorphic belongsto |
||
|
||
class Picture < ActiveRecord::Base | ||
belongs_to :imageable, polymorphic: true | ||
end | ||
|
||
module Spam; end | ||
Spam::UnrelatedLink = Class.new(Model) | ||
|
||
|
@@ -233,6 +241,16 @@ def maker | |
end | ||
end | ||
|
||
PolymorphicHasManySerializer = Class.new(ActiveModel::Serializer) do | ||
attributes :id, :name | ||
end | ||
|
||
PolymorphicBelongsToSerializer = Class.new(ActiveModel::Serializer) do | ||
attributes :id, :title | ||
|
||
has_one :imageable, serializer: PolymorphicHasManySerializer | ||
end | ||
|
||
Spam::UnrelatedLinkSerializer = Class.new(ActiveModel::Serializer) do | ||
cache only: [:id] | ||
attributes :id | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we merge #1453, this will have to change (to mention the
polymorphic
option).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. Will wait for the word then and update docs/tests if necessary.