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

Derive group ID from qualified conversation ID and, if applicable, subconversation ID #3309

Merged
merged 7 commits into from
Jun 1, 2023

Conversation

stefanwire
Copy link
Contributor

@stefanwire stefanwire commented May 23, 2023

Checklist

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

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label May 23, 2023
@stefanwire stefanwire force-pushed the FS-1974 branch 8 times, most recently from fe50fad to bade9a2 Compare May 30, 2023 13:35
@stefanwire stefanwire changed the title Simplify group_id → conv_id mapping logic Derive group ID from qualified conversation ID and, if applicable, subconversation ID May 30, 2023
@stefanwire stefanwire marked this pull request as ready for review May 30, 2023 13:59
@stefanwire stefanwire marked this pull request as draft May 30, 2023 14:33
@stefanwire stefanwire requested review from pcapriotti and removed request for pcapriotti and smatting May 30, 2023 14:34
@stefanwire stefanwire marked this pull request as ready for review May 31, 2023 12:34
@stefanwire
Copy link
Contributor Author

let newGid = fromRight (convToGroupId' (flip SubConv scnvId <$> tUntagged lcnvId)) $ nextGenGroupId gid
should get extra attention during the review, since it swallows the error when creating a new group ID while deleting a subconversation.

@stefanwire
Copy link
Contributor Author

testAddRemoteMemberInvalidDomain :: TestM ()
testAddRemoteMemberInvalidDomain = do
alice <- randomUser
bobId <- randomId
let remoteBob = Qualified bobId (Domain "invalid.example.com")
convId <- decodeConvId <$> postConv alice [] (Just "remote gossip") [] Nothing Nothing
localDomain <- viewFederationDomain
let qconvId = Qualified convId localDomain
connectWithRemoteUser alice remoteBob
postQualifiedMembers alice (remoteBob :| []) qconvId
!!! do
const 422 === statusCode
const (Just "/federation/api-version")
=== preview (ix "data" . ix "path") . responseJsonUnsafe @Value
const (Just "invalid.example.com")
=== preview (ix "data" . ix "domain") . responseJsonUnsafe @Value
has been removed, since it tests removed code.

-- This test is a safeguard to ensure adding remote members will fail
-- on environments where federation isn't configured (such as our production as of May 2021)
testAddRemoteMemberFederationDisabled :: TestM ()
testAddRemoteMemberFederationDisabled = do
alice <- randomUser
remoteBob <- flip Qualified (Domain "some-remote-backend.example.com") <$> randomId
qconvId <- decodeQualifiedConvId <$> postConv alice [] (Just "remote gossip") [] Nothing Nothing
connectWithRemoteUser alice remoteBob
-- federator endpoint not configured is equivalent to federation being disabled
-- This is the case on staging/production in May 2021.
let federatorNotConfigured = optFederator .~ Nothing
withSettingsOverrides federatorNotConfigured $
postQualifiedMembers alice (remoteBob :| []) qconvId !!! do
const 400 === statusCode
const (Right "federation-not-enabled") === fmap label . responseJsonEither
-- the member is not actually added to the conversation
conv <- responseJsonError =<< getConvQualified alice qconvId <!! const 200 === statusCode
liftIO $ map omQualifiedId (cmOthers (cnvMembers conv)) @?= []
has been removed, since it tests removed code.

Removed federation endpoints
- on-new-remote-conversation,
- on-new-remote-subconversation, and
- on-delete-mls-conversation.

Removed effects
- Galley.Effects.SubConversationSupply.
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