fix: re-calculate totals when invoice is edited#26486
Conversation
|
@mananjadhav 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] |
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
| * @param {Boolean} isExpenseReport | ||
| * @returns {Object} | ||
| */ | ||
| function editIOUTotal(iouReport, initialTransaction, updatedTransaction, isExpenseReport = false) { |
There was a problem hiding this comment.
@mananjadhav Just saw the guidelines and wanted to confirm:
This can surely be replaced by using updateIOUOwnerAndTotal twice (once to remove the old amount and then to add the new amount) but thought of adding a utils component to make it look better, if DRY is more important then I can remove this, thanks!
There was a problem hiding this comment.
I checked and I think it makes sense to use updateIOUOwnerAndTotal from your current L48 onwards. Can we please update that? We can keep the rest of the method as is, and agreed on keeping it in IOUUtils.
|
@mananjadhav gentle ping |
| * @param {Boolean} isExpenseReport | ||
| * @returns {Object} | ||
| */ | ||
| function editIOUTotal(iouReport, actorAccountID, initialTransaction, updatedTransaction, isExpenseReport = false) { |
There was a problem hiding this comment.
I am a bit concerned about the name editIOUTotal. We're already using updateIOUOwnerAndTotal.
Should this be updateIOUTotalWithTransaction
|
@shubham1206agra Can you please merge with the latest main? |
|
@mananjadhav Wrong PR😅 |
|
Sorry @shubham1206agra. @im-adithya Can you please merge with the latest main? |
|
thanks for the delay, ig the fix has been merged in the meantime #25498 |
Details
Fixed Issues
$ #26054
PROPOSAL: #26054 (comment)
Tests
Offline tests
6. Go back to IOU report and observe that the total is wrongQA Steps
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
Screen.Recording.2023-08-31.at.8.51.01.AM.mov
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android