Skip to content

Comments

Do not fail user deletion when a remote notification fails#1912

Merged
smatting merged 6 commits intodevelopfrom
FS-124-user-deletion-nonavail-remotes
Nov 9, 2021
Merged

Do not fail user deletion when a remote notification fails#1912
smatting merged 6 commits intodevelopfrom
FS-124-user-deletion-nonavail-remotes

Conversation

@smatting
Copy link
Contributor

@smatting smatting commented Nov 8, 2021

Checklist

https://wearezeta.atlassian.net/browse/FS-124

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information:
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

@smatting smatting force-pushed the FS-124-user-deletion-nonavail-remotes branch from 60a843d to 6991453 Compare November 8, 2021 17:07
Copy link
Contributor

@pcapriotti pcapriotti 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 to me.

Copy link
Contributor

@mdimjasevic mdimjasevic left a comment

Choose a reason for hiding this comment

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

This looks good. A few smaller comments are left inlined.

Comment on lines 278 to 281
case eitherFErr of
Left fErr -> do
logFederationError (tDomain uids) fErr
-- FUTUTREWORK: Do something better here?
-- FUTUREWORK: Write test that this happens
throwM $ federationErrorToWai fErr
Right () -> pure ()
Copy link
Contributor

Choose a reason for hiding this comment

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

A pattern like this is encapsulated by whenLeft :: Applicative m => Either a b -> (a -> m ()) -> m ().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nice, done.

Comment on lines 1284 to 1288
case (requestMethod, requestPath) of
(_methodDelete, ["i", "user"]) -> do
let response = Wai.responseLBS Http.status200 [(Http.hContentType, "application/json")] (cs $ Aeson.encode EmptyResponse)
pure response
_ -> error "not mocked"
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to pattern match on requestMethod as it is vacuous, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. I changed this to a proper pattern match in d92e7fa

@smatting smatting merged commit 7371390 into develop Nov 9, 2021
@smatting smatting deleted the FS-124-user-deletion-nonavail-remotes branch November 9, 2021 11:32
@akshaymankar akshaymankar mentioned this pull request Nov 15, 2021
@smatting smatting mentioned this pull request Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants