search

CCI-MOC / process_csv_report

Some scripts to help process our billing reports
0 stars 3 forks source link

Refactored PI-specific invoices #73

Closed QuanMPhm closed 2 weeks ago

QuanMPhm commented 2 months ago

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?

joachimweyl commented 2 months ago

@knikolla please rereview.

QuanMPhm commented 1 month ago

@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 commented 1 month ago

@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 commented 4 weeks ago

@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.