(PC-30007)[API] feat: add pdf to beneficiary extract #12936

Meewan · 2024-06-25T15:40:06Z

But de la pull request

Ticket Jira (ou description si BSR) : https://passculture.atlassian.net/browse/PC-30007

Note le rendu du pdf final a été validé par le DPO (le client de la feature)

Vérifications

J'ai écrit les tests nécessaires
J'ai relu attentivement les migrations, en particulier pour éviter les locks, et je préviens les équipes Shérif et Data
J'ai ajouté des screenshots pour d'éventuels changements graphiques

prouzet

J'ai pas mal de commentaires mais sur la seule lecture du code.
Je n'ai pas testé ce que ça rend visuellement.

api/src/pcapi/tasks/gdpr_tasks.py

prouzet · 2024-06-26T10:13:32Z

api/src/pcapi/tasks/gdpr_tasks.py

+    pdf_bytes = generate_pdf_from_html(html_content=html_content)
+    file_info = zipfile.ZipInfo(
+        filename=f"{container.internal.user.firstName} {container.internal.user.lastName}.pdf",
+        date_time=datetime.utcnow().timetuple()[:6],


Il n'y a pas de notion de fuseau horaire dans ce champ.
Ne voudrait-on pas la date locale dans le fichier ?
Cela n'a cependant pas une extrême importance.

tu veux dire en se basant sur le département du user ? cette méthode étant appelée avec une cloud task potentiellement des heures/jours après la demande je en sais pas si ça a du sens.

Oui ça pourrait être tout simplement :
date_time=utc_datetime_to_department_timezone(datetime.utcnow(), container.internal.user.departementCode).timetuple()[:6]
voire même sans le second paramètre si on considère que c'est généré à Paris.

Mais c'est assez secondaire, c'est juste une date de fichier.

prouzet · 2024-06-26T10:15:18Z

api/src/pcapi/templates/extracts/beneficiary_extract.html

+        <meta charset="utf-8">
+        <title>Réponse à ta demande d'accès</title>
+        <style>
+            @import url('https://fonts.googleapis.com/css2?family=Montserrat:ital,wght@0,400;0,600;0,700;0,800;1,400');


Il me semblait qu'on avait souhaité avoir toutes les ressources locales, et ne pas importer des URL externes dans le code. Cela ne serait-il pas le cas ici aussi ?

C'est repris de la template utilisée en Finance. Je pars du principe que si c'est ok pour eux ça l'est aussi pour nous (si l'appel échoue ça ne fait pas échouer le rendu, ça met juste pas la bonne police)

Je ne crois pas que ce soit une question de disponibilité de la ressource, mais plus une question de sécurité.

prouzet · 2024-06-26T10:18:21Z

api/src/pcapi/templates/extracts/beneficiary_extract.html

