Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pc 31774 use atomic in bo #14083

Merged
merged 5 commits into from
Nov 6, 2024
Merged

Pc 31774 use atomic in bo #14083

merged 5 commits into from
Nov 6, 2024

Conversation

vroullier-pass
Copy link
Contributor

But de la pull request

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

Vérifications

  • J'ai écrit les tests nécessaires
  • J'ai mis à jour le fichier des plans de tests du portail pro si nécessaire
  • J'ai mis à jour la liste des routes et des titres de pages du portail pro si j'en ai rajouté/modifié ou supprimé une.
  • 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

@vroullier-pass vroullier-pass force-pushed the PC-31774-use-atomic-in-BO branch 2 times, most recently from 6b1e1c3 to 4c61324 Compare September 11, 2024 15:35
Copy link
Contributor

@Meewan Meewan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Il faudrait marquer la transaction comme invalid a chaque fois qu'il y a une erreur même si on a pas ecrit dans la db.

@@ -263,7 +267,7 @@ def create_offerer() -> utils.BackofficeResponse:
venue = offerers_api.create_venue(venue_creation_info, current_user)
offerers_api.create_venue_registration(venue.id, new_onboarding_info.target, new_onboarding_info.webPresence)

transactional_mails.send_welcome_to_pro_email(pro_user, venue)
on_commit(partial(transactional_mails.send_welcome_to_pro_email, pro_user, venue))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dans cette fonction on a cette pile d'appel create_venue > get_or_create_address qui contient un rollback

pass-culture-main/api/src/pcapi/core/offerers/api.py:2630

idem pour la fonction get_or_create_offerer_address

pass-culture-main/api/src/pcapi/core/offerers/api.py:2671

@vroullier-pass vroullier-pass force-pushed the PC-31774-use-atomic-in-BO branch 2 times, most recently from e2953a1 to c0cfcda Compare September 23, 2024 12:42
Comment on lines 2720 to 2829
if is_managed_transaction():
mark_transaction_as_invalid()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je crain que ça empeche de créer un offerer si l'adresse existe deja.
Une solution serait re rajouter un context manager atomic autour du try except

Comment on lines 2676 to 2785
if is_managed_transaction():
mark_transaction_as_invalid()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je crain que ça empeche de créer un offerer si l'adresse existe deja.
Une solution serait re rajouter un context manager atomic autour du try except

@Meewan Meewan force-pushed the PC-31774-use-atomic-in-BO branch 7 times, most recently from 83907d5 to 05f6907 Compare October 9, 2024 15:40
Copy link

@dnguyen1-pass dnguyen1-pass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm à part les transaction

api/src/pcapi/core/offerers/api.py Show resolved Hide resolved
api/src/pcapi/core/offerers/api.py Show resolved Hide resolved
@rpaoloni-pass rpaoloni-pass force-pushed the PC-31774-use-atomic-in-BO branch 2 times, most recently from d5e2347 to 14045b8 Compare November 6, 2024 14:30
@rpaoloni-pass rpaoloni-pass merged commit 71c51e9 into master Nov 6, 2024
23 checks passed
@rpaoloni-pass rpaoloni-pass deleted the PC-31774-use-atomic-in-BO branch November 6, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants