Fix replies & pinned messages #15689
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 (63)
|
cammellos
commented
Apr 19, 2023
| [quo2.core :as quo] | ||
| [status-im2.contexts.chat.messages.avatar.style :as style])) | ||
|
|
||
| (defn avatar [from size] |
There was a problem hiding this comment.
"Connected" component https://react-redux.js.org/api/connect , so it can be reused in multiple places without having to subscribe everywhere
| @@ -32,16 +32,29 @@ | |||
| ;; TODO this is too expensive, probably we could mark message somehow and just hide it in the UI | |||
| (message-list/rebuild-message-list {:db (update-in db [:messages chat-id] dissoc message-id)} chat-id)) | |||
|
|
|||
| (defn add-pinned-message [acc chat-id message-id message] | |||
There was a problem hiding this comment.
We check if the message is pinned so we can update it correctly.
cammellos
force-pushed
the
bug/reply-pinned-messages-fixes
branch
3 times, most recently
from
3cd2980 to
d729408
Compare
cammellos
force-pushed
the
bug/reply-pinned-messages-fixes
branch
2 times, most recently
from
ac20367 to
e30823c
Compare
cammellos
force-pushed
the
bug/reply-pinned-messages-fixes
branch
from
e30823c to
3fbc185
Compare
cammellos
commented
Apr 20, 2023
| [status-im2.contexts.chat.messages.avatar.style :as style])) | ||
|
|
||
| (defn avatar | ||
| [from size] |
There was a problem hiding this comment.
oh yes, public-key probably better?
J-Son89
reviewed
Apr 20, 2023
cammellos
force-pushed
the
bug/reply-pinned-messages-fixes
branch
from
3fbc185 to
93dc328
Compare
|
I checked out the branch and played around a bit with pinned messages. Looks good to me 👍 |
J-Son89
reviewed
Apr 20, 2023
cammellos
force-pushed
the
bug/reply-pinned-messages-fixes
branch
from
93dc328 to
5cd1f9b
Compare
J-Son89
approved these changes
Apr 20, 2023
flexsurfer
reviewed
Apr 20, 2023
flexsurfer
approved these changes
Apr 20, 2023
ilmotta
approved these changes
Apr 20, 2023
33% of end-end tests have passedNot executed tests (26)Failed tests (2)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (1)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
|
cammellos
force-pushed
the
bug/reply-pinned-messages-fixes
branch
from
5cd1f9b to
e1bf52a
Compare
79% of end-end tests have passedNot executed tests (10)Failed tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Passed tests (15)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
83% of end-end tests have passedFailed tests (5)Click to expandClass TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (24)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@cammellos ISSUE 8: when tapping on pinned message in the bottom sheet, there is no "Unpin" optionSteps:
Expected result: FILE.2023-04-24.17.13.26.mp4OS: IOS, Android |
cammellos
force-pushed
the
bug/reply-pinned-messages-fixes
branch
from
136ba12 to
4e8a3f6
Compare
|
@churik should be fixed in the last build, thank you! |
83% of end-end tests have passedFailed tests (5)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (24)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
20% of end-end tests have passedFailed tests (4)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (1)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
@cammellos |
|
@churik I'll fix it, I missed it :) |
cammellos
force-pushed
the
bug/reply-pinned-messages-fixes
branch
2 times, most recently
from
abecbb6 to
ed776e4
Compare
|
@churik it should be fixed, I scheduled a re-run of the tests, thank you |
50% of end-end tests have passedFailed tests (2)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Passed tests (2)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@cammellos ISSUE 9: logs sending is not really working on Android, and the intended mail window is not opened instantly on IOS as wellAndroid: FILE.2023-04-25.15.27.09.mp4IOS: FILE.2023-04-25.15.29.41.mp4 |
|
@churik do you have status logs for this issue? Thanks! |
|
It works on nightly; let me build latest develop just in case |
|
built the latest develop; it works there and doesn't work in the current PR |
|
UPD: issue 9 could not reproduce (magic???) ISSUE 10: error on single tap on a message from the pinned bottom sheet (IOS only)Steps:
Actual result:
OS: IOS Logs: |
|
Issue 10 will be logged and addressed separately. |
status-im/status-go@e8ceed1...213dc46 Both replies and pinned messages relied on subscribing their data from messages. This worked only as long as we loaded the message in the database, so it would break say if another user replied to a message that wasn't in the current user view. It also changes the way pinned messages are handled, before the notification was actually sent over the wire, but that's unnecessary, since it can be generated locally on both parts. This is a bit of a breaking change with the previous version, since if you pin a message with this version, older version will not see a system message. this can be easily fixed by restoring the previous behavior of sending the message, but not sure it's worth it. It also adds the ability to Delete message for everyone that have Deleted for me (discussed with John) and the ability of unpin messages that have been deleted for me.
cammellos
force-pushed
the
bug/reply-pinned-messages-fixes
branch
from
ed776e4 to
2438af4
Compare
|
Thank you @VladimrLitvinenko @churik !!! |
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.
Both replies and pinned messages relied on subscribing their data from
messages.This worked only as long as we loaded the message in the database, so it would break say if another user replied to a message that wasn't in the current user view.
It also changes the way pinned messages are handled, before the notification was actually sent over the wire, but that's unnecessary, since it can be generated locally on both parts.
This is a bit of a breaking change with the previous version, since if you pin a message with this version, older version will not see a system message. this can be easily fixed by restoring the previous behavior of sending the message, but not sure it's worth it.
it also adds the ability to Delete message for everyone that have Deleted for me (discussed with John) and the ability of unpin messages that have been deleted for me.
I will add a couple of unit tests to validate the new features, but in the meantime I would like to get it through the testing pipeline as likely there will be some issues to address.
status-go PR status-im/status-go#3414
I wanted to move the UI code, but it's already quite a few changes so I wanted to minimize disruptions.
Area to tests (communities/group-chat/1-to-1):
I'll try to address this in this PR, but I think it should be ok to address separately, if qa is happy with it.