[PP-1216] Zip email reports #1818
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1818 +/- ##
==========================================
- Coverage 90.03% 90.03% -0.01%
==========================================
Files 299 299
Lines 39543 39543
Branches 8588 8588
==========================================
- Hits 35604 35602 -2
- Misses 2613 2614 +1
- Partials 1326 1327 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dbernstein
force-pushed
the
PP-1216-zip-email-reports
branch
2 times, most recently
from
7cfb5c8 to
a4186d4
Compare
src/palace/manager/celery/tasks/generate_inventory_and_hold_reports.py
Outdated
Show resolved
Hide resolved
tests/manager/celery/tasks/test_generate_inventory_and_hold_reports.py
Outdated
Show resolved
Hide resolved
|
Thank you for all the great feedback @jonathangreen. I took your advice and reworked the PR. I couldn't use a temp dir because it doesn't support the same delete flag as an file. But as you can see it was possible to do it with temporary files and nested context managers. |
dbernstein
force-pushed
the
PP-1216-zip-email-reports
branch
2 times, most recently
from
8ae7482 to
f2061f2
Compare
There was a problem hiding this comment.
The refactoring you did looks great here @dbernstein.
Added one small comment here, the temp files aren't being cleaned up now since delete=False is always set. Once thats fixed this is good to go.
| ) | ||
|
|
||
| def create_temp_file(self) -> _TemporaryFileWrapper[str]: | ||
| return tempfile.NamedTemporaryFile("w", delete=False, encoding="utf-8") |
There was a problem hiding this comment.
Since delete=False is set here, these files are never getting cleaned up right? I think you can completely drop the delete=False here.
| f"Emailed inventory and holds reports for {library.name}({library.short_name})." | ||
| ) | ||
|
|
||
| def create_temp_file(self) -> _TemporaryFileWrapper[str]: |
There was a problem hiding this comment.
You can type hint this as the more generic IO[str] to avoid having to use the private _TemporaryFileWrapper object in the hint.
| return temp.name | ||
| self, | ||
| _db: Session, | ||
| csv_file: _TemporaryFileWrapper[str], |
There was a problem hiding this comment.
Same as above, this can be hinted as IO[str]
dbernstein
force-pushed
the
PP-1216-zip-email-reports
branch
from
cdf411a to
27dd28a
Compare
Description
With this update, instead of attaching two csv reports to the email, we are zipping the csvs and attaching the resulting archive to the email.
Motivation and Context
https://ebce-lyrasis.atlassian.net/browse/PP-1216
How Has This Been Tested?
An existing unit test has been modified to account for the change.
Checklist