18638- [Communities] Implement Permissions Drawer #18988
Merged
Conversation
This file contains 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
Jenkins BuildsClick to see older builds (56)
|
ajayesivan
reviewed
Feb 27, 2024
|
|
||
| (def container | ||
| {:flex 1 | ||
| :background-color :white}) |
There was a problem hiding this comment.
Setting bg color to white won't work since we need a different color for dark theme.
| (get token-permissions permission-type)) | ||
| permission-types)) | ||
|
|
||
| (defn get-token-role |
There was a problem hiding this comment.
filter-tokens and get-token-role can be marked as private using defn-
| (defn get-token-role | ||
| [token-permissions] | ||
| (->> token-permissions | ||
| (remove (fn [[level _]] (#{5 6} level))) ; Exclude levels 5 and 6, focusing on token roles |
There was a problem hiding this comment.
Instead of hardcoding we should use constants -
status-mobile/src/status_im/constants.cljs
Lines 155 to 156 in 4f5480e
| (defn get-token-role | ||
| [token-permissions] | ||
| (->> token-permissions | ||
| (remove (fn [[level _]] (#{5 6} level))) ; Exclude levels 5 and 6, focusing on token roles |
There was a problem hiding this comment.
If you are attempting to filter out the 'become a member' permission, then we should filter permission type 2 instead of removing types 5 and 6.
status-mobile/src/status_im/constants.cljs
Line 152 in 4f5480e
Comment on lines
30
to
31
| tokens (filter-tokens token-permissions [1 2 3]) | ||
| collectible (filter-tokens token-permissions [5 6]) |
There was a problem hiding this comment.
We should use constants here too.
src/status_im/subs/communities.cljs
Outdated
| @@ -2,7 +2,8 @@ | |||
| (:require | |||
| [clojure.string :as string] | |||
| [legacy.status-im.ui.screens.profile.visibility-status.utils :as visibility-status-utils] | |||
| [re-frame.core :as re-frame] | |||
| [re-frame.core :as re-frame] ;; [re-frame.db :as rf-db] | |||
As you can see in the screenshot 10SNT is become an admin permission, but it is being listed in member permissions. 
Also, small UI issue: the divider width is incorrect. |
|
We have a permission button on the 'request to join' screen and the 'address for airdrop' screen. Just pointing it out, perhaps we can address it in a follow-up. |
ajayesivan
force-pushed
the
18638-communities-implement-permission-drawer
branch
4 times, most recently
from
f8a1095
to
5394436
Compare
ajayesivan
force-pushed
the
18638-communities-implement-permission-drawer
branch
from
5394436
to
77d3966
Compare
cammellos
reviewed
Mar 19, 2024
| @@ -16,6 +17,7 @@ | |||
| [] | |||
| (let [{id :community-id} (rf/sub [:get-screen-params]) | |||
| {:keys [name color images joined]} (rf/sub [:communities/community id]) | |||
| {:keys [has-permissions?]} (rf/sub [:community/token-permissions id]) | |||
There was a problem hiding this comment.
if you are pulling a single key, I think it's best to just create a new subscription
| {:content (fn [] [confirm-discard-drawer | ||
| community-id])}]))) | ||
| [identical-choices?])] | ||
| {:keys [has-permissions?]} (rf/sub [:community/token-permissions community-id]) |
| (rf/dispatch [:show-bottom-sheet | ||
| {:content (fn [] [confirm-discard-drawer | ||
| community-id])}]))) | ||
| [identical-choices?]) |
There was a problem hiding this comment.
should not this be tracking community-id as well? I take if community-id changes the whole component will be thrown away, but still seems safer to track
|
|
||
| (defn view | ||
| [id] | ||
| (let [{:keys [permissions]} (rf/sub [:community/token-permissions id]) |
| (defn view | ||
| [id] | ||
| (let [{:keys [permissions]} (rf/sub [:community/token-permissions id]) | ||
| role-text (fn [role] |
There was a problem hiding this comment.
this could be defined outside, as it will be recreated at each render (I don't think it matters, but it also looks a bit neater to defined it outside, since let to me implies is a closure
| (map (fn [{:keys [tokens role satisfied?]}] | ||
| (when (seq tokens) | ||
| ^{:key role} | ||
| [rn/view {:style {:margin-bottom 20}} |
There was a problem hiding this comment.
I would extract this component in a separate function
| (rf/reg-event-fx :communities/check-permissions-to-join-community-with-all-addresses-success | ||
| (fn [{:keys [db]} [community-id result]] | ||
| {:db (-> db | ||
| (assoc-in [:communities/permissions-check-all community-id] |
| {:db (assoc-in db [:communities/permissions-check community-id :checking?] true) | ||
| :json-rpc/call [{:method "wakuext_checkPermissionsToJoinCommunity" | ||
| :params [(cond-> {:communityId community-id} | ||
| addresses |
There was a problem hiding this comment.
This check is always true
(set nil) -> #{} (when #{} true) -> true
src/status_im/subs/communities.cljs
Outdated
| constants/community-token-permission-become-member) | ||
| (:satisfied %)) | ||
| roles) | ||
| permissions (into [] |
There was a problem hiding this comment.
instead of (into [] (map (mapv will put them directly into a vector
ajayesivan
force-pushed
the
18638-communities-implement-permission-drawer
branch
from
b49df50
to
ae371d3
Compare
cammellos
approved these changes
Mar 19, 2024
ajayesivan
force-pushed
the
18638-communities-implement-permission-drawer
branch
from
ae371d3
to
2fb6ae6
Compare
96% of end-end tests have passedFailed tests (1)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
jo-mut
approved these changes
Mar 19, 2024
|
@ajayesivan thanks for the PR. Please take a look at the issues. ISSUE 1 Permissions drawer is not scrollabletelegram-cloud-document-2-5465640381477571149.mp4 |
ISSUE 2 Permission drawer is not updated based on selected addresses with required tokens@ajayesivan not sure if this issue is valid, need to check with design team. Steps:
Actual result: permission drawer is not updated. It still shows that user is eligible to join as despite user unselected account this tokens that are required for this role. telegram-cloud-document-2-5465640381477571174.mp4 |
ISSUE 3 Elements disappear on Request to join screen after backgrounding the appSteps:
Actual result: Permissions, Slide buttons and community logo disappear telegram-cloud-document-2-5465640381477571185.mp4 |
ajayesivan
force-pushed
the
18638-communities-implement-permission-drawer
branch
from
2fb6ae6
to
031e66b
Compare
|
Hi @pavloburykh, Could you please retest the PR. Let me know if you have any questions, thank you! |
83% of end-end tests have passedFailed tests (7)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (40)Click to expandClass TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
@ajayesivan thank you for the fixes. Looks good to me. One more question: how do we order the list of permissions in the drawer? I noticed that ordering is different on IOS and Android. On the video below you can see ordering on different devices of the same user: ordering.mp4@ajayesivan anyway, this does not block PR from merging, we can address separately is there is an issue. So PR is ready for merge. |
|
@pavloburykh Currently, it's not being ordered manually, but ideally, it should be in the same order on all devices, and we should implement some sort of sorting. I will create a follow-up issue to take care of this. Thank you for testing the PR! Edit: Follow-up issue: #19353 |
ajayesivan
force-pushed
the
18638-communities-implement-permission-drawer
branch
3 times, most recently
from
92038eb
to
2cdee67
Compare
ajayesivan
force-pushed
the
18638-communities-implement-permission-drawer
branch
from
478d5d6
to
04f3f09
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 #18638
Follows Figma
Has a follow-up to design system
Summary
This PR introduces a detailed token permission bottom sheet within the Addresses For Permissions Screen. It is designed to encapsulate the full spectrum of logic and complexity associated with community token-gated rules, offering a robust framework that enhances user interaction and comprehension.
I appreciate your careful review and testing. 😀
Review notes
Testing notes
Platforms
Functional
Steps to test
Create a Community in the Desktop App:
Invite a Mobile User:
Join the Community:
Access 'All Addresses' View:
Identify the 'Address for Permission' Label:
Track Permissions:
Before and after screenshots comparison
This is the expected behave
1 - The community has no token-gated permissions set:
1-test-permissions-NO-permissions.mov
2 -The community has a member token-gated permissions set:
1-test-permissions-token-permissions.mov
3 - The community has a collectibles token-gated permissions set and also admin set:
1-test-permissions-roles.mov
4 - Current wallet state using Testnet to validate this implementation:
status: ready!