bdemers commented on code in PR #448:
URL: https://github.com/apache/directory-scimple/pull/448#discussion_r1424365505
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java:
##########
@@ -465,6 +467,39 @@ public void deleteItemWithComplexFilter() throws
FilterParseException {
));
}
+ @Test
+ public void addItemToCollection() throws FilterParseException {
+ PatchOperation op = new PatchOperation();
+ op.setOperation(ADD);
+ op.setPath(PatchOperationPath.fromString("members"));
+ op.setValue(List.of(
+ Map.of(
+ "value", "9876",
+ "display", "testUser2",
+ "type", "User")
+ ));
+
+ ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op));
+ assertThat(updatedGroup.getMembers().size()).isEqualTo(3);
+ }
+
+ /**
+ * This test covers Azure-style remove member operations, where a value is
+ * specified in the operation body, rather than in the path.
+ * For example: { "op": "remove", "path": "members", "value": [ { "value":
"1234" } ] }
+ */
+ @Test
+ public void removeItemFromCollection() throws FilterParseException {
+ PatchOperation op = new PatchOperation();
+ op.setOperation(REMOVE);
+ op.setPath(PatchOperationPath.fromString("members"));
+ op.setValue(List.of(Map.of("value", "1234")));
+
+ ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op));
+ assertThat(updatedGroup.getMembers()).isNotNull();
+ assertThat(updatedGroup.getMembers().size()).isEqualTo(1);
Review Comment:
As an aside, (and not part of this PR, selfishly asking for #435), do you
think creating custom AssertJ assertions would be helpful? or do you think it's
more readable to have multiple assertions?
Maybe something, that would read like:
```java
assertThat(updatedGroup)
.hasMemberCount(3)
.containsMember(<expected>)
```
Mostly thinking out loud, we have a util method in PatchGeneratorTest, but
I've been thinking the messages would be more clear with custom assertions.
https://github.com/apache/directory-scimple/blob/ebb01f4d022fda36f9d28315ad0ac1d9427f9ac2/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchGeneratorTest.java#L754-L762
(I've been using Hamcrest for years, so its taken longer than it should have
to get comfortable with AssertJ 🤣 )
##########
scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchHandlerTest.java:
##########
@@ -465,6 +467,39 @@ public void deleteItemWithComplexFilter() throws
FilterParseException {
));
}
+ @Test
+ public void addItemToCollection() throws FilterParseException {
+ PatchOperation op = new PatchOperation();
+ op.setOperation(ADD);
+ op.setPath(PatchOperationPath.fromString("members"));
+ op.setValue(List.of(
+ Map.of(
+ "value", "9876",
+ "display", "testUser2",
+ "type", "User")
+ ));
+
+ ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op));
+ assertThat(updatedGroup.getMembers().size()).isEqualTo(3);
+ }
+
+ /**
+ * This test covers Azure-style remove member operations, where a value is
+ * specified in the operation body, rather than in the path.
+ * For example: { "op": "remove", "path": "members", "value": [ { "value":
"1234" } ] }
+ */
+ @Test
+ public void removeItemFromCollection() throws FilterParseException {
+ PatchOperation op = new PatchOperation();
+ op.setOperation(REMOVE);
+ op.setPath(PatchOperationPath.fromString("members"));
+ op.setValue(List.of(Map.of("value", "1234")));
+
+ ScimGroup updatedGroup = patchHandler.apply(group(), List.of(op));
+ assertThat(updatedGroup.getMembers()).isNotNull();
+ assertThat(updatedGroup.getMembers().size()).isEqualTo(1);
Review Comment:
As an aside, (and not part of this PR, selfishly asking for #435), do you
think creating custom AssertJ assertions would be helpful? or do you think it's
more readable to have multiple assertions?
Maybe something, that would read like:
```java
assertThat(updatedGroup)
.hasMemberCount(3)
.containsMember(<expected>)
```
Mostly thinking out loud, we have a util method in PatchGeneratorTest, but
I've been thinking the messages would be more clear with custom assertions.
https://github.com/apache/directory-scimple/blob/ebb01f4d022fda36f9d28315ad0ac1d9427f9ac2/scim-core/src/test/java/org/apache/directory/scim/core/repository/PatchGeneratorTest.java#L754-L762
(I've been using Hamcrest for years, so its taken longer than it should have
to get comfortable with AssertJ 🤣 )
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@directory.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@directory.apache.org
For additional commands, e-mail: dev-h...@directory.apache.org
