Spar tests #539
Spar tests #539
Conversation
test is there, now we just need to make it pass... :-) @neongreen any comments what I'm doing wrong? |
|
please review commit-by-commit. |
|
(@neongreen you can review this already as is, but there is more coming up.) |
|
Merged (and deleted) branch |
I was lying. Pushed a 'Fixup' commit, now all spar tests pass at least locally. (Let's see if the CI is back.) |
services/spar/src/Spar/SCIM.hs
Outdated
| @@ -309,16 +308,15 @@ instance SCIM.UserDB Spar where | |||
| SCIM.serverError "The IdP corresponding to the provisioning token \ | |||
| \was not found" | |||
| Just idpConfig -> pure (idpConfig ^. SAML.idpMetadata . SAML.edIssuer) | |||
| let uref = SAML.UserRef issuer (SAML.opaqueNameID extId) | |||
| let uref = traceShowId $ SAML.UserRef issuer (SAML.opaqueNameID extId) | |||
There was a problem hiding this comment.
Oops. My process always was to remove the import Debug.Trace, and then let the compiler lead me to the places to wipe up. I guess that doesn't work any more unless I tweak Imports a little, and then you'll be sad. I'll try to adapt to Imports for a while I guess.
There was a problem hiding this comment.
A solution I've seen in some other prelude is adding {-# DEPRECATED #-} to traceShow and friends so that it would compile but emit a warning. Would that be better for your workflow?
There was a problem hiding this comment.
Yes, i like that idea. I'll make another PR at some point, thanks!
| import Control.Lens | ||
| import Data.Id | ||
| import Data.String.Conversions | ||
| import Data.UUID as UUID | ||
| import Data.UUID.V4 as UUID | ||
| import Imports |
There was a problem hiding this comment.
Eh, it's at the beginning everywhere else (on purpose). I'm not against sorted imports everywhere, but I think inconsistencies are bad because they encourage sloppiness in general.
(And yeah, I'm rather guilty in this respect.)
There was a problem hiding this comment.
I fixed this, but I am sloppy with imports because I think it's a waste of time to think about how they should be ordered. Looking forward to brittany!
| liftIO $ userid'' `shouldBe` userid | ||
|
|
||
| -- /self should contain the expected UserSSOId | ||
| self :: ResponseLBS |
There was a problem hiding this comment.
Hm, don't we have this client function already written somewhere?
There was a problem hiding this comment.
Perhaps, but it's harder to find than the write.
| -- Get all users via SCIM | ||
| users <- listUsers Nothing | ||
| users <- listUsers tok Nothing |
There was a problem hiding this comment.
Perhaps let's call it token? (Similarly to the push for "team" and so on instead of "tid")
There was a problem hiding this comment.
token is already in scope here. it's a local name, so i would prefer not to worry about it too much, but feel free to suggest something else.
| @@ -35,6 +39,28 @@ import qualified Web.SCIM.Schema.Meta as SCIM | |||
| import qualified Web.SCIM.Schema.User as SCIM.User | |||
|
|
|||
|
|
|||
| registerIdPAndSCIMToken :: HasCallStack => TestSpar (ScimToken, (UserId, TeamId, IdP)) | |||
There was a problem hiding this comment.
Can you add a haddock? Stating at least that a) the user is the owner of the created team and b) the token can be used to manipulate the team
| (, team) <$> registerSCIMToken teamid (Just (idp ^. idpId)) | ||
|
|
||
| registerSCIMToken :: HasCallStack => TeamId -> Maybe IdPId -> TestSpar ScimToken | ||
| registerSCIMToken teamid midpid = do |
There was a problem hiding this comment.
It might be called more than once so let's add a random suffix or something to the token.
stack test --fast --test-arguments=--quickcheck-replay=762030435