(PC-29882)[BO] feat: Logging of location modification during venue up… #12919

anougbele-pass · 2024-06-25T08:50:29Z

logger dans action_history les modification de localisation lors de la modification d'un lieu

But de la pull request

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

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

github-actions · 2024-06-25T08:51:51Z

mypy cop report: 411 (master) ↗ 412 (your branch)

xordoquy · 2024-06-26T13:36:31Z

api/src/pcapi/core/offerers/api.py

        address = get_or_create_address(location_data)
+        location_data_ = {


Je trouve que le nom porte à confusion. Est-ce que l'on pourrait prendre quelque chose comme snapshot_location_data ?

xordoquy · 2024-06-26T15:56:44Z

api/src/pcapi/routes/backoffice/filters.py

@@ -756,6 +756,20 @@ def format_modified_info_name(info_name: str) -> str:
            return "Date d'éligibilité à la nouvelle interface Pro"
        case "confidenceRule.confidenceLevel":
            return "Validation des offres"
+        case "Offereraddress.addressid":
+            return "Table Addresse <> ID Adresse"


Est-ce que les noms ont été vus avec PO ?

Non j'ai proposé ça mais je reboucle avec lui.

Je te laisse revoir, mais une chose est sûre : un seul « d » dans Adresse en français 🙂

xordoquy · 2024-06-26T15:57:29Z

api/src/pcapi/routes/backoffice/filters.py

@@ -756,6 +756,20 @@ def format_modified_info_name(info_name: str) -> str:
            return "Date d'éligibilité à la nouvelle interface Pro"
        case "confidenceRule.confidenceLevel":
            return "Validation des offres"
+        case "Offereraddress.addressid":


Est-ce pertinent d'avoir l'ID ici ? Je ne vois pas d'autres IDs définies ailleurs dans ce case.

oui c'est un peu particulier par ce qu'on a ce comportement que sur OA. C'est pour marquer l'association de l'OA à une autre adresse. Est ce qu'on l'omet ?

Je ne suis pas convaincu que ce soit une information compréhensible pour le support.
En tout cas, la "traduction" ne semble pas fonctionner :

J'en profite aussi pour ressusciter ça : je suis pas hyper convaincu non plus d'afficher le changement d'id, je sais pas si ça va vraiment servir à l'utilisateur...

xordoquy · 2024-06-26T15:58:45Z

api/tests/routes/backoffice/venues_test.py

@@ -1105,6 +1165,7 @@ def test_update_venue_without_double_model_writing(self, authenticated_client, o
        assert mails_testing.outbox[0]["template"] == TransactionalEmail.VENUE_NEEDS_PICTURE.value.__dict__
        assert mails_testing.outbox[0]["params"]["VENUE_NAME"] == venue.common_name
        assert mails_testing.outbox[0]["params"]["VENUE_FORM_URL"] == urls.build_pc_pro_venue_link(venue)
+        # assert not double action_history data


prouzet

Peux-tu rebase sur master s'il te plaît, pour clarifier les différences après résolution des conflits ?

prouzet · 2024-06-28T14:34:08Z

api/src/pcapi/core/offerers/api.py

+    if not venue.isVirtual and FeatureToggle.ENABLE_ADDRESS_WRITING_WHILE_CREATING_UPDATING_VENUE.is_active():
+        venue_snapshot = update_venue_location(venue, modifications, author=author, is_manual_edition=is_manual_edition)
+    else:
+        venue_snapshot = history_api.ObjectUpdateSnapshot(venue, author)


Je pense qu'il serait préférable de garder la création de l'objet snapshot toujours au même endroit et de la passer en paramètre, plutôt que le créer soit ici soit dans l'autre fonction en fonction de la situation.

Suggested change

if not venue.isVirtual and FeatureToggle.ENABLE_ADDRESS_WRITING_WHILE_CREATING_UPDATING_VENUE.is_active():

venue_snapshot = update_venue_location(venue, modifications, author=author, is_manual_edition=is_manual_edition)

else:

venue_snapshot = history_api.ObjectUpdateSnapshot(venue, author)

venue_snapshot = history_api.ObjectUpdateSnapshot(venue, author)

if not venue.isVirtual and FeatureToggle.ENABLE_ADDRESS_WRITING_WHILE_CREATING_UPDATING_VENUE.is_active():

update_venue_location(venue, modifications, author=author, venue_snapshot, is_manual_edition=is_manual_edition)

Mais du coup la fonction update location ne peut pas realiser les update (en db) vu que l'update doit etre fait juste avant le .add_action pour garder qu'une ligne dans l'historique. On aura alors une fonction update_location qui update rien. ça peut prêter a confusion.

Je ne comprends pas le problème, puisque ma suggestion est de toujours créer l'objet ici, donc avant l'appel à la fonction plutôt qu'exactement la même ligne au tout début de la fonction appelée. Quelle différence dans le résultat ?

On arrive à la même chose. j'avoue que je m'attend à une homogénéité des signature sur les fonctions qui sont logiquement semblable. Mais c'est un autre sujet

ok je te laisse choisir ce qui te semble la meilleure solution en fonction aussi des autres utilisations de cette fonction.

prouzet · 2024-06-28T14:37:33Z

api/src/pcapi/routes/backoffice/filters.py

@@ -756,6 +756,20 @@ def format_modified_info_name(info_name: str) -> str:
            return "Date d'éligibilité à la nouvelle interface Pro"
        case "confidenceRule.confidenceLevel":
            return "Validation des offres"
+        case "Offereraddress.addressid":


Je ne suis pas convaincu que ce soit une information compréhensible pour le support.
En tout cas, la "traduction" ne semble pas fonctionner :

prouzet · 2024-06-28T14:38:07Z

api/src/pcapi/routes/backoffice/filters.py

@@ -756,6 +756,20 @@ def format_modified_info_name(info_name: str) -> str:
            return "Date d'éligibilité à la nouvelle interface Pro"
        case "confidenceRule.confidenceLevel":
            return "Validation des offres"
+        case "Offereraddress.addressid":
+            return "Table Addresse <> ID Adresse"


Je te laisse revoir, mais une chose est sûre : un seul « d » dans Adresse en français 🙂

prouzet · 2024-06-28T14:42:34Z

api/tests/routes/backoffice/venues_test.py

+        assert update_snapshot["latitude"]["new_info"] == "48.87171"
+        assert update_snapshot["longitude"]["new_info"] == "2.308289"  # rounding due to Decimal column in db
+        assert update_snapshot["venueTypeCode"]["new_info"] == data["venue_type_code"]
+        assert update_snapshot["offererAddress.address.latitude"]["new_info"] == "48.87171"


Peux-tu ajouter vérifier dans un test qu'on n'a pas de modification sur latitude et longitude lorsqu'elle n'a pas changé ? Car en édition manuelle j'obtiens.

Peut-être l'un des tests en-dessous se prête-t-il à la vérification.

Indice pour corriger le bug : https://github.com/pass-culture/pass-culture-main/blob/v296.0.1/api/src/pcapi/core/history/api.py#L94

prouzet · 2024-06-28T14:51:16Z

api/src/pcapi/routes/backoffice/filters.py

@@ -748,6 +748,20 @@ def format_modified_info_name(info_name: str) -> str:
            return "Date d'éligibilité à la nouvelle interface Pro"
        case "confidenceRule.confidenceLevel":
            return "Validation des offres"
+        case "Offereraddress.addressid":
+            return "Table Addresse <> ID Adresse"
+        case "offereraddress.address.city":


Vraisemblablement la correspondance ne se fait pas pour tous les champs :

N'oublie pas de vérifier en utilisant l'interface du BO :)

Je ne vois que l'inseecode qui manque.
Est ce que le wording: ex "Table Address <> Longitude" te va ?

J'aurais probablement simplement écrit « Adresse - Longitude » mais ça reste mineur et se corrige.
Certes inseeCode manque, mais addressid et address.city ne sont pas bien affichés.

vroullier-pass · 2024-07-01T09:31:17Z

api/src/pcapi/routes/backoffice/filters.py

+        case "offererAddress.addressId":
+            return "Adresse - ID Adresse"
+        case "offererAddress.address.inseeCode":
+            return "Address - Code Insee"


Suggested change

return "Address - Code Insee"

return "Adresse - Code Insee"

Tu n'as pas modifié...

vroullier-pass · 2024-07-01T09:31:49Z

api/src/pcapi/core/offerers/api.py

@@ -248,13 +260,39 @@ def update_venue_location(venue: models.Venue, modifications: dict, is_manual_ed
            )

        address = get_or_create_address(location_data, is_manual_edition=is_manual_edition)
-
+        snaphot_location_data = {


Suggested change

snaphot_location_data = {

snapshot_location_data = {

Et pourquoi pas snapshot_address_data d'ailleurs ?

vroullier-pass · 2024-07-01T09:39:19Z

api/src/pcapi/core/offerers/api.py

+                if venue.offererAddress
+                else geography_models.Address()


Je comprends pas ce if else. On teste juste au-dessus l'existence de venue.offererAddressId...

c'est surtout pour contenter mypy.

Dans ce cas, soit tu peux faire un if venue.offererAddress plus haut, soit tu peux utiliser un assert juste avant, avec une mention que c'est pour mypy (cf les nombreux # helps mypy que tu peux trouver)

J'insiste un peu pour ces ifs, ça ne rend vraiment pas le code clair

vroullier-pass · 2024-07-01T09:40:04Z

api/src/pcapi/core/offerers/api.py

+            target = venue.offererAddress if venue.offererAddress else models.OffererAddress()
+            venue_snapshot.trace_update(
+                {"addressId": address.id}, target=target, field_name_template="offererAddress.{}"
+            )


C'est un doublon ? Ou alors je comprends pas ce qu'on fait...

en fait il y a OffererAddress.AddressId et OffererAddress.address.id. Il faudrait avoir un id dans le snapshot. mais la on aurait doublon.

Ah... Mais ça donnera du coup un doublon dans l'historique non ? Enfin, sur le BO, on aura les deux lignes de modif ?

vroullier-pass · 2024-07-01T09:40:40Z

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

@@ -695,6 +689,7 @@ def update_venue(venue_id: int) -> utils.BackofficeResponse:
            contact_data=contact_data,
            criteria=criteria,
            external_accessibility_url=form.acceslibre_url.data if hasattr(form, "acceslibre_url") else "",
+            is_manual_edition=((not venue.isVirtual) and form.is_manual_address.data == "on"),


On ne garde pas le if du FF ?

il est dans le update_venue. et l'argument est utilisé que la bas.

vroullier-pass · 2024-07-02T07:28:37Z

api/src/pcapi/core/offerers/api.py

+                if venue.offererAddress
+                else geography_models.Address()


J'insiste un peu pour ces ifs, ça ne rend vraiment pas le code clair

vroullier-pass · 2024-07-02T07:28:55Z

api/src/pcapi/core/offerers/api.py

+            venue_snapshot.trace_update(
+                snapshot_location_data, target=target, field_name_template="offererAddress.address.{}"
+            )
+            target = venue.offererAddress if venue.offererAddress else models.OffererAddress()


Et pareil pour ce if

vroullier-pass · 2024-07-02T07:29:11Z

api/src/pcapi/routes/backoffice/filters.py

+        case "offererAddress.addressId":
+            return "Adresse - ID Adresse"
+        case "offererAddress.address.inseeCode":
+            return "Address - Code Insee"


Tu n'as pas modifié...

api/src/pcapi/routes/backoffice/filters.py

…date

anougbele-pass force-pushed the PC-29882-bo-modifier-laction-history-de-changement-de-localisation-de-lieu-pour-prendre-en-compte-les-changements-dans-lofferer-address-liee branch from 0ecd149 to 03679c9 Compare June 25, 2024 09:55

xordoquy reviewed Jun 26, 2024

View reviewed changes

prouzet reviewed Jun 27, 2024

View reviewed changes

anougbele-pass force-pushed the PC-29882-bo-modifier-laction-history-de-changement-de-localisation-de-lieu-pour-prendre-en-compte-les-changements-dans-lofferer-address-liee branch 2 times, most recently from 0d0b773 to a74c7ff Compare June 28, 2024 14:07

anougbele-pass requested review from prouzet and xordoquy June 28, 2024 14:20

prouzet reviewed Jun 28, 2024

View reviewed changes

anougbele-pass force-pushed the PC-29882-bo-modifier-laction-history-de-changement-de-localisation-de-lieu-pour-prendre-en-compte-les-changements-dans-lofferer-address-liee branch 2 times, most recently from 8d86f3d to 94840c4 Compare July 1, 2024 09:06

anougbele-pass requested a review from vroullier-pass July 1, 2024 09:23

vroullier-pass reviewed Jul 1, 2024

View reviewed changes

anougbele-pass force-pushed the PC-29882-bo-modifier-laction-history-de-changement-de-localisation-de-lieu-pour-prendre-en-compte-les-changements-dans-lofferer-address-liee branch from 94840c4 to 789e7f5 Compare July 1, 2024 13:27

anougbele-pass requested a review from vroullier-pass July 2, 2024 06:51

vroullier-pass reviewed Jul 2, 2024

View reviewed changes

anougbele-pass force-pushed the PC-29882-bo-modifier-laction-history-de-changement-de-localisation-de-lieu-pour-prendre-en-compte-les-changements-dans-lofferer-address-liee branch from 789e7f5 to 19cd424 Compare July 2, 2024 08:05

vroullier-pass approved these changes Jul 2, 2024

View reviewed changes

anougbele-pass force-pushed the PC-29882-bo-modifier-laction-history-de-changement-de-localisation-de-lieu-pour-prendre-en-compte-les-changements-dans-lofferer-address-liee branch from 19cd424 to c1541ae Compare July 3, 2024 14:04

(PC-29882)[BO] feat: Logging of location modification during venue up…

82d0b2c

…date

anougbele-pass force-pushed the PC-29882-bo-modifier-laction-history-de-changement-de-localisation-de-lieu-pour-prendre-en-compte-les-changements-dans-lofferer-address-liee branch from c1541ae to 82d0b2c Compare July 3, 2024 17:33

anougbele-pass merged commit 6e2c6aa into master Jul 3, 2024
22 checks passed

anougbele-pass deleted the PC-29882-bo-modifier-laction-history-de-changement-de-localisation-de-lieu-pour-prendre-en-compte-les-changements-dans-lofferer-address-liee branch July 3, 2024 18:09

		address = get_or_create_address(location_data)
		location_data_ = {

(PC-29882)[BO] feat: Logging of location modification during venue up… #12919

(PC-29882)[BO] feat: Logging of location modification during venue up… #12919

Conversation

anougbele-pass commented Jun 25, 2024 • edited Loading

But de la pull request

Vérifications

github-actions bot commented Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prouzet Jun 28, 2024 • edited Loading

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

anougbele-pass Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prouzet Jun 28, 2024 • edited Loading

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

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

anougbele-pass commented Jun 25, 2024 •

edited

Loading

github-actions bot commented Jun 25, 2024 •

edited

Loading

prouzet Jun 28, 2024 •

edited

Loading

anougbele-pass Jun 28, 2024 •

edited

Loading

prouzet Jun 28, 2024 •

edited

Loading