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.

Sign up for GitHub

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

Jump to bottom

chore: refactor common code across lambda functions and fixes incorrect typing #403

Merged
merged 5 commits into from
Aug 29, 2024
Merged

chore: refactor common code across lambda functions and fixes incorrect typing #403

merged 5 commits into from
Aug 29, 2024

Conversation

as1729
Copy link
Contributor

@as1729 as1729 commented Aug 28, 2024 •
edited
Loading

This PR was created to fix two things:

  1. Typing errors within the python code related to TemporaryFileWrapper.
  2. Issue with the usage of json.dump. Unclear why this wasn't generating test failures but the function call was raising a type-issue for a missing argument.
  3. Consolidates the generation for s3 file keys for output files and stores it within treasury_generation_common.py. While this doesn't reduce the lines of code- I'm hoping that consolidating this reduces the chance of having a malformed object key anywhere else in the code as it appears to be recreated in multiple places.

@github-actions github-actions bot added bug Something isn't working python labels Aug 28, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 28, 2024 •
edited
Loading

Terraform Summary

Step Result
🖌 Terraform Format & Style ✅
⚙️ Terraform Initialization ✅
🤖 Terraform Validation ✅
📖 Terraform Plan ✅

Hint: If "Terraform Format & Style" failed, run terraform fmt -recursive from the terraform/ directory and commit the results.

Output

Validation Output 
Success! The configuration is valid.
Plan Summary
CHANGE RESOURCE
add aws_s3_object.origin_dist_artifact["static/js/app.0de605db.js"]
aws_s3_object.origin_dist_artifact["static/js/app.0de605db.js.LICENSE.txt"]
update aws_ecs_service.console
aws_s3_object.origin_dist_artifact["200.html"]
aws_s3_object.origin_dist_artifact["build-manifest.json"]
aws_s3_object.origin_dist_artifact["chunk-references.json"]
aws_s3_object.origin_dist_artifact["index.html"]
module.lambda_function-cpfCreateArchive.aws_lambda_function.this[0]
module.lambda_function-cpfValidation.aws_lambda_function.this[0]
module.lambda_function-graphql.aws_lambda_function.this[0]
module.lambda_function-processValidationJson.aws_lambda_function.this[0]
module.lambda_function-subrecipientTreasuryReportGen.aws_lambda_function.this[0]
module.lambda_function-treasuryProjectFileGeneration.aws_lambda_function.this[0]
recreate aws_ecs_task_definition.console
aws_s3_object.lambda_artifact-graphql
aws_s3_object.lambda_artifact-processValidationJson
aws_s3_object.lambda_artifact-python
module.lambda_function-cpfCreateArchive.aws_lambda_permission.current_version_triggers["StepFunctionTrigger"]
module.lambda_function-cpfValidation.aws_lambda_permission.current_version_triggers["S3BucketNotification"]
module.lambda_function-graphql.aws_lambda_permission.current_version_triggers["APIGateway"]
module.lambda_function-processValidationJson.aws_lambda_permission.current_version_triggers["S3BucketNotification"]
module.lambda_function-subrecipientTreasuryReportGen.aws_lambda_permission.current_version_triggers["StepFunctionTrigger"]
module.lambda_function-treasuryProjectFileGeneration.aws_lambda_permission.current_version_triggers["S3BucketNotification"]
module.lambda_function-treasuryProjectFileGeneration.aws_lambda_permission.current_version_triggers["StepFunctionTrigger"]
delete aws_s3_object.origin_dist_artifact["static/js/app.eac6033b.js"]
aws_s3_object.origin_dist_artifact["static/js/app.eac6033b.js.LICENSE.txt"]

Pusher: @as1729, Action: pull_request_target, Workflow: Continuous Integration

@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 28, 2024 •
edited
Loading

QA Summary

See our documentation for tips on how to resolve failing QA checks.

QA Check Result
🌐 Web Tests ✅
🔗 API Tests ✅
🐍 Python Tests ✅
📏 ESLint ✅
🧼 Ruff ✅
🧹 TFLint ✅

Test Coverage

