Skip to content

Update receipt not smart scanned copy and render link in error #60830

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

Closed
wants to merge 32 commits into from

Conversation

neil-marcellini
Copy link
Contributor

@neil-marcellini neil-marcellini commented Apr 24, 2025

Explanation of Change

In relation to this PR to scan attached receipts in the background, extract the code changes that render links in error messages. It will be generally useful to be able to do such a thing, for example this PR [App] [HOLD pull 58099] Add cta to update to usd modal is waiting until that is implemented. The background scan PR is blocked until more backend changes are made, so let's get this part out now.

To create this PR with only these changes I branched off the original PR and then restored all files to main that didn't have anything to do with rendering the links. That's why there's so many commits here.

Related Issues

#53535
PROPOSAL: N/A

Tests

  1. Create a collect workspace if needed
  2. Invite a member
  3. Log in as the member
  4. Create an expense on the workspace entering an amount and merchant
  5. Open the expense and upload a receipt with a different amount and merchant
  6. Submit the report if it isn't already
  7. Log in as the workspace owner and open the expense report
  8. Verify that a violation appears explaining why the receipt was not smart scanned
  9. Verify it has a link taking you to the help page for receipt auditing
  10. Open the expense detail view
  11. Verify the link is displayed
2025-04-24_07-24-46.mp4

Text wraps well for errors

2025-04-29_07-00-11.mp4
  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Same as tests

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
    • MacOS: Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

See above, only tested on web since it should be platform independent.

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop

@neil-marcellini neil-marcellini self-assigned this Apr 24, 2025
@neil-marcellini neil-marcellini marked this pull request as ready for review April 24, 2025 17:02
@neil-marcellini
Copy link
Contributor Author

Ok @hoangzinh @shawnborton the text wrapping is fixed now and it's ready for more review.

@hoangzinh
Copy link
Contributor

The perf-test has failed. @neil-marcellini can you try to re-run it? I don't think this PR causes any perf issue.

@shawnborton
Copy link
Contributor

Nice, are there updated videos somewhere?

@neil-marcellini
Copy link
Contributor Author

There is one updated video in the PR description showing the text wrapping

@shawnborton
Copy link
Contributor

Ah okay, thanks! Here it is for quick review:
CleanShot 2025-04-29 at 11 48 14@2x

I thought we wanted to truncate after 1 line for these though? Was that your understanding too @trjExpensify ?

(PS - sorry for all of the pings today mate!)

@trjExpensify
Copy link
Contributor

I thought we wanted to truncate after 1 line for these though? Was that your understanding too @trjExpensify ?

Yeeeep, for the expense rows in the report view:

Violations appear on the expense rows in-line, making use of the available space to display the error text when screen size allows. Multiple violations will be shown comma separated and truncated at one line.

@shawnborton
Copy link
Contributor

Awesome, thanks for confirming - @neil-marcellini can we update please? Thanks!

@neil-marcellini
Copy link
Contributor Author

Ah ok, I'm glad I asked because it did seem a little intentional. I'll revert to what I had before. I'm cool with it because the user can always click into it to show the full violation.

That being said I might have to fix it so it truncates with ellipsis.

@neil-marcellini neil-marcellini marked this pull request as draft April 29, 2025 17:02
@neil-marcellini
Copy link
Contributor Author

On the latest main I don't see us rendering the transaction item with the error message anymore, so I think we're all good here. Do let me know if you find steps that lead to that though and I can fix it.

image

@hoangzinh
Copy link
Contributor

Yep, in the latest main, we don't show errors even in Reports.

Screen.Recording.2025-05-01.at.07.16.46.mov

@shawnborton
Copy link
Contributor

Hmm that seems like a bug, @Kicu @mountiny can you confirm? On the table view, if the expense has a violation, it should still appear right within the table row below all of the row data.

@neil-marcellini
Copy link
Contributor Author

neil-marcellini commented May 1, 2025

Can we please move this forward despite any bugs on main? That's not really in scope for PRs typically. This PR is also holding two other issues.

Ah maybe that's a bad idea, because it will still truncate without ellipsis. I'll see if I can find out why it's not rendering the error.

@shawnborton
Copy link
Contributor

Well, we need to make sure we are truncating after one line for this view when main does indeed get fixed:
CleanShot 2025-04-30 at 20 28 59@2x

How do you want to proceed?

@neil-marcellini
Copy link
Contributor Author

Yep I edited my message above, I will investigate

@neil-marcellini
Copy link
Contributor Author

I found the logic here which controls whether this is rendered. I think I can make it display and fix the truncating. Back to draft 🙃

@neil-marcellini neil-marcellini marked this pull request as draft May 1, 2025 00:38
@shawnborton
Copy link
Contributor

Sounds good, thanks Neil!

@hoangzinh
Copy link
Contributor

@neil-marcellini could you also check in native apps? I couldn't click in the link "Learn more"

Screen.Recording.2025-05-01.at.08.01.58.mov

@neil-marcellini
Copy link
Contributor Author

I'm a bit out of my depths here, so I created an issue to get an expert on it. I like to think I could figure it out eventually, but it will be much more efficient this way 😄

@MarioExpensify
Copy link
Contributor

@neil-marcellini we won't be moving forward with this PR right? Can we close it?

@hoangzinh
Copy link
Contributor

I think we can close this PR. It has been implemented here #61748

@neil-marcellini
Copy link
Contributor Author

Yes good point, thanks for helping me clean up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants