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.

Sign up for GitHub

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

Communities: Share Selective Account #18382

Merged
merged 1 commit into from
Jan 5, 2024
+99 −63
Merged

Communities: Share Selective Account #18382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/quo/components/wallet/account_permissions/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
[:checked? {:optional true} [:maybe :boolean]]
[:disabled? {:optional true} [:maybe :boolean]]
[:on-change {:optional true} [:maybe fn?]]
[:container-style {:optional true} [:maybe :map]]
[:theme :schema.common/theme]]]]
:any])

Expand Down Expand Up @@ -58,11 +59,11 @@

(defn- view-internal
[{:keys
[checked? disabled? on-change token-details keycard? theme]
[checked? disabled? on-change token-details keycard? theme container-style]
{:keys
[name address emoji customization-color]} :account}]
[rn/view
{:style (style/container theme)
{:style (merge (style/container theme) container-style)
:accessibility-label :wallet-account-permissions}
[rn/view {:style style/row1}
[account-avatar/view
Expand Down
26 changes: 20 additions & 6 deletions src/status_im/contexts/communities/actions/accounts_selection/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,29 @@
[utils.re-frame :as rf]))

(defn- join-community-and-navigate-back
[id]
[id addresses-for-permissions]
(rf/dispatch [:password-authentication/show
{:content (fn [] [password-authentication/view])}
{:label (i18n/label :t/join-open-community)
:on-press #(rf/dispatch [:communities/request-to-join
{:community-id id :password %}])}])
{:community-id id
:password %
:addresses-to-reveal addresses-for-permissions}])}])
(rf/dispatch [:navigate-back]))

(defn view
(defn f-view-internal
[]
(let [{id :community-id} (rf/sub [:get-screen-params])
{:keys [name color images]} (rf/sub [:communities/community id])
accounts (rf/sub [:wallet/accounts-with-customization-color])]
accounts (rf/sub [:wallet/accounts-with-customization-color])
addresses-for-permissions (rf/sub [:communities/addresses-for-permissions])
selected-accounts (filter #(contains? addresses-for-permissions
(:address %))
accounts)]
(rn/use-effect (fn []
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

(rf/dispatch [:communities/set-addresses-for-permissions
(set (map :address accounts))]))
[])
[rn/view {:style style/container}
[quo/page-nav
{:text-align :left
Expand Down Expand Up @@ -53,7 +63,7 @@
:action :arrow
:label :preview
:label-props {:type :accounts
:data accounts}
:data selected-accounts}
:description-props {:text (i18n/label :t/all-addresses)}}
{:title (i18n/label :t/for-airdrops)
:on-press #(rf/dispatch [:open-modal :airdrop-addresses
Expand All @@ -77,4 +87,8 @@
:track-text (i18n/label :t/slide-to-request-to-join)
:track-icon :i/face-id
:customization-color color
:on-complete #(join-community-and-navigate-back id)}]]]))
:on-complete #(join-community-and-navigate-back id addresses-for-permissions)}]]]))

(defn view
[]
[:f> f-view-internal])
21 changes: 7 additions & 14 deletions src/status_im/contexts/communities/actions/addresses_for_permissions/style.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,15 @@
(ns status-im.contexts.communities.actions.addresses-for-permissions.style
(:require [quo.foundations.colors :as colors]))
(ns status-im.contexts.communities.actions.addresses-for-permissions.style)

(def container {:flex 1})

(def account-item-container
{:font-size 30
:border-radius 16
:flex-direction :row
:border-width 1
:height 56
:padding-horizontal 12
:align-items :center
:margin-bottom 8
:gap 8
:border-color colors/neutral-90})

(def buttons
{:flex-direction :row
:gap 12
:padding-horizontal 20
:padding-vertical 12})

(def error-message
{:flex-direction :row
:gap 4
:justify-content :center
:margin-bottom 8})
97 changes: 59 additions & 38 deletions src/status_im/contexts/communities/actions/addresses_for_permissions/view.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,55 +1,76 @@
(ns status-im.contexts.communities.actions.addresses-for-permissions.view
(:require [quo.core :as quo]
[quo.foundations.colors :as colors]
[react-native.core :as rn]
[reagent.core :as reagent]
[status-im.common.not-implemented :as not-implemented]
[status-im.contexts.communities.actions.addresses-for-permissions.style :as style]
[utils.i18n :as i18n]
[utils.re-frame :as rf]))

(defn- account-item
[item]
[rn/view
{:style style/account-item-container}
[quo/account-avatar (assoc item :size 32)]
[rn/view
[quo/text
{:size :paragraph-1
:weight :semi-bold}
(:name item)]
[quo/address-text
{:address (:address item)
:format :short}]]])
[item _ _ selected-addresses]
[quo/account-permissions
{:account {:name (:name item)
:address (:address item)
:emoji (:emoji item)
:customization-color (:customization-color item)}
:token-details []
:checked? (contains? @selected-addresses (:address item))
:on-change (fn [checked?]
(if checked?
(swap! selected-addresses conj (:address item))
(swap! selected-addresses disj (:address item))))
:container-style {:margin-bottom 8}}])

(defn view
[]
(let [{id :community-id} (rf/sub [:get-screen-params])
{:keys [name color images]} (rf/sub [:communities/community id])
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]))]
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up issue: #18405

Copy link
Member

Choose a reason for hiding this comment

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

Hi, Thank you @ajay for linking to the discussion.

Wrapping a subscription inside a reagent atom seems wrong also to me. But making changes globally also doesn't look nice for the screen which has a confirm button. This causes weird scenarios/bugs like mentioned here in PR description: #18759

(fn []
[rn/safe-area-view {:style style/container}
[quo/drawer-top
{:type :context-tag
:title (i18n/label :t/addresses-for-permissions)
:community-name name
:button-icon :i/info
:on-button-press not-implemented/alert
:community-logo (get-in images [:thumbnail :uri])
:customization-color color}]

[quo/drawer-top
{:type :context-tag
:title (i18n/label :t/addresses-for-permissions)
:community-name name
:button-icon :i/info
:on-button-press not-implemented/alert
:community-logo (get-in images [:thumbnail :uri])
:customization-color color}]
[rn/flat-list
{:render-fn account-item
:render-data selected-addresses
:content-container-style {:padding 20}
:key-fn :address
:data accounts}]

[rn/flat-list
{:render-fn account-item
:content-container-style {:padding 20}
:key-fn :address
:data accounts}]
(when (empty? @selected-addresses)
[rn/view
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please ignore the inaccurate colors, this is a temporary implementation. I will replace this with drawer/bottom-actions component after updating that component.

#15006

{:style style/error-message}
[quo/icon
:i/info
{:color colors/danger-50
:size 16}]
[quo/text
{:size :paragraph-2
:style {:color colors/danger-50}}
(i18n/label :t/no-addresses-selected)]])

[rn/view {:style style/buttons}
[quo/button
{:type :grey
:container-style {:flex 1}
:on-press #(rf/dispatch [:navigate-back])}
(i18n/label :t/cancel)]
[quo/button
{:container-style {:flex 1}
:customization-color color
:on-press #(rf/dispatch [:navigate-back])}
(i18n/label :t/confirm-changes)]]]))
[rn/view {:style style/buttons}
[quo/button
{:type :grey
:container-style {:flex 1}
:on-press #(rf/dispatch [:navigate-back])}
(i18n/label :t/cancel)]
[quo/button
{:container-style {:flex 1}
:customization-color color
:disabled? (empty? @selected-addresses)
:on-press (fn []
(rf/dispatch [:communities/set-addresses-for-permissions
@selected-addresses])
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

(rf/dispatch [:navigate-back]))}
(i18n/label :t/confirm-changes)]]])))
4 changes: 4 additions & 0 deletions src/status_im/contexts/communities/events.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,3 +184,7 @@
:params []
:on-success #(rf/dispatch [:communities/fetched-collapsed-categories-success %])
:on-error #(log/error "failed to fetch collapsed community categories" %)}]}))

