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

feat(RECAPDocument): Validate attachment number against document type #4703

Merged
merged 14 commits into from
Nov 19, 2024

Conversation

elisa-a-v
Copy link
Contributor

Resolves #3982

This PR adds a model-wide validation for the RECAPDocument model that checks that:

  • If the doc is a main PACER doc, no attachment number is provided
  • If the doc is an attachment, an attachment number is provided

It also fixes an issue where previously empty RECAPDocuments didn't have their document_type updated, so if the rd was an attachment, the attachment number was added but the document type remained as a main PACER document. We now update the document type every time, so this should not happen again.

Given the nature of the changes, we're now logging errors in several places for future troubleshooting, and a new sentry setting was added which enables attaching the entire stack trace to sentry logs.

this also adds tests and updates docs to reflect changes.

…ocument_type

- Added validation to ensure attachment_number is null for main PACER documents.
- Moved the preexisting attachment validation from othe save method to the clean method to enhance separation of concerns.
- Updated save to call clean ensuring model-wide validation
… ATTACHMENT for RECAPDocuments with attachment_number
- Separated handling of `DoesNotExist` and `MultipleObjectsReturned` exceptions in the document creation process.
- Added logging for cases where multiple `RECAPDocument` objects are unexpectedly returned.
- Retained existing behavior of creating a new `RECAPDocument` in these cases, with Sentry logging for future debugging.
- Refined error message to reflect possible causes, including unique constraint violations or document type validation issues.
- Added the `attach_stacktrace` setting in Sentry configuration to include stack traces in logged errors.
…ean method

- Added tests to verify that attachments must have an attachment_number.
- Ensured main PACER documents do not have an attachment_number.
- Checked that appropriate ValidationErrors are raised with correct messages.
- Confirmed that valid scenarios save without errors.
Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

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

This looks about right to me. I didn't read the tests, and I'm interested to see how the sentry change affects other things, but it looks basically right. I'll let Eduardo make the final call!

Thank you!

Copy link
Contributor

@ERosendo ERosendo left a comment

Choose a reason for hiding this comment

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

@elisa-a-v Code looks good. Thanks for updating the comments. There's just one small detail we need to address before merging.

cl/search/models.py Show resolved Hide resolved
@mlissner mlissner merged commit 99c184a into main Nov 19, 2024
15 checks passed
@mlissner mlissner deleted the recap-document-validation branch November 19, 2024 17:38
Copy link

sentry-io bot commented Nov 19, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ attachment_number must be null for a main PACER document. /api/rest/{version}/recap/ View Issue
  • ‼️ ValidationError: {'attachment_number': ['attachment_number must be null for a main PACER document.']} cl.corpus_importer.tasks.upload_pdf_to_ia View Issue
  • ‼️ attachment_number must be null for a main PACER document. cl.corpus_importer.tasks.upload_pdf_to_ia View Issue
  • ‼️ attachment_number must be null for a main PACER document. cl.recap.tasks.fetch_docket View Issue

Did you find this useful? React with a 👍 or 👎

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

Successfully merging this pull request may close these issues.

RECAP Upload API shouldn't accept attachment numbers for non-attachments
3 participants