Skip to content

Conversation

fisx
Copy link
Contributor

@fisx fisx commented Aug 10, 2023

https://wearezeta.atlassian.net/browse/WPB-2959

reviewer: please read commit by commit.

without the fix, but with the test:

$ make ci-safe package=spar
[...]
  test-integration/Test/Spar/Scim/UserSpec.hs:1810:7:
  1) WireIdPAPIV1.Spar.Scim.User, PUT /Users/:id, updates role
       expected: Just RoleOwner
        but got: Just RoleMember

  To rerun use: --match "/WireIdPAPIV1/Spar.Scim.User/PUT /Users/:id/updates role/"

  test-integration/Test/Spar/Scim/UserSpec.hs:1810:7:
  2) WireIdPAPIV2.Spar.Scim.User, PUT /Users/:id, updates role
       expected: Just RoleOwner
        but got: Just RoleMember

  To rerun use: --match "/WireIdPAPIV2/Spar.Scim.User/PUT /Users/:id/updates role/"
[...]

with both:

$ make ci-safe package=spar
[...]
WireIdPAPIV1
  Spar.Scim.User
    PUT /Users/:id
      updates role [✔]
[...]

Checklist

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

fisx added 3 commits August 9, 2023 17:40
More specifically: if role is not set ([], null, or field missing), do
not change role to default.
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 10, 2023
@fisx
Copy link
Contributor Author

fisx commented Aug 10, 2023

a few more spar tests are failing, but i think that's because the tests need fixing.

@fisx fisx marked this pull request as ready for review August 10, 2023 15:21
@fisx fisx requested a review from battermann August 10, 2023 15:25
Copy link
Contributor

@battermann battermann left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1780,13 +1780,14 @@ testUpdateUserRole = do
let galley = env ^. teGalley
(owner, tid) <- call $ createUserWithTeam brig galley
tok <- registerScimToken tid Nothing
let mTargetRoles = Nothing : map Just [minBound ..]
let mRoles = Nothing : map Just [minBound @Role ..]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this type annotation is not needed, is it a left over or is it meant for documenting the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

both! :)

@fisx fisx merged commit 369ce53 into develop Aug 11, 2023
@fisx fisx deleted the WPB-2959-fix-roles-in-scim branch August 11, 2023 08:33
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