[CP Staging] Fix warning on the server#35918
Conversation
|
@akinwale , I added @aimane-chnaif as this fixes a regression added by a PR reviewed by him. |
|
Thanks for catching this. Can we fix all instances in this file? |
|
Lint failing on main. fixed in #35920 |
|
@cristipaval please pull main. lint fixed now |
Found 2 more cases. Just pushed the fix for them as well. |
|
@ready again for review, @aimane-chnaif @VickyStash |
|
These are to verify:
|
|
perf-tests failing. Not sure it's known or caused by this PR |
| const isMoneyRequestReport = ReportUtils.isMoneyRequestReport(report); | ||
| const currentChatReport = isMoneyRequestReport ? ReportUtils.getReport(report.chatReportID) : report; | ||
| const moneyRequestReportID = isMoneyRequestReport ? report.reportID : ''; | ||
| const moneyRequestReportID = isMoneyRequestReport ? report.reportID : '0'; |
There was a problem hiding this comment.
Seems like this change breaks IOU request in offline. Not reproducible before this change
Screen.Recording.2024-02-06.at.7.16.07.PM.mov
There was a problem hiding this comment.
ok so we can't do this change. There's if (moneyRequestReportID) { check in getMoneyRequestInformation()
There was a problem hiding this comment.
I think we shouldn't touch anything else if we don't have problems raised in the logs. Let's methodically fix each issue when it occurs. If tests passed for the other '' cases, let's not touch them.
aimane-chnaif
left a comment
There was a problem hiding this comment.
Sorry, let's revert last change I suggested.
| const isMoneyRequestReport = ReportUtils.isMoneyRequestReport(report); | ||
| const currentChatReport = isMoneyRequestReport ? ReportUtils.getReport(report.chatReportID) : report; | ||
| const moneyRequestReportID = isMoneyRequestReport ? report.reportID : ''; | ||
| const moneyRequestReportID = isMoneyRequestReport ? report.reportID : '0'; |
|
@aimane-chnaif , just reverted the last change. Ready again for review. |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid: mWeb ChromeiOS: NativeiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2024-02-06.at.7.32.36.PM.movMacOS: Desktop |
|
@MonilBhavsar 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] |
|
🎯 @aimane-chnaif, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #35940. |
|
Should we CP this? |
|
perf-tests still failing but I don't think it's caused by this PR |
|
Just merged main to see if this helps perf tests pass |
|
@cristipaval looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
Perf tests fail on all PRs atm, though they pass locally. It might be something related to the GH actions cache. |
[CP Staging] Fix warning on the server (cherry picked from commit 92db113)
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.4.37-7 🚀
|
Details
This fixes a regression added by this PR. The server expects an integer for the
createdReportActionIDparameter.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/367085
PROPOSAL:
Tests
Offline tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop