Implement reviewal of contact requests from contact profile #19119
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
seanstrom
force-pushed
the
seanstrom/contact-profile-contact-request-received
branch
from
87e2571
to
356b0f4
Compare
Jenkins BuildsClick to see older builds (18)
|
seanstrom
commented
Mar 6, 2024
| @@ -44,3 +44,4 @@ | |||
| :button-one-props {:disabled? (string/blank? message) | |||
| :on-press on-message-submit} | |||
| :button-one-label (i18n/label :t/send-contact-request)}]])) | |||
|
|
|||
There was a problem hiding this comment.
🧹 End of file whitespace is being added by formatter
Comment on lines
9
to
24
| (defn view | ||
| [] | ||
| (let [{:keys [public-key customization-color] | ||
| :as profile} (rf/sub [:contacts/current-contact]) | ||
| {contact-request-id :id | ||
| :as contact-request} (rf/sub [:activity-center/contact-request-from-contact-id public-key]) | ||
| ;; TODO: remove :blue when #18733 merged. | ||
| customization-color (or customization-color :blue) | ||
| full-name (profile.utils/displayed-name profile) | ||
| profile-picture (profile.utils/photo profile) | ||
| on-contact-accept (rn/use-callback (fn [] | ||
| (rf/dispatch [:hide-bottom-sheet]) | ||
| (rf/dispatch [:activity-center.contact-requests/accept | ||
| contact-request-id])) | ||
| [contact-request-id]) | ||
| on-contact-ignore (rn/use-callback (fn [] |
There was a problem hiding this comment.
Here I'm using the activity-center subscription and events for accessing the contact-request data. It may be useful to refactor this to access contact-requests from a separate event/sub namespace that manages all contact requests directly. But I'm not sure if that's necessary since activity-center seems to be doing that.
There was a problem hiding this comment.
I don't have any strong opinion about it but maybe @ilmotta any thought?
There was a problem hiding this comment.
We need to be a little bit careful with this piece of data.
Pending contact requests are actually fetched from notifications. Back when I wrote that part I added a long docstring explaining why that works and why it's not the ideal solution. See
status-mobile/src/status_im/contexts/shell/activity_center/events.cljs
Lines 411 to 418 in 62f68ff
So just be aware that grabbing a specific contact request from what's stored in the app-db has limitations, but should be fine for now (unless the user has "lots" of pending contact request notifications, i.e. more than we paginate).
There was a problem hiding this comment.
Thank you for providing this context 🙌
I think this PR assumes the subscribed contact-request is pending, but I can try naming the subscription something like pending-contact-request-from-contact-id so the reader knows this limitation, thoughts?
Also do you think I should create a separate issue for possibly rewiring the way contact requests are stored? For example, maybe we can store them in a map that's indexed by contact-id? I suppose that would assume we can only have one contact-request from a person at a time though.
There was a problem hiding this comment.
Something I'm just noticing is that when we try to supporting dismissing a notification, we'll need to support retrieving dismissed contact-requests. But I think from what you mention, that currently is not possible because the contact request will be marked as read when dismissed, and then removed from the activity-center.
Am I understanding this correctly?
There was a problem hiding this comment.
@seanstrom, one thing that could clarify things a bit is that we intentionally descoped managing pending contact requests (see the feature in Figma https://www.figma.com/file/7KIYbhoqNGAIFonE0w9TDz/Messages-for-Mobile?type=design&node-id=2010-507764&mode=design&t=to5BwBfcY6wkr3s1-0).
I remember back then I thought a good excuse to improve the client/server code would be if I worked on that feature because it would help me understand the ideal solution. But this doesn't seem to be a priority yet (you could double-check, that was a long time ago). Notifications of type contact request and contact requests themselves are different entities in the backend db, so ideally our code should improve on this area.
Also do you think I should create a separate issue for possibly rewiring the way contact requests are stored?
Sure! 👍🏼
ilmotta
reviewed
Mar 6, 2024
src/status_im/contexts/profile/contact/contact_review/view.cljs
Outdated
Show resolved
Hide resolved
seanstrom
force-pushed
the
seanstrom/contact-profile-contact-request-received
branch
from
b4ccbf3
to
eacc328
Compare
ilmotta
approved these changes
Mar 7, 2024
seanstrom
force-pushed
the
seanstrom/contact-profile-contact-request-received
branch
from
eacc328
to
5889937
Compare
seanstrom
force-pushed
the
seanstrom/contact-profile-contact-request-received
branch
4 times, most recently
from
cf73b2a
to
c4cb8c2
Compare
73% of end-end tests have passedFailed tests (12)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (35)Click to expandClass TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
|
|
@status-im/mobile-qa Can you please review the results of the E2E? 🙏 |
Hi @seanstrom all fails are not PR related. I have rerun them one more time |
92% of end-end tests have passedFailed tests (3)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (44)Click to expandClass TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
|
@seanstrom E2E are okay. If PR does not require manual QA - it is ready for merge. |
@seanstrom If the component is not yet in use in app - we can skip manual qa. Since e2e are okay PR is ready for merge. |
…quest-state is mutual
seanstrom
force-pushed
the
seanstrom/contact-profile-contact-request-received
branch
from
c4cb8c2
to
c625ee3
Compare
|
@pavloburykh Awesome! Thanks for the help 🙌 |
seanstrom
deleted the
seanstrom/contact-profile-contact-request-received
branch
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 #18968
fixes #18969
Summary
quo/locked-inputquo/locked-inputto have a minimum height instead of a fixed-height.quo/locked-inputto only render a label when provided the label text.Testing Notes
These changes to the
quo/locked-inputmay requiremanual-qa, though it may not since thequo/locked-inputcomponent does not seem to be used yet. It seems that it has been defined in the design system, but this may be the first use of the component (?), meaning that changes to it may not affect anything.Platforms
Areas that maybe impacted
Functional
Steps to test
new-contact-uifeature flag on mobileBefore and after screenshots comparison
Before
After changes
Screen.Recording.2024-03-07.at.18.48.12.mov
status: ready