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

[IMPROVE] General federation improvements #26150

Merged
merged 118 commits into from
Aug 24, 2022

Conversation

MarcosSpessatto
Copy link
Member

@MarcosSpessatto MarcosSpessatto commented Jul 6, 2022

Proposed changes (including videos or screenshots)

I know this changed a lot of files, but the main goal for this PR is not to change any behavior, the goals for the PR are:

  • Refactor the code;
  • Solve any tech debt;
  • Simplify and reuse some parts of the code;
  • Remove duplicated code;
  • Remove all unsafe type castings;
  • Solve all Eslint errors and warnings;
  • Split too big files;
  • Encapsulate the business logic in a better way, avoiding exposing and leaking internal logic to the unintended layers;
  • Improve the actual test cases;
  • Add more test cases, since a lot of cases were omitted during the release phase;
  • Remove unsafe Object.assign statements and prefer to use the class constructor instead;

Issue(s)

Steps to test or reproduce

Further comments

@MarcosSpessatto MarcosSpessatto changed the title [IMPROVE]: General federation improvements [IMPROVE] General federation improvements Jul 6, 2022
@ggazzo ggazzo added the stat: ready to merge PR tested and approved waiting for merge label Aug 22, 2022
@CLAassistant
Copy link

CLAassistant commented Aug 23, 2022

CLA assistant check
All committers have signed the CLA.

tassoevan
tassoevan previously approved these changes Aug 23, 2022
@kodiakhq kodiakhq bot removed the stat: ready to merge PR tested and approved waiting for merge label Aug 24, 2022
@kodiakhq
Copy link
Contributor

kodiakhq bot commented Aug 24, 2022

This PR currently has a merge conflict. Please resolve this and then re-add the ['stat: ready to merge', 'automerge'] label.

@ggazzo ggazzo added the stat: ready to merge PR tested and approved waiting for merge label Aug 24, 2022
@MarcosSpessatto
Copy link
Member Author

I don't have the knowledge to say it will in fact break both systems, but the isRemote flag is also a generic flag to identify "not local users".. introducing a new flag can cause other issues as well, like we need to make sure to filter both out..

there is at least another place missing an update to consider the new flag, the statistics for federated users:
Rocket.Chat/apps/meteor/app/statistics/server/lib/statistics.ts

Line 275 in 8c2c016

statistics.federatedUsers = federationOverviewData.numberOfFederatedUsers;
I wonder if there are others as well

I understand your point and I agree with the statistics example you gave. Indeed both flags are being used for the same purpose, but I respectfully disagree we should use the same for either "old federation" and the new version of it, since if we are using both at the same time, changes happening in one feature will affect directly the other. Another example will be, if you are using the old federation and you enable the new one, some unpredictable things could happen since you will start to use data from one federation to another. Maybe @alansikora can help us on this as well.

@alansikora
Copy link
Contributor

It is exactly what @MarcosSpessatto said. We can't.

And the old federation is going to be removed anyway, so we should not bother about it. We will ensure a full cleanup when the time to remove the old federation comes.

@casalsgh casalsgh dismissed sampaiodiego’s stale review August 24, 2022 18:04

Just so that PR can be merged and Diego is on PTO. Sorry Diego

@kodiakhq kodiakhq bot merged commit 12481d5 into develop Aug 24, 2022
@kodiakhq kodiakhq bot deleted the improve/federation-general-improvements branch August 24, 2022 19:25
gabriellsh added a commit that referenced this pull request Aug 26, 2022
…hreads

* 'develop' of github.com:RocketChat/Rocket.Chat: (93 commits)
  Chore: Upgrade dependencies (#26694)
  Chore: More Omnichannel tests (#26691)
  Regression: Banner - Room not found - Omnichannel room (#26693)
  [NEW] Capability to search visitors by custom fields (#26312)
  Chore: Create tests for Omnichannel admin add a custom fields (#26609)
  [FIX] Avatars of other chats disappear when they located near chat with broken avatar (#26689)
  [IMPROVE] Added identification on calls to/from existing contacts (#26334)
  Regression: invalid statistics format  (#26684)
  Regression: "Cache size is not a function" error when booting (#26683)
  [FIX] Correct IMAP configuration for email inbox (#25789)
  [FIX] Active users count on `@all` and `@here`  (#25957)
  [FIX] Autotranslate method should respect setting (#26549)
  Chore: Remove italic/bold font-style from system messages (#26655)
  Chore: Convert AppSetting to tsx (#26625)
  Chore: Remove & Test old closeChat templates (#26631)
  [IMPROVE] General federation improvements (#26150)
  [NEW] Warn admins about running multiple instances of the monolith (#26667)
  Regression: Prevent message from being temp forever (#26668)
  Regression: Add alsoSendThreadToChannel to user settings api (#26663)
  [IMPROVE] Spotlight search user results (#26599)
  ...
csuadev pushed a commit that referenced this pull request Aug 26, 2022
@murtaza98 murtaza98 mentioned this pull request Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
squad: federation stat: QA tested stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants