[ProvisioningAPI] Allow specifying group display name during creation #27089
Conversation
|
@kesselb The failed check (continuous-integration/drone/pr) seems to be unrelated to this commit. Should I do something to continue the PR? Note that my first commit run this check successful and the second commit only fixed the indentation. |
kesselb
added
3. to review
Thanks for your pull request 👍 Please ignore the ci failure. |
| } | ||
| $this->groupManager->createGroup($groupid); | ||
| $group = $this->groupManager->createGroup($groupid); | ||
| if ($displayname !== '') { |
There was a problem hiding this comment.
for null, the parameter list needs to be adjusted. but it might be that null is passed when displayname was not provided by the client - at least i believe to remember, but didn't doublecheck. Integration tests pass, should be ok
There was a problem hiding this comment.
I'm just repeating what psalm is saying 😉 🦜
There was a problem hiding this comment.
@skjnldsv I added a null check and I throw an exception if it is the case. However, this results in a failure of the nodb test (see continuous integration). Shouldn't I throw an exception in this case or is the failing nodb test wrong? It seems to me appropriate that an error message is returned in case the group creation failed.
|
I would prefer having integration tests here, extending on https://github.com/nextcloud/server/blob/master/build/integration/features/provisioning-v1.feature#L199-L217 |
@blizzz I added some test: see my last commit. Do I need to add other test cases? |
|
CI is unhappy |
I know, a nodb test is failing, because I throw now an exception in case the group creation failed. This seems appropriate to me to report the failure to the api caller. See my remark to @skjnldsv who requested a null check. What solution do you suggest? Not throwing or changing the failing nodb test? |
Signed-off-by: Dries Mys <[email protected]>
Signed-off-by: Dries Mys <[email protected]>
Signed-off-by: Dries Mys <[email protected]>
Signed-off-by: Dries Mys <[email protected]>
situation. Returned group object is now used to set the displayname if provided. Signed-off-by: Dries Mys <[email protected]>
m7913d
force-pushed
the
feature/addGroupDisplayNames
branch
from
6490308 to
e16682a
Compare
|
@blizzz @skjnldsv I corrected the failed test as the test environment did not return a valid group. The acceptance-user test is still failing, but I assume this is unrelated to this PR as after rebasing this feature branch to the latest master branch, another scenario of this test failed with the following reason: "The user disabledUser in the list of users is not shown yet after 100 seconds". |
|
I saw it failing somewhere else, too, let me restart nevertheless |
|
@blizzz Thanks for rerunning the failed check. Now all checks have passed/ |
|
Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22 |
Pytal
added
4. to release
5 participants
Signed-off-by: Dries Mys [email protected]