-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
fix: prevent open context menu on money request #48375
Conversation
@hoangzinh Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-09-20.at.07.16.53.android.movAndroid: mWeb ChromeScreen.Recording.2024-09-03.at.21.59.19.android.chrome.moviOS: NativeiOS: mWeb SafariScreen.Recording.2024-09-20.at.07.28.41.ios.safari.movMacOS: Chrome / SafariScreen.Recording.2024-09-20.at.07.13.20.web.movMacOS: DesktopScreen.Recording.2024-09-20.at.07.21.57.deskto.mov |
@daledah I still reproduce open context menu with split expense. By the way, the test step should be
Not expense Screen.Recording.2024-09-03.at.22.04.34.mov |
@daledah Do you have any update on this review #48375 (comment)? Thank you |
@hoangzinh I'll update soon. |
@hoangzinh I think #48901 fixed this issue. |
Oh you're right. Let's watch that PR if it goes to Prod |
@hoangzinh #48901 is merged, looks like this bug is fixed. |
@hoangzinh Should we close this PR and continue the next step in the issue page? |
Hi @daledah can you share the recording of your test on latest main branch to prove that this issue is fixed? Thank you |
@hoangzinh Looks like #48901 missed some cases of Context menu, so I applied their change and update missing places. Please review again. Thanks! |
Thanks. I will review today |
Code looks good. Testing! |
Hi @daledah one more thing, can you add another test cases, one for 1:1 expense, another for group split expense? Thank you |
@hoangzinh I updated, here are the tests Screen.Recording.2024-09-20.at.15.06.54.movScreen.Recording.2024-09-20.at.15.07.37.mov |
Thanks @daledah. What is different between 1.1 and 2.1, btw? |
@hoangzinh 1.1 is for WS chat, I thought you want me to differentiate the tests. If you think it's not necessary then I'll remove 2.1. |
Nah, we can keep it. Btw, can you clarify in 1.1 that we create expense in WS chat? Thank you. It will help our QA team to test easier |
@hoangzinh Test steps updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect. Thank you
🚀 Deployed to staging by https://github.com/lakchote in version: 9.0.40-0 🚀
|
🚀 Deployed to production by https://github.com/AndrewGable in version: 9.0.40-6 🚀
|
Details
Fixed Issues
$ #47743
PROPOSAL: #47743 (comment)
Tests
1.1. Submit an expense in WS chat with code block as the description
1.2. Tap on the expense preview
1.3. Long press on the code block
2.1. Submit an expense to another user with code block as the description
2.2. Repeat step 1.2 and 1.3
3.1. Split an expense with code block as description in a group chat.
3.2. Repeat step 1.2 and 1.3
4. Verify that: In all cases, context menu is not shown.
Offline tests
QA Steps
1.1. Submit an expense in WS chat with code block as the description
1.2. Tap on the expense preview
1.3. Long press on the code block
2.1. Submit an expense to another user with code block as the description
2.2. Repeat step 1.2 and 1.3
3.1. Split an expense with code block as description in a group chat.
3.2. Repeat step 1.2 and 1.3
4. Verify that: In all cases, context menu is not shown.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-08-31.at.16.45.08.mov
Android: mWeb Chrome
Screen.Recording.2024-08-31.at.16.03.45.mov
iOS: Native
Screen.Recording.2024-08-31.at.16.26.49.mov
iOS: mWeb Safari
Screen.Recording.2024-08-31.at.16.23.08.mov
MacOS: Chrome / Safari
Screen.Recording.2024-08-31.at.15.53.46.mov
MacOS: Desktop
Screen.Recording.2024-08-31.at.16.46.17.mov