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

Refactored PI-specific invoices #73

Merged
merged 1 commit into from
Sep 19, 2024

Conversation

QuanMPhm
Copy link
Contributor

Closes #69. The PI-specific is now implemented in the PIInvoice class. This class takes in the entire billable invoice and will export the PI invoices to local storage and S3. Because of its unique export file paths, it overrides the export()

Note that to follow the file path has changed slightly, from:
f"{self.name}/{pi_instituition}_{pi}_{self.invoice_month}.csv"
to...
f"{self.name}/{pi_instituition}_{pi} {self.invoice_month}.csv"

Note that replacement of the "_" to " ". I do this to be more consistent with the filename pattern for the other invoices.
Aside from that, I've also removed upload_to_s3() and its corresponding unit test, since it is no longer needed (after this PR, and certainly after all the refactor PRs are merged). @knikolla @naved001 Should I add a test for each invoice to see if they are exporting to desired S3 paths?

Copy link
Contributor

@knikolla knikolla left a comment

Choose a reason for hiding this comment

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

A general note, try to split large sections of common code to separate clearly named functions or methods.

process_report/invoices/pi_specific_invoice.py Outdated Show resolved Hide resolved
@joachimweyl
Copy link
Contributor

@knikolla please rereview.

@QuanMPhm
Copy link
Contributor Author

@knikolla @naved001 Since all the invoices now implement their own S3 export functions, this meant that the upload_to_s3 function in process_report.py was removed, along with its test case to check that files were uploaded to their intended s3 filepaths. I wanted to ask if this means I should implement a new test case that checks the export function of every invoice to do the same test.

@knikolla
Copy link
Contributor

knikolla commented Sep 5, 2024

@knikolla @naved001 Since all the invoices now implement their own S3 export functions, this meant that the upload_to_s3 function in process_report.py was removed, along with its test case to check that files were uploaded to their intended s3 filepaths. I wanted to ask if this means I should implement a new test case that checks the export function of every invoice to do the same test.

You can write a test for the exporting functionality of the base class Invoice, but it's unnecessary for invoices that don't modify that in any way.

@QuanMPhm
Copy link
Contributor Author

QuanMPhm commented Sep 6, 2024

@knikolla @naved001 Since the export_s3 function does not need to be concerned about file extensions anymore, I have simplified the current test case for s3 exporting. I also updated the commit message to reflect this change.

The PI-specific is now implemented in the `PIInvoice` class.
This class takes in the entire billable invoice and will
export the PI invoices to local storage and S3

Because the `upload_to_s3` function is no longer used, it is removed.
Since the `export_s3` function in the Invoice class does not need to be
concerned about file extensions, the test case for s3 exporting is
somewhat simplified, only checking that the format of the output paths
are correct.
process_report/process_report.py Show resolved Hide resolved
@QuanMPhm QuanMPhm merged commit f43f2a8 into CCI-MOC:main Sep 19, 2024
3 checks passed
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.

Refactor the PI-specific invoices
4 participants