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

fix(expense): Expenses with no summary fields #379

Merged
merged 1 commit into from
Jun 20, 2024

Conversation

athewsey
Copy link
Contributor

Issue #, if available: #370

Description of changes:

  • Stop textractor.parsers.parser_analyze_expense_response from ignoring ExpenseDocuments where no summary fields were detected.
  • Add a test case to verify that expense documents with no summary fields are not ignored.
  • Add a warning in the event that page number can't be determined for an ExpenseDocument and it needs to be skipped: Since silent skipping was confusing for users & debugging.

Confirmed with customer from #370 / @arsher-b in call that they're okay for the shared document to be included as a test case.

I didn't bother creating an extra test case for the warning branch (page number could not be inferred) yet because:

  1. It seemed tricky
    • Do we have any existing tests for whether logging.warning gets called?
    • I can't think of any image that would generate it, so think we'd need to just create a synthetic JSON
  2. I don't think it was covered by previously existing tests anyway

...But guess we could revisit this if needed


By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Stop textractor.parsers.parser_analyze_expense_response from ignoring
ExpenseDocuments where no summary fields were detected.
@athewsey athewsey requested a review from Belval June 20, 2024 06:15
Copy link
Contributor

@Belval Belval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Belval Belval merged commit ef16cf2 into aws-samples:master Jun 20, 2024
1 of 2 checks passed
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.

2 participants