Skip to content

#290 fix passing relationships on create #320

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 13, 2018
Merged

#290 fix passing relationships on create #320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion lib/json_api_client/relationships/relations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ class Relations
def initialize(record_class, relations)
@record_class = record_class
self.attributes = relations
clear_changes_information
end

def present?
Expand Down
1 change: 1 addition & 0 deletions lib/json_api_client/resource.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ def load(params)
new(params).tap do |resource|
resource.mark_as_persisted!
resource.clear_changes_information
resource.relationships.clear_changes_information
end
end

Expand Down
20 changes: 12 additions & 8 deletions test/unit/creation_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,7 @@ def after_create_method
class Author < TestResource
end

def setup
super
def stub_simple_creation
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the thinking behind moving this out of setup? does this stub interfere with other tests?

Copy link
Member Author

@senid231 senid231 Nov 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

when this code were in setup in cause false positive test #test_create_with_relationships_in_payload
because in this test create request has different payload (and different stub) but it pass because stub from #setup aws matched instead of stub written in test itself.

I move stub into separate method and use it only in tests that relies on it. it will save us from future false positive tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That makes sense. I figured it was something like that, but wasn't sure. Thanks!

stub_request(:post, "http://example.com/articles")
.with(headers: {content_type: "application/vnd.api+json", accept: "application/vnd.api+json"}, body: {
data: {
Expand Down Expand Up @@ -47,6 +46,7 @@ def setup
end

def test_can_create_with_class_method
stub_simple_creation
article = Article.create({
title: "Rails is Omakase"
})
Expand All @@ -57,6 +57,7 @@ def test_can_create_with_class_method
end

def test_changed_attributes_empty_after_create_with_class_method
stub_simple_creation
article = Article.create({
title: "Rails is Omakase"
})
Expand All @@ -65,6 +66,7 @@ def test_changed_attributes_empty_after_create_with_class_method
end

def test_can_create_with_new_record_and_save
stub_simple_creation
article = Article.new({
title: "Rails is Omakase"
})
Expand Down Expand Up @@ -139,6 +141,7 @@ def test_can_create_with_includes_and_fields
end

def test_can_create_with_links
stub_simple_creation
article = Article.new({
title: "Rails is Omakase"
})
Expand Down Expand Up @@ -234,6 +237,7 @@ def test_correct_create_with_nil_attribute_value
end

def test_changed_attributes_empty_after_create_with_new_record_and_save
stub_simple_creation
article = Article.new({title: "Rails is Omakase"})

article.save
Expand Down Expand Up @@ -278,18 +282,18 @@ def test_create_with_relationships_in_payload
.with(headers: {content_type: 'application/vnd.api+json', accept: 'application/vnd.api+json'}, body: {
data: {
type: 'articles',
attributes: {
title: 'Rails is Omakase'
},
relationships: {
comments: {
data: [
{
id: '2',
type: 'comments'
type: 'comments',
id: '2'
}
]
}
},
attributes: {
title: 'Rails is Omakase'
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this whitespace only?

Copy link
Member Author

Choose a reason for hiding this comment

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

no
key order in Hash is changed because it affects key order in json body
stub will not be matched without this change

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah. 👍

}
}
}.to_json)
Expand All @@ -303,7 +307,7 @@ def test_create_with_relationships_in_payload
}
}.to_json)

article = Article.new(title: 'Rails is Omakase', relationships: {comments: [Comment.new(id: 2)]})
article = Article.new(title: 'Rails is Omakase', relationships: {comments: [Comment.new(id: '2')]})
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this a significant behavior change? will id: 2 still work?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not significant behavior change because this test never actually work without bug that I'm trying to fix.
current behavior it that if you add id as integer to relationship it will be added to payload as is.

I think we should convert all ids into strings because JSONAPI spec says that id MUST be a string
but it's an idea of different PR. this PR fixes another problem

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we cast .to_s as to avoid interface changes? Won't this break everyone who is upgrading from an older version?


assert article.save
assert article.persisted?
Expand Down
15 changes: 13 additions & 2 deletions test/unit/updating_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ def after_save_method
end
end

def setup
super
def stub_simple_fetch
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment as aboev: what's the thinking behind moving this out of setup?

Copy link
Member Author

Choose a reason for hiding this comment

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

same that I said in above comment - to prevent false positive tests pass in future

stub_request(:get, "http://example.com/articles/1")
.to_return(headers: {content_type: "application/vnd.api+json"}, body: {
data: {
Expand All @@ -38,6 +37,7 @@ def setup
end

def test_can_update_found_record
stub_simple_fetch
articles = Article.find(1)
article = articles.first

Expand Down Expand Up @@ -69,6 +69,7 @@ def test_can_update_found_record
end

def test_changed_attributes_blank_after_update
stub_simple_fetch
articles = Article.find(1)
article = articles.first

Expand Down Expand Up @@ -109,6 +110,7 @@ def test_changed_attributes_blank_after_update
end

def test_can_update_found_record_in_bulk
stub_simple_fetch
articles = Article.find(1)
article = articles.first

Expand Down Expand Up @@ -141,6 +143,7 @@ def test_can_update_found_record_in_bulk
end

def test_can_update_found_record_in_builk_using_update_method
stub_simple_fetch
articles = Article.find(1)
article = articles.first

Expand Down Expand Up @@ -173,6 +176,7 @@ def test_can_update_found_record_in_builk_using_update_method
end

def test_can_update_single_relationship
stub_simple_fetch
articles = Article.find(1)
article = articles.first

Expand Down Expand Up @@ -216,6 +220,7 @@ def test_can_update_single_relationship
end

def test_can_update_single_relationship_via_setter
stub_simple_fetch
articles = Article.find(1)
article = articles.first

Expand Down Expand Up @@ -259,6 +264,7 @@ def test_can_update_single_relationship_via_setter
end

def test_can_update_single_relationship_with_all_attributes_dirty
stub_simple_fetch
articles = Article.find(1)
article = articles.first

Expand Down Expand Up @@ -311,6 +317,7 @@ def test_can_update_single_relationship_with_all_attributes_dirty
end

def test_can_update_has_many_relationships
stub_simple_fetch
articles = Article.find(1)
article = articles.first

Expand Down Expand Up @@ -360,6 +367,7 @@ def test_can_update_has_many_relationships
end

def test_can_update_has_many_relationships_via_setter
stub_simple_fetch
articles = Article.find(1)
article = articles.first

Expand Down Expand Up @@ -409,6 +417,7 @@ def test_can_update_has_many_relationships_via_setter
end

def test_can_update_has_many_relationships_with_all_attributes_dirty
stub_simple_fetch
articles = Article.find(1)
article = articles.first

Expand Down Expand Up @@ -757,6 +766,7 @@ def test_callbacks_on_update
end

def test_can_update_with_includes_and_fields
stub_simple_fetch
stub_request(:patch, "http://example.com/articles/1")
.with(
headers: {content_type: "application/vnd.api+json", accept: "application/vnd.api+json"},
Expand Down Expand Up @@ -844,6 +854,7 @@ def test_can_update_with_includes_and_fields
end

def test_can_update_with_includes_and_fields_with_keep_request_params
stub_simple_fetch
stub_request(:patch, "http://example.com/articles/1")
.with(
headers: {content_type: "application/vnd.api+json", accept: "application/vnd.api+json"},
Expand Down