Skip to content
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

[HOLD for payment 2024-05-09] [Violations] [$500] Implement Receipt Audit Feature / Note type violations #36288

Closed
JmillsExpensify opened this issue Feb 9, 2024 · 80 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@JmillsExpensify
Copy link

JmillsExpensify commented Feb 9, 2024

Relevant Background - High-level

image

Notes for completeness:

  • Note type violations are largely “informational ” and arise from “Receipt Audit” (e.g. we scan the receipt, whether or not the employee does).
  • Note type violations do not generate an RBR in the LHN for admins or members, because notes are informational and there is nothing for either admin or member to fix. However, we will do the following:
    • Update the dot separator sub-state for the request preview with Review required
      • Note: Once the report moves to either an approved or reimbursed state, whichever comes first, the Review required is removed as a sub-state. This is because while the informational note remains, there is never anything in particular to resolve or dismiss, like other violations.
  • If a receipt exists on a request and workspace where Receipt Auditing scans the receipt information in the background, Receipt Audit will appear immediately below the receipt image.
  • Receipt Audit is shown in the “supporting text” color, along with a summary of how many (or no) issues were found
    • If one or more issues exist, an icon of a paper/magnifying glass appear to the left of the text: Receipt Audit * %numIssues% issues found (note: “issue” is singular if only one issue).
    • If no issues exist, an icon of a paper overlaid with a check mark appear to the left of the text: Receipt Audit * no issues found

Detailed (Note: This is outdated and proposals should note suggested updates)

