-
Notifications
You must be signed in to change notification settings - Fork 4
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/2646 total errors aggregates #2800
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2800 +/- ##
===========================================
- Coverage 93.62% 93.56% -0.06%
===========================================
Files 262 265 +3
Lines 6055 6080 +25
Branches 508 510 +2
===========================================
+ Hits 5669 5689 +20
- Misses 291 298 +7
+ Partials 95 93 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
@@ -27,6 +27,8 @@ def parse(data_file_id): | |||
|
|||
if "Case Data" in data_file.section: |
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.
not related to this PR but we should have had "Case Data" as a CONSTANT, and I would put it in .DataFile.py
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.
Looks good!
month_int = month_to_int(month) | ||
rpt_month_year = int(f"{calendar_year}{month_int}") | ||
|
||
error_count = ParserError.objects.filter( |
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.
we should be able to query all ParserErrors before running the for loop, and have the counts in a table.
Another note: as we are parsing more and more files, this query would take forever to run, the problem is df is indexed easily but the rpt_month_year will be slow.
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 pulled the main ParserError.objects.all()
out of the loop - are you suggesting grouping by rpt_month_year or something to that effect as well?
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.
more efficient query!
@jtimpe did this fall off my radar? |
nope! i am still addressing PR feedback and getting approvals - once i've done so i'll follow back up with you |
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!
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.
Good to go!
@jtimpe there is a merge conflict here. can you ping me when its ready again? |
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 @jtimpe 🚀
- Section 3 and 4 total errors for tanf, tribal, and ssp get populated with 0 or non-zero values when status !=
Rejected
- there are a couple of edge cases that I presume will be addressed in later tickets (let me know if this is not the case):
Accepted with errors
status and only error detected is in trailer record: Total Errors == 0⚠️ (maybe Cat1 Cleanup will resolve this )- any
Partially Accepted with Errors
status: Total Errors == 0 – this status only comes up when at least one data record (not header/trailer) is not correct length to parse. stratum and aggregate data files only have one data record. so this status does not make sense.⚠️ will be addressed in #2754
test notes here
Summary of Changes
Pull request closes #2646
total_errors_by_month
aggregation function, implemented for section 3+4 submissionsSubmissionHistory
component structure to be more readable and limit conditional rendering. some small code duplications and inefficiencies can be ironed out if necessaryHow to Test
List the steps to test the PR
These steps are generic, please adjust as necessary.
Deliverables
More details on how deliverables herein are assessed included here.
Deliverable 1: Accepted Features
Checklist of ACs:
case_aggregates
populated with the correct data for the sectionlfrohlich
and/oradpennington
confirmed that ACs are met.Deliverable 2: Tested Code
CodeCov Report
comment in PR)CodeCov Report
comment in PR)Deliverable 3: Properly Styled Code
Deliverable 4: Accessible
iamjolly
andttran-hub
using 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):