-
Notifications
You must be signed in to change notification settings - Fork 3k
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: change message when reimbursementChoice is reimburseNo #49837
fix: change message when reimbursementChoice is reimburseNo #49837
Conversation
@ikevin127 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] |
Reviewer Checklist
Screenshots/Videos |
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.
@koko57 I just completed the reviewer checklist, it tests well following the test steps provided in description, except for the case when the step is replaced by the OpenReport
response if we visit the report while online
.
web-to-clarify.mov
I noticed in the issue #49333 (comment) that @srikarparsi said:
Ah thanks for the looking into this. Do you think you could help with the offline (optimistic) behavior here and I can work on a backend PR that returns the right next steps for this case?
So I guess we're good to go here since the back-end PR will fix this case once it's live 👍
Hm, I actually envisioned the implementation for this to be a little different. I'm sorry for not clarifying this earlier. When an expense is submitted when So, when a report is created with But to clarify the backend behavior we need to emulate is: If a report is on a workspace with:
Then the report should optimistically be created with:
For internal reference, this is the Auth code which is based on @trjExpensify's comment. |
Hm, why? It's totally possible to have approvals enabled and no payments, so the report would be
I don't think we should be doing that if the workspace has approvals enabled. @koko57's issue (as I understand it) is fixing the next steps for the case where an expense report is done and "no further action is required" which can be possible when the report is either |
@srikarparsi @trjExpensify so should I do anything here? I will change the check I used for arePaymentsEnabled but should I do anything else then? |
That probably depends, need to see what @srikarparsi has to say about #49837 (comment). |
Hey! In the first part of this comment, I generalized a bit to try to explain the implementation approach (the part about reimburseChoice = no leads to closed reports). I tried to explain the behavior in detail with the second part:
Do you agree with the second part @trjExpensify? Basically, if the three conditions are met (payments disabled, submit&close approval and instant submit), then the report should be created in the closed state. I believe that is the behavior we ended up with on this issue but please correct me if I'm wrong. |
Yes, I agree with that. I'm confused by this mismatch with the matrix though, was that a typo on
|
Yeahh, the matrix is a little confusing when compared with code. 2(status) and 2(state) in the matrix correlates with the I just said
because in the App code, state 2 is called APPROVED |
Okay, as if it wasn't confusing enough 😂 |
So we good to move on here? |
Yupp, I believe so. @koko57 do you think you could switch the implementation to this comment? Specifically this part:
Once again, sorry for not clarifying this earlier. |
@srikarparsi I see that we have this in submitReport |
@srikarparsi what should I change on my side then? |
So for these reports, with these conditions, we won't actually ever hit the SubmitReport function because the workspace is on InstantSubmit. You can test this by going offline and then creating an expense report in a policy with payments disabled, instant submit, and no approvals: When you create an expense report in offline mode in this policy, the report is created in 1 state and 1 status: But this report should optimistically be created in state 2 and status 2 if these conditions are met. It looks like the optimistic data for the RequestMoney function is built here so we just need to modify the report to be 2 state 2 status (kind of like how it was done for RequestMoney but with the above conditions) |
@srikarparsi thanks for explanation! I was a bit confused with this one. I think I've finally made this work - I needed to create a new function to get stateNum and statusNum for the expense report. |
@koko57 Is the PR ready to be retested ? |
@ikevin127 yes, it's ready to be retested cc @srikarparsi |
I went through the testing steps again, same flow I followed before in #49837 (review) and the behaviour changed to:
cc @srikarparsi To verify if everything is good / needs changes. |
case CONST.REPORT.STATUS_NUM.CLOSED: | ||
optimisticNextStep = { |
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.
Can we add optimisticNextStep = noActionRequired
and the break
. I think it would improve readability a little.
The And for the translation, I'm getting confirmation internally and will leave a comment with the correct translation. Thanks for pointing these out! |
src/libs/NextStepUtils.ts
Outdated
@@ -125,6 +135,12 @@ function buildNextStep(report: OnyxEntry<Report>, predictedNextStatus: ValueOf<t | |||
], | |||
}; | |||
|
|||
if (!PolicyUtils.arePaymentsEnabled(policy)) { |
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.
Sorry one more thing, @koko57 do you think you could explain why you added this? If payments are disabled and there is some non instant auto reporting frequency (weekly, monthly, etc.), the next steps should still be "Waiting for x's expenses to automatically submit" (below text). cc @trjExpensify for confirmation.
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.
A delayed submission frequency other than manually
, yes.
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.
@srikarparsi I understood that it also must be that way. So I will remove it
@srikarparsi changes reverted |
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.
Awesome, this looks good, thank you!
✋ 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/srikarparsi in version: 9.0.49-0 🚀
|
This PR is failing because of issue #50785 |
🚀 Deployed to production by https://github.com/marcaaron in version: 9.0.49-2 🚀
|
Details
Fixed Issues
$ #49333
PROPOSAL:
Tests
Offline tests
QA Steps
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
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)Design
label so the design team can review the changes.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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop