fix: 1:1 chat concerns #17795
Merged
fix: 1:1 chat concerns #17795
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
BalogunofAfrica
requested review from
flexsurfer,
yqrashawn,
OmarBasem and
ibrkhalil
Jenkins BuildsClick to see older builds (94)
|
BalogunofAfrica
force-pushed
the
fix/1-1-chat-concerns
branch
from
28a3abd to
4e56ec3
Compare
|
The |
BalogunofAfrica
force-pushed
the
fix/1-1-chat-concerns
branch
2 times, most recently
from
67b8c9c to
caa178a
Compare
|
Thanks for your review, @Francesca-G . I'd create a follow up issue for the sticky user details |
80% of end-end tests have passedFailed tests (6)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (36)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
|
BalogunofAfrica
force-pushed
the
fix/1-1-chat-concerns
branch
from
caa178a to
249b8e4
Compare
flexsurfer
requested changes
Nov 6, 2023
src/status_im/integration_test.cljs
Outdated
| @@ -179,8 +179,8 @@ | |||
| (logout!) | |||
| (rf-test/wait-for [::logout/logout-method])))))) | |||
|
|
|||
| (deftest delete-chat-test | |||
| (log/info "========= delete-chat-test ==================") | |||
| (deftest close-chat-test | |||
There was a problem hiding this comment.
could you pls elaborate on why we rename delete/remove to close
There was a problem hiding this comment.
The rename is due to the copy update from the designs: https://www.figma.com/file/7KIYbhoqNGAIFonE0w9TDz/Messages-for-Mobile?type=design&node-id=1447-349773&mode=design&t=OtI9WtlvD7M56ztS-0
There was a problem hiding this comment.
yeah, but it doesn't mean we should rename it in the code base, copy is enough, its still should have same functionality, or?
There was a problem hiding this comment.
The functionality changed, here's a link to the discuss:
https://discord.com/channels/624634427930312714/928625369542713396/1167063940816179271
| @@ -108,6 +108,7 @@ | |||
| :on-press #(hide-sheet-and-dispatch [:group-chats.ui/remove-chat-confirmed | |||
| chat-id])}])])))) | |||
|
|
|||
| ;; Still in use? Not referenced anywhere | |||
There was a problem hiding this comment.
feel free to remove anything which is not used anymore
| [{:keys [db now] :as cofx} chat-id] | ||
| (rf/merge cofx | ||
| {:effects/push-notifications-clear-message-notifications [chat-id] | ||
| :dispatch [:shell/close-switcher-card | ||
| chat-id]} | ||
| (deactivate-chat chat-id) | ||
| (offload-messages chat-id))) | ||
| (remove-chat chat-id))) |
There was a problem hiding this comment.
could you elaborate on this change, it looks pretty critical
There was a problem hiding this comment.
The requirement for the destructive actions in the context drawer for 1:1 chat was changed. One of the actions was a delete message, but it was changed to close message where the message is just removed from the chat list while still retaining history.
This was communicated here: https://discord.com/channels/624634427930312714/928625369542713396/1167063940816179271
There was a problem hiding this comment.
mhm i think we already had something similar cc @churik
There was a problem hiding this comment.
and we have like active? flag or something
There was a problem hiding this comment.
actually i looked at the code, and it seems like there should be no changes only copy should be changed, because it already works as expected
There was a problem hiding this comment.
I don't see the history after I delete it and try chatting with the other device again, even after closing the app and re-opening 🤔
There was a problem hiding this comment.
so there are two parts where we have data, react and status-go, in your PR as soon as you relogin chat will be back, im not sure that is expected, so i would say current version in develop works as expected
There was a problem hiding this comment.
I am not sure I understand 🤔, here's my experience with develop:
Screen.Recording.2023-11-06.at.12.37.04.mov
I don't get to see the messages when I go back to chat with the user who I closed(deleted) their chat, but the second point in the new requirement noted that we should be able to see their chat history, just that it would be removed from the list of chats
There was a problem hiding this comment.
oh right, probably we have something like deleted-at and we don't load messages, this should be changed then, it's more complicated changes needed, we need to see how deactivated method works in status-go
There was a problem hiding this comment.
@flexsurfer Updated the DeactivateChat method from status-go.
Link to that PR here: status-im/status-go#4324
flexsurfer
reviewed
Nov 6, 2023
translations/en.json
Outdated
| "delete-chat": "Delete chat", | ||
| "delete-chat-confirmation": "Are you sure you want to delete this chat?", | ||
| "close-chat": "Close chat", | ||
| "close-chat?": "Confirm", |
There was a problem hiding this comment.
we already have "confirm": "Confirm",
flexsurfer
reviewed
Nov 6, 2023
| "delete-chat": "حذف گفتگو؟", | ||
| "delete-chat-confirmation": "آیا مطمئنید که می خواهید این چت را حذف کنید ؟", | ||
| "delete-chat?": "حذف گفتگو؟", | ||
| "close-chat": "حذف گفتگو؟", |
There was a problem hiding this comment.
@jinhojang6 what do you think, is it better to keep key or its better to rename key as well in such cases?
BalogunofAfrica
force-pushed
the
fix/1-1-chat-concerns
branch
2 times, most recently
from
caaf53a to
f6c0ec9
Compare
|
Hey @BalogunofAfrica. Please don't merge the current PR. We've encountered an issue on the status go side (#18092) merging this PR affects our mobile develop |
|
Hey @BalogunofAfrica could you rebase status go https://github.com/status-im/status-go/tree/fix/deactivate-chat and current PR. I quickly rerun tests once more before merging this PR. Thanx! |
BalogunofAfrica
force-pushed
the
fix/1-1-chat-concerns
branch
from
7c91fca to
1525d8e
Compare
|
Hi @VolodLytvynenko I have updated the PR |
65% of end-end tests have passedFailed tests (10)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Expected to fail tests (7)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (32)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
@BalogunofAfrica thank you for your work. PR is ready to be merged |
80% of end-end tests have passedFailed tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (7)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (39)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
BalogunofAfrica
force-pushed
the
fix/1-1-chat-concerns
branch
from
1525d8e to
22c0607
Compare
BalogunofAfrica
force-pushed
the
fix/1-1-chat-concerns
branch
from
22c0607 to
4ca35ef
Compare
This was referenced Dec 11, 2023
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
#17539
#17912
#17913
Summary
Fixes the design concerns in #17539, and also implements the changes to the context drawer for 1:1 chat seen here: https://www.figma.com/file/7KIYbhoqNGAIFonE0w9TDz/Messages-for-Mobile?type=design&node-id=1447-349773&mode=design&t=faJfVMrRatdEVuv1-0.
Review notes
Testing notes
Platforms
Areas that maybe impacted
Functional
status: ready