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-33125)[API] fix: public api: edit event's location #15152

Merged

Conversation

jbaudet-pass
Copy link
Contributor

@jbaudet-pass jbaudet-pass commented Nov 20, 2024

But de la pull request

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

Permettre l'édition de l'emplacement d'un événement via l'API publique. Le portail pro le permet mais pas l'API, cette PR corrige cet écart.

J'ai essayé d'adopter une approche la plus proche possible de ce qui se fait pour la création (venue et offerer_address passés à part).

Copy link
Contributor

github-actions bot commented Nov 20, 2024

mypy cop report: 466 (master) = 466 (your branch)

@jbaudet-pass jbaudet-pass force-pushed the PC-33125/fix_public_api_offer_edit_can_update_address branch from 144dcce to f770add Compare November 21, 2024 14:11
) -> models.Offer:
aliases = set(body.dict(by_alias=True))
fields = body.dict(by_alias=True, exclude_unset=True)

# updated using the pro interface
Copy link
Contributor

Choose a reason for hiding this comment

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

Je pense qu'il y a peut-être une refacto à faire ici mais dans un deuxième temps. Le code de l'update est ici trop orienté portail pro (mais je sais bien que ce n'est pas de ton fait).

@@ -428,7 +439,7 @@ def update_offer(

withdrawal_fields = {"bookingContact", "withdrawalDelay", "withdrawalDetails", "withdrawalType"}
withdrawal_updated = updates_set & withdrawal_fields
oa_updated = "offererAddress" in updates
oa_updated = "offererAddress" in updates or (offerer_address is not None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Je ne suis pas sûr qu'il faille ajouter la condition or (offerer_address is not None) ici parce que si on a renseigné un offerer_addressmais qu'il est identique au précédent offerer_address alors normalement il n'y a pas d'update (si on se réfère à la ligne 376).

Comment on lines 251 to 254
with suppress(AttributeError):
if body.location.venue_id: # type: ignore[union-attr]
venue = utils.get_venue_with_offerer_address(body.location.venue_id) # type: ignore[union-attr]
offerer_address = venue.offererAddress # default offerer_address
Copy link
Contributor

Choose a reason for hiding this comment

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

Plutôt que d'utiliser suppress(AttributeError) et d'avoir à ajouter des # type: ignore[union-attr], le mieux ici c'est sans doute de procéder ainsi

Suggested change
with suppress(AttributeError):
if body.location.venue_id: # type: ignore[union-attr]
venue = utils.get_venue_with_offerer_address(body.location.venue_id) # type: ignore[union-attr]
offerer_address = venue.offererAddress # default offerer_address
location = body.location
if location:
venue = utils.get_venue_with_offerer_address(location.venue_id)
if location.type == "address":
address = utils.get_address_or_raise_404(location.address_id)
offerer_address = offerers_api.get_or_create_offerer_address(
offerer_id=venue.managingOffererId,
address_id=address.id,
label=location.address_label,
)
else:
offerer_address = venue.offererAddress

Comment on lines 258 to 268
with suppress(AttributeError):
if body.location.type == "address": # type: ignore[union-attr]
assert venue is not None

address = utils.get_address_or_raise_404(body.location.address_id) # type: ignore[union-attr]
offerer_address = offerers_api.get_or_create_offerer_address(
offerer_id=venue.managingOffererId,
address_id=address.id,
label=body.location.address_label, # type: ignore[union-attr]
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Avec ma proposition précédente tu peux enlever cette partie :)

body: offers_schemas.UpdateOffer,
venue: offerers_models.Venue | None = None,
offerer_address: offerers_models.OffererAddress | None = None,
is_from_private_api: bool = False,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ce qui pourrait être cool c'est que tu ajoutes des tests pour les changements que tu as apportés à update_offer

@jbaudet-pass jbaudet-pass force-pushed the PC-33125/fix_public_api_offer_edit_can_update_address branch from f770add to 61477a7 Compare November 22, 2024 16:17
@jbaudet-pass jbaudet-pass force-pushed the PC-33125/fix_public_api_offer_edit_can_update_address branch from 61477a7 to bea5d34 Compare November 22, 2024 16:23
@jbaudet-pass jbaudet-pass merged commit 8273291 into master Nov 22, 2024
24 checks passed
@jbaudet-pass jbaudet-pass deleted the PC-33125/fix_public_api_offer_edit_can_update_address branch November 22, 2024 17:16
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.

2 participants