-
Notifications
You must be signed in to change notification settings - Fork 37
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 relatioship fieldset #236
Fix relatioship fieldset #236
Conversation
* Allow for self link generation for custom routes * Fix code standard error
…maas#232) * Fixed links.last when there are no any items in response * Added a unit tests to cover scenarios when we have items in response for less than one page
Sorry for all those commits in this again... we were using this internally and we needed the changes from both PRs and I did not know how to handle this better. ~Cheers |
Any update on this? |
I'm sorry, this completely slipped past me during the holidays. I will take a look at this very soon. |
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.
Nice, simple PR 👍
The only small thingy is the asserts in the test, I think we should assert both positive (expected value is here) and negative (not expected values are not there), similar to what the attribute test does.
Tests/JsonApiSerializerTests.cs
Outdated
@@ -83,6 +83,22 @@ public void UsesQueryFieldsetExpressions() | |||
Assert.NotNull(result["data"][0]["attributes"]["location"]); | |||
Assert.Null(result["data"][0]["attributes"]["number-of-employees"]); | |||
} | |||
|
|||
[Fact(DisplayName = "Uses query fieldset expressions also on relationships if specified")] |
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 guess the "also" will lose its meaning pretty soon after this is merged 😄
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 do not quite get what you mean here, I hope I resolved the issue.
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.
You did, it wasn't really an issue just a stylistic thing 😄 if you read the test title by itself, the "also" didn't make sense, it only did in the context of the other tests. It was a very minor thing anyway.
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.
Ahh I see. Didn't get it probably because english istn't my first language :)
|
||
var relationships = result["data"][0]["relationships"]; | ||
|
||
Assert.Null(relationships["family-members"]); |
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.
Should you also assert that Job
actually is there?
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 should and I will :)
Thanks very much! |
This is the fix for #235
As mentioned there relationships should also be filtered by the sparse fieldset.