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.

Sign up for GitHub

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

Jump to bottom

(BSR)[API] feat: public api: add @atomic to products module #15082

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

(BSR)[API] feat: public api: add @atomic to products module #15082

wants to merge 1 commit into from
+184 −137

Conversation

jbaudet-pass
Copy link
Contributor

But de la pull request

Ajout du décorateur @atomic aux routes du module products de l'API publique.
Ces ajouts s'accompagnent d'autres : on_commit pour les fonctions appelées (in)directement par ces routes et qui nécessitent que la transaction soit validée avant de s'exécuter.

@jbaudet-pass jbaudet-pass force-pushed the jbaudet/2024_11_18_journee_back_atomic_to_public_api branch from 4436206 to 5fb9f57 Compare November 21, 2024 17:13
@jbaudet-pass jbaudet-pass force-pushed the jbaudet/2024_11_18_journee_back_atomic_to_public_api branch 4 times, most recently from e1dc8fc to 396df39 Compare December 17, 2024 14:13
@jbaudet-pass jbaudet-pass marked this pull request as ready for review December 17, 2024 14:15
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 17, 2024 •
edited
Loading

Visit the preview URL for this PR (updated for commit e6af3e8):

https://pc-pro-testing--pr15082-jbaudet-2024-11-18-j-2nmsbk8c.web.app

(expires Sat, 11 Jan 2025 16:14:12 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 032d233ee67e1c50d6af12e29c936c7076770eb1

@jbaudet-pass jbaudet-pass force-pushed the jbaudet/2024_11_18_journee_back_atomic_to_public_api branch from 396df39 to 4474a62 Compare December 23, 2024 15:43
@jbaudet-pass jbaudet-pass force-pushed the jbaudet/2024_11_18_journee_back_atomic_to_public_api branch from 4474a62 to 30fcee6 Compare January 8, 2025 09:17
rpaoloni-pass
rpaoloni-pass reviewed Jan 8, 2025
View reviewed changes
Copy link
Contributor

@rpaoloni-pass rpaoloni-pass left a comment

Choose a reason for hiding this comment

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

revue partiel parce que c'est toujours les deux mêmes erreurs et si je continue je vais me répéter. Il y en a probablement d'autres similaire dans la suite

api/src/pcapi/routes/public/individual_offers/v1/products.py
@@ -276,6 +281,7 @@ def _create_stock(product: offers_models.Offer, body: serialization.ProductOffer
)
),
)
@atomic()
Copy link
Contributor

Choose a reason for hiding this comment

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

cette fonction appeles indirectement create_stock qui appele async_index_offer_ids qui devrait être dans un on_commit

api/src/pcapi/routes/public/individual_offers/v1/products.py
@@ -333,6 +339,7 @@ def post_product_offer(body: serialization.ProductOfferCreation) -> serializatio
)
),
)
@atomic()
Copy link
Contributor

Choose a reason for hiding this comment

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

_create_or_update_ean_offers.delay devrait être dans un on_commit sinon on va avoir des problemes de concurrence et l'endpoint va parfois pas marcher (ce qui est assez difficile a débugger)

api/src/pcapi/core/offers/api.py Outdated
@@ -443,9 +443,10 @@ def update_offer(
withdrawal_updated = updates_set & withdrawal_fields
oa_updated = "offererAddress" in updates
if should_send_mail and (withdrawal_updated or oa_updated):
transactional_mails.send_email_for_each_ongoing_booking(offer)
on_commit(partial(transactional_mails.send_email_for_each_ongoing_booking, offer))
Copy link
Contributor

Choose a reason for hiding this comment

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

attention on ne peut pas passer des objets liés a la session (ici offer) a des fonctions utilisées dans un on_commit parce que ces fonctions sont appelées après la fin de la session. Dans le meilleur des cas ça rajoute un appel a la db apres la session, dans le pire des cas ça plante.

(si tu as besoin d'aider pour migrer la fonction hésites pas a me ping)

api/src/pcapi/core/offers/api.py

reason = search.IndexationReason.OFFER_UPDATE

search.async_index_offer_ids([offer.id], reason=reason, log_extra={"changes": updates_set})
Copy link
Contributor

Choose a reason for hiding this comment

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

les appels asynchrones doivent être dans des on_commit

api/src/pcapi/core/offers/api.py
reason=search.IndexationReason.STOCK_CREATION,
)

search.async_index_offer_ids([offer.id], reason=search.IndexationReason.STOCK_CREATION)
Copy link
Contributor

Choose a reason for hiding this comment

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

idem

api/src/pcapi/core/offers/api.py Outdated
Comment on lines 1048 to 1050
search.async_index_offer_ids(
[stock.offerId],
reason=search.IndexationReason.STOCK_DELETION,
Copy link
Contributor

Choose a reason for hiding this comment

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

idem

api/src/pcapi/core/offers/api.py
[offer.id],
reason=search.IndexationReason.MEDIATION_CREATION,
)
search.async_index_offer_ids([offer.id], reason=search.IndexationReason.MEDIATION_CREATION)
Copy link
Contributor

Choose a reason for hiding this comment

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

idem

api/src/pcapi/core/offers/api.py Outdated
@@ -1037,10 +1034,17 @@ def _delete_stock(stock: models.Stock, author_id: int | None = None, user_connec
)
if cancelled_bookings:
for booking in cancelled_bookings:
transactional_mails.send_booking_cancellation_by_pro_to_beneficiary_email(booking)
transactional_mails.send_booking_cancellation_confirmation_by_pro_email(cancelled_bookings)
on_commit(partial(transactional_mails.send_booking_cancellation_by_pro_to_beneficiary_email, booking))
Copy link
Contributor

Choose a reason for hiding this comment

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

cette fonction (et spécifiquement celle ci) n'a pas besoin d'être dans un on_commit

api/src/pcapi/core/offers/api.py Outdated
transactional_mails.send_booking_cancellation_confirmation_by_pro_email(cancelled_bookings)
on_commit(partial(transactional_mails.send_booking_cancellation_by_pro_to_beneficiary_email, booking))

on_commit(partial(transactional_mails.send_booking_cancellation_confirmation_by_pro_email, cancelled_bookings))
Copy link
Contributor

Choose a reason for hiding this comment

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

Cancelled_bookins ne peut pas êter passé a un on_commit :(

api/src/pcapi/core/offers/api.py Outdated
else:
if isinstance(offer, models.Offer):
compliance.update_offer_compliance_score(offer, is_primary=False)
on_commit(partial(compliance.update_offer_compliance_score, offer, is_primary=False))
Copy link
Contributor

Choose a reason for hiding this comment

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

offer ne peut pas êter passé dans on_commit :(

@jbaudet-pass
(BSR)[API] feat: public api: add @atomic to products module
e6af3e8 
Add @atomic to routes and some nested functions called by them.
@jbaudet-pass jbaudet-pass force-pushed the jbaudet/2024_11_18_journee_back_atomic_to_public_api branch from 30fcee6 to e6af3e8 Compare January 9, 2025 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rpaoloni-pass rpaoloni-pass rpaoloni-pass left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jbaudet-pass @rpaoloni-pass