-
Notifications
You must be signed in to change notification settings - Fork 985
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.
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
Add lastOpenedAt key to communities #18333
Conversation
Jenkins Builds
|
79% of end-end tests have passed
Failed tests (5)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (38)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
[id] | ||
(let [id (or id (rf/sub [:get-screen-params :community-overview])) | ||
customization-color (rf/sub [:profile/customization-color])] | ||
(rn/use-effect #(rf/dispatch [:communities/update-last-opened-at id]) [id]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ibrkhalil, Thank you very much for your PR.
Maybe we can create another event, something like :communities/navigate-to-community
(similar to :chat/navigate-to-chat
) and handle this update there. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the only benefit for that would be better naming, Will do.
@@ -169,6 +170,13 @@ | |||
{} | |||
categories))})) | |||
|
|||
(rf/reg-event-fx :communities/update-last-opened-at | |||
(fn [_ [community-id]] | |||
{:json-rpc/call [{:method "wakuext_updateLastOpenedAt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, probably we should rename this end point. It somewhat gives idea that we are providing time at which community is opened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Samuel mentioned in original issue, maybe something like CommunityOpened
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about wakuext_communityUpdateLastOpenedAt
Hey @ibrkhalil! What is the status of this PR? I see that it is not approved and remains in E2E column for couple of weeks already. Once you get PR approval, please rebase it (both mobile and status go branches) and add one of the labels (request manual qa or skip manual qa). In case manual qa is not required - we still need to rerun e2e tests after rebase. In case manual qa is required - please add testing notes in the PR description. Thank you. |
Closed in favor of: #18439 |
Required for #17337
Adds the last opened at key to communities to know when was a community last opened (This is done currently for overview pages, Internal links not tested yet.)
status: ready