Skip to content

Conversation

@valscion
Copy link
Member

We seem to have missed a test case for the related_records_with_context cases and so have got this bug. Credits to @nbarthel for discovering this case in #65, thanks!

Closes #65

valscion added 3 commits May 22, 2017 10:16
This makes it more clear that we're not testing the other kinds of
`relation_type` calls at all, thus leading to breakage
In has_one situation, the `data[:records]` is actually a single record
and not an array.
@valscion valscion mentioned this pull request May 22, 2017
@valscion valscion added the wip label May 22, 2017
@valscion
Copy link
Member Author

Do these changes look good to you, @gnfisher? After all, you were the one these relationship checks in #60 so I value your input on this.

Do you think the data[:records] would need a rename in the related_records_with_context as it can contain just a single record, too? I don't yet have a good idea on what to call it, if we deem records can be a bit deceiving. Do you have any ideas?

@valscion valscion requested a review from gnfisher May 22, 2017 07:43
@gnfisher
Copy link
Contributor

The changes look good. I'm trying to remember back to this and how the has_one scenario wasn't an issue that came up in my use/testing (we're using a version of this in our project at work and haven't run up against this scenario - thankfully ;)).

As for naming the variable, this morning nothing strikes me as any better than records off hand. Ideally I'd like for data[:records] to always be an Array, which I'm sure is what I thought I was doing at the time. This way, even if it's a singular has_one relationship, it is still an array, just one with a single item.

@valscion
Copy link
Member Author

Ideally I'd like for data[:records] to always be an Array, which I'm sure is what I thought I was doing at the time. This way, even if it's a singular has_one relationship, it is still an array, just one with a single item.

Yeah I thought about this as well, but then it would make some quite funky code in authorize_related_resources when replace_to_one_relationship is called. We'd have to unwrap the array and only take the first item from there, in order to not to change the API of replace_to_one_relationship to also accept an array of records. 😕

@valscion
Copy link
Member Author

Oh, and if the changes look good to you, could you approve the review? The big red "Review required" text doesn't really encourage me to bypass it 😄

screen shot 2017-05-22 at 16 14 17

@valscion valscion merged commit 730aea9 into master May 22, 2017
@valscion valscion deleted the fix-has-one-create branch May 22, 2017 13:21
@valscion valscion mentioned this pull request May 22, 2017
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

2 participants