-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 for #3955 #3956
Fix for #3955 #3956
Conversation
|
||
if (attrs) { | ||
for (key in attrs) { | ||
var normalizedKey = null; |
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.
I think we could leave the declaration in top and just set normalizedKey = payloadKey
in the for
. That way we don't have to do the extra check on line 736.
Would be great if you could add a test for this. |
Hey @jpaas. I'd love to get this bug fixed and this pr merged. Let me know if you need any direction with adding the test. |
@bmac Sorry I haven't had a chance to look at this yet, but I will try this weekend. I don't know anything about the tests for this project, so if I have any questions I'll let you know. |
@jpaas LGTM, can you squash to one commit and prefix the commit message with |
|
||
var post = env.store.serializerFor("post").normalizeResponse(env.store, Post, jsonHash, '1', 'findRecord'); | ||
|
||
assert.equal(post.data.attributes.title, "Rails is omakase"); |
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.
I'm just curious, but according to the test description, it looks like there should be an assertion which checks that notInMapping
is not serialized, right?
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.
Ya I suppose there should be an assert that checks it isn't added, but the primary problem this PR is trying to fix is that the value of notInMapping gets assigned to the title key.
LGTM |
@jpaas Do you have time to squash this pr. Let me know if you need help with the git commands to do the squash. |
@bmac Yes please, I could use some help on what you're looking for me to do here. |
The easiest way to squash this down would be to use the interactive rebase command. You can start it using the following command which tells git you would like to edit the last 4 commits on this branch.
After you run that git should open up you editor with the following text:
We want to tell git to keep the first commit and "fixup" the 3 commits after that. "fixup" merges the commits with the previous commit and discards the commit message. We can do that by changing the word Edit the file to look like this and save and close your editor.
After closing the editor git will merge the last 3 commits with the first commit. You can now force push your I hope this helps. Let me know if you run into any trouble. |
@bmac Thanks for the git masterclass :) Hopefully I did it right. It now looks like there are some conflicts. Not sure if that's something I need to worry about or whoever is merging the PR. |
Ref #2955