Fix patch issues with group members#14
Conversation
| { | ||
| group.DisplayName = value.Value; | ||
| } | ||
| group.DisplayNamePatchOperation(operation); |
There was a problem hiding this comment.
Method names should be actions, so it's recommended that you call the methods PatchDisplayName and PatchMembers
|
|
||
| private static void DisplayNamePatchOperation(this Core2Group group, PatchOperation2 operation) | ||
| { | ||
| var value = operation.Value.SingleOrDefault(); |
There was a problem hiding this comment.
Use strongly typed named wherever possible instead of vars.
string value = operation?.Value?.SingleOrDefault();
|
|
||
| //patch is asking to remove members completely. | ||
| if (operation.Value?.FirstOrDefault()?.Value == null && | ||
| (operation.Path?.SubAttributes == null || operation.Path?.SubAttributes.Count == 0)) |
There was a problem hiding this comment.
Instead of the extensive logic here, the deserialization of the incoming request into PatchOperation2 object needs to be changed so that it can handle the singled valued removal that you mentioned in the description of this pull request. We'll make changes to handle that soon.
| { | ||
| var subAttr = operation.Path.SubAttributes.Single(); | ||
|
|
||
| if (!subAttr.AttributePath.Equals("value", StringComparison.Ordinal)) |
There was a problem hiding this comment.
No magic strings. There is a variable that should have it's value as "value" (AttributeNames.Value).
| { | ||
| public bool Equals([AllowNull] Member x, [AllowNull] Member y) | ||
| { | ||
| var xVal = x.Value ?? ""; |
There was a problem hiding this comment.
Magic constants, should instead be string.Empty
| } | ||
| } | ||
|
|
||
| private static void MemberPatchOperation(this Core2Group group, PatchOperation2 operation) |
There was a problem hiding this comment.
Methods in the repo, in general, are arranged alphabetically. It's encouraged to follow the same pattern.
|
@steverash closing this PR since the issue should be resolved by this PR - #25 Please reopen if anything is missing. |
3 participants
Fixes #10
As an alternative fix for the patch members, I've created a PR based on refactoring the existing extension method.
The problem was that patch does not currently cope with requests to remove users by path.
The common request made is in this format
Currently this will cause the code to reset the whole members to an empty array, as if you have done
The PR also refactors into a number of simpler methods to make reading the code (hopefully) a bit simpler.
It has been tested with the Postman collection and seems to work. I've also used it in our own project with a series of mocked test requests.