Communities now correctly listed in pending/joined sections. #14515

vkjr · 2022-12-09T15:27:01Z

Summary

all communities are visible now, not only Status
fixed tabs pending/joined/opened to correctly show communities
added "node offline" information box from Communities join screens (Non token gated) - Add owner node offline information box #14456

Review notes

The behavior of added information box was not really defined (agreed to wait for John on the design meeting), so its adding doesn't really close an issue. Moreover, you won't see it in UI because currently, when you join an open community, it is treated as fully joined even if the user didn't have a network connection at the moment of joining. Not sure if this behavior is correct. @churik, maybe you know. If not, we can probably open a bug.

Platforms

Android
iOS

status: ready

status-im-auto · 2022-12-09T15:30:49Z

Jenkins Builds

Click to see older builds (20)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`e6b9508`	#1	2022-12-09 15:30:48	~3 min	`tests`	📄`log`
✔️	`e6b9508`	#1	2022-12-09 15:34:53	~7 min	`android`	🤖`apk` 📲
✔️	`e6b9508`	#1	2022-12-09 15:34:54	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`e6b9508`	#1	2022-12-09 15:45:13	~17 min	`ios`	📱`ipa` 📲

✔️	`67ac994`	#2	2022-12-12 17:09:24	~3 min	`tests`	📄`log`
✔️	`67ac994`	#2	2022-12-12 17:14:11	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`67ac994`	#2	2022-12-12 17:14:33	~8 min	`android`	🤖`apk` 📲
✔️	`67ac994`	#2	2022-12-12 17:24:21	~18 min	`ios`	📱`ipa` 📲

✔️	`ce7f586`	#3	2022-12-12 17:45:05	~2 min	`tests`	📄`log`
✔️	`ce7f586`	#3	2022-12-12 17:50:16	~7 min	`android`	🤖`apk` 📲
✔️	`ce7f586`	#3	2022-12-12 17:50:21	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`ce7f586`	#3	2022-12-12 17:57:39	~15 min	`ios`	📱`ipa` 📲

❌	`c3ff1d1`	#4	2022-12-12 18:15:18	~1 min	`tests`	📄`log`
✔️	`c3ff1d1`	#4	2022-12-12 18:21:47	~8 min	`android-e2e`	🤖`apk` 📲
✔️	`c3ff1d1`	#4	2022-12-12 18:22:00	~8 min	`android`	🤖`apk` 📲
✔️	`c3ff1d1`	#4	2022-12-12 18:33:12	~19 min	`ios`	📱`ipa` 📲

✔️	`7e2120e`	#5	2022-12-13 10:09:28	~2 min	`tests`	📄`log`
✔️	`7e2120e`	#5	2022-12-13 10:15:18	~8 min	`android-e2e`	🤖`apk` 📲
✔️	`7e2120e`	#5	2022-12-13 10:17:29	~10 min	`android`	🤖`apk` 📲
✔️	`7e2120e`	#5	2022-12-13 10:24:57	~17 min	`ios`	📱`ipa` 📲

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`2a2f926`	#6	2022-12-13 13:02:08	~3 min	`tests`	📄`log`
✔️	`2a2f926`	#6	2022-12-13 13:06:04	~7 min	`android`	🤖`apk` 📲
✔️	`2a2f926`	#6	2022-12-13 13:06:22	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`2a2f926`	#6	2022-12-13 13:17:47	~19 min	`ios`	📱`ipa` 📲

✔️	`51cf65c`	#7	2022-12-13 20:14:43	~2 min	`tests`	📄`log`
✔️	`51cf65c`	#7	2022-12-13 20:20:19	~7 min	`android`	🤖`apk` 📲
✔️	`51cf65c`	#7	2022-12-13 20:20:25	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`51cf65c`	#7	2022-12-13 20:29:19	~16 min	`ios`	📱`ipa` 📲

vkjr · 2022-12-09T15:38:05Z

@J-Son89, as I mentioned in review notes, this one doesn't really fix the issue, that it was originally expected to fix, so I'm not sure if I should add the "fixes" header, wdyt?

J-Son89 · 2022-12-09T15:38:37Z

src/status_im2/contexts/communities/overview/view.cljs

@@ -159,8 +159,31 @@

 (def channel-list-component (memoize channel-list-component-fn))

+(defn join-community [{:keys [joined can-join? requested-to-join-at community-color] :as community}]


J-Son89 · 2022-12-09T15:40:57Z

src/status_im2/subs/communities.cljs

@@ -105,6 +105,28 @@
 (fn [communities]
   (map :id communities)))

