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-1643] Servantify brig account API 2 - GET /activate #2700

Conversation

battermann
Copy link
Contributor

@battermann battermann commented Sep 19, 2022

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

Checklist

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

@battermann battermann temporarily deployed to cachix September 19, 2022 08:01 Inactive
@battermann battermann temporarily deployed to cachix September 19, 2022 08:01 Inactive
@battermann battermann force-pushed the SQSERVICES-1643-backend-servantify-brig-account-api-1-get-activate branch from 4e811eb to 01a355c Compare September 19, 2022 08:02
@battermann battermann temporarily deployed to cachix September 19, 2022 08:02 Inactive
@battermann battermann temporarily deployed to cachix September 19, 2022 08:02 Inactive
@battermann battermann changed the title Sqservices 1643 backend servantify brig account api 1 get activate [SQSERVICES-1643] Sservantify brig account API 1 - GET /activate Sep 19, 2022
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Sep 19, 2022
@battermann battermann temporarily deployed to cachix September 19, 2022 08:04 Inactive
@battermann battermann temporarily deployed to cachix September 19, 2022 08:04 Inactive
@battermann battermann marked this pull request as ready for review September 19, 2022 08:06
@battermann battermann requested a review from fisx September 19, 2022 08:07
@battermann battermann changed the title [SQSERVICES-1643] Sservantify brig account API 1 - GET /activate [SQSERVICES-1643] Sservantify brig account API 2 - GET /activate Sep 19, 2022
@battermann battermann changed the title [SQSERVICES-1643] Sservantify brig account API 2 - GET /activate [SQSERVICES-1643] Servantify brig account API 2 - GET /activate Sep 19, 2022
Schema.objectWithDocModifier "ActivationResponse" (description ?~ "Response body of a successful activation request") $
ActivationResponse
<$> activatedIdentity Schema..= userIdentityObjectSchema
<*> activatedFirst Schema..= (fromMaybe False <$> Schema.optFieldWithDocModifier "first" (description ?~ "Whether this is the first successful activation (i.e. account activation).") Schema.schema)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<*> activatedFirst Schema..= (fromMaybe False <$> Schema.optFieldWithDocModifier "first" (description ?~ "Whether this is the first successful activation (i.e. account activation).") Schema.schema)
<*> activatedFirst Schema..= (fromMaybe False <$> Schema.optFieldWithDocModifier "first" (description ?~ "Whether this is the first successful activation of this account.") Schema.schema)

(Not sure I understand, though. How often can one account be activated successfully?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is from the original swagger documentation. IMO it should not be changed in this PR. Also it is not obvious to me either how this should be phrased correctly.

activateH :: Public.ActivationKey ::: Public.ActivationCode -> (Handler r) Response
activateH (k ::: c) = do
activate :: Public.ActivationKey -> Public.ActivationCode -> (Handler r) ActivationRespWithStatus
activate k c = do
Copy link
Contributor

Choose a reason for hiding this comment

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

(functionNameH traditionally means "functionName, but there is a servant handler with that name, and this is just the wrapper with the wai-route plumbing in it; will be removed once we switch to servant". So it's probably/usually a good idea to merge the two, but yes, in this case I'm not sure how to do that off the top of my head.)

@battermann battermann force-pushed the SQSERVICES-1643-backend-servantify-brig-account-api-1-get-activate branch from dfdd1d6 to 776df23 Compare September 20, 2022 08:03
@battermann battermann temporarily deployed to cachix September 20, 2022 08:03 Inactive
@battermann battermann temporarily deployed to cachix September 20, 2022 08:03 Inactive
@battermann battermann force-pushed the SQSERVICES-1643-backend-servantify-brig-account-api-1-delete branch from 860ab66 to 035d17d Compare September 20, 2022 08:28
@battermann battermann force-pushed the SQSERVICES-1643-backend-servantify-brig-account-api-1-get-activate branch from 776df23 to ea4975a Compare September 20, 2022 08:28
@battermann battermann temporarily deployed to cachix September 20, 2022 08:28 Inactive
@battermann battermann temporarily deployed to cachix September 20, 2022 08:28 Inactive
@battermann battermann temporarily deployed to cachix September 20, 2022 12:28 Inactive
@battermann battermann temporarily deployed to cachix September 20, 2022 12:28 Inactive
Base automatically changed from SQSERVICES-1643-backend-servantify-brig-account-api-1-delete to develop September 20, 2022 13:17
@battermann battermann force-pushed the SQSERVICES-1643-backend-servantify-brig-account-api-1-get-activate branch from ca38058 to 665bb3c Compare September 20, 2022 13:19
@battermann battermann temporarily deployed to cachix September 20, 2022 13:19 Inactive
@battermann battermann temporarily deployed to cachix September 20, 2022 13:19 Inactive
@battermann battermann merged commit 8131654 into develop Sep 20, 2022
@battermann battermann deleted the SQSERVICES-1643-backend-servantify-brig-account-api-1-get-activate branch September 20, 2022 14:34
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