+                margin: 2cm 1cm;
+                @bottom-left {
+                    font-size: 6pt;
+                    content: "Pass Culture - SAS au capital de 1,000,000 € - 87/89 rue de la Boétie 75008 PARIS - RCS Paris B 853 318 459 – Siren 853318459 – NAF 7021 Z - N° TVA FR65853318459";


1,000,000 € est une écriture anglophone.

Suggested change

content: "Pass Culture - SAS au capital de 1,000,000 € - 87/89 rue de la Boétie 75008 PARIS - RCS Paris B 853 318 459 – Siren 853318459 – NAF 7021 Z - N° TVA FR65853318459";

content: "Pass Culture - SAS au capital de 1 000 000 € - 87/89 rue de la Boétie 75008 PARIS - RCS Paris B 853 318 459 – Siren 853318459 – NAF 7021 Z - N° TVA FR65853318459";

c'est le pieds de page de la Finance, donc je ne pense pas le modifier.

Ou alors modifier aussi dans la page de finance ? 🙂
Qui est responsable du modèle ?

api/src/pcapi/templates/extracts/beneficiary_extract.html

api/tests/tasks/gdpr_tasks_test.py

prouzet

Voici mes remarques en testant la branche dans mon environnement de développement.
Même s'il y a des petites choses à corriger dans le formatage, je trouve le rendu super propre !

prouzet · 2024-06-28T12:15:38Z

api/src/pcapi/routes/backoffice/accounts/blueprint.py

@@ -1263,6 +1262,9 @@ def create_extract_user_gdpr_data(user_id: int) -> utils.BackofficeResponse:
    if not user:
        raise NotFound()

+    if not (user.is_beneficiary or user.roles == []):
+        raise NotFound()
+
    if has_gdpr_extract(user=user):
        flash("Une extraction de données est déjà en cours pour cet utilisateur.", "error")


Par homogénéité et parce qu'en testant, "error" n'affiche aucun fond :

Suggested change

flash("Une extraction de données est déjà en cours pour cet utilisateur.", "error")

flash("Une extraction de données est déjà en cours pour cet utilisateur.", "warning")

prouzet · 2024-06-28T13:12:32Z

api/src/pcapi/tasks/gdpr_tasks.py

@@ -175,16 +180,27 @@ def _extract_brevo_data(user: users_models.User) -> dict:
 def _dump_as_json_bytes(container: serializers.DataContainer) -> tuple[zipfile.ZipInfo, bytes]:
    json_bytes = container.json(indent=4).encode("utf-8")
    file_info = zipfile.ZipInfo(
-        filename=f"{container.internal.user.firstName}_{container.internal.user.lastName}.json",
+        filename=f"{container.internal.user.firstName} {container.internal.user.lastName}.json",


Pour un utilisateur sans nom ni prénom (avant de remplir ses infos), je vois des fichiers None None dans le zip, est-ce souhaitable ? devrait-on avoir l'adresse mail comme le nom du zip ?

je ne suis aps fan d'avoir un @ dans un nom de fichier mais je vais chercher une solution a ce probleme

Tu peux mettre le User ID, je proposais l'email car c'est le nom du fichier zip au moment du téléchargement, donc je pensais que ça te convenait 🙂

prouzet · 2024-06-28T13:14:02Z

api/src/pcapi/templates/extracts/beneficiary_extract.html

+            <tr><td>Nom</td><td>{{container.internal.user.firstName}}</td></tr>
+            <tr><td>Prénom</td><td>{{container.internal.user.lastName}}</td></tr>


Nom et prénom peuvent être vides si l'utilisateur n'a pas encore rempli ses infos personnelles :

prouzet · 2024-06-28T13:19:29Z

api/src/pcapi/templates/extracts/beneficiary_extract.html

+                    <tr><td>Valeur</td><td>{{('%0.2f' % deposit.amount|float) | replace('.', ',')}}€</td></tr>
+                    <tr><td>Source</td><td>{{deposit.source}}</td></tr>
+                    {% if deposit.dateUpdated %}<tr><td>Dernière modification</td><td>{{deposit.dateUpdated.strftime('%d/%m/%Y %H:%M:%S')}}</td></tr>{% endif %}
+                    <tr><td>Type de crédit</td><td>{{deposit.type.value}}</td></tr>


Il manque un formatage vers "Pass 15-17" ou "Pass 18" car GRANT_15_17 est un nom technique

prouzet · 2024-06-28T13:19:34Z

api/src/pcapi/templates/extracts/beneficiary_extract.html

+                <table class="borderless">
+                    <tr><td>Date d'obtention</td><td>{{deposit.dateCreated.strftime('%d/%m/%Y %H:%M:%S')}}</td></tr>
+                    {% if deposit.expirationDate %}<tr><td>Date d'expiration</td><td>{{deposit.expirationDate.strftime('%d/%m/%Y %H:%M:%S')}}</td></tr>{% endif %}
+                    <tr><td>Valeur</td><td>{{('%0.2f' % deposit.amount|float) | replace('.', ',')}}€</td></tr>


Suggested change

<tr><td>Valeur</td><td>{{('%0.2f' % deposit.amount|float) | replace('.', ',')}}€</td></tr>

<tr><td>Valeur</td><td>{{('%0.2f' % deposit.amount|float) | replace('.', ',')}} €</td></tr>

prouzet · 2024-06-28T13:19:53Z

api/src/pcapi/templates/extracts/beneficiary_extract.html

+                <table class="borderless">
+                    <tr><td>Nom</td><td>{{booking.name}}</td></tr>
+                    <tr><td>Quantité</td><td>{{booking.quantity}}</td></tr>
+                    <tr><td>Prix unitaire</td><td>{{'%0.2f' % booking.amount|float }}€</td></tr>


Suggested change

<tr><td>Prix unitaire</td><td>{{'%0.2f' % booking.amount|float }}€</td></tr>

<tr><td>Prix unitaire</td><td>{{ ('%0.2f' % deposit.amount|float) | replace('.', ',') }} €</td></tr>

prouzet · 2024-06-28T13:21:08Z

api/src/pcapi/templates/extracts/beneficiary_extract.html

+                    <tr><td>Date de réservation</td><td>{{booking.dateCreated.strftime('%d/%m/%Y %H:%M:%S')}}</td></tr>
+                    {% if booking.dateUsed %}<tr><td>Date de retrait</td><td>{{booking.dateUsed.strftime('%d/%m/%Y %H:%M:%S')}}</td></tr>{% endif %}
+                    {% if booking.cancellationDate %}<tr><td>Date d'annulation</td><td>{{booking.cancellationDate.strftime('%d/%m/%Y %H:%M:%S')}}</td></tr>{% endif %}
+                    <tr><td>État</td><td>{{booking.status.value}}</td></tr>


Ici aussi il manque un formatage de la valeur technique vers un libellé d'affichage, probablement "Validée" dans le cas de cette capture d'écran :

Il faut probablement utiliser les mêmes mots que dans l'app.

prouzet · 2024-06-28T13:26:13Z

api/src/pcapi/templates/extracts/beneficiary_extract.html

+                <table class="borderless">
+                    <tr><td>Date de la validation</td><td>{{validation.dateCreated.strftime('%d/%m/%Y %H:%M:%S')}}</td></tr>
+                    <tr><td>Moyen de la validation</td><td>{{validation.type.value}}</td></tr>
+                    {% if validation.status %}<tr><td>Résultat</td><td>{{validation.status.value}}</td></tr>{%endif%}


Il manque un formatage vers le libellé en français, peut-être le même que dans l'app si c'est affiché.
Ici probablement « en attente »

prouzet · 2024-06-28T13:32:53Z

api/src/pcapi/templates/extracts/beneficiary_extract.html

+        {% endif %}
+
+        {% if  history in container.internal.emailsHistory %}
+            <h3>📧 <span class=underline>Historique des changements d'adresse de messagerie</span></h3>


J'ai testé sur un utilisateur pour lequel j'ai changé deux fois l'email depuis le backoffice : cet historique n'apparaît pas. Dans la table user_email_history, l'eventType est ADMIN_UPDATE, qui n'est pas dans la liste de la fonction _extract_email_history. Cela mériterait d'étoffer le test auto pour s'assurer que ce cas fonctionne, car il s'agit d'une information personnelle importante.

De plus, dans cette même fonction, ne devrait-on pas se baser sur CONFIRMATION plutôt que UPDATE_REQUEST ? À tester de bout en bout.

prouzet · 2024-06-28T13:35:39Z

api/src/pcapi/templates/extracts/beneficiary_extract.html

+            {% for action in container.internal.actionsHistory %}
+                <table class="borderless">
+                    <tr><td>Date</td><td>{{action.actionDate.strftime('%d/%m/%Y %H:%M:%S')}}</td></tr>
+                    <tr><td>Action</td><td>{{action.actionType.value}}</td></tr>


Peut-être souhaite-t-on garder la raison la suspension confidentielle, cependant je pense qu'on devrait au moins préciser si la suspension est à la demande de l'utilisateur ou par le pass Culture. Qu'en pense le DPO ?

Meewan force-pushed the pc-30007-rpa-generate-pdf-html-extraction branch 2 times, most recently from a603d49 to fb6aa7b Compare June 26, 2024 10:00

prouzet reviewed Jun 26, 2024

View reviewed changes

Meewan force-pushed the pc-30007-rpa-generate-pdf-html-extraction branch 2 times, most recently from b1b94aa to 1156d48 Compare June 26, 2024 14:06

Meewan requested a review from prouzet June 26, 2024 14:06

(PC-30007)[API] feat: add pdf to beneficiary extract

9d5997e

Meewan force-pushed the pc-30007-rpa-generate-pdf-html-extraction branch from 1156d48 to 9d5997e Compare June 27, 2024 14:10

prouzet reviewed Jun 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PC-30007)[API] feat: add pdf to beneficiary extract #12936

(PC-30007)[API] feat: add pdf to beneficiary extract #12936

Meewan commented Jun 25, 2024 •

edited

Loading

prouzet left a comment

prouzet Jun 26, 2024

Meewan Jun 26, 2024

prouzet Jun 27, 2024

prouzet Jun 27, 2024

prouzet Jun 26, 2024

Meewan Jun 26, 2024

prouzet Jun 27, 2024

prouzet Jun 26, 2024

Meewan Jun 26, 2024

prouzet Jun 27, 2024

prouzet left a comment

prouzet Jun 28, 2024

prouzet Jun 28, 2024

Meewan Jun 28, 2024

prouzet Jun 28, 2024

prouzet Jun 28, 2024

prouzet Jun 28, 2024

prouzet Jun 28, 2024

prouzet Jun 28, 2024

prouzet Jun 28, 2024

prouzet Jun 28, 2024

prouzet Jun 28, 2024

prouzet Jun 28, 2024

	content: "Pass Culture - SAS au capital de 1,000,000 € - 87/89 rue de la Boétie 75008 PARIS - RCS Paris B 853 318 459 – Siren 853318459 – NAF 7021 Z - N° TVA FR65853318459";
	content: "Pass Culture - SAS au capital de 1 000 000 € - 87/89 rue de la Boétie 75008 PARIS - RCS Paris B 853 318 459 – Siren 853318459 – NAF 7021 Z - N° TVA FR65853318459";

	flash("Une extraction de données est déjà en cours pour cet utilisateur.", "error")
	flash("Une extraction de données est déjà en cours pour cet utilisateur.", "warning")

		<tr><td>Nom</td><td>{{container.internal.user.firstName}}</td></tr>
		<tr><td>Prénom</td><td>{{container.internal.user.lastName}}</td></tr>

	<tr><td>Valeur</td><td>{{('%0.2f' % deposit.amount\|float) \| replace('.', ',')}}€</td></tr>
	<tr><td>Valeur</td><td>{{('%0.2f' % deposit.amount\|float) \| replace('.', ',')}} €</td></tr>

	<tr><td>Prix unitaire</td><td>{{'%0.2f' % booking.amount\|float }}€</td></tr>
	<tr><td>Prix unitaire</td><td>{{ ('%0.2f' % deposit.amount\|float) \| replace('.', ',') }} €</td></tr>

(PC-30007)[API] feat: add pdf to beneficiary extract #12936

Are you sure you want to change the base?

(PC-30007)[API] feat: add pdf to beneficiary extract #12936

Conversation

Meewan commented Jun 25, 2024 • edited Loading

But de la pull request

Vérifications

prouzet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prouzet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Meewan commented Jun 25, 2024 •

edited

Loading