+
+;; Return communities splitted by level of user participation. Some communities user


Can this comment not go as a doc string in the function?
e.g

(re-frame/reg-sub :communities/community-ids-by-user-involvement :<- [:communities/communities] "doc string goes here" ...

also imo we only really need the last line
Result map has form: {:joined [id1, id2] :pending [id3, id5] :opened [id4]}"

Unfortunately, docstring doesn't work here, because it is a function call, and looks like reg-sub doesn't support docstring (I tried).
Also, I wasn't able to remove newlines between the description and code - linter argues.

As for description, I would say that mentioning that opened communities remain in database can add a piece of information for the reader

@vkjr, you're right about the docstring, we can't use it here, but could you join the comments with the reg-sub call? There are two empty lines between them.

@ilmotta, for some reason linter returns an error when I put the comments right before the code :-/

Such a weird recommendation from cljfmt! You're totally right, I guess if you prefer you can always add the comments at the top of the anonymous function, but not much of an improvement 🤷‍♂️

(re-frame/reg-sub :communities/community-ids-by-user-involvement :<- [:communities/communities] ;; Return communities splitted by level of user participation. Some communities ;; user already joined, to some of them join request sent and others were opened ;; one day and their data remained in app-db. Result map has form: ;; {:joined [id1, id2] :pending [id3, id5] :opened [id4]}" (fn [communities] (reduce (fn [{:keys [joined pending open] :as acc} community] (let [joined? (:joined community) requested? (pos? (:requested-to-join-at community)) id (:id community)] (cond joined? (assoc acc :joined (conj joined id)) requested? (assoc acc :pending (conj pending id)) :else (assoc acc :opened (conj open id))))) {:joined [] :pending [] :opened []} communities)))

Hmm, nice idea! For me, it really seems like the best option! I would go that way)

flexsurfer · 2022-12-09T15:54:56Z

translations/en.json

@@ -1894,4 +1895,4 @@
    "remove-user-from-group": "Remove {{username}} from the group",
    "edit-name-image": "Edit name and image",
    "owner": "Owner"
-}
+}


could you please rollback new line

flexsurfer · 2022-12-09T15:56:09Z

src/status_im2/contexts/communities/overview/view.cljs

+     (when-not joined
+       [quo/button
+        {:on-press  #(rf/dispatch [:bottom-sheet/show-sheet
+                                   {:content (constantly [requests.actions/request-to-join community])


why constantly? its hard to read here

This part was extracted as-is from a bigger component)

could you change it please?

Humm, let's not generalize that constantly is hard to read. This is just personal preference. I find it quite clear in many contexts, and it fits like a glove in this piece of code. But of course I'm okay with switching to just (fn [] [requests.actions/request-to-join community])

i mean we've never used constantly so im not used to it, so we need to use the same pattern for all such cases in the code, constantly is good in java i guess , to safely have a function with any number of arguments if you don't use them, in javascript we don't care, so constantly is useless

flexsurfer · 2022-12-09T15:56:31Z

src/status_im2/contexts/communities/overview/view.cljs

+                                    :content-height 300}])
+         :override-background-color community-color
+         :style
+         {:width "100%"


move to styles ns

flexsurfer · 2022-12-09T15:57:46Z

src/status_im2/subs/communities.cljs

+                   requested? (pos? (:requested-to-join-at community))
+                   id (:id community)]
+               (cond
+                 joined?    (assoc acc :joined (conj joined id))


update could be used

ilmotta · 2022-12-09T19:05:00Z

src/status_im2/contexts/communities/home/view.cljs


       :opened
-       [communities-list communities])]))
+       [communities-list (:opened communities-ids)])]))


case without a default branch will throw an exception. If you're sure the tab is always within the valid set of branches, another alternative is to remove case and just use the following:

[communities-list (get communities-ids tab)]

I would prefer not to rely on using the same keywords for naming tabs and for naming vectors of communities, since they have different logical meanings and can change in the future. So I would add the :default section with simple error indication to visually catch this case. Wdyt?