(rf/reg-event-fx :communities/set-addresses-for-permissions
(fn [{:keys [db]} [addresses]]
{:db (assoc-in db [:communities/addresses-for-permissions] addresses)}))
Comment on lines +188 to +190
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Checked the UI again. I'm fine with this solution.

7 changes: 4 additions & 3 deletions src/status_im/contexts/communities/overview/events.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,10 @@
err))}]}))))

(defn request-to-join
[{:keys [db]} [{:keys [community-id password]}]]
(let [pub-key (get-in db [:profile/profile :public-key])
addresses-to-reveal []]
[{:keys [db]}
[{:keys [community-id password addresses-to-reveal]
:or {addresses-to-reveal []}}]]
(let [pub-key (get-in db [:profile/profile :public-key])]
{:fx [[:json-rpc/call
[{:method "wakuext_generateJoiningCommunityRequestsForSigning"
:params [pub-key community-id addresses-to-reveal]
Expand Down
1 change: 1 addition & 0 deletions src/status_im/subs/root.cljs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@
(reg-root-key-sub :communities/collapsed-categories :communities/collapsed-categories)
(reg-root-key-sub :communities/selected-tab :communities/selected-tab)
(reg-root-key-sub :contract-communities :contract-communities)
(reg-root-key-sub :communities/addresses-for-permissions :communities/addresses-for-permissions)

;;activity center
(reg-root-key-sub :activity-center :activity-center)
Expand Down
1 change: 1 addition & 0 deletions translations/en.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@
"community-rules": "Community rules",
"address-to-share": "Addresses to share",
"addresses-for-permissions": "Addresses for permissions",
"no-addresses-selected": "At least 1 address must be shared with community",
"confirm-changes": "Confirm changes",
"airdrop-addresses": "Address for airdrops",
"join-as-a-member": "Join as a Member",
Expand Down