fix(communities): view-only channels unsupported#22226
Merged
Conversation
ulisesmac
commented
Mar 3, 2025
Comment on lines
27
to
29
| "canPost" :can-post? | ||
| "can-view" :can-view? | ||
| "canView" :can-view? | ||
| "hideIfPermissionsNotMet" :hide-if-permissions-not-met? |
Contributor
Author
There was a problem hiding this comment.
the can-view key never appeared, we were handling this prop wrongly, maybe due to an update on the status-go response?
ulisesmac
commented
Mar 3, 2025
ulisesmac
commented
Mar 3, 2025
ulisesmac
commented
Mar 3, 2025
Member
Jenkins BuildsClick to see older builds (20)
|
ulisesmac
commented
Mar 3, 2025
ilmotta
approved these changes
Mar 4, 2025
seanstrom
approved these changes
Mar 4, 2025
ulisesmac
force-pushed
the
22803-add-support-for-view-only-permissions-channels
branch
from
d9758e7 to
ee3bef3
Compare
vkjr
reviewed
Mar 5, 2025
vkjr
approved these changes
Mar 5, 2025
pavloburykh
force-pushed
the
22803-add-support-for-view-only-permissions-channels
branch
from
ee3bef3 to
6a24b68
Compare
Member
32% of end-end tests have passedFailed tests (42)Click to expandClass TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestFallbackMultipleDevice:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedThree:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (4)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletCollectibles:
Passed tests (22)Click to expandClass TestWalletMultipleDevice:
Class TestAndroid13:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestAndroid12:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
Contributor
|
@ulisesmac thanks for the PR. Please take a look at the issues below. ISSUE 1 Reactions are displayed in message longtap menu in view only channelsThis is probably some old issue, not related directlly to PR. But would be nice to be fixed in scope of current PR. It's up to you @ulisesmac Steps:
Expected result: reactions should not be displayed, as user cannot set reactions in view only channel Actual result: reactions are displayed, despite user cannot apply them in view only channel telegram-cloud-document-2-5375245824001337222.mp4 |
Contributor
|
@ulisesmac @ilmotta Wanted to share my experience regarding testing permissions. In some aspects it is quite painful because permission updates seem to work very bad. Below I will describe couple of cases. These issues are not related to current PR, so probably should be investigated separately. Case 1 Updates of channel permissions on Admins side are not applied for Mobile member until he re-joins communityPreconditions: Mobile user is a member of community and has access to token gated channel (meets requirements). Steps:
Actual result: channel is not locked, user can enter the channel view and post messages depending on channel permissions. Permissions are updated only after user leaves and re-joins community from scratch (see video below). The same situation in other directions: admin updates permission so mobile member who previously didn't meet requirements now meets them but still does not have access to the channel. telegram-cloud-document-2-5375245824001337312.mp4Case 2 Removing/adding the additional wallet account which holds tokens required for channel permission does not impact accessibility of token gated channelsPreconditions: Mobile user imports wallet account which holds required tokens for access in token gated channel Steps:
Expected result: user should loose access to token gated channel as he no longer holds required tokens Actual result: user still has access to token gated channel despite he does not meet requirements anymore. Moreover, even if user leaves and then re-joins community he will still have access to channels. Looks like if user once shared imported account during initial community join this account somehow caches on backend side and system acts like user still has this account despite it has been already removed. Same works in other side: if Mobile member previously didn't have access to token gated channel, he will not get this access even after importing wallet account which holds required tokens. |
Contributor
|
There might be more cases, those are only some of them. Just wondering, at which point permission updates on control node side should be applied for community members? And at which point updates on community member side should be applied on control node side? Probably it sometimes may take a lot of time, but how much? Or maybe there are some existing issues on client side which result in problems related to sending/receiving data to/from control node. |
Contributor
|
Basically, this PR has fixed initial issue with View-only permission not been working at all on mobile. But when I proceed with more deeper testing of permissions I am just facing global issues (as described above) and probably not related to this PR. |
Contributor
|
@ulisesmac @pavloburykh If you uncovered many issues outside the scope of the PR, I think it's still valuable to capture them as issues outside this PR (unless they are already documented). If it's difficult to QA, it's probably going to be bad for users, so let's endure some of the pain. But perhaps then, let's not try to merge this PR necessarily if it's difficult, but since we are here, we can use this PR for learning purposes to help build up the (bug) scope regarding token gated communities. Given our goal of validating community features to users and marketing it, with the help of the Desktop team, we may need to actually fix some of these issues in 2.34 or later. If not, understanding how problematic the features operate, the investigation from QA can at least be more evidence of how we should consider simplifying/removing certain token-gated features. Does that sound good and is the plan clear in your opinion? |
Contributor
Author
|
@ilmotta Since I'm already paying attention to communities, I can help to solve the issues. I'd prefer to have the issues reported separately, merge this PR and then fix them gradually. |
ulisesmac
force-pushed
the
22803-add-support-for-view-only-permissions-channels
branch
from
6a24b68 to
ddb8c53
Compare
Contributor
@ulisesmac Token gated channels are full of problems as far as we've seen and most fixes will require changes in status-go. The expertise on fixing some of these bugs lie in the Desktop team members, but we can help as well. I agree on reporting bugs unrelated to your PR in separate issues, that would be helpful for us to understand better the full scope. @pavloburykh But in 2.34 we won't be able to work much on token-gated related bugs. Next Monday we will have a call with the Desktop team and Design team to review the product briefs (high-level features defined by Volo) that we will need to work on in 2.34 and I can tell you we will be busy with the upcoming features and the other bugs we already have on our plate :) Once you finish this PR, a good one that I got approval for our team to work for a few days is #21896 or #21649. |
Contributor
|
@ulisesmac thank you for the PR. I agree with @ilmotta that probably most of the described above issues are related to status-go. I will log them separately so we can investigate once we decide it to be a priority. Current PR is rested and ready for merge. What has been tested:
I have logged a separate issue related to channel permissions where I described couple of bugs which probably have common root cause #22296. I can log separate issues for each bug if needed. I am not even sure this is something which should be handled by mobile team, it is probably related to status-go as has been mentioned by @ilmotta and we are living with those issues for quite a long time I suppose. Another issue which I have logged #22297 is related to mobile and can fixed depending on priorities. I consider it to be low priority but feel free to prioritise @ilmotta in case you disagree. |
ulisesmac
deleted the
22803-add-support-for-view-only-permissions-channels
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #22083
Summary
This PR adds support for view-only channels by fixing bugs related to it.
Review notes
Some code reformating were also performed.
Testing notes
You can use the community in the issue description to test this feature.
Platforms
Areas that may be impacted
Chats in general. The changes affect locked/unlocked channels and the permissions to reply in chats, so please, focus on this areas.
status: ready