-
Notifications
You must be signed in to change notification settings - Fork 3k
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: Message and Notification Swapping in IOU #47478
Conversation
Reviewer Checklist
Screenshots/VideosAndroid: Native01_Android_Native.mp4Android: mWeb Chrome02_Android_Chrome.mp4iOS: Native03_iOS_Native.mp4iOS: mWeb Safari04_iOS_Safari.mp4MacOS: Chrome / Safari05_MacOS_Chrome.mp4MacOS: Desktop06_MacOS_Desktop.mp4 |
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.
LGTM but let's test with an ad-hoc build.
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.
I'll get the adhoc build going and then ping QA to start testing it.
This comment has been minimized.
This comment has been minimized.
OK, I've requested the testing from QA (ref: thread). |
OK, It's building again now. QA has this on their list to do, but they have two other ad-hoc builds in front of this one that they are testing, so I don't imagine they will get to this one until the end of next week probably. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
Update: QA currently has 4-5 other adhoc builds that they are testing. This PR has been moved to the end-of-the-line priority-wise. I imagine it could start being tested next week. |
@nkdengineer It sounds like QA is ready to test this. Could you please update your branch so it is current with |
@tgolen I merged. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel! Expense - Banner shows "transaction pending" when there is no pending Ecard transactionVersion Number: 9.0.24-2 Action Performed:
Expected Result:The banner will not show "Transaction pending. It may take a few days to post." because there is no pending Ecard transaction Actual Result:The banner shows "Transaction pending. It may take a few days to post Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6583277_1724692432421.20240827_011138.mp4 |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel! Task – Character limit for the task description is 1000Version Number: 9.0.24-2 Action Performed:
Expected Result:Character limit for the task description is 500 Actual Result:Character limit for the task description is 1000, but not 500 Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6583335_1724695608472.task_description_character_limit.mp4 |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel! Android - room-Leaving room with public visibility app crashesVersion Number: 9.0.24-2 Action Performed:
Expected Result:Leaving room with public visibility app must not crash Actual Result:Leaving room with public visibility app crashes. Workaround:Unknown Platforms:Which of our officially supported platforms is this issue occurring on?
Screenshots/VideosBug6583333_1724695552091.Screenrecorder-2024-08-26-23-30-31-668_compress_1.mp4 |
@lanitochka17 I checked and these issues may not be related to this PR. |
QA is completed |
@nkdengineer Are you sure of this and the problems have been reproduced on main or staging? If so, it sounds fine to merge, but it would be nice to have certainty about what is causing those issues. @lanitochka17 Have you tried to reproduce those on staging? |
OK, thank you! Sounds like this can go ahead and be merged then. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/tgolen in version: 9.0.28-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.28-3 🚀
|
Details
Fixed Issues
$ #46393
PROPOSAL: #46393 (comment)
Tests
Issue 1:
Issue 2:
Offline tests
QA Steps
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
android.mov
Android: mWeb Chrome
ios-mweb.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-mweb.mov
MacOS: Chrome / Safari
web-resize.mp4
MacOS: Desktop
desktop.mov