-
Notifications
You must be signed in to change notification settings - Fork 2
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
Updated submission POST based on story, updated pytests #106
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
src/services/submission_processor.py
Outdated
async def validate_submission(lei: str, submission: SubmissionDAO, content: bytes, background_tasks: BackgroundTasks): | ||
df = pd.read_csv(BytesIO(content), dtype=str, na_filter=False) | ||
validator_version = imeta.version("regtech-data-validator") | ||
|
||
submission.state = SubmissionState.VALIDATION_IN_PROGRESS | ||
submission.validation_ruleset_version = validator_version | ||
# Set VALIDATION_IN_PROGRESS | ||
await update_submission( | ||
SubmissionDAO( | ||
submitter=submission_id, | ||
state=SubmissionState.VALIDATION_IN_PROGRESS, | ||
validation_ruleset_version=validator_version, | ||
) | ||
) | ||
background_tasks.add_task(validate_and_update_submission, df, lei, submission_id, validator_version) | ||
submission = await update_submission(submission) | ||
background_tasks.add_task(validate_and_update_submission, df, lei, submission) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this needs to be a task to just update the status anymore, once the file's uploaded to storage, we can say validation has started, then this method would just merge with the validate_and_update; don't think we need to have 2 bg tasks. (submission_uploaded will be valid once we have a separate microservice that listens on s3 events to actually do the validation much further down the line, but for now, I think we can just skip it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #51