-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
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.
A general note, try to split large sections of common code to separate clearly named functions or methods.
0d2acfa
to
6ffc7bb
Compare
@knikolla please rereview. |
6ffc7bb
to
743c295
Compare
@knikolla @naved001 Since all the invoices now implement their own S3 export functions, this meant that the |
You can write a test for the exporting functionality of the base class |
743c295
to
17ba421
Compare
17ba421
to
d1e623a
Compare
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.
d1e623a
to
6a36c24
Compare
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 theexport()
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?