-
Notifications
You must be signed in to change notification settings - Fork 992
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
Various fixes for Contact Request flows (2nd attempt) #15685
Conversation
Jenkins BuildsClick to see older builds (12)
|
bba48e1
to
6fdfd48
Compare
93% of end-end tests have passed
Failed tests (2)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Passed tests (27)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
|
Hi @ilmotta, could you update the |
6fdfd48
to
af6a59d
Compare
@qoqobolo, just rebased :) Thanks for reviewing this PR. I might be a bit slower today to reply, since I'm off. It's worth mentioning that in PRs like this one, where the dev has changed |
Totally makes sense, thanks for the detailed explanation as always @ilmotta! Regarding the PR itself, found one issue so far that looks like a regression (testing is still in progress). Please take a look when you have time. ISSUE 1:
|
@qoqobolo, I swear I've seen ISSUE 1 happen in other branches a couple times, since I wasn't surprised when I read you reported ISSUE 1. I tried to reproduce now in
I wouldn't know that without further investigation @qoqobolo. Tbh I don't know what's the expected behavior either. |
Okay, now we have three points that need clarification/rechecking. 1. ISSUE 1
Hmm, never seen this before the current PR. Asked other QAs, and they didn't see that either. And two mismatches between the behavior in current PR and the status-go PR: 2. Chat appears for both users after becoming mutual contacts @MishkaRogachev just fixed this issue in the status-go PR. @ilmotta could you update this PR so I can test the fix on our side, please? 3. No default CR message in chat should appear (currently, the default CR message is still displayed at the top of the history for both users on mobile) @cammellos could you help us define what is the expected behavior for mobile right now? Other than that, I didn't find any other issues, the PR looks good. |
@qoqobolo 3 can be addressed separately, we can clarify the behavior with the UX team. 2 should be addressed here, since @MishkaRogachev already made the changes I believe 1 probably we want to understand why it happens if we are to merge and just figure out if it's due to this PR or no |
Thank you @cammellos |
@ilmotta @MishkaRogachev regarding 1) I believe the code is using This is probably ok for toasts, since they are very ephemeral, but not ok for permanent notifications, as the name change (not sure whether we use it or not actually, but worth checking). But this indicates that Thank you |
Sure, i will check that notification's name matches |
b71c0fe
to
9ef84d1
Compare
You're right @qoqobolo, maybe it was really just my mind playing tricks, or probably I've seen this issue while creating this PR and thought it existed in @MishkaRogachev and @qoqobolo, I've updated this PR to point to the latest revision from the status-go PR and tested on two devices. It seems ISSUE 1 is indeed fixed. I haven't tested the other issues though. Thanks @cammellos for helping us understand what to do. |
I suppose there can be more similar cases, when we expect |
93% of end-end tests have passed
Failed tests (2)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Passed tests (27)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
Confirming that Issue 1 is fixed and chats appear on both sides now. |
9ef84d1
to
c6af945
Compare
Fixes #15679
Summary
A quick and dirty PR that serves two purposes:
wakuext_addContact
withwakuext_sendContactRequest
. This is what's been aligned with the desktop team, as we will go on to removewakuext_addContact
in the near future.Note: Once this PR has been approved and tested by QAs, the status-go PR will be merged first, business as usual, and I'll update the
status-go-version.json
file to point to an actual status-go tag.Out of scope
Refactoring; improvements in the existing code; UX.
Platforms
Steps to test (for QAs)
Contact request flows may be impacted. The status-go PR status-im/status-go#3379 describes the changes being introduced, but perhaps for good measure all important CR flows should be double-checked in mobile.
status: ready