Reapply "Merge pull request #56092 from callstack-internal/feature/al…#65001
Conversation
…eature/allow-negative" This reverts commit e3255ba.
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
| return; | ||
| } | ||
| initializeAmount(amount); | ||
| initializeAmount(absoluteAmount); |
There was a problem hiding this comment.
@pasyukevich, we also need to set isNegative to false here to fix this issue.
There was a problem hiding this comment.
Thanks
I will check this
|
Updates:
|
|
👋 @pasyukevich did you get any further on the ETA to address the regressions in this PR? We're trying to decide if this will make it into the product release message going out on Monday or not. |
|
Hi! @trjExpensify Yes, I have. The fixes should be ready in the second half of next week. But I think it’s a bit early to announce that, as there might still be some hidden issues or missing links. We’ll also need to do more testing after the updates. |
|
Tommorow will be OOO I will continue work here on monday |
| * Return the amount field from the transaction, return the modifiedAmount if present. | ||
| */ | ||
| function getAmount(transaction: OnyxInputOrEntry<Transaction>, isFromExpenseReport = false, isFromTrackedExpense = false): number { | ||
| function getAmount(transaction: OnyxInputOrEntry<Transaction>, isFromExpenseReport = false, isFromTrackedExpense = false, allowNegative = false, disableOppositeConversion = false): number { |
There was a problem hiding this comment.
Can you double check wherever getAmount is called, that it's passing the proper value for isFromExpenseReport? In Transaction.ts, seems like we're not passing anything, but maybe we should
There was a problem hiding this comment.
Yes, we are not passing the value from isFromExpenseReport in Transaction
I did not find anything that was broken due to this
I think we should create a separate PR to address this and verify - for me looks like a tech debt and more like an inconsistency that could lead to edge case bugs
That will update getTransactionPreviewTextAndTranslationPaths from TransactionPreviewUtils.ts
And getUpdateMoneyRequestParams from IOU.ts
|
✋ 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/luacmartins in version: 9.2.28-0 🚀
|
| if (reportUtilsIsPolicyExpenseChat(report) && !isEmptyObject(iouReport)) { | ||
| lastMessageTextFromReport = formatReportLastMessageText(getReportName(iouReport)); |
There was a problem hiding this comment.
I believe this change is related to this issue (a demoted deploy blocker) #72139
There was a problem hiding this comment.
@pasyukevich, this seems to be on purpose. Is there a reason/discussion about it?
There was a problem hiding this comment.
Yes, here is the discussion - https://expensify.slack.com/archives/C07NMDKEFMH/p1759260448692239?thread_ts=1758645326.423979&cid=C07NMDKEFMH
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.2.28-5 🚀
|
|
|
||
| return ( | ||
| <> | ||
| <ScrollView contentContainerStyle={styles.flexGrow1}> |
There was a problem hiding this comment.
This caused the issue Split input field is not aligned on the right on confirm page, and we fixed it in this PR: #73228.
| if (!isEmptyObject(iouReportAction) && !isIOUReport(report) && iouReportAction && isSplitBillReportAction(iouReportAction)) { | ||
| // This covers group chats where the last action is a split expense action | ||
| const linkedTransaction = getLinkedTransaction(iouReportAction); | ||
| if (isEmptyObject(linkedTransaction)) { |
| const transactionViolations = useTransactionViolations(transaction?.transactionID); | ||
| const [outstandingReportsByPolicyID] = useOnyx(ONYXKEYS.DERIVED.OUTSTANDING_REPORTS_BY_POLICY_ID, {canBeMissing: true}); | ||
|
|
||
| const allowNegativeAmount = shouldEnableNegative(report, policy); |
There was a problem hiding this comment.
Coming from #72165 checklist: this change missed adding the iouType parameter, causing invoices to incorrectly return false for negative amount allowance. We should pass iouType the same way as in IOURequestStepAmount
| onMouseDown={(event) => focusTextInput(event, [NUMBER_VIEW_ID])} | ||
| style={[styles.moneyRequestAmountContainer, styles.flexRow, styles.w100, styles.alignItemsCenter, styles.justifyContentCenter]} | ||
| > | ||
| {textInputComponent} |
…low-negative"
This reverts commit e3255ba.
Re-apply changes of reverted PR: $#56092
Regressions reported previous time:
$ #65147
$ #64851
$ #64853
$ #64855
$ #64856
$ #64857
$ #64863
$ #64868
$ #64870
$ #64872
$ #64874
$ #64876
$ #64880
$ #64896
$ #64927
$ #64964
$ #71012
Explanation of Change
Fixed Issues
$ #53441
PROPOSAL:
Tests
📱 Mobile
1️⃣ Verify negative expense can be created without a workspace only to DM
(account without workspaces needed)
2️⃣ Verify negative value functionality
(workspace required before running this test)
3️⃣ Verify negative value option is not hidden for DM
4️⃣ Verify negative value option is hidden for Split Bill
💻 Desktop
-.1️⃣ Verify negative expense cannot be created without a workspace
(account without workspaces needed)
-followed by a number.2️⃣ Verify negative value functionality
(workspace required before running this test)
-followed by a number.-and submit.3️⃣ Verify negative value option is not hidden for DM
-.4️⃣ Verify negative value option is hidden for Split Bill
Start creating a Split Bill.
Attempt to enter
-in the amount field.Verify that:
Complete creation of the Split Bill.
Open the created Split Bill.
Update the amount (without minus).
Verify that the amount updated correctly and the history comment is accurate.
Verify that no errors appear in the JS console
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso 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-native-converted.webm
Android: mWeb Chrome
android-web-converted.webm
iOS: Native
ios-native-converted.mp4
iOS: mWeb Safari
ios-web-converted.mp4
MacOS: Chrome / Safari
mac-web-converted.mov
MacOS: Desktop
mac-desktop-converted.mov