Don't sync AC notifications, only decisions/state #17600
Conversation
Jenkins BuildsClick to see older builds (25)
|
84% of end-end tests have passedFailed tests (7)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (36)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
|
|
@qfrank thank you for the PR! Before we continue with testing could you please describe a scenario and expected behaviour to make it more clear? Thank you! |
Hi @pavloburykh , sorry for the confusing, basically, prepare 3 devices, 2 accounts let's say UserA/UserB:
|
|
Hi @qfrank thank you for PR. Take a look please a found issues: ISSUE 1: The State of an accepted community notification Is not synced when the community is acceptedPreconditions:
Steps to Reproduce:
Actual Result:On the synced My.Movie.1.mp4Expected Result:The acceptance of the community request should be synced to both devices in the AC |
ISSUE 2: The sent CR status is displayed as "pending" in AC on a synced device even if the current CR is acceptedPreconditions:
Steps to Reproduce:1. Actual Result:
Expected Result:The CR status on |
|
Hi @qfrank. I'm still a bit unclear about the requirements for the AC notifications in this PR. Specifically, I'd like to understand how data should be presented in the AC if Device1 already contains data, and then Device2 syncs with Device1. Is addressing this part of the PR scope? Right now, there appear to be some mismatches after syncing: Preconditions:Before
Steps:
Actual result:The following data is not shown in the AC for synced
The following read data is shown as unread for synced
Expected result:Should the existing data in AC be synced and displayed on |
Hi @qfrank I checked the same flow on the last develop build and observed the same behavior. After Device1 accepts the CR, the current CR is displayed as "Accepted" in the AC of synced Device2. It seems like it's worked in the same way on the nightly. Still a bit confused about which specific flow this is fixed in order to be tested |
@qfrank I rechecked this issue on the development build, and it's not reproducible there. It appears to be related to this specific PR. |
|
The current issue is not reproducible on the last nightly ISSUE 3: Display Name of synced user is not shown in message after current user sends message and is AC when user is mentionedPreconditions:
Steps to Reproduce:
Actual Result:When using Expected Result:The synced user should see their display name correctly when sending messages and checking mentions in the AC |
|
Thank you for your feedback! @VolodLytvynenko
We need more attention on business logic that how notification get changed(like accept/reject/read/unread notification) with the new way implemented in this PR, we don't sync notification now, we only sync |
ISSUE 4: Unread badge persists for Synced
|
|
The current issue is not reproducible on the last nightly ISSUE 5: 1-1 Chat does not appear for a synced user after CR Is Accepted until at least 1 message is sent/receivedPreconditions:
Steps to Reproduce:
Actual Result:The 1-1 chat with Expected Result:The 1-1 chat with The current issue is not reproducible on the last nightly |
ISSUE 6: The bell in the AC becomes unread colored for synced devices after removing a notification, and the count of notifications doesn't decreasePreconditions:
Steps:
Actual result:
My.Movie.5.mp4Expected result:The bell icon should remain gray (indicating no new messages) for |
@qfrank thank you for the explanation. Just to clarify, as far as I understood visual sync logic probably has not changed in this PR. and the goal of this PR is to improve the synchronization process and assess how it works in comparison to the development branch. Please correct me if I've misunderstood this |
|
Hi @VolodLytvynenko , i feed bad for the message reliability, |
visual sync logic changed a little bit:
|
Yes, message reliability sometimes wishes to be best, but I've never experienced a situation where a CR is not received at all in the current PR. I have encountered message delivery delays, but not this specific issue. I also noticed that on your first device, the user has an ENS name (if the user has an ENS, then the rings should not be shown on the avatar). However, on the second device, the rings are not present. Is it possible that you paired them using the old pairing flow, as shown in the screenshot below? This old pairing flow can be quite buggy and won't be used in the future. It's better to sync devices using the QR code scanning method. Probably this was a reason why you didn't receive the CR |
no , i used the QR code scanning method and didn't use the old pairing flow 🤦♂️ @VolodLytvynenko |
86% of end-end tests have passedNot executed tests (2)Failed tests (2)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (4)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Passed tests (37)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
Hi @qfrank thank you for fixes. Issue 2 is fixed. The issue 1 #17600 (comment) still reproduced if the . Steps are the same |
weired , i remember i tested it locally and passed, i'll retest, thank you |
qfrank
force-pushed
the
fix/sync_activity_center_notification
branch
from
177653d
to
37bc24a
Compare
|
Hi @VolodLytvynenko issue 1 should be fixed now, pls retry, thank you |
71% of end-end tests have passedFailed tests (10)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (32)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
78% of end-end tests have passedFailed tests (7)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (35)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
|
hi @qfrank thank you for your work. No issues from my side. PR can be merged |
qfrank
force-pushed
the
fix/sync_activity_center_notification
branch
from
37bc24a
to
b6b5ad3
Compare
status-im/status-go@d7e7792...74396b4 * fix outgoing contact request state not show correctly * update status-go-version.json
fixes issue
relate PR for status-go
statemeans if there exist unread notificationsdecisionmeans accept/dismiss/markAsXXX(read/unread/deleted)Testing notes
pls check if syncing AC notification works as expected
Platforms
status: ready