Coverage report for api suite
St File % Stmts % Branch % Funcs % Lines Uncovered Line #s
🔴 All files 47.82 30.24 55.55 48.24
🔴  src 0 100 0 0
🔴   server.ts 0 100 0 0 6-13
🟢  src/directives/requireAuth 100 100 100 100
🟢   requireAuth.ts 100 100 100 100
🟡  src/directives/skipAuth 50 100 0 50
🟡   skipAuth.ts 50 100 0 50 13
🔴  src/functions 0 100 0 0
🔴   graphql.ts 0 100 0 0 15-27
🔴  src/functions/processValidationJson 38.55 50 44.44 37.8
🟢   processValidationJson.scenarios.ts 100 100 100 100
🔴   processValidationJson.ts 37.8 50 44.44 37.03 59-98,118-119,157-165,177-178,193-196,201-242,255-256,270-309,321-324,332
🔴  src/graphql 0 100 100 0
🔴   agencies.sdl.ts 0 100 100 0 1
🔴   expenditureCategories.sdl.ts 0 100 100 0 1
🔴   inputTemplates.sdl.ts 0 100 100 0 1
🔴   organizations.sdl.ts 0 100 100 0 1
🔴   outputTemplates.sdl.ts 0 100 100 0 1
🔴   projects.sdl.ts 0 100 100 0 1
🔴   reportingPeriods.sdl.ts 0 100 100 0 1
🔴   subrecipientUploads.sdl.ts 0 100 100 0 1
🔴   subrecipients.sdl.ts 0 100 100 0 1
🔴   uploadValidations.sdl.ts 0 100 100 0 1
🔴   uploads.sdl.ts 0 100 100 0 1
🔴   users.sdl.ts 0 100 100 0 1
🔴   validationRuleses.sdl.ts 0 100 100 0 1
🔴  src/lib 13.12 9.57 11.7 13.43
🟡   auth.ts 62.96 48.48 57.14 65.38 60-61,77-78,84-85,101-102,124,131,134,139-146,170,174
🔴   aws.ts 25.42 18.75 25 25.42 53-58,74-97,121-123,150-171,186-272
🟢   constants.ts 100 100 100 100
🔴   db.ts 45.45 50 50 45.45 15-35,41,43,50
🔴   ec-codes.ts 0 100 100 0 1
🟢   logger.ts 100 100 100 100
🔴   persist-upload.js 0 0 0 0 16-295
🔴   preconditions.ts 0 0 0 0 2-3
🔴   records.js 0 0 0 0 12-214
🔴   templateRules.ts 0 0 0 0
🔴   tracer.ts 0 100 100 0 5-14
🔴   validate-upload.js 0 0 0 0 18-790
🟢   validation-error.ts 83.33 100 50 83.33 22
🔴   validation-rules.js 0 0 0 0 6-194
🟡  src/services/agencies 67.34 50 80 67.34
🟢   agencies.scenarios.ts 100 100 100 100
🟡   agencies.ts 65.21 50 75 65.21 40-51,60-64,97-98,104,113-121
🟡  src/services/expenditureCategories 78.57 66.66 88.88 78.57
🟢   expenditureCategories.scenarios.ts 100 100 100 100
🟡   expenditureCategories.ts 77.77 66.66 88.88 77.77 30-34,49-52,60,91
🟡  src/services/inputTemplates 77.77 66.66 85.71 77.77
🟢   inputTemplates.scenarios.ts 100 100 100 100
🟡   inputTemplates.ts 76.92 66.66 85.71 76.92 25-29,39-40,50,85
🟡  src/services/organizations 76.81 88.88 55.55 76.81
🟢   organizations.scenarios.ts 100 100 100 100
🟡   organizations.ts 75.75 88.88 50 75.75 36-40,75,147-177,185,203-227
🟢  src/services/outputTemplates 82.85 66.66 85.71 82.85
🟢   outputTemplates.scenarios.ts 100 100 100 100
🟢   outputTemplates.ts 82.35 66.66 85.71 82.35 26-30,40-41,51,114
🟡  src/services/passage 74.07 62.5 100 74.07
🟡   passage.ts 74.07 62.5 100 74.07 18-19,65-76
🟡  src/services/projects 80 100 62.5 80
🟢   projects.scenarios.ts 100 100 100 100
🟡   projects.ts 78.57 100 62.5 78.57 45-51
🟡  src/services/reportingPeriods 69.23 70 54.54 69.23
🟢   reportingPeriods.scenarios.ts 100 100 100 100
🟡   reportingPeriods.ts 68.42 70 54.54 68.42 25-29,39-40,55-58,74,105-121
🟢  src/services/subrecipientUploads 88.88 83.33 85.71 88.88
🟢   subrecipientUploads.scenarios.ts 100 100 100 100
🟢   subrecipientUploads.ts 86.36 83.33 80 86.36 64,94-99
🟢  src/services/subrecipients 92.85 100 88.88 92.85
🟢   subrecipients.scenarios.ts 100 100 100 100
🟢   subrecipients.ts 90 100 81.81 90 53-58
🟡  src/services/uploadValidations 57.14 100 14.28 57.14
🟢   uploadValidations.scenarios.ts 100 100 100 100
🟡   uploadValidations.ts 53.84 100 14.28 53.84 10,16,30,38,45-48
🟢  src/services/uploads 91.34 71.42 88.63 91.34
🟢   uploads.scenarios.ts 100 100 100 100
🟢   uploads.ts 88.31 71.42 75 88.31 37,103,131-145,258-262
🟡  src/services/users 77.37 62 92 77.37
🟢   users.scenarios.ts 100 100 100 100
🟡   users.ts 75.96 62 88.88 75.96 202-204,222,230-239,252-255,277-279,288-292,310-311,325-328,346-348,356-357,362,371-374
🟢  src/services/validationRuleses 85.71 100 71.42 85.71
🟢   validationRuleses.scenarios.ts 100 100 100 100
🟢   validationRuleses.ts 84.61 100 71.42 84.61 43-48
Coverage report for web suite
St File % Stmts % Branch % Funcs % Lines Uncovered Line #s
🔴 All files 10.43 11.71 8.74 9.91
🟢  api/src/lib 100 100 100 100
🟢   constants.ts 100 100 100 100
🔴  web/src 28.57 18.75 66.66 28.57
🔴   App.tsx 0 0 0 0 3-36
🟢   Routes.tsx 100 100 100 100
🟡   auth.ts 50 50 100 50 19-24
🔴   entry.client.tsx 0 0 100 0 10-22
🔴  web/src/auth 7.14 0 4.16 7.14
🔴   localAuth.ts 9.09 0 8.33 9.09 39-68,76-80
🔴   passageAuth.ts 5 0 0 5 22-25,31-60
🔴  web/src/components/Agency/Agencies 0 100 0 0
🔴   Agencies.tsx 0 100 0 0 9-21
🔴  web/src/components/Agency/AgenciesCell 0 100 0 0
🔴   AgenciesCell.tsx 0 100 0 0 8-39
🔴  web/src/components/Agency/Agency 0 0 0 0
🔴   Agency.tsx 0 0 0 0 10-78
🔴  web/src/components/Agency/AgencyCell 0 100 0 0
🔴   AgencyCell.tsx 0 100 0 0 7-27
🔴  web/src/components/Agency/AgencyForm 0 0 0 0
🔴   AgencyForm.tsx 0 0 0 0 25-45
🔴  web/src/components/Agency/EditAgencyCell 0 100 0 0
🔴   EditAgencyCell.tsx 0 100 0 0 10-59
🔴  web/src/components/Agency/NewAgency 0 100 0 0
🔴   NewAgency.tsx 0 100 0 0 9-35
🟢  web/src/components/Navigation 100 60 100 100
🟢   Navigation.tsx 100 60 100 100 24-57
🔴  web/src/components/Organization/EditOrganizationCell 0 100 0 0
🔴   EditOrganizationCell.tsx 0 100 0 0 13-64
🔴  web/src/components/Organization/EditOrganizationForm 0 0 0 0
🔴   EditOrganizationForm.tsx 0 0 0 0 27-41
🔴  web/src/components/Organization/NewOrganization 0 100 0 0
🔴   NewOrganization.tsx 0 100 0 0 9-37
🔴  web/src/components/Organization/NewOrganizationForm 0 0 0 0
🔴   NewOrganizationForm.tsx 0 0 0 0 25-54
🔴  web/src/components/Organization/Organization 0 0 0 0
🔴   Organization.tsx 0 0 0 0 10-70
🔴  web/src/components/Organization/OrganizationCell 0 100 0 0
🔴   OrganizationCell.tsx 0 100 0 0 7-28
🔴  web/src/components/Organization/OrganizationPickListsCell 40 0 27.27 36.36
🟡   OrganizationPickListsCell.mock.ts 50 100 0 100
🔴   OrganizationPickListsCell.stories.tsx 0 0 0 0 6-32
🟡   OrganizationPickListsCell.tsx 64.28 100 50 58.33 14-16,47-70
🔴  web/src/components/Organization/Organizations 0 100 0 0
🔴   Organizations.tsx 0 100 0 0 9-21
🔴  web/src/components/Organization/OrganizationsCell 0 100 0 0
🔴   OrganizationsCell.tsx 0 100 0 0 8-37
🔴  web/src/components/OutputTemplate/EditOutputTemplateCell 0 100 0 0
🔴   EditOutputTemplateCell.tsx 0 100 0 0 18-81
🔴  web/src/components/OutputTemplate/NewOutputTemplate 0 0 0 0
🔴   NewOutputTemplate.tsx 0 0 0 0 17-126
🔴  web/src/components/OutputTemplate/OutputTemplate 0 0 0 0
🔴   OutputTemplate.tsx 0 0 0 0 17-97
🔴  web/src/components/OutputTemplate/OutputTemplateCell 0 100 0 0
🔴   OutputTemplateCell.tsx 0 100 0 0 17-47
🔴  web/src/components/OutputTemplate/OutputTemplateForm 0 0 0 0
🔴   OutputTemplateForm.tsx 0 0 0 0 18-63
🔴  web/src/components/OutputTemplate/OutputTemplates 0 0 0 0
🔴   OutputTemplates.tsx 0 0 0 0 18-94
🔴  web/src/components/OutputTemplate/OutputTemplatesCell 0 100 0 0
🔴   OutputTemplatesCell.tsx 0 100 0 0 18-52
🔴  web/src/components/ReportingPeriod/EditReportingPeriodCell 0 100 0 0
🔴   EditReportingPeriodCell.tsx 0 100 0 0 13-74
🔴  web/src/components/ReportingPeriod/NewReportingPeriod 0 100 0 0
🔴   NewReportingPeriod.tsx 0 100 0 0 9-35
🔴  web/src/components/ReportingPeriod/ReportingPeriod 0 0 0 0
🔴   ReportingPeriod.tsx 0 0 0 0 12-101
🔴  web/src/components/ReportingPeriod/ReportingPeriodCell 0 100 0 0
🔴   ReportingPeriodCell.tsx 0 100 0 0 7-33
🔴  web/src/components/ReportingPeriod/ReportingPeriodForm 0 0 0 0
🔴   ReportingPeriodForm.tsx 0 0 0 0 18-43
🔴  web/src/components/ReportingPeriod/ReportingPeriods 0 0 0 0
🔴   ReportingPeriods.tsx 0 0 0 0 13-96
🔴  web/src/components/ReportingPeriod/ReportingPeriodsCell 0 100 0 0
🔴   ReportingPeriodsCell.tsx 0 100 0 0 8-43
🟡  web/src/components/ReportingPeriodsCell 57.14 0 60 50
🟢   ReportingPeriodsCell.mock.ts 100 100 100 100
🔴   ReportingPeriodsCell.stories.tsx 0 0 0 0 6-32
🟢   ReportingPeriodsCell.tsx 100 100 100 100
🔴  web/src/components/Subrecipient/SubrecipientTableUploadLinksDisplay 0 0 0 0
🔴   SubrecipientTableUploadLinksDisplay.stories.tsx 0 100 100 0 5-82
🔴   SubrecipientTableUploadLinksDisplay.tsx 0 0 0 0 5-77
🔴  web/src/components/Subrecipient/Subrecipients 0 0 0 0
🔴   Subrecipients.tsx 0 100 0 0 5-8
🔴   columns.tsx 0 0 0 0 7-93
🔴  web/src/components/Subrecipient/SubrecipientsCell 0 100 0 0
🔴   SubrecipientsCell.tsx 0 100 0 0 7-62
🔴  web/src/components/TableBuilder 0 0 0 0
🔴   DebouncedInput.tsx 0 0 0 0 13-32
🔴   Filter.tsx 0 0 0 0 6-15
🔴   TableBuilder.tsx 0 0 0 0 22-70
🔴   TableHeader.tsx 0 0 0 0 5-42
🔴   TableRow.tsx 0 100 0 0 3-7
🟡  web/src/components/TemplateUploadReportingPeriodCell 55 0 55.55 47.05
🟢   TemplateUploadReportingPeriodCell.mock.ts 100 100 100 100
🔴   TemplateUploadReportingPeriodCell.stories.tsx 0 0 0 0 11-37
🟢   TemplateUploadReportingPeriodCell.tsx 100 100 100 100
🔴  web/src/components/TreasuryGeneration/DownloadTreasuryFiles 0 100 0 0
🔴   DownloadTreasuryFiles.tsx 0 100 0 0 6-30
🔴  web/src/components/TreasuryGeneration/NewTreasuryGeneration 0 100 0 0
🔴   NewTreasuryGeneration.tsx 0 100 0 0 8-39
🔴  web/src/components/TreasuryGeneration/NewTreasuryGenerationForm 0 0 0 0
🔴   NewTreasuryGenerationForm.tsx 0 0 0 0 20-31
🔴  web/src/components/Upload/EditUploadCell 0 100 0 0
🔴   EditUploadCell.tsx 0 100 0 0 10-66
🔴  web/src/components/Upload/NewUpload 0 100 0 0
🔴   NewUpload.tsx 0 100 0 0 7-35
🔴  web/src/components/Upload/Upload 0 0 0 0
🔴   Upload.stories.tsx 0 100 100 0 [5-93](https://github.com/usdigitalresponse/cpf-reporter/blob/5c86f03d654e50a0cd127f987a8722afa3f02d37/web/src/components/Upload/Upl...*[Comment body truncated]*

as1729
as1729 commented Aug 28, 2024
View reviewed changes
python/src/functions/generate_treasury_report.py
file=csv_file,
)
# Store project_id_agency_id to row number in a json file
with tempfile.NamedTemporaryFile("w") as json_file:
json_file = json.dump(project_agency_id_to_row_map)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vshia would be great to get your input here. I updated this based on typing errors I noticed. However I wasn't sure if this breaks any of the existing functionality.

