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

2711-catch-rpt-month-year-mismatches #2789

Merged
merged 21 commits into from
Jan 22, 2024

Conversation

raftmsohani
Copy link

@raftmsohani raftmsohani commented Dec 21, 2023

Summary of Changes

Pull request closes #2711 _

Closes the gap between reporting month/year from frontend and month/year of the header in the reporting file.

How to Test

Start the app by:

cd tdrs-backend && docker-compose -f docker-compose.yml -f docker-compose.local.yml up -d 
cd tdrs-frontend && docker-compose -f docker-compose.yml -f docker-compose.local.yml up -d
  1. Open http://localhost:3000/ and sign in.
  2. Submit a file. Make sure the reporting year/month/quarter matches the header file.
  3. Confirm that the file is submitted successfully.
  4. Submit another file and mismatch the frontend reporting year/quarter with the file header.
  5. Confirm that the file throws error and is not accepted.

Deliverables

More details on how deliverables herein are assessed included here.

Deliverable 1: Accepted Features

Checklist of ACs:

  • DataFile with header RMY mismatch is rejected via pre-check and case_aggregates show as "N/A"
  • lfrohlich and/or adpennington confirmed that ACs are met.

Deliverable 2: Tested Code

  • Are all areas of code introduced in this PR meaningfully tested?
    • If this PR introduces backend code changes, are they meaningfully tested?
    • If this PR introduces frontend code changes, are they meaningfully tested?
  • Are code coverage minimums met?
    • Frontend coverage: [insert coverage %] (see CodeCov Report comment in PR)
    • Backend coverage: [insert coverage %] (see CodeCov Report comment in PR)

Deliverable 3: Properly Styled Code

  • Are backend code style checks passing on CircleCI?
  • Are frontend code style checks passing on CircleCI?
  • Are code maintainability principles being followed?

Deliverable 4: Accessible

  • Does this PR complete the epic?
  • Are links included to any other gov-approved PRs associated with epic?
  • Does PR include documentation for Raft's a11y review?
  • Did automated and manual testing with iamjolly and ttran-hub using Accessibility Insights reveal any errors introduced in this PR?

Deliverable 5: Deployed

  • Was the code successfully deployed via automated CircleCI process to development on Cloud.gov?

Deliverable 6: Documented

  • Does this PR provide background for why coding decisions were made?
  • If this PR introduces backend code, is that code easy to understand and sufficiently documented, both inline and overall?
  • If this PR introduces frontend code, is that code easy to understand and sufficiently documented, both inline and overall?
  • If this PR introduces dependencies, are their licenses documented?
  • Can reviewer explain and take ownership of these elements presented in this code review?

Deliverable 7: Secure

  • Does the OWASP Scan pass on CircleCI?
  • Do manual code review and manual testing detect any new security issues?
  • If new issues detected, is investigation and/or remediation plan documented?

Deliverable 8: User Research

Research product(s) clearly articulate(s):

  • the purpose of the research
  • methods used to conduct the research
  • who participated in the research
  • what was tested and how
  • impact of research on TDP
  • (if applicable) final design mockups produced for TDP development

@raftmsohani
Copy link
Author

raftmsohani commented Dec 28, 2023

@ADPennington: I have the following two questions:

  1. Are we only checking for quarter to match? or do we need to check year as well?
  2. Should we account for fiscal quarter when validating the quarter?

@ADPennington
Copy link
Collaborator

@ADPennington: I have the following two questions:

  1. Are we only checking for quarter to match? or do we need to check year as well?

both, and together, i think. this ticket originated here.

  1. Should we account for fiscal quarter when validating the quarter?

i'm not sure if fiscal quarter can be validated by itself. generally speaking, fiscal year + fiscal quarter selections need to map to the calendar periods in the files. If they dont map correctly, the file should be rejected and not parsed. i think this looks a bit different by section:

  • section 1: FY2024Q1 (frontend selections) means RPT_MONTH_YEAR in the T1/M2, T2/M2, and M3/T3 records should have values equal to 202310, 202311 or 202312, and so on...
  • section 2: similar to Section 1 but for T4/M4 and T5/M5 records
  • section 3: FY2024Q1 means CALENDAR_QUARTER in the T6/M6 record should have a value equal to 20234, and so on...
  • section 4: similar to section 3 but for T7/M7 records

@raftmsohani
Copy link
Author

@ADPennington: I have the following two questions:

  1. Are we only checking for quarter to match? or do we need to check year as well?

both, and together, i think. this ticket originated here.

  1. Should we account for fiscal quarter when validating the quarter?

i'm not sure if fiscal quarter can be validated by itself. generally speaking, fiscal year + fiscal quarter selections need to map to the calendar periods in the files. If they dont map correctly, the file should be rejected and not parsed. i think this looks a bit different by section:

  • section 1: FY2024Q1 (frontend selections) means RPT_MONTH_YEAR in the T1/M2, T2/M2, and M3/T3 records should have values equal to 202310, 202311 or 202312, and so on...
  • section 2: similar to Section 1 but for T4/M4 and T5/M5 records
  • section 3: FY2024Q1 means CALENDAR_QUARTER in the T6/M6 record should have a value equal to 20234, and so on...
  • section 4: similar to section 3 but for T7/M7 records