We’ll update src/components/ReportActionItem/MoneyRequestView.js with a similar approach we took for showing inline violations

  • Under {hasReceipt && ( <View style={styles.moneyRequestViewImage}>
  • We’ll add a new component ReceiptAudit and pass a prop notes with the value transactionVioltions.notice
  • That value will be an array with the notices for the transaction.
  • If the array is empty, we’ll show the green we’ll show Receipt Verified No issues found
  • Otherwise, we’ll show 🔎 Receipt Audit N issues found, and the copy of the notices in red below
  • <ReceiptAudit notice={transactionViolation.notice} />
  • Here’s a POC version of that component
  • Note: we need to add translations for “X issues found” and “No Issues found”
import Icon from './Icon';
import PropTypes from 'prop-types';

const propTypes = {
    notice: PropTypes.arrayOf(PropTypes.string).required
};
const defaultProps = {
    notice: [],
};
function ReceiptAudit(notice) {
    debugger;
    return (
        <>
            <Icon width={32} height={32} src={notice.length > 0 ? Expensicons.Receipt : Expensicons.Checkmark} />
            <Text>
                Receipt Audit • {notice.length > 0 ? `${notice.length} Issue(s) Found` : 'No issues Found'}
            </Text>
        </>
    );
}```
<details><summary>Upwork Automation - Do Not Edit</summary>
    <ul>
        <li>Upwork Job URL: https://www.upwork.com/jobs/~019f84ef0521d4c9c9</li>
        <li>Upwork Job ID: 1756094841883127808</li>
        <li>Last Price Increase: 2024-02-23</li>
<li>Automatic offers: </li>
<ul>
<li>Krishna2323 | Contributor | 0</li>
</ul></li>
    </ul>
</details>

<details><summary>Issue Owner</summary>Current Issue Owner: @JmillsExpensify</details>
@JmillsExpensify JmillsExpensify added Daily KSv2 NewFeature Something to build that is a new item. labels Feb 9, 2024
@JmillsExpensify JmillsExpensify self-assigned this Feb 9, 2024
Copy link

melvin-bot bot commented Feb 9, 2024

Current assignee @JmillsExpensify is eligible for the NewFeature assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Feb 9, 2024
@JmillsExpensify JmillsExpensify added External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors labels Feb 9, 2024
@melvin-bot melvin-bot bot changed the title Implement Receipt Audit Feature / Note type violations [$500] Implement Receipt Audit Feature / Note type violations Feb 9, 2024
Copy link

melvin-bot bot commented Feb 9, 2024

Job added to Upwork: https://www.upwork.com/jobs/~019f84ef0521d4c9c9

Copy link

melvin-bot bot commented Feb 9, 2024

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 9, 2024
@neonbhai
Copy link
Contributor

neonbhai commented Feb 9, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Implement Receipt Audit Feature / Note type violations

What is the root cause of that problem?

New Feature

What changes do you think we should make in order to solve the problem?

We will:

  • Introduce a new component named ReceiptAudit
  • Add it to MoneyRequestView file we'll render it right after receipt here
    <OfflineWithFeedback pendingAction={getPendingFieldAction('amount')}>
  • Pass a prop named notes to the ReceiptAudit component, containing the notices for transaction violations.
  • Display a message based on the presence of notices:
    • If the notices array is empty, show "Receipt Verified: No issues found" in green.
    • If the notices array contains elements, show "Receipt Audit: N issues found" in red, along with the notices listed below.
  • Ensure localization by adding translations for Issues found messages
  • To add Review Required on money request preview, we will add the hasNoticeViolations check here
    const hasNoticeViolations = violations.filter((v) => v.type === 'notice').length > 1
    
      ...
    
    message += ` • ${ (isTooLong && hasNoticeViolations) ? ...

@Krishna2323
Copy link
Contributor

Krishna2323 commented Feb 10, 2024

Proposal

Please re-state the problem that we are trying to solve in this issue.

Implement Receipt Audit Feature / Note type violations

What is the root cause of that problem?

New Feature

What changes do you think we should make in order to solve the problem?

Steps we will follow:

  1. Create a new component called ReceiptAudit.tsx, which accepts notes as a prop.
The component will look like:
import React from 'react';
import {View} from 'react-native';
import Icon from '@components/Icon';
import * as Expensicons from '@components/Icon/Expensicons';
import Text from '@components/Text';
import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';

export default function ReceiptAudit({notes}: {notes: string[]}) {
    const styles = useThemeStyles();
    const theme = useTheme();

    return (
        <View style={[styles.mt2, styles.mb1, styles.ph5]}>
            <View style={[styles.flexRow, styles.alignItemsCenter, styles.gap2]}>
                <View style={[styles.textReceiptAuditTitle, {backgroundColor: notes.length ? theme.danger : theme.success}]}>
                    <Icon
                        width={18}
                        height={18}
                        src={notes.length > 0 ? Expensicons.Receipt : Expensicons.Checkmark}
                        fill={theme.white}
                    />
                    <Text style={[styles.textLabel, styles.textStrong, styles.textWhite]}>
                        {notes.length > 0 ? `Receipt Audit : ${notes.length} Issue(s) Found` : 'Receipt Verified : No issues Found'}
                    </Text>
                </View>
            </View>
            
            // If notes is a array of strings, map through it & show notes.
            <View style={[styles.mv1, styles.gap1]}>{notes.length > 0 && notes.map((message) => <Text style={[styles.textLabelError]}>{message}</Text>)}</View>
        </View>
    );
}
  1. Below this line add the component and pass notes={transactionViolation.notice}

  2. Add translations for: Receipt Audit,Receipt Verified, X Issue(s) Found, No issues Found.

  3. Use the translation inside ReceiptAudit component.

  4. If required add translations and util functions for the notes messages just like we do for ViolationMessages.

  5. Hide the ReceiptAudit component when receipt is being scanned or show scanning... inside ReceiptAudit.

  6. For RBR for in LHS we can pass something like hasNotesViolations to getOptionData util function and based on that we will set result.brickRoadIndicator, we will use notes violation. Or we can modify hasViolation to also check for notes violation and return appropriate boolean based on that.

result.brickRoadIndicator = hasErrors || hasViolations ? CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR : '';

  1. We need to modify code blow to also check for notes violation. Or we can create new elseIf statement for notes violation and we will use ReportUtils.isReportApproved(iouReport) && !ReportUtils.isSettled(iouReport) to get the approved/reimbursed state.
    if (hasViolations && transaction) {
    const violations = TransactionUtils.getTransactionViolations(transaction, transactionViolations);
    if (violations?.[0]) {
    const violationMessage = ViolationsUtils.getViolationTranslation(violations[0], translate);
    const isTooLong = violations.filter((v) => v.type === 'violation').length > 1 || violationMessage.length > 15;
    message += ` • ${isTooLong ? translate('violations.reviewRequired') : violationMessage}`;
    }

Minor stylings can be adjusted by taking suggestions by the design team.

Result

desktop_error
More SSs of the same style implementation. android_chrome_success android_chrome desktop_success

Style 2 Result

Screenshot 2024-02-10 at 5 19 12 PM
More SS of the same style styles_2_web_error

@kevalacme
Copy link

hi

@melvin-bot melvin-bot bot added the Overdue label Feb 12, 2024
Copy link

melvin-bot bot commented Feb 12, 2024

📣 @kevalacme! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

  1. Make sure you've read and understood the contributing guidelines.
  2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
  3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
  4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@JmillsExpensify
Copy link
Author

@sobitneupane Thoughts on the proposals so far?

@melvin-bot melvin-bot bot removed the Overdue label Feb 12, 2024
@sobitneupane
Copy link
Contributor

Thanks for the proposal @neonbhai and @Krishna2323

Note type violations do not generate an RBR in the LHN for admins or members, because notes are informational and there is nothing for either admin or member to fix. However, we will do the following:
Update the dot separator sub-state for the request preview with Review required
Note: Once the report moves to either an approved or reimbursed state, whichever comes first, the Review required is removed as a sub-state. This is because while the informational note remains, there is never anything in particular to resolve or dismiss, like other violations.
Highlight notes in Guided Review for extra review (more in out of scope)

This portion of the request is missing in both the proposals.

@sobitneupane
Copy link
Contributor

@JmillsExpensify Can we get more details on this portion of the requirement.

  • Note type violations do not generate an RBR in the LHN for admins or members, because notes are informational and there is nothing for either admin or member to fix. However, we will do the following:

    • Update the dot separator sub-state for the request preview with Review required

      • Note: Once the report moves to either an approved or reimbursed state, whichever comes first, the Review required is removed as a sub-state. This is because while the informational note remains, there is never anything in particular to resolve or dismiss, like other violations.
    • Highlight notes in Guided Review for extra review (more in out of scope)

@kevalacme
Copy link

Contributor details
Your Expensify account email: [email protected]
Upwork Profile Link: https://www.upwork.com/fl/~01de57443ab6a9b794

Copy link

melvin-bot bot commented Feb 15, 2024

⚠️ Missing/invalid email or upwork profile link. Please make sure you add both your Expensify email and Upwork profile link in the format specified.

Copy link

melvin-bot bot commented Feb 16, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Feb 19, 2024
Copy link

melvin-bot bot commented Feb 20, 2024

@JmillsExpensify, @sobitneupane Huh... This is 4 days overdue. Who can take care of this?

@JmillsExpensify
Copy link
Author

@sobitneupane Can you help me understand what detail you're looking for? Comments on each point though.

Note type violations do not generate an RBR in the LHN for admins or members, because notes are informational and there is nothing for either admin or member to fix.

Any questions on this. RBR is the red dot that appears next to a chat that appears in the LHN

  • Update the dot separator sub-state for the request preview with Review required
    - Note: Once the report moves to either an approved or reimbursed state, whichever comes first, the Review required is removed as a sub-state. This is because while the informational note remains, there is never anything in particular to resolve or dismiss, like other violations.

Does this image help?
Screenshot 2024-02-20 at 18 52 33

  • Highlight notes in Guided Review for extra review (more in out of scope)

This is a forward looking concern, so not relevant for this issue. I removed it from the OP above.

@melvin-bot melvin-bot bot removed the Overdue label Feb 21, 2024
@JmillsExpensify
Copy link
Author

I believe that is answered in the screenshot above. No red dot visible in the LHN, so no RBR.

@JmillsExpensify
Copy link
Author

For the sake of clarity though, the only time RBR should appear for the transaction thread is when both and these statements are true:

  • There are violations AND
  • Submitter can still make edits (either open or processing report, depending on the configuration of the workspace.

If both of those statements are not true, then RBR won't show in the LHN for the submitter. Further the transaction thread is never RBR for the manager or the admin/owner.

@cead22 cead22 self-assigned this Apr 30, 2024
Copy link

melvin-bot bot commented May 1, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Copy link

melvin-bot bot commented May 1, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Copy link

melvin-bot bot commented May 2, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 2, 2024
@melvin-bot melvin-bot bot changed the title [Violations] [$500] Implement Receipt Audit Feature / Note type violations [HOLD for payment 2024-05-09] [Violations] [$500] Implement Receipt Audit Feature / Note type violations May 2, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label May 2, 2024
Copy link

melvin-bot bot commented May 2, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented May 2, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.69-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-05-09. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented May 2, 2024

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@sobitneupane] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
  • [@JmillsExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels May 8, 2024
@Krishna2323
Copy link
Contributor

@JmillsExpensify, bump for payments 😄

@melvin-bot melvin-bot bot added the Overdue label May 10, 2024
Copy link

melvin-bot bot commented May 13, 2024

@cead22, @JmillsExpensify, @lakchote, @sobitneupane, @Krishna2323 Eep! 4 days overdue now. Issues have feelings too...

@JmillsExpensify
Copy link
Author

Payment summary:

@melvin-bot melvin-bot bot removed the Overdue label May 14, 2024
@JmillsExpensify
Copy link
Author

Contributor contract paid out via Upwork. C+ is paid via New Expensify, so I'm going to go ahead and close the issue and circle back when I receive a payment request. @sobitneupane Don't forget to get the BZ checklist completed. I definitely think we need a series of regression tests for this feature.

@sobitneupane
Copy link
Contributor

Oops I missed the issue.
Regression Test Proposal:

  1. Submit an expense to a paid policy group with a valid receipt.
  2. Wait for the receipt to be scanned and then verify that the "Receipt Verified" message is shown above the receipt.
  3. Change the amount to something higher than the scanned receipt.
  4. Verify that:
    - The "Issue Found" message is displayed above the receipt.
    - A violation message is displayed below the Amount field.
    - The "Review required" substate is displayed in the Request Preview.
  5. Change the date field.
  6. Verify that:
    - The "Issue Found" header is displayed above the receipt.
    - A violation message is displayed below the Date field.
    - The "Review required" substate is displayed in the Request Preview.
  7. If the receipt scan is not successful, the "Receipt not verified. Please confirm accuracy." violation message should be shown below the receipt with the "Issue Found" message above the receipt and "Review required" substate in Request Preview.

Do we agree 👍 or 👎

@cead22
Copy link
Contributor

cead22 commented Jun 20, 2024

@JmillsExpensify I can add a test to the violations test script based on this ^

@JmillsExpensify
Copy link
Author

Ok thanks @cead22! Sounds good.

I need to have someone else confirm this payment summary that isn't me, so I reached out to someone else on the BZ team.

@mallenexpensify
Copy link
Contributor

Contributor: @Krishna2323 paid $500 via Upwork
Contributor+: @sobitneupane owed $500 via NewDot

@JmillsExpensify
Copy link
Author

$500 approved for @sobitneupane

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
Archived in project
Development

No branches or pull requests

10 participants