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-30105)[API] future offer script #12920

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

qdasilva-pass
Copy link
Contributor

But de la pull request

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

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

@qdasilva-pass qdasilva-pass force-pushed the qds_PC-30105_future_offer_script branch 2 times, most recently from 84a6d40 to bcb4e00 Compare June 25, 2024 12:08
@qdasilva-pass qdasilva-pass marked this pull request as ready for review June 25, 2024 12:10
@qdasilva-pass qdasilva-pass force-pushed the qds_PC-30105_future_offer_script branch 2 times, most recently from 9283459 to edd4baf Compare June 25, 2024 13:40
@qdasilva-pass qdasilva-pass force-pushed the qds_PC-30105_future_offer_script branch from edd4baf to 986c0e3 Compare June 26, 2024 09:39
@qdasilva-pass qdasilva-pass force-pushed the qds_PC-30105_future_offer_script branch from 986c0e3 to 5039e68 Compare June 26, 2024 09:51
Comment on lines +495 to +496
def activate_future_offers(publication_date: datetime.datetime | None = None) -> None:
query = offers_repository.get_offers_by_publication_date(publication_date=publication_date)
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 / nommage] : ici je nommerais publication_date autrement, car on veut "publier" les offres dont la publication_datecorrespond à l'instant que l'on compare. Donc un truc genre current_time ou autre histoire de clarifier l'action effectuée

Copy link
Contributor

Choose a reason for hiding this comment

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

et je sais pas à quel point c'est non-pythonic / moche, mais ça pourrait se faire de faire ça ?

Suggested change
def activate_future_offers(publication_date: datetime.datetime | None = None) -> None:
query = offers_repository.get_offers_by_publication_date(publication_date=publication_date)
def activate_future_offers(publication_date: datetime.datetime | None = datetime.datetime.utcnow() ) -> None:
query = offers_repository.get_offers_by_publication_date(publication_date=publication_date)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sur le nommage publication_date ça me paraissait naturel de le nommer de cette façon. Il y a la fonction get_offers_by_publication_date avec un paramètre publication_date et du coup ce paramètre "se transmet" au fil des appels

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pour le datetime.datetime.utcnow() en valeur par défaut, en plus du côté non-pythonic ça donnerait toujours la même datetime (évaluée au lancement de l'application), et on souhaite plutôt que la date de publication soit évaluée au moment où la fonction s'exécute :

>>> import time
>>> from datetime import datetime
>>> 
>>> def func_1(dt: datetime = datetime.utcnow()):
...     print(dt)
... 
>>> 
>>> for _ in range(3):
...     func_1()
...     time.sleep(1)
... 
2024-06-26 15:17:29.989627
2024-06-26 15:17:29.989627
2024-06-26 15:17:29.989627
>>> 
>>> def func_2(dt: datetime | None = None):
...     if dt is None:
...         dt = datetime.utcnow()
...     print(dt)
... 
>>> 
>>> for _ in range(3):
...     func_2()
...     time.sleep(1)
... 
2024-06-26 15:18:52.595448
2024-06-26 15:18:53.600778
2024-06-26 15:18:54.604743

@qdasilva-pass qdasilva-pass merged commit 027ca8b into master Jun 26, 2024
22 checks passed
@qdasilva-pass qdasilva-pass deleted the qds_PC-30105_future_offer_script branch June 26, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants