Closed kbecciv closed 8 months ago
Triggered auto assignment to @miljakljajic (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Job added to Upwork: https://www.upwork.com/jobs/~018b84474aa7b66f4e
Platforms
in OP are β
)Triggered auto assignment to Contributor-plus team member for initial proposal review - @shubham1206agra (External
)
Production on previous build
Expense - Missing "Required" and "Request" for Category & Tag and Merchant respectively
We have not passed the right label prop for category and tag [here]()
We should pass the prop if policy requires category and tag:
rightLabel={Boolean(props.policy.requiresCategory) ? translate('common.required') : ''}
rightLabel={Boolean(props.policy.requiresTag) ? translate('common.required') : ''}
We may also have a canUseViolations
check.
Expense - Missing "Required" and "Request" for Category & Tag and Merchant respectively
The rightLabel
prop is passed correctly in these places:
However, it is not used here:
This is the function responsible for rendering the component here: https://github.com/Expensify/App/blob/aace20dcdbe79a3a144b8113daf5d1445fd9dc9c/src/pages/iou/request/step/IOURequestStepConfirmation.js#L8
We need to add the conditional properly, along with the usePermissions
hook.
const {canUseViolations} = usePermissions();
rightLabel={canUseViolations && Boolean(props.policy.requiresCategory) ? translate('common.required') : ''}
rightLabel={canUseViolations && Boolean(props.policy.requiresTag) ? translate('common.required') : ''}
N/A
On confirmation page,
"Required" is shown on Category and Tag row. "Request" is shown in Merchant field by default.
we should add
rightLabel={canUseViolations && Boolean(props.policy.requiresCategory) ? translate('common.required') : ''}
rightLabel={canUseViolations && Boolean(props.policy.requiresTag) ? translate('common.required') : ''}
And then in here https://github.com/Expensify/App/blob/3a44f772e2c381b1cdb025f845551c5f0985bad5/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js#L504
we should add an error handling if category/tag is required and the user doesn't select category/tag.
if (!iouCategory) {
setFormError('common.error.invalidCategory');
return;
}
if (!iouTag) {
setFormError('common.error.invalidTag');
return;
}
In en.ts and es.ts we also need to add the translation for 'common.error.invalidCategory' and 'common.error.invalidTag'
title={iouMerchant || CONST.TRANSACTION.DEFAULT_MERCHANT}
like we did with the date field https://github.com/Expensify/App/blob/961d5010bfd708bee0707c625244d0983846dc6d/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js#L660
And then when requesting money we also do the same thing: checking if iouMerchant is empty we will use CONST.TRANSACTION.DEFAULT_MERCHANT
https://github.com/Expensify/App/assets/141406735/6f9dc92d-4575-40a6-91e5-43381bc739e2
@miljakljajic, @shubham1206agra Whoops! This issue is 2 days overdue. Let's get this updated quick!
No proposal seems to have complete solution, i.e., missing Request
in merchant.
@shubham1206agra Updated proposal
@DylanDylann I don't think your solution is correct for setting merchant
@shubham1206agra is this expected
No I mean your solution method by which you are setting a merchant
@shubham1206agra I don't see any way to setup Merchant to request on olddot. So I think that the reporter want to display request as default value
No I mean the use of useEffect hook is not ideal here π
@shubham1206agra Updated proposal with another way like we did in the date field
@tgolen I just want to confirm the default merchant. Should we set it to Request or it was removed for some reason?
I am not sure. I have not heard of it being a requested change, but that doesn't mean it wasn't. @dylanexpensify @mountiny @luacmartins might know.
It seems like we still use Request
as the default merchant, no? What do you mean by it was removed
?
It seems like we still use
Request
as the default merchant, no? What do you mean byit was removed
?
I mean as a default merchant.
@DylanDylann's proposal looks good to me in that case.
πππ C+ reviewed
Triggered auto assignment to @tgolen, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
π£ @shubham1206agra π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @DylanDylann π An offer has been automatically sent to your Upwork account for the Contributor role π Thanks for contributing to the Expensify app!
Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review π§βπ» Keep in mind: Code of Conduct | Contributing π
@luacmartins In case the user enables "Required" category/tags, should we disable request money if user don't select category/tags ?
IIRC we don't block the user in that case and instead will show a violation. Is that right @cead22?
@cead22 Could you help to confirm this one ?
To answer the question, Carlos is right, we don't block the user
- Which file are we keeping in the end, App/src/components/MoneyTemporaryForRefactorRequestConfirmationList.js?
Yes
- I say we close this and I'll handle this in a separate issues as part of the violations project
@cead22 Since @DylanDylann has nearly completed this issue fix, let us finish this issue up.
The issue fix is a 3 line change, and we don't have a PR with tests or anything yet. I still think we should close and let the people working on violations fix this. This isn't really a bug, it's a small oversight coming from the fact that we have 2 files doing the same thing, one of which is temporary due to a refactor.
Ultimately, I'll lean on what @miljakljajic and @tgolen want to do
@tgolen, @miljakljajic, @shubham1206agra, @DylanDylann Whoops! This issue is 2 days overdue. Let's get this updated quick!
The change @shubham1206agra is referring to is that for expenses we will require a merchant to be set during creation so the Request
will be removed from there. In case of Scan request too there should be no way to add the merchant during creation so Request wont be default either.
@shubham1206agra With that said, I think you can omit changes to the Merchant
field in this PR and only handle the category and tag.
We are handling the merchant changes in here https://github.com/Expensify/App/pull/32486
@shubham1206agra The PR is ready for review. Could you help to take a look?
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.21-4 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-01-11. :confetti_ball:
After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
For reference, here are some details about the assignees on this issue:
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
@shubham1206agra make sure you accept the offer so that we can pay promptly when the seven day hold is up!
@shubham1206agra @DylanDylann payment issued for you both! Do you have suggestions for additional test rail steps we should add for this issue?
Not really. This is an edge case handling and is behind a beta right now.
Issue is ready for payment but no BZ is assigned. @greg-schroeder you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!
Unassigning myself as it seems Milja already handled this
I'll close this out as payment has been issued and no test rail steps are required.
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: v1.4.12-2 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:
Action Performed:
Precondition:
Expected Result:
On confirmation page,
Actual Result:
On confirmation page,
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/93399543/6a507a0e-9441-41b0-8302-acdc35a6e055
View all open jobs on GitHub
Upwork Automation - Do Not Edit