:default [quo/information-box {:type :error :icon :i/info} (i18n/label :t/error)] ```

Makes total sense to me @vkjr. Thanks

ilmotta · 2022-12-09T19:07:03Z

src/status_im2/contexts/communities/home/view.cljs

@@ -46,20 +46,20 @@
    :render-fn                         render-fn}])

 (defn segments-community-lists [selected-tab]
-  (let [communities (rf/sub [:communities/community-ids])
+  (let [communities-ids (rf/sub [:communities/community-ids-by-user-involvement])


Whenever I read <something>-ids, I think it's a collection (it's very conventional), but in reality it's a map indexed by the user "involvement". I would suggest renaming this binding to something like ids-by-user-involvement.

ilmotta · 2022-12-09T19:09:43Z

src/status_im2/contexts/communities/overview/view.cljs

+     (when node-offline?
+       [quo/information-box
+        {:type :informative
+         :icon :main-icons/info


[Guidelines] Use the :i prefix instead of :main-icons.

ilmotta · 2022-12-09T19:12:05Z

src/status_im2/contexts/communities/overview/view.cljs

+        {:type :informative
+         :icon :main-icons/info
+         :style {:margin-top 12}}
+        (i18n/label :request-processed-after-node-online)])]))


[Guidelines] Use the :t qualification in translation keywords.

ilmotta · 2022-12-09T19:22:17Z

src/status_im2/subs/communities.cljs

+
+
+(re-frame/reg-sub
+ :communities/community-ids-by-user-involvement


How about tests for this new layer-3 subscription? It's begging for some! If it's a new layer-3 subscription such as this one, we should properly test it. Feel free to ping me if it's not clear how to test this. You can find examples in:

subs/communities_test.cljs

subs/wallet/wallet_test.cljs

subs/activity_center_test.cljs

On a side note, I actually think soon we should re-evaluate our minimum requirements for test coverage in PRs.

ilmotta · 2022-12-12T17:31:07Z

src/status_im2/contexts/communities/home/view.cljs

-       [communities-list communities])]))
+       [communities-list (:opened ids-by-user-involvement)]
+
+       :default


Just a minor correction here @vkjr, the default branch in the case macro doesn't take an option, only the cond macro understands that.

;; This explodes :boom: (let [tab :unknown] (case tab :joined :joined :pending :pending :opened :opened :default :error)) (let [tab :unknown] (cond (= tab :joined) :joined (= tab :pending) :pending (= tab :opened) :opened :default :error))

🤦 , thanks for noticing!

flexsurfer · 2022-12-12T17:54:50Z

src/status_im2/contexts/communities/overview/style.cljs

+
+(def node-offline-information-box
+  {:type :informative


we should have only styles in style ns, not properties

flexsurfer · 2022-12-12T17:55:03Z

src/status_im2/contexts/communities/overview/style.cljs

+(def blur-channel-header
+  {:blur-amount 32


we should have only styles in style ns, not properties

status-im-auto · 2022-12-13T12:09:49Z

100% of end-end tests have passed

Total executed tests: 8
Failed tests: 0
Passed tests: 8
Not executed tests: 2

IDs of not executed tests: 702807,702808

Not executed tests (2)

Click to expand

Rerun not executed tests

Passed tests (8)

Click to expand

Class TestDeeplinkOneDeviceNewUI:

1. test_deep_link_with_invalid_user_public_key_own_profile_key, id: 702774
Device sessions Device 1: Steps, video, logs
2. test_public_chat_open_using_deep_link, id: 702776
Device sessions Device 1: Steps, video, logs
3. test_deep_link_open_user_profile, id: 702775
Device sessions Device 1: Steps, video, logs

Class TestOneToOneChatMultipleSharedDevicesNewUi:

1. test_1_1_chat_pin_messages, id: 702731
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
2. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
3. test_1_1_chat_message_reaction, id: 702730
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
4. test_1_1_chat_text_message_delete_push_disappear, id: 702733
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

Class TestGroupChatMediumMultipleDeviceNewUI:

1. test_group_chat_pin_messages, id: 702732
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

status-im-auto · 2022-12-13T17:02:49Z

100% of end-end tests have passed

Total executed tests: 8
Failed tests: 0
Passed tests: 8
Not executed tests: 2

IDs of not executed tests: 702807,702808

Not executed tests (2)

Click to expand

Rerun not executed tests

Passed tests (8)

Click to expand

Class TestOneToOneChatMultipleSharedDevicesNewUi:

1. test_1_1_chat_text_message_delete_push_disappear, id: 702733
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
2. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
3. test_1_1_chat_message_reaction, id: 702730
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs
4. test_1_1_chat_pin_messages, id: 702731
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

Class TestGroupChatMediumMultipleDeviceNewUI:

1. test_group_chat_pin_messages, id: 702732
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

Class TestDeeplinkOneDeviceNewUI:

1. test_public_chat_open_using_deep_link, id: 702776
Device sessions Device 1: Steps, video, logs
2. test_deep_link_with_invalid_user_public_key_own_profile_key, id: 702774
Device sessions Device 1: Steps, video, logs
3. test_deep_link_open_user_profile, id: 702775
Device sessions Device 1: Steps, video, logs

vkjr added the communities label Dec 9, 2022

vkjr requested review from ilmotta, flexsurfer, briansztamfater and J-Son89 December 9, 2022 15:27

vkjr self-assigned this Dec 9, 2022

status-github-bot bot added this to REVIEW in Pipeline for QA Dec 9, 2022

J-Son89 reviewed Dec 9, 2022

View reviewed changes

J-Son89 approved these changes Dec 9, 2022

View reviewed changes

flexsurfer reviewed Dec 9, 2022

View reviewed changes

ilmotta reviewed Dec 9, 2022

View reviewed changes

vkjr force-pushed the 14456_node_offline_box branch from e6b9508 to 67ac994 Compare December 12, 2022 17:06

ilmotta approved these changes Dec 12, 2022

View reviewed changes

vkjr requested a review from flexsurfer December 12, 2022 17:25

ilmotta reviewed Dec 12, 2022

View reviewed changes

vkjr force-pushed the 14456_node_offline_box branch from 67ac994 to ce7f586 Compare December 12, 2022 17:42

flexsurfer approved these changes Dec 12, 2022

View reviewed changes

vkjr force-pushed the 14456_node_offline_box branch 2 times, most recently from c3ff1d1 to 7e2120e Compare December 13, 2022 10:06

vkjr moved this from REVIEW to E2E Tests in Pipeline for QA Dec 13, 2022

vkjr force-pushed the 14456_node_offline_box branch from 7e2120e to 2a2f926 Compare December 13, 2022 12:58

vkjr moved this from E2E Tests to CONTRIBUTOR in Pipeline for QA Dec 13, 2022

vkjr moved this from CONTRIBUTOR to E2E Tests in Pipeline for QA Dec 13, 2022

Communities now correctly listed in pending/joined sections.

51cf65c

vkjr force-pushed the 14456_node_offline_box branch from 2a2f926 to 51cf65c Compare December 13, 2022 20:12

vkjr merged commit 68ea7cd into develop Dec 13, 2022

Pipeline for QA automation moved this from E2E Tests to DONE Dec 13, 2022

vkjr deleted the 14456_node_offline_box branch December 13, 2022 20:55

du82 mentioned this pull request Dec 13, 2022

Communities join screens (Non token gated) - Add owner node offline information box #14456

Closed

yevh-berdnyk pushed a commit that referenced this pull request Dec 15, 2022

Communities now correctly listed in pending/joined sections. (#14515)

a08df48

		@@ -159,8 +159,31 @@

		(def channel-list-component (memoize channel-list-component-fn))

		(defn join-community [{:keys [joined can-join? requested-to-join-at community-color] :as community}]



		(re-frame/reg-sub
		:communities/community-ids-by-user-involvement

Communities now correctly listed in pending/joined sections. #14515

Communities now correctly listed in pending/joined sections. #14515

Conversation

vkjr commented Dec 9, 2022 • edited Loading

Summary

Review notes

Platforms

status-im-auto commented Dec 9, 2022 • edited Loading

Jenkins Builds

vkjr commented Dec 9, 2022

Choose a reason for hiding this comment

J-Son89 Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flexsurfer Dec 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilmotta Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

status-im-auto commented Dec 13, 2022

100% of end-end tests have passed

Not executed tests (2)

Passed tests (8)

Class TestDeeplinkOneDeviceNewUI:

Class TestOneToOneChatMultipleSharedDevicesNewUi:

Class TestGroupChatMediumMultipleDeviceNewUI:

status-im-auto commented Dec 13, 2022

100% of end-end tests have passed

Not executed tests (2)

Passed tests (8)

Class TestOneToOneChatMultipleSharedDevicesNewUi:

Class TestGroupChatMediumMultipleDeviceNewUI:

Class TestDeeplinkOneDeviceNewUI:

vkjr commented Dec 9, 2022 •

edited

Loading

status-im-auto commented Dec 9, 2022 •

edited

Loading

J-Son89 Dec 9, 2022 •

edited

Loading

flexsurfer Dec 10, 2022 •

edited

Loading

ilmotta Dec 12, 2022 •

edited

Loading