#290 fix passing relationships on create #320
Conversation
test/unit/creation_test.rb
Outdated
| } | ||
| } | ||
| } | ||
| }.to_json) |
There was a problem hiding this comment.
is this only whitespace changes, or am i missing something subtle here?
There was a problem hiding this comment.
yes, it's only whitespace. My IDE has different notation. will fix it shortly
There was a problem hiding this comment.
no worries. Just making sure i'm not missing anything : D
| @@ -18,35 +18,35 @@ def after_create_method | |||
| class Author < TestResource | |||
| end | |||
|
|
|||
| def setup | |||
| super | |||
| def stub_simple_creation | |||
There was a problem hiding this comment.
what's the thinking behind moving this out of setup? does this stub interfere with other tests?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That makes sense. I figured it was something like that, but wasn't sure. Thanks!
| } | ||
| ] | ||
| } | ||
| }, | ||
| attributes: { | ||
| title: 'Rails is Omakase' |
There was a problem hiding this comment.
no
key order in Hash is changed because it affects key order in json body
stub will not be matched without this change
| @@ -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')]}) | |||
There was a problem hiding this comment.
is this a significant behavior change? will id: 2 still work?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Should we cast .to_s as to avoid interface changes? Won't this break everyone who is upgrading from an older version?
| @@ -23,21 +23,21 @@ def after_save_method | |||
| end | |||
| end | |||
|
|
|||
| def setup | |||
| super | |||
| def stub_simple_fetch | |||
There was a problem hiding this comment.
same comment as aboev: what's the thinking behind moving this out of setup?
There was a problem hiding this comment.
same that I said in above comment - to prevent false positive tests pass in future
test/unit/updating_test.rb
Outdated
| title: "Rails is Omakase" | ||
| } | ||
| } | ||
| }.to_json) |
also fix false positive tests
senid231
force-pushed
the
290-fix-passing-relationship-on-create
branch
from
231a88b to
ce8963c
Compare
|
@gaorlov done with reverting whitespace unnecessary changes and answering your questions |
|
This looks good. My only concern is whether making |
|
it doesn't concern current PR |
|
Ohhh, i see. The int was always invalid, but we just let it, right? |
|
@gaorlov yeap |
add JsonApiClient#315 and JsonApiClient#320 to unreleased section of CHANGELOG.md
|
forgot to add changelog |
|
huzzah! thanks, @senid231 |
related issues #290
related PR #317 #319 #318