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

feat: Add stopping to federate (WPB-203) #15583

Merged
merged 26 commits into from
Aug 16, 2023
Merged

feat: Add stopping to federate (WPB-203) #15583

merged 26 commits into from
Aug 16, 2023

Conversation

thisisamir98
Copy link
Contributor

@thisisamir98 thisisamir98 commented Aug 11, 2023

TaskWPB-203 [Web] System message when stopping to federate

This pull request implements a set of client-side actions to handle events related to federated communication termination. The specification outlines the steps for two types of events: federation.delete and federation.connectionRemoved. The main focus is on cleaning up local state after receiving these events and inserting a system message to explain what happened to the user.

For the federation.delete event: (Backend A has stopped federating with us)

  • receive the event from backend
  • leave the conversations locally that are owned by the backend A which was deleted.
  • remove the deleted backend A users locally from our own conversations.
  • insert system message to the affected conversations about federation termination.

For the federation.connectionRemoved event: (Backend A & B stopped federating, user is on C)

  • receive the event from backend
  • Identify all conversations that are not owned from A or B domain and that contain users from A and B
    • remove users from A and B from those conversations
    • insert system message in those conversations about backend A and B stopping to federate
  • identify all conversations owned by domain A that contains users from B
    • remove users from B from those conversations
    • insert system message in those conversations about backend A and B stopping to federate
  • Identify all conversations owned by domain B that contains users from A
    • remove users from A from those conversations
    • insert system message in those conversations about backend A and B stopping to federate

@thisisamir98 thisisamir98 marked this pull request as ready for review August 14, 2023 14:20
@thisisamir98 thisisamir98 requested review from otto-the-bot and a team as code owners August 14, 2023 14:20
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #15583 (1be8626) into dev (72d4f4b) will decrease coverage by 0.15%.
The diff coverage is 8.00%.

@@            Coverage Diff             @@
##              dev   #15583      +/-   ##
==========================================
- Coverage   43.51%   43.37%   -0.15%     
==========================================
  Files         651      653       +2     
  Lines       22189    22280      +91     
  Branches     5080     5096      +16     
==========================================
+ Hits         9656     9664       +8     
- Misses      11294    11377      +83     
  Partials     1239     1239              

Comment on lines 326 to 335
if (type === FEDERATION_EVENT.FEDERATION_DELETE) {
const {domain: deletedDomain} = data;

await this.onFederationDelete(deletedDomain);
}

if (type === FEDERATION_EVENT.FEDERATION_CONNECTION_REMOVED) {
const {domains: deletedDomains} = data;
await this.onFederationConnectionRemove(deletedDomains);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

let's convert that into a switch in case we will need to handle more events of this type in the future.
Also we could early return in case the type has been matched

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 76d1df0

Comment on lines +300 to +304
case ClientEvent.CONVERSATION.FEDERATION_STOP: {
messageEntity = this._mapEventFederationStop(event);
break;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

don't we need to handle the connectionRemove event (and display it) as well?

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 the event that we are creating on the client side for the system message, connection removed is handled here:
https://github.com/wireapp/wire-webapp/pull/15583/files#diff-8424911ba55dc95ddd7ee35e77e651a8894faa0c33cd485cbbf62f6537fe3d24R327-R329

Copy link
Contributor

Choose a reason for hiding this comment

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

but there is no system message for connectionRemoved event?

Comment on lines 324 to 332
case EVENT_TYPE.CALL:
amplify.publish(WebAppEvents.CALL.EVENT_FROM_BACKEND, event, source);
break;
case EVENT_TYPE.FEDERATION:
amplify.publish(WebAppEvents.FEDERATION.EVENT_FROM_BACKEND, event, source);
break;
case EVENT_TYPE.CONVERSATION:
amplify.publish(WebAppEvents.CONVERSATION.EVENT_FROM_BACKEND, event, source);
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

note for the future, I'd like the EventRepo to not really filter the type of events.
It should just send all the events in a single event and all the repos subscribing to it will make the work of filtering depending on the event type. :)

const allConversations = this.conversationState.conversations();

allConversations
.filter(conversation => conversation.domain === domainOne)
Copy link
Contributor

Choose a reason for hiding this comment

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

these two blocks are almost identical except domain type. We can create a function and call it twice with each domain.

const handleFederationUsers = async (domainToFilter: string, userDomainToDelete: string) => {
  allConversations
    .filter(conversation => conversation.domain === domainToFilter)
    .forEach(async conversation => {
      const usersToDelete = conversation.allUserEntities().filter(user => user.domain === userDomainToDelete);
      if (usersToDelete.length > 0) {
        await this.removeDeletedFederationUsers(conversation, usersToDelete);
        await this.insertFederationStopSystemMessage(conversation, [domainOne, domainTwo]);
      }
    });
};

await handleFederationUsers(domainOne, domainTwo);
await handleFederationUsers(domainTwo, domainOne);

Copy link
Contributor

Choose a reason for hiding this comment

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

that's is a good suggestion, indeed. But without unit tests to back it, it's also risky.
I'd recommend writing tests first (I can help with that), make sure that we are compliant with the specs and then refactoring to a cleaner/more performant code.

src/script/conversation/ConversationRepository.ts Outdated Show resolved Hide resolved
src/script/conversation/ConversationRepository.ts Outdated Show resolved Hide resolved
@thisisamir98 thisisamir98 dismissed arjita-mitra’s stale review August 16, 2023 10:14

will do it in separate PR

@thisisamir98 thisisamir98 merged commit ed8688f into dev Aug 16, 2023
11 checks passed
@thisisamir98 thisisamir98 deleted the feat/WPB-203 branch August 16, 2023 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants