Enable editing of money requests #23703
Conversation
…cy to the MoneyRequestView component
…ic edit iou transaction work
mountiny
changed the title
|
Thanks for the quick reviews guys, reliable as always ❤️ 🚀 I think the raised points can be covered in the follow up now and its better to merge this to get the feature out.
I agree, added here #23961 |
src/libs/ReportUtils.js
Outdated
| isSendMoneyFlow = false, | ||
| receipt = {}, | ||
| ) { | ||
| function buildOptimisticIOUReportAction(type, amount, currency, comment, participants, transactionID, paymentType = '', iouReportID = '', isSettlingUp = false, isSendMoneyFlow = false) { |
There was a problem hiding this comment.
we should also update this call to stop passing the receipt
There was a problem hiding this comment.
I think this just must be some bad merge conflict resolve, cant recall why we should be removing this
| currency={defaultCurrency} | ||
| amount={defaultAmount} | ||
| ref={(e) => (textInput.current = e)} | ||
| onCurrencyButtonPress={() => null} |
There was a problem hiding this comment.
maybe we can make this prop not required and add a default to it?
There was a problem hiding this comment.
This will be updated in a quick follow up so I think there is no need for this here
| // In case the comment hasn't been changed, do not make the API request. | ||
| if (transactionChanges.comment.trim() === transactionDescription) { | ||
| Navigation.dismissModal(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
NAB maybe we should add all these checks to IOU.editMoneyRequest
There was a problem hiding this comment.
We could add some verify method there
There was a problem hiding this comment.
I will add this to the clean ups
|
@rushatgabhane @allroundexperts @luacmartins should be ready for another look |
|
@luacmartins should be ready and this Auth Pr goes along with the created format changes |
|
✋ 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: 1.3.54-0 🚀
|
1 similar comment
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.3.54-0 🚀
|
|
Thanks! The total not updating is captured here #23961 and will be worked on as a follow up |
|
@mountiny Mind confirming the correct compensation for @rushatgabhane for this PR review? Related, should that regression above be factored into payment? Thanks! |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.54-13 🚀
|
| const originalMessage = getModifiedExpenseOriginalMessage(oldTransaction, transactionChanges, isFromExpenseReport); | ||
| return { | ||
| actionName: CONST.REPORT.ACTIONS.TYPE.MODIFIEDEXPENSE, | ||
| actorAccountID: currentUserAccountID, |
There was a problem hiding this comment.
This issue is probably caused by the change introduce in this PR.
Issue: Changing Currency in IOU Causes Account Email to Replace Request Email
Steps to reproduce:
- Click on the plus sign and select "Request Money".
- Enter the desired amount and email address for the money request.
- Double-click on the requested money in IOU and change the currency by clicking on the amount.
- Go back twice and observe that the requested email has been replaced by the account email.
Details
cc @luacmartins
This PR is enabling edits of money requests by calling a new API
EditIOUTransactioncommand.In this PR we are adding basics for allowing money request editing. Users should be able to edit:
They cannot edit the currency yet and the button is disabled, @koko57 will kindly help us get that fixed, there is some trickiness involved with needing to navigate to another IOU Selection component and I consider this as not a blocker which Agata can help us resolve on Friday.
The IOUPreview and Transaction details in the top of the transaction thread are taken from the parent report action which is IOU report action of type
create. This action is a historic record of the request and it does not change when its edited, the data which changes is in the transaction object itself and as we change it we add the MODIFIEDEXPENSE report action which holds the information about what data has changed.This will be fixed once we start puling the data from transaction objects which is handled in this issue #22208 and pr should be raised soon. While its lackluster experience for the user, I think we dont have to block on this as the requests are still not used in NewDot yet.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/270589
Partially https://github.com/Expensify/Expensify/issues/295091
Partially https://github.com/Expensify/Expensify/issues/290384
Tests
changed the request description to "test2" (previously "test")was added to the reportchanged the request date to 2023-07-20 (previously 2023-07-13)was added to the reportchanged the request to £4.00 (previously £5.00)was added to the reportOffline tests
Same as 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)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
Mobile Web - Chrome
Mobile Web - Safari
iosMweb23703.mp4
Desktop
Same as web
iOS
ios23703.mp4
Android