Skip to content
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

[SQSERVICES-253] Support provisioning role information with scim PATCH #2855

Conversation

battermann
Copy link
Contributor

@battermann battermann commented Nov 21, 2022

https://wearezeta.atlassian.net/browse/SQSERVICES-253

support for PATCH

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@battermann battermann temporarily deployed to cachix November 21, 2022 14:27 Inactive
@battermann battermann temporarily deployed to cachix November 21, 2022 14:27 Inactive
@battermann battermann temporarily deployed to cachix November 21, 2022 14:28 Inactive
@battermann battermann temporarily deployed to cachix November 21, 2022 14:28 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Nov 21, 2022
@battermann battermann requested a review from fisx November 21, 2022 14:29
@battermann battermann temporarily deployed to cachix November 21, 2022 14:44 Inactive
@battermann battermann temporarily deployed to cachix November 21, 2022 14:44 Inactive
@battermann battermann changed the title patch works (remove, add, replace) [SQSERVICES-253] Support provisioning role information with scim PATCH Nov 21, 2022
@battermann battermann temporarily deployed to cachix November 22, 2022 09:17 Inactive
@battermann battermann temporarily deployed to cachix November 22, 2022 09:17 Inactive
@battermann battermann force-pushed the SQSERVICES-253-feature-support-provisioning-role-information-with-scim-patch branch from e6512c5 to f4c4f88 Compare November 23, 2022 09:16
@battermann battermann temporarily deployed to cachix November 23, 2022 09:16 Inactive
@battermann battermann temporarily deployed to cachix November 23, 2022 09:16 Inactive
Base automatically changed from SQSERVICES-253-feature-support-provisioning-role-information-with-scim to develop November 23, 2022 15:02
@battermann battermann force-pushed the SQSERVICES-253-feature-support-provisioning-role-information-with-scim-patch branch 2 times, most recently from bc357cd to fbdaec6 Compare November 23, 2022 16:18
@battermann battermann temporarily deployed to cachix November 23, 2022 16:18 Inactive
@battermann battermann temporarily deployed to cachix November 23, 2022 16:18 Inactive
@battermann battermann temporarily deployed to cachix November 25, 2022 09:11 Inactive
@battermann battermann temporarily deployed to cachix November 25, 2022 09:11 Inactive
@battermann battermann force-pushed the SQSERVICES-253-feature-support-provisioning-role-information-with-scim-patch branch from 6984090 to 7455f79 Compare November 25, 2022 09:14
@battermann battermann temporarily deployed to cachix November 25, 2022 09:14 Inactive
@battermann battermann temporarily deployed to cachix November 25, 2022 09:14 Inactive
@battermann battermann temporarily deployed to cachix November 25, 2022 10:42 Inactive
@battermann battermann temporarily deployed to cachix November 25, 2022 10:42 Inactive
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

i have two nit-picks, but they can either be done in a future release or forgotten entirely.

libs/hscim/src/Web/Scim/Schema/User.hs Show resolved Hide resolved
Comment on lines 1985 to 1986
let testWithInitialRole r = forM_ [minBound .. maxBound] (testPatchRoles brig replaceOrAdd tid owner tok r)
forM_ [minBound .. maxBound] testWithInitialRole
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suggested a different way to write this, but this time i have another suggestion :-)

Suggested change
let testWithInitialRole r = forM_ [minBound .. maxBound] (testPatchRoles brig replaceOrAdd tid owner tok r)
forM_ [minBound .. maxBound] testWithInitialRole
let values = [[], ["member", "admin"], ["notarole"]] <> (:[]) <$> [minBound .. maxBound]
testWithInitialRole r = forM_ values (testPatchRoles brig replaceOrAdd tid owner tok r)
forM_ values testWithInitialRole

(this would require to take [Role] instead of Role in initial and target below.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this trick could also be used to simplify the tests for POST, PUT?

Copy link
Contributor Author

@battermann battermann Nov 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suggested a different way to write this, but this time i have another suggestion :-)

I know, but that didn't compile, and I tried to come as close as possible.

@battermann battermann temporarily deployed to cachix November 28, 2022 13:09 Inactive
@battermann battermann temporarily deployed to cachix November 28, 2022 13:09 Inactive
@battermann battermann merged commit 8ce351e into develop Nov 28, 2022
@battermann battermann deleted the SQSERVICES-253-feature-support-provisioning-role-information-with-scim-patch branch November 28, 2022 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants