Skip to content

Remove clear_changes_information in relations #317

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

Closed
wants to merge 1 commit into from
Closed

Remove clear_changes_information in relations #317

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
139 changes: 70 additions & 69 deletions test/unit/creation_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,62 +18,80 @@ def after_create_method
class Author < TestResource
end

def setup
super
stub_request(:post, "http://example.com/articles")
.with(headers: {content_type: "application/vnd.api+json", accept: "application/vnd.api+json"}, body: {
data: {
type: "articles",
attributes: {
title: "Rails is Omakase"
}
}
}.to_json)
.to_return(headers: {content_type: "application/vnd.api+json"}, body: {
data: {
type: "articles",
id: "1",
attributes: {
describe 'default stub' do
def setup
stub_request(:post, "http://example.com/articles")
.with(headers: {content_type: "application/vnd.api+json", accept: "application/vnd.api+json"}, body: {
data: {
type: "articles",
attributes: {
title: "Rails is Omakase"
},
links: {
self: "http://example.com/articles/1",
other: {
href: "http://example.com/other"
}
}
}
}.to_json)
end
}.to_json)
.to_return(headers: {content_type: "application/vnd.api+json"}, body: {
data: {
type: "articles",
id: "1",
attributes: {
title: "Rails is Omakase"
},
links: {
self: "http://example.com/articles/1",
other: {
href: "http://example.com/other"
}
}
}
}.to_json)
end

def test_can_create_with_class_method
article = Article.create({
title: "Rails is Omakase"
})
def test_can_create_with_class_method
article = Article.create({
title: "Rails is Omakase"
})

assert article.persisted?, article.inspect
assert_equal "1", article.id
assert_equal "Rails is Omakase", article.title
end
assert article.persisted?, article.inspect
assert_equal "1", article.id
assert_equal "Rails is Omakase", article.title
end

def test_changed_attributes_empty_after_create_with_class_method
article = Article.create({
title: "Rails is Omakase"
})
def test_changed_attributes_empty_after_create_with_class_method
article = Article.create({
title: "Rails is Omakase"
})

assert_empty article.changed_attributes
end
assert_empty article.changed_attributes
end

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

assert article.new_record?
assert article.save
assert article.persisted?
assert_equal(false, article.new_record?)
assert_equal "1", article.id
assert article.new_record?
assert article.save
assert article.persisted?
assert_equal(false, article.new_record?)
assert_equal "1", article.id
end

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

assert article.save
assert article.persisted?
assert_equal "http://example.com/articles/1", article.links.self
end

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

article.save
assert_empty article.changed_attributes
end
end

def test_can_create_with_includes_and_fields
Expand Down Expand Up @@ -138,16 +156,6 @@ def test_can_create_with_includes_and_fields
assert_equal "it is isn't it ?", article.comments.first.body
end

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

assert article.save
assert article.persisted?
assert_equal "http://example.com/articles/1", article.links.self
end

def test_can_create_with_new_record_with_relationships_and_save
stub_request(:post, "http://example.com/articles")
.with(headers: {content_type: "application/vnd.api+json", accept: "application/vnd.api+json"}, body: {
Expand Down Expand Up @@ -233,13 +241,6 @@ def test_correct_create_with_nil_attribute_value
assert author.save
end

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

article.save
assert_empty article.changed_attributes
end

def test_callbacks_on_update
stub_request(:post, "http://example.com/callback_tests")
.with(headers: {
Expand Down Expand Up @@ -278,18 +279,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'
}
}
}.to_json)
Expand Down
8 changes: 8 additions & 0 deletions test/unit/serializing_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,14 @@ def test_update_data_only_includes_relationship_data
expected = {
"type" => "articles",
"id" => "1",
"relationships"=> {
"author"=> {
"data"=>{
"type"=>"people",
"id"=>"9"
}
}
},
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that in this case relationships should be serialized
#as_json_api is used for gathering a payload that will be sent to server
and we should not add to payload relationships that weren't changed by user
example:

post = Post.includes(:author).find(1)
post.title = 'changed title'
post.save # author relationship should not be present in payload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can agree with that, this part was more of a side effect of removing the clear_changes_information, if there is another solution that allows relationships to be passed on create and does not change this I am all for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, #318 does not address the issues with creation

Copy link
Member

Choose a reason for hiding this comment

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

@fzf can you please provide some code snippet that explains issue with creation that you have encountered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Relationship data is never actually sent, it is cleared before it can be sent. Here is a spec showing that it is a false positive. #319

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#319 had a naming issue you can see the failure now @senid231

"attributes" => {}
}
assert_equal expected, article.as_json_api
Expand Down