Outgoing contact requests #14853
Merged
Outgoing contact requests #14853
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 (145)
|
alwx
force-pushed
the
feature/contact-notifications
branch
from
8ad5f59
to
9ce6fa0
Compare
alwx
force-pushed
the
feature/contact-notifications
branch
from
ca3b4de
to
70e1fb6
Compare
This was referenced Jan 24, 2023
alwx
force-pushed
the
feature/contact-notifications
branch
from
a300b77
to
8eb7c46
Compare
ilmotta
reviewed
Jan 25, 2023
There was a problem hiding this comment.
for some mysterious reason it was supporting only two buttons OR status message ("Failed", "Pending", etc), and not both of them which is required by designs
@alwx There's no mysterious reason, up until this point, there was never a need to display buttons and status simultaneously, so the code ignored this requirement from the Design ;) Good thing in improved it now!
src/status_im2/contexts/activity_center/notification/contact_requests/view.cljs
Outdated
Show resolved
Hide resolved
src/status_im2/contexts/activity_center/notification/contact_requests/view.cljs
Outdated
Show resolved
Hide resolved
| :accessibility-label :decline-contact-request | ||
| :on-press (fn [] | ||
| (rf/dispatch [:activity-center.contact-requests/decline-request id]) | ||
| (rf/dispatch [:activity-center.notifications/mark-as-read |
There was a problem hiding this comment.
FYI: Recently @MishkaRogachev opened a PR in status-go to automatically mark notifications with call to action as read. Soon we'll be able to update all places in status-mobile. We recently had an issue due to concurrent dispatches to do X + mark as read.
src/status_im2/contexts/activity_center/notification/contact_requests/events.cljs
Show resolved
Hide resolved
J-Son89
reviewed
Jan 25, 2023
ilmotta
approved these changes
Jan 26, 2023
ilmotta
requested changes
Jan 26, 2023
There was a problem hiding this comment.
You need to run ./scripts/update-status-go.sh <revision>, so that the artifacts point to the branch this PR requires. Once the status-go branch is merged you can change from the commit hash to the actual tag in status-go.
Just a reminder that this PR should go over QA processes, given that it introduces new flows in the app. Reference: https://notes.status.im/M73LlV1gT3OJ8kdSMY-ZQQ
I would also strongly suggest you add to the PR's description the steps to test the new/affected user flows, as this is very helpful for the QA team.
|
@ilmotta done |
alwx
force-pushed
the
feature/contact-notifications
branch
from
ddcd85c
to
67bc036
Compare
ilmotta
approved these changes
Jan 26, 2023
65% of end-end tests have passedFailed tests (9)Click to expandClass TestActivityCenterMultipleDevicePR:
Class TestDeeplinkOneDeviceNewUI:
Class TestCommunityMultipleDeviceMerged:
Passed tests (17)Click to expandClass TestActivityCenterMultipleDevicePR:
Class TestGroupChatMediumMultipleDeviceNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
alwx
force-pushed
the
feature/contact-notifications
branch
from
8be3d60
to
6c835ea
Compare
alwx
force-pushed
the
feature/contact-notifications
branch
from
7be7127
to
017cff2
Compare
|
@pavloburykh done, please recheck. Issue 10 is fixed now (was not really an easy fix though 😅) |
46% of end-end tests have passedFailed tests (14)Click to expandClass TestActivityCenterMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (12)Click to expandClass TestDeeplinkOneDeviceNewUI:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterMultipleDevicePR:
|
62% of end-end tests have passedFailed tests (10)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (16)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterMultipleDevicePR:
Class TestDeeplinkOneDeviceNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@alwx thanx for working on this PR. It is tested and ready to be merged. |
This was referenced Feb 24, 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.
status-gochange: status-im/status-go#3120quo2.components.notifications.activity-log.view/view— for some mysterious reason it was supporting only two buttons OR status message ("Failed", "Pending", etc), and not both of them which is required by designs — the new API ofactivity-logmakes it easier to specify any components in any order, not just two buttons namedbutton-1andbutton-2status-im.contact.coretostatus-im2.contexts.activity-center.notification.contact-requests.eventsBasically this PR introduces everything that's specified in this Figma file EXCEPT failed contact requests (because the definition of what to count as failure is unclear, especially when we retry sending anyway) https://www.figma.com/file/eDfxTa9IoaCMUy5cLTp0ys/Shell-for-Mobile?node-id=4537%3A515620&t=EecPU3qz7E6eYOfa-0
Out of scope:
Read/unread indicators for messages (covered by this issue: #14415)
Review notes
Nothing crazy, most of the work is actually done in Go code.
Testing notes
Platforms
Areas that maybe impacted
Functional
Status: ready but requires status-im/status-go#3120 to be merged first