3205 reparse command refactor #3361
Conversation
…TANF-app into 3205-reparse-command-refactor
| @@ -1,6 +1,4 @@ | |||
| # Base Docker compose for all environments | |||
| version: "3.4" | |||
| delete_associated_models, | ||
| count_total_num_records, | ||
| calculate_timeout, | ||
| handle_datafiles, |
There was a problem hiding this comment.
does it make sense to just have all these functions in the reparse.py file?
There was a problem hiding this comment.
The idea was to have a cleaner reparse file while also we can re-use some of these functions if needed. However, for some of these functions it might make sense to move them to reparse, for example handle_datafile is specific to reparse
| ) | ||
|
|
||
| is_sequential = assert_sequential_execution(log_context) | ||
| should_exit(not is_sequential) |
There was a problem hiding this comment.
Should we update this to raise an exception/return an error to the user so that they know why the reparse didn't happen? Even writing to the console would be a start.
There was a problem hiding this comment.
Also, can we move this to the very beginning of the function to avoid unnecessary computation if we arent sequential?
| fiscal_quarter = None | ||
| fiscal_year = None | ||
| all_reparse = False | ||
| new_indices = False |
There was a problem hiding this comment.
In a future ticket, could we deduce these fields from the selected datafiles?
There was a problem hiding this comment.
I agree, I will leave a todo
| ###### | ||
| files = DataFile.objects.filter(id__in=selected_files) | ||
| backup_file_name = "/tmp/reparsing_backup" | ||
| backup_file_name += "_selected_files" |
There was a problem hiding this comment.
Should this be removed since we won't put all the file IDs or other file meta in the backup name?
|
|
||
| delete_associated_models(meta_model, file_ids, new_indices, log_context) | ||
|
|
||
| meta_model.timeout_at = meta_model.created_at + calculate_timeout( |
There was a problem hiding this comment.
I know it'll be a bit slower. But could we calculate this at the very top of the function by summing the number of records in the selected files first instead of deleting stuff first and then updating the timeout? It feels like that could save us some grief in the future.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3361 +/- ##
===========================================
- Coverage 91.48% 91.09% -0.40%
===========================================
Files 299 301 +2
Lines 8595 8644 +49
Branches 636 637 +1
===========================================
+ Hits 7863 7874 +11
- Misses 615 653 +38
Partials 117 117
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
| return True | ||
|
|
||
|
|
||
| def should_exit(condition): |
There was a problem hiding this comment.
This should be needed after the management command is removed
Summary of Changes
Pull request closes #3205 _
Moved all the utility functions to utilize.py, moved frontend reparse command to a separate function and cleaned up clean_and_reparse management command.
How to Test
Follow steps below:
Deliverables
More details on how deliverables herein are assessed included here.
Deliverable 1: Accepted Features
Checklist of ACs:
lfrohlichand/oradpenningtonconfirmed that ACs are met.Deliverable 2: Tested Code
CodeCov Reportcomment in PR)CodeCov Reportcomment in PR)Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
iamjollyandttran-hubusing Accessibility Insights reveal any errors introduced in this PR?Deliverable 5: Deployed
Deliverable 6: Documented
Deliverable 7: Secure
Deliverable 8: User Research
Research product(s) clearly articulate(s):