-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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 expense report total when deleting requests #23822
Conversation
Please ignore the giant drop button in the composer on iOS/android. That seems to be a regression from this PR |
@luacmartins looks like the linked issue doesn't work or the issue has been removed? |
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!
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chromemweb.chrome.movMobile Web - Safarimsafari.web.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
@stitesExpensify 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] |
@ArekChr the linked issue is an internal (private repo) issue, so you won't have access to it. |
When I'm setting up the workspace, I can only see the public and announce slightly different channels. How can I access the main workspace like in the attached video? |
@ArekChr you need to be part of a beta. What's the email of the test account you're using? |
@luacmartins arkadiusz.chrabaszczewski@callstack.com and arkadiusz.chrabaszczewski+{1 - 20}@callstack.com |
Ok, I added arkadiusz.chrabaszczewski@callstack.com and arkadiusz.chrabaszczewski+{1-3}@callstack.com to the beta, so you should have 4 accounts to test this functionality. The beta might take a little bit to take effect (a few minutes to an hour) |
@luacmartins Found some bugs but I'm assuming that they are probably not related to this PR but are worth mentioning. Are they tracked or is it worth going deeper? Generally, the functionality of the fix for this issue works as it should, Uploading videos and finishing the checklist. |
Yea, I've seen those on main too and I think we had issues for both issues you mentioned. |
@stitesExpensify I believe this is ready for you to take a look at :) |
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.
EZPZ
✋ 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/stitesExpensify in version: 1.3.51-0 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 1.3.51-2 🚀
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.52-0 🚀
|
🚀 Deployed to staging by https://github.com/stitesExpensify in version: 1.3.52-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.52-5 🚀
|
Details
We were passing the wrong param to
ReportUtils.isExpenseReport
(whoopsies), so the report total was not being properly calculated.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/302985
Tests
Offline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
web.mov
Mobile Web - Chrome
chrome.mov
Mobile Web - Safari
safari.mov
Desktop
desktop.mov
iOS
Please ignore the giant drop button - that's a regression from this PR
ios.mov
Android
Please ignore the giant drop button - that's a regression from this PR
android.mov