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

fix: blocked by peer status #5732

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

fix: blocked by peer status #5732

wants to merge 2 commits into from

Conversation

lorux0
Copy link
Collaborator

@lorux0 lorux0 commented Sep 21, 2023

What does this PR change?

Re-enables the propagation of the blocked list through comms and improves UI (user context menu & passport) flows when the peer has blocked you.
NOTE: it may have an impact in the comms infra throughput/costs.

Screen Shot 2023-09-21 at 17 15 57 Screen Shot 2023-09-21 at 17 18 57

How to test the changes?

  1. Launch the explorer
  2. Block a user or get blocked by a user
  3. Check that the flows in the passport are correct. If the user has blocked you, you shouldn't be able to interact. If you blocked the user, you should be able to unblock it
  4. Open any user context menu. If the user has blocked you, you shouldn't be able to interact, for example send a friend request. If you blocked the user, you should be able to unblock it
  5. Try send messages. You shouldn't be able to receive or send to/from blocked users.

Our Code Review Standards

https://github.com/decentraland/unity-renderer/blob/master/docs/code-review-standards.md

Copilot summary

🤖 Generated by Copilot at 758c31c

This pull request fixes the blocked status propagation in the comms layer and updates the passport and user context menu UIs to reflect the blocked status of the user and the peer. It renames the field hasBlocked to isBlockedByPeer in the PlayerPassportModel and the PassportPlayerInfoComponentController classes, and uses the blocked status from the comms layer and the user profile model in the PassportPlayerInfoComponentView and the UserContextMenu classes. It also comments out the lines that clear the blocked and muted lists of the user profile in the sagas.ts file.

@lorux0 lorux0 requested a review from a team as a code owner September 21, 2023 20:25
@@ -410,7 +410,9 @@ async function stripSnapshots(profile: Avatar): Promise<Avatar> {
...profile,
avatar: { ...profile.avatar, snapshots: newSnapshots as Snapshots },
// THIS IS IMPORTANT, the blocked and muted sizes are too big for the network and are unnecesary
blocked: [],
// blocked status propagation has been re-enabled so the current user knows when a peer has blocked it
Copy link
Contributor

Choose a reason for hiding this comment

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

can't messages be ignored locally instead? I'm worried about the big warning above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you please elaborate?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the warning in the comment "the blocked and muted sizes are too big for the network", can we simply ignore the messages if we get one from a blocked user?

Copy link
Collaborator Author

@lorux0 lorux0 Sep 22, 2023

Choose a reason for hiding this comment

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

Is not only the chat messages. You shouldnt be able to interact with friend requests or preview the passport of peers that blocked you. We have special states in the UI for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, understood, but I don't think we can simply send this through comms, some users have thousands of addresses in the block list, and this message is broadcasted to everyone around the user.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will try to make a workaround by fetching the user profile from the catalyst when this information is required

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Trying to fetch the blocked list from the catalyst when needed adds too much complexity thus a mess to handle with the current architecture. A new proposal is coming on which profile information will always be fetched from the catalyst instead of being broadcasted through comms. This way would fix the problem

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