Sort communities by currently active tab #18812
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 (75)
|
cammellos
reviewed
Feb 14, 2024
| @@ -100,7 +100,10 @@ | |||
| :header [common.header-spacing/view] | |||
| :render-fn item-render | |||
| :style {:margin-top -1} | |||
| :data selected-items | |||
| :data (sort-by (fn [{:keys [last-opened-at joined-at]}] | |||
There was a problem hiding this comment.
should be done in subscription
ibrkhalil
commented
Feb 14, 2024
| previous-last-opened-at (get-in db [:communities id :last-opened-at])] | ||
| {:db (assoc-in db | ||
| [:communities id] | ||
| (assoc community :last-opened-at (max last-opened-at previous-last-opened-at))) |
There was a problem hiding this comment.
This is a fix to handle a race condition that happens between fetching the last community details and updating the last opened at key when opening a community.
ibrkhalil
changed the title
ibrkhalil
requested review from
cammellos,
Parveshdhull,
briansztamfater and
J-Son89
shivekkhurana
requested changes
Feb 15, 2024
src/status_im/subs/communities.cljs
Outdated
| @@ -107,7 +107,10 @@ | |||
| pending? (update acc :pending conj community) | |||
| :else (update acc :opened conj community)))) | |||
| {:joined [] :pending [] :opened []} | |||
| (vals communities))] | |||
| (sort-by (fn [{:keys [last-opened-at joined-at]}] | |||
There was a problem hiding this comment.
This reducer seems to be going out of control. Can you simplify it ?
(reduce (fn [acc community]
(let [joined? (:joined community)
community-id (:id community)
pending? (boolean (get requests community-id))]
(cond
joined? (update acc :joined conj community)
pending? (update acc :pending conj community)
:else (update acc :opened conj community))))
{:joined [] :pending [] :opened []}
(sort-by (fn [{:keys [last-opened-at joined-at]}]
(or last-opened-at joined-at))
#(compare %2 %1)
(vals communities)))I think we can get rid of the reducer completely. Something like this should be doable:
(->> communities
vals
(sort-by ...)
(group-by ...)
(merge-with ...))Links to relavent functions:
There was a problem hiding this comment.
Done, And thank you for your help on this Shivek!
0% of end-end tests have passedFailed tests (47)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
|
88% of end-end tests have passedFailed tests (5)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (42)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
ISSUE 1: Opened tab isn't sorted by the last opened if user opens communities from links in 1 to 1 chat Preconditions: User B (desktop) created 3 communities with required approve Steps:
Actual result: Most opened on the bottom video_2024-02-22_12-21-42.mp4Expected result: Most opened on top Devices: iOS/Android |
|
ISSUE 2: Most recent joined are not on the top Preconditions: User B (desktop) created 3 communities with required approve Steps:
Actual result:
So there is no "Most recent joined are not on the top" logic here Expected result: Most recent joined on the top Red |
|
@mariia-skrypnyk Both issues should be fixed now, Can you please take another look? |
12% of end-end tests have passedFailed tests (41)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (6)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
|
Done |
|
Hi @ibrkhalil ! Issue 2 is fixed! According to #17337 (comment) we need such implementation:
https://github.com/status-im/status-mobile/assets/147824399/649a63db-bbce-4e7a-bd01-98da3db86593
|
88% of end-end tests have passedFailed tests (5)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (42)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
|
Should be fixed now. |
88% of end-end tests have passedFailed tests (5)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (42)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
|
|
Hi @ibrkhalil ! Thanks for such a long sorting journey! |
Cater for requested to join at, To make the pending tap has the latest requested to join at community at the top
…ty existed on visitor side
ibrkhalil
changed the title
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 #17337
Summary
Sorts communities by last-opened-at property, And falls back to joined-at property if last-opened-at doesn't exist.
status: ready