CDR : add multiple tickets generation #544
Conversation
Malem38
force-pushed
the
feat/generate-tickets
branch
6 times, most recently
from
183fc8e
to
3b37163
Compare
### Description Please explain the changes you made here. ### Checklist - [ ] Created tests which fail without the change (if possible) - [ ] All tests passing - [ ] Extended the documentation, if necessary --------- Co-authored-by: Armand Didierjean <95971503+armanddidierjean@users.noreply.github.com> Co-authored-by: Maxime Roucher <maximeroucher@gmail.com>
Rotheem
force-pushed
the
feat/generate-tickets
branch
from
f97db16
to
26f2ec0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #544 +/- ##
==========================================
- Coverage 81.97% 81.24% -0.73%
==========================================
Files 125 125
Lines 9518 9773 +255
==========================================
+ Hits 7802 7940 +138
- Misses 1716 1833 +117 ☔ View full report in Codecov by Sentry. |
| recipient=email.email, | ||
| subject=f"Résultats de ventes pour {seller.name}", | ||
| content=f"Bonjour,\n\nVous trouverez en pièce jointe le fichier Excel contenant les résultats de ventes pour la CdR pour l'association {seller.name}.", | ||
| settings=get_settings(), |
There was a problem hiding this comment.
Can not call get_settings directly
| freeze_panes=(2, 3), | ||
| engine="xlsxwriter", | ||
| ) | ||
| send_email( |
There was a problem hiding this comment.
You should always call send_email in a background task
There was a problem hiding this comment.
Hum, you might need to look at the endpoint_users.py file, there was quite a few missed, anyway it is fixed for this one
| data_fields=product_fields, | ||
| users_answers=users_answers, | ||
| ) | ||
| file_path = f"/tmp/CdR {datetime.now(tz=UTC).year} ventes {seller.name}.xlsx" # noqa: S108 |
There was a problem hiding this comment.
I'm not completely sure if it would work, but S108 looks interesting.
There was a problem hiding this comment.
If we don't use a temp file as proposed by S108, please place this file in /data/... which is gitignored.
Always use file manipulation utils to access the file system, to prevent injections in filepath
There was a problem hiding this comment.
The file creation is done by pandas it does not seem to be compatible
| subject: str, | ||
| content: str, | ||
| settings: "Settings", | ||
| file_path: str | None = None, |
There was a problem hiding this comment.
Ask for folder name and file uuid, then use file manipulation utils, to prevent access to files outside /data
| from typing import TYPE_CHECKING | ||
|
|
||
| if TYPE_CHECKING: | ||
| from app.core.config import Settings | ||
|
|
||
|
|
||
| def send_email(recipient: str, subject: str, content: str, settings: "Settings"): | ||
| def send_email( | ||
| recipient: str | list[str], |
There was a problem hiding this comment.
I think we could ask for a list[str] directly instead of allowing str | list[str]
There was a problem hiding this comment.
This is a convenient system which allow to not have inconsistency such as using a list of email when trying to send a personal token, moreover I didn't want to touch the old system
| @@ -12,6 +12,7 @@ google-auth-oauthlib==1.2.1 | |||
| icalendar==5.0.13 | |||
| jellyfish==1.0.4 # String Matching | |||
| Jinja2==3.1.4 # template engine for html files | |||
| pandas==2.2.2 | |||
There was a problem hiding this comment.
I'm not sure, but I think we could use python list instead of pandas dataframe
There was a problem hiding this comment.
This allows to create a ready-to-use excel, added to the fact that it was a way I knew and it took me far less time than finding a new one
| @@ -351,6 +355,197 @@ async def get_online_sellers( | |||
| return await cruds_cdr.get_online_sellers(db) | |||
|
|
|||
|
|
|||
| def construct_dataframe_from_users_purchases( | |||
There was a problem hiding this comment.
Move the util to app/modules/cdr/utils_cdr.py
Description
Please explain the changes you made here.
Checklist