@as1729 as1729 changed the title chore: refactor common code across lambda functions and fixes incorre… chore: refactor common code across lambda functions and fixes incorrect typing Aug 29, 2024
@as1729 as1729 enabled auto-merge (squash) August 29, 2024 12:03
laurelkorwin
laurelkorwin reviewed Aug 29, 2024
View reviewed changes
python/src/functions/generate_treasury_report.py
@@ -177,6 +178,10 @@ def process_event(payload: ProjectLambdaPayload, logger):
with tempfile.NamedTemporaryFile() as file:
# Download projects from S3
created_at = file_info.createdAt

# TODO: Add a verification to ensure that the objectKey here is formatted correctly.
# Path must be: uploads/{organization_id}/{agency_id}/{reporting_period_id}/{upload_id}/{filename}.xlsm
Copy link
Contributor

Choose a reason for hiding this comment

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

You could potentially borrow logic here from matchRegex in processValidationJson, which has the same key structure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯 - will address in a separate PR

laurelkorwin
laurelkorwin reviewed Aug 29, 2024
View reviewed changes
python/src/lib/treasury_generation_common.py


def get_generated_output_file_key(
file_type: OutputFileType, project: str, organization: OrganizationObj
Copy link
Contributor

Choose a reason for hiding this comment

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

any appetite for making project values an Enum too? I know we're grabbing from the dictionary below so perhaps not strictly necessary but could make the flow here more legible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

💯 - I'll create a separate PR with this updated

@as1729 as1729 merged commit 4429033 into main Aug 29, 2024
22 checks passed
@as1729 as1729 deleted the as/fix-typing-errors-in-treasury-lambdas branch August 29, 2024 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurelkorwin laurelkorwin laurelkorwin approved these changes

@vshia vshia Awaiting requested review from vshia

@greg-adams greg-adams Awaiting requested review from greg-adams

@TylerHendrickson TylerHendrickson Awaiting requested review from TylerHendrickson

Assignees
No one assigned
Labels
bug Something isn't working python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@as1729 @laurelkorwin