@ADPennington After discussing with the team, we decided to compare the HEADER year and quarter with frontend date, and draft another ticket for further work to compare each field with frontend date.

@ADPennington
Copy link
Collaborator

@ADPennington: I have the following two questions:

  1. Are we only checking for quarter to match? or do we need to check year as well?

both, and together, i think. this ticket originated here.

  1. Should we account for fiscal quarter when validating the quarter?

i'm not sure if fiscal quarter can be validated by itself. generally speaking, fiscal year + fiscal quarter selections need to map to the calendar periods in the files. If they dont map correctly, the file should be rejected and not parsed. i think this looks a bit different by section:

  • section 1: FY2024Q1 (frontend selections) means RPT_MONTH_YEAR in the T1/M2, T2/M2, and M3/T3 records should have values equal to 202310, 202311 or 202312, and so on...
  • section 2: similar to Section 1 but for T4/M4 and T5/M5 records
  • section 3: FY2024Q1 means CALENDAR_QUARTER in the T6/M6 record should have a value equal to 20234, and so on...
  • section 4: similar to section 3 but for T7/M7 records

@ADPennington After discussing with the team, we decided to compare the HEADER year and quarter with frontend date, and draft another ticket for further work to compare each field with frontend date.

sounds good @raftmsohani; thank you. please note the header year/quarter represent a calendar period, and frontend year/qtr represents a fiscal period.

Copy link

codecov bot commented Jan 4, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (d5a44ff) 93.64% compared to head (3f600d5) 93.64%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2789   +/-   ##
========================================
  Coverage    93.64%   93.64%           
========================================
  Files          262      263    +1     
  Lines         6057     6060    +3     
  Branches       504      507    +3     
========================================
+ Hits          5672     5675    +3     
- Misses         288      290    +2     
+ Partials        97       95    -2     
Flag Coverage Δ
dev-backend 93.80% <95.65%> (+<0.01%) ⬆️
dev-frontend 92.83% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
tdrs-backend/tdpservice/parsers/fields.py 88.63% <ø> (ø)
tdrs-backend/tdpservice/parsers/parse.py 87.43% <100.00%> (+0.49%) ⬆️
tdrs-backend/tdpservice/parsers/row_schema.py 94.33% <100.00%> (+0.86%) ⬆️
...s-backend/tdpservice/parsers/schema_defs/ssp/m1.py 100.00% <100.00%> (ø)
...s-backend/tdpservice/parsers/schema_defs/ssp/m2.py 100.00% <100.00%> (ø)
...s-backend/tdpservice/parsers/schema_defs/ssp/m3.py 100.00% <100.00%> (ø)
...s-backend/tdpservice/parsers/schema_defs/ssp/m4.py 100.00% <100.00%> (ø)
...s-backend/tdpservice/parsers/schema_defs/ssp/m5.py 100.00% <100.00%> (ø)
...s-backend/tdpservice/parsers/schema_defs/ssp/m6.py 100.00% <100.00%> (ø)
...s-backend/tdpservice/parsers/schema_defs/ssp/m7.py 100.00% <100.00%> (ø)
... and 18 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5a44ff...3f600d5. Read the comment docs.

@raftmsohani
Copy link
Author

As a follow up, have created this issue: #2799

@raftmsohani raftmsohani added raft review This issue is ready for raft review and removed WIP labels Jan 5, 2024
Copy link
Collaborator

@andrew-jameson andrew-jameson left a comment

Choose a reason for hiding this comment

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

LGTM

@raftmsohani raftmsohani force-pushed the 2711-catch-rpt-month-year-mismatches branch from 2c42c71 to 620bd22 Compare January 18, 2024 20:46
@ADPennington ADPennington added the Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI label Jan 19, 2024
Copy link
Collaborator

@ADPennington ADPennington left a comment

Choose a reason for hiding this comment

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

lgtm @raftmsohani 🚀

Verified the following:

  • pre-parsing error is raised when header cal period does not map properly to frontend fiscal periods
    e.g. Submitted reporting year:2022, quarter:Q4 doesn't match file reporting year:2022, quarter:Q2.
  • this error is raised for all sections and all data types (TANF, SSP, Tribal TANF)
  • file status is rejected.

#2799 to deal with the scenario when other records have a reporting period that is not expected.

@ADPennington ADPennington added Ready to Merge and removed raft review This issue is ready for raft review QASP Review Deploy with CircleCI-qasp Deploy to https://tdp-frontend-qasp.app.cloud.gov through CircleCI labels Jan 19, 2024
@andrew-jameson andrew-jameson merged commit 92b0a69 into develop Jan 22, 2024
25 checks passed
@andrew-jameson andrew-jameson deleted the 2711-catch-rpt-month-year-mismatches branch January 22, 2024 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Catch rpt_month_year mismatches
5 participants