Ensure users can share a community url #19710
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 (30)
|
ibrkhalil
requested changes
Apr 18, 2024
seanstrom
force-pushed
the
seanstrom/fix-sharing-community
branch
from
8fde739
to
51a7fae
Compare
40% of end-end tests have passedFailed tests (29)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (21)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestWalletMultipleDevice:
|
seanstrom
force-pushed
the
seanstrom/fix-sharing-community
branch
from
51a7fae
to
a03d8ec
Compare
|
Hi @seanstrom ! I am moving back your PR to the corresponding column as it doesn't meet the requirements outlined here: Please, make sure you added appropriate label for QA's. |
58% of end-end tests have passedFailed tests (20)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (30)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletOneDevice:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
@seanstrom thanks for the PR! Please take a look at the issue. @seanstrom this issue does not seem to be regression of this PR, but I am wondering if we can handle it here. WDYT? ISSUE 1 Shared community/channel url is duplicated (Android only)Steps:
Actual result: url is duplicated. 
|
79% of end-end tests have passedFailed tests (9)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (41)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestWalletOneDevice:
|
|
@pavloburykh thanks for finding that issue 🙏 |
|
@pavloburykh which external application were you using when you discovered that issue? |
Telegram |
|
Hey @pavloburykh 👋 |
Oh, @seanstrom I am very sorry for the confusion. The issue occurs only on Android. IOS works fine. Sorry once again. I am reproducing on Android 12, Samsung Galaxy A52. |
|
@pavloburykh no worries 🙏, I got it reproduced now and I'll try fixing it today. thank you! 🙌 |
seanstrom
force-pushed
the
seanstrom/fix-sharing-community
branch
2 times, most recently
from
e6c6128
to
a992977
Compare
|
Okay I think I've been able to fix this issue, and I think I also fixed it for when we're sharing a community channel too 🙌. Here's some screen captures showing the behaviour after my changes. Sharing CommunityScreen.Recording.2024-04-25.at.09.18.59.movSharing Community ChannelScreen.Recording.2024-04-25.at.09.20.14.mov |
50% of end-end tests have passedFailed tests (24)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Expected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (26)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@seanstrom thanks a lot for the fixes. PR is ready for merge. |
seanstrom
force-pushed
the
seanstrom/fix-sharing-community
branch
from
990ef19
to
275d9a3
Compare
seanstrom
commented
Apr 29, 2024
Comment on lines
-84
to
-101
|
|
||
| (rf/reg-event-fx :communities/share-community-channel-url-with-data | ||
| (fn [_ [chat-id]] | ||
| (let [title (i18n/label :t/channel-on-status) | ||
| on-success (fn [url] | ||
| (share/open | ||
| (if platform/ios? | ||
| {:activityItemSources [{:placeholderItem {:type "text" | ||
| :content title} | ||
| :item {:default {:type "url" | ||
| :content url}} | ||
| :linkMetadata {:title title}}]} | ||
| {:title title | ||
| :subject title | ||
| :message url | ||
| :url url | ||
| :isNewTask true})))] | ||
| {:fx [[:dispatch [:communities/get-community-channel-share-data chat-id on-success]]]}))) |
There was a problem hiding this comment.
Removing a duplicated definition
seanstrom
commented
Apr 29, 2024
| :on-press #(hide-sheet-and-dispatch [:communities/share-community-pressed id])}) | ||
| :on-press #(rf/dispatch [:communities/share-community-pressed id])}) |
There was a problem hiding this comment.
Changed this to only dispatch because we're opening a share-sheet with this action and calling hide-sheet-and-dispatch will also hide the share-sheet.
seanstrom
commented
Apr 29, 2024
| @@ -52,7 +50,6 @@ | |||
| :linkMetadata {:title title}}]} | |||
| {:title title | |||
| :subject title | |||
| :message url | |||
There was a problem hiding this comment.
Removing message when sharing a community channel on Android because we already pass the url with the url field.
seanstrom
commented
Apr 29, 2024
Comment on lines
+70
to
+87
| (rf/reg-event-fx :communities/share-community-url-with-data | ||
| (fn [_ [community-id]] | ||
| (let [title (i18n/label :t/community-on-status) | ||
| on-success (fn [url] | ||
| (rf/dispatch [:open-share | ||
| {:options (if platform/ios? | ||
| {:activityItemSources | ||
| [{:placeholderItem {:type :text | ||
| :content title} | ||
| :item {:default {:type :url | ||
| :content url}} | ||
| :linkMetadata {:title title}}]} | ||
| {:title title | ||
| :subject title | ||
| :url url | ||
| :isNewTask true})}]))] | ||
| {:fx [[:dispatch [:communities/get-community-share-data community-id on-success]]]}))) | ||
|
|
There was a problem hiding this comment.
Here we're adding the definition for :communities/share-community-url-with-data because it was accidentally removed.
seanstrom
force-pushed
the
seanstrom/fix-sharing-community
branch
from
275d9a3
to
51f5f7c
Compare
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 #19724
fixes #19725
Summary
Platforms
Areas that maybe impacted
Functional
Steps to test
Before and after screenshots comparison
Before
Screen.Recording.2024-04-18.at.11.37.27.mov
Action Menu
Screen.Recording.2024-04-18.at.11.13.23.mov
Community Invites
After
Action Menu
Screen.Recording.2024-04-18.at.11.42.08.mov
Community Invites
Screen.Recording.2024-04-18.at.12.58.39.mov
status: ready