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

(PC-33575)[API] feat: add addressId param in GET Events and GET Products API endpoints #15512

Merged
merged 1 commit into from
Dec 16, 2024
Merged

(PC-33575)[API] feat: add addressId param in GET Events and GET Products API endpoints #15512

merged 1 commit into from
Dec 16, 2024

Conversation

tcoudray-pass
Copy link
Contributor

@tcoudray-pass tcoudray-pass commented Dec 13, 2024 •
edited
Loading

Drop required constraint on venueId.

But de la pull request

Ticket Jira : https://passculture.atlassian.net/browse/PC-33575

image

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
  • J'ai fait la revue fonctionnelle de mon ticket

@github-actions GitHub Actions
Copy link
Contributor

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

mypy cop report: 465 (master) ↘ 463 (your branch)

Have yourself a merry little break and learn a few facts about the year 463.

@github-actions GitHub Actions
Copy link
Contributor

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

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

https://pc-pro-testing--pr15512-tcoudray-pass-pc-335-qrn9dls7.web.app

(expires Wed, 18 Dec 2024 12:20:18 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 032d233ee67e1c50d6af12e29c936c7076770eb1

@tcoudray-pass tcoudray-pass force-pushed the tcoudray-pass/PC-33575-add-address-fitler-on-get-events branch from 3a0bc10 to 8aab847 Compare December 13, 2024 09:23
@tcoudray-pass tcoudray-pass force-pushed the tcoudray-pass/PC-33575-add-address-fitler-on-get-events branch 2 times, most recently from 4887617 to a802d5e Compare December 13, 2024 10:52
prouzet-pass
prouzet-pass reviewed Dec 13, 2024
View reviewed changes
Copy link
Contributor

@prouzet-pass prouzet-pass left a comment

Choose a reason for hiding this comment

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

Simple relecture sans tester.

api/documentation/src/pages/change-logs.md Outdated Show resolved Hide resolved
api/documentation/static/openapi.json
Comment on lines -4063 to -4065
"required": [
"venueId"
],
Copy link
Contributor

Choose a reason for hiding this comment

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

Question : est-il donc autorisé de n'avoir ni venueId ni addressId ?
(je ne sais pas si le format permet d'avoir une contrainte sur « l'un ou l'autre »)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes cette contrainte n'a pas vraiment de sens. Pourquoi forcément obliger de filtrer sur une venueId ou sur une addressId alors que de toute façon la requête est paginée ?

api/tests/routes/public/individual_offers/v1/get_events_test.py Show resolved Hide resolved
api/tests/routes/public/individual_offers/v1/get_products_test.py Show resolved Hide resolved
api/tests/routes/public/individual_offers/v1/get_products_test.py Outdated Show resolved Hide resolved
@tcoudray-pass tcoudray-pass force-pushed the tcoudray-pass/PC-33575-add-address-fitler-on-get-events branch from a802d5e to 8fc1093 Compare December 16, 2024 09:30
@tcoudray-pass tcoudray-pass force-pushed the tcoudray-pass/PC-33575-add-address-fitler-on-get-events branch from 8fc1093 to 1a44dee Compare December 16, 2024 10:05
jbaudet-pass
jbaudet-pass approved these changes Dec 16, 2024
View reviewed changes
api/tests/routes/public/individual_offers/v1/get_products_test.py
response = client.with_explicit_token(plain_api_key).get(self.endpoint_url)

assert response.status_code == 200
assert [product["id"] for product in response.json["products"]] == [offer.id for offer in offers]
Copy link
Contributor

Choose a reason for hiding this comment

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

(détail) il y a toujours un petit risque avec les comparaisons de listes comme ça, surtout que l'ordre n'est pas très important ici et qu'il pourrait changer à l'avenir.

Suggested change
assert [product["id"] for product in response.json["products"]] == [offer.id for offer in offers]
assert {product["id"] for product in response.json["products"]} == {offer.id for offer in offers}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alors si justement parce que ce endpoint renvoie les produits par ordre croissant d'id (c'est ce qui permet la pagination par index).

Copy link
Contributor

Choose a reason for hiding this comment

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

on n'a rien vu, on n'a rien vu 😶‍🌫️

api/tests/routes/public/individual_offers/v1/get_products_test.py
Comment on lines +55 to +57
offers = offers_factories.ThingOfferFactory.create_batch(
12, venue=venue_provider.venue
) + offers_factories.ThingOfferFactory.create_batch(12, venue=venue2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Est-il nécessaire de créer autant d'objets ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Non c'est assez arbitraire pour le coup

api/tests/routes/public/individual_offers/v1/get_products_test.py Outdated Show resolved Hide resolved
@tcoudray-pass
(PC-33575)[API] feat: add addressId param in GET Events and GET Pro…
bc2bf3c 
…ducts API endpoints

drop required constraint on `venueId`
@tcoudray-pass tcoudray-pass force-pushed the tcoudray-pass/PC-33575-add-address-fitler-on-get-events branch from 1a44dee to bc2bf3c Compare December 16, 2024 12:17
@tcoudray-pass tcoudray-pass merged commit 0391417 into master Dec 16, 2024
25 of 26 checks passed
@tcoudray-pass tcoudray-pass deleted the tcoudray-pass/PC-33575-add-address-fitler-on-get-events branch December 16, 2024 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prouzet-pass prouzet-pass prouzet-pass left review comments

@jbaudet-pass jbaudet-pass jbaudet-pass approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@tcoudray-pass @prouzet-pass @jbaudet-pass