Communities: Share Selective Account #18382
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
ajayesivan
commented
Jan 4, 2024
| :key-fn :address | ||
| :data accounts}] | ||
| (when (empty? @selected-addresses) | ||
| [rn/view |
There was a problem hiding this comment.
Please ignore the inaccurate colors, this is a temporary implementation. I will replace this with drawer/bottom-actions component after updating that component.
ajayesivan
force-pushed
the
18010-share-selective-account
branch
from
ac18484 to
9a01883
Compare
Jenkins BuildsClick to see older builds (9)
|
ajayesivan
force-pushed
the
18010-share-selective-account
branch
from
9a01883 to
46b8572
Compare
yqrashawn
reviewed
Jan 5, 2024
Comment on lines
+188
to
+190
| (rf/reg-event-fx :communities/set-addresses-for-permissions | ||
| (fn [{:keys [db]} [addresses]] | ||
| {:db (assoc-in db [:communities/addresses-for-permissions] addresses)})) |
There was a problem hiding this comment.
I am not sure if this was discussed in prev meetings. From what I understand, each community typically manages its own :communities/addresses-for-permissions.
I was thinking, might it be more streamlined to tie the community ID directly to the permissions? We could do something like this:
(assoc-in db [:communities/addresses-for-permissions community-id] addresses)
Or assoc this into existing community?
There was a problem hiding this comment.
We haven't discussed this yet. Currently, I'm using :communities/addresses-for-permissions solely to store the selected addresses for the community we're attempting to join. Each time we open the request-to-join page, it updates the key with all addresses, allowing users to modify their selection. At the moment, I'm only utilizing it for the request-to-join UI.
Your suggestion to tie or link it with community data makes sense. However, I'm uncertain if we'll receive shared addresses from status-go once we join a community. If that's possible, keeping that information in the database tied to the community would be more reliable. I'll check if status-go shares this information. Please let me know if you have any insights on this.
Feel free to correct me if my thought process seems incorrect.
There was a problem hiding this comment.
Checked the UI again. I'm fine with this solution.
48% of end-end tests have passedFailed tests (23)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (23)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
|
ajayesivan
force-pushed
the
18010-share-selective-account
branch
from
46b8572 to
4e93be5
Compare
yqrashawn
approved these changes
Jan 5, 2024
ilmotta
approved these changes
Jan 5, 2024
There was a problem hiding this comment.
Hey @ajayesivan, PR looks good to me except for the way the selected addresses are being handled with local Reagent atoms mixed with re-frame. We can refactor in a separate PR if you prefer.
Feature is starting to look nice!
Interestingly, with the toggle enabled, it's possible to reproduce exactly the issue reported here #18308. The user is limited to join a community with the Ethereum account only. I'm already assigned to the bug issue as you remember.
| selected-accounts (filter #(contains? addresses-for-permissions | ||
| (:address %)) | ||
| accounts)] | ||
| (rn/use-effect (fn [] |
There was a problem hiding this comment.
You could avoid use-effect and just call [rf/dispatch [:communities/set-addresses-for-permissions ...] in the external function of a form-2 component. Apparently, if we do move in the direction of UIx or just plain React, everything will be "functional" components (in Reagent's terminology). Because of that I don't feel strong in any way about not using use-effect here.
There was a problem hiding this comment.
@ilmotta, If I'm not mistaken, with form-2 I will also have to move accounts subscription to the outer function.
(defn view
[]
(let [accounts (rf/sub [:wallet/accounts-with-customization-color])]
(rf/dispatch [:communities/set-addresses-for-permissions
(set (map :address accounts))])
(fn []
(let [{id :community-id} (rf/sub [:get-screen-params])
{:keys [name color images]} (rf/sub [:communities/community id])This looks a bit more complicated to me compared to the use-effect approach.
There was a problem hiding this comment.
Fair enough :) It would be more about the performance advantage because functional components in Reagent are slower. Thanks, it's fine to keep the code as is.
| accounts (rf/sub [:wallet/accounts-with-customization-color])] | ||
| [rn/safe-area-view {:style style/container} | ||
| accounts (rf/sub [:wallet/accounts-with-customization-color]) | ||
| selected-addresses (reagent/atom (rf/sub [:communities/addresses-for-permissions]))] |
There was a problem hiding this comment.
It's a smell that we are wrapping rf/sub in a Reagent atom. It's the first time I've seen this.
The whole address selection state during a request to join is global, there's never a competing screen where the user is doing the same thing, hence there's no need to mix Reagent atoms with re-frame, this brings more complexity. Only re-frame would suffice, and account-item would just dispatch the necessary events to toggle the respective address, not swap a temporary Reagent atom.
There was a problem hiding this comment.
Sure, I'll address this in a follow-up PR. I will also try to add unit tests along with it.
I couldn't find a similar implementation anywhere 😅. My rationale was that since the selected state is only used in this screen, it seemed more fitting to keep it local.
There was a problem hiding this comment.
No worries @ajayesivan I understand there's room for interpretation about when to use re-frame, and when not to use it. In my experience so far, it's easier to keep the UI simple & boring if we use re-frame as much as we can.
I think we can all agree that, in general (not always), it's a joy to work on UI code where there are no local state and only dispatches and subscriptions. Reagent atoms are definitely important, but should be used IMO only when strictly necessary, not the other way around. We lose so much when we move out of re-frame, e.g. harder to debug local state, less observability (no logs, no re-frisk), harder to unit test since the UI dictates more things, harder to write our integration tests because events require more arguments to be passed around, etc.
There was a problem hiding this comment.
Follow-up issue: #18405
There was a problem hiding this comment.
| :disabled? (empty? @selected-addresses) | ||
| :on-press (fn [] | ||
| (rf/dispatch [:communities/set-addresses-for-permissions | ||
| @selected-addresses]) |
There was a problem hiding this comment.
Similar here, the selected addresses state should be in the app db, so there would be no need to pass it around when dispatching events. Moving the state to the app db would also allow us to cover more of the logic with unit tests for event handlers.
65% of end-end tests have passedFailed tests (13)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (4)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (31)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
ajayesivan
force-pushed
the
18010-share-selective-account
branch
from
4e93be5 to
7936159
Compare
yevh-berdnyk
pushed a commit
that referenced
this pull request
Jan 5, 2024
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 #18010
This PR enables selectively sharing addresses when joining a community.
Screen Recording:
Selectively.Share.Accounts.mp4
Test Note
community-accounts-selection-enabled?flag insrc/status_im2/config.cljstotrue.