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

Ensy 187 bursar transfer success email from bursar transfer lambda should include number of lines and total charges #82

Conversation

adamshire123
Copy link
Contributor

@adamshire123 adamshire123 commented Sep 20, 2023

What does this PR do?

Adds record count and total amount of charges to the values returned by the lambda
so that these value can be included in the success email triggered by the step function

Helpful background context

The CHASS webform has required fields for total records and total charges when submitting
a file. We should send these value in the success email so that staff don't have to open the
datafile and calculate them.

How can a reviewer manually see the effects of these changes?

follow the local testing instruction in the readme. observe new return values.

Includes new or updated dependencies?

YES

What are the relevant tickets?

https://mitlibraries.atlassian.net/browse/ENSY-187

Developer

  • All new ENV is documented in README (or there is none)
  • Stakeholder approval has been confirmed (or is not needed)

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

When uploading the .csv to CHASS, the operator is required to fill in form fields with the
total of all the charges being submitted, and the number of records in the input file.

* calculates total charges and record count
* returns these values in the dict which the step function can access
so they can be included in the success email.

Step function email needs to be updated to take advantage of
these new values

* https://mitlibraries.atlassian.net/browse/ENSY-187
@adamshire123 adamshire123 marked this pull request as ready for review September 20, 2023 18:45
Copy link

@ehanson8 ehanson8 left a comment

Choose a reason for hiding this comment

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

Looks good, 2 suggestions

lambdas/bursar_transfer.py Show resolved Hide resolved
lambdas/bursar_transfer.py Outdated Show resolved Hide resolved
Copy link

@ghukill ghukill left a comment

Choose a reason for hiding this comment

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

Other than one small request, looks good to me!

lambdas/bursar_transfer.py Outdated Show resolved Hide resolved
* Add docstring
* Add rounding to sum of charges
* changed record count variable name
to add clarity to what it represents.
* changed record count key in return dict
this will require a corresponding change
in the step function definition.
@adamshire123 adamshire123 force-pushed the ENSY-187-bursar-transfer-success-email-from-bursar-transfer-lambda-should-include-number-of-lines-and-total-charges branch from 276c76d to 211e52a Compare September 22, 2023 16:03
Copy link

@ehanson8 ehanson8 left a comment

Choose a reason for hiding this comment

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

Looks good to me but I'll give Graham the final sign-off

Copy link

@ghukill ghukill left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@adamshire123 adamshire123 merged commit 01ee673 into main Sep 29, 2023
5 checks passed
@adamshire123 adamshire123 deleted the ENSY-187-bursar-transfer-success-email-from-bursar-transfer-lambda-should-include-number-of-lines-and-total-charges branch October 20, 2023 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants