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-33423)[API] feat: add status isActive to headline_offer #15647

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

(PC-33423)[API] feat: add status isActive to headline_offer #15647

wants to merge 5 commits into from

Conversation

ogeber-pass
Copy link
Contributor

@ogeber-pass ogeber-pass commented Dec 23, 2024 •
edited
Loading

But de la pull request

Ticket Jira (ou description si BSR) : https://passculture.atlassian.net/browse/PC-33423
commit 1 : retirer les contraintes d'unicité sur les offres & venues (pour préparer la suite)
commit 2: ajouter un timespan et des contraintes d'unicité sur l'overlap du timespan par offre et venue
commit 3: mettre à jour la fin du timespan si une offre n'est plus active (sera ajoutée à la crontab)
commit 4: mise à jour de la sandbox pour avoir des offres à la une valide
commit 5: mise à jour des routes GET offerer / headline_offer (PRO + NATIF)

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 23, 2024 •
edited
Loading

mypy cop report: 464 (master) ↗ 465 (your branch)

@github-actions GitHub Actions
Copy link
Contributor

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

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

https://pc-pro-testing--pr15647-ogeber-pc-33423-add-zlx9x20n.web.app

(expires Thu, 26 Dec 2024 15:28:45 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 032d233ee67e1c50d6af12e29c936c7076770eb1

@ogeber-pass ogeber-pass force-pushed the ogeber/pc-33423-add-status-to-headline-offer branch from ef1827d to d77ec93 Compare December 24, 2024 08:43
@ogeber-pass ogeber-pass marked this pull request as ready for review December 24, 2024 09:05
mgeoffray-pass
mgeoffray-pass reviewed Dec 24, 2024
View reviewed changes
api/src/pcapi/core/offers/api.py Outdated
try:
headline_offer = models.HeadlineOffer(offer=offer, venue=offer.venue, timespan=(datetime.datetime.utcnow(),))
db.session.add(headline_offer)
db.session.commit()
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi le commit ici ? Si c'est une nouvelle route, je pense qu'on devrait utiliser @atomic et le remplacer par un flush.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai mis un loooooong commentaire au cas où

api/src/pcapi/alembic/versions/20241220T095131_4c3be4ff5274_add_timespan_on_headline_offer.py Outdated Show resolved Hide resolved
api/src/pcapi/alembic/versions/20241220T095131_4c3be4ff5274_add_timespan_on_headline_offer.py Outdated
Comment on lines 22 to 30
ALTER TABLE headline_offer
ADD CONSTRAINT exclude_offer_timespan EXCLUDE USING gist (
"offerId"
WITH
=,
timespan
WITH
&&
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Le Add Constraint prends un lock un peu énervé et sera bloqué par les select sur cette table. Si on a une requête qui fait une jointure sur cette table (et je vois qu'il en éxiste déjà), on aura surement un problème pour prendre le lock. Il faut les créer en NOT VALID et dans une migration suivante valider la contrainte

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Malheureusement, une contrainte EXCLUDE ne peut pas être créé en NOT VALID, ce n'est pas supporté par postgresql

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sqlalchemy.exc.NotSupportedError: (psycopg2.errors.FeatureNotSupported) EXCLUDE constraints cannot be marked NOT VALID

[SQL: ALTER TABLE headline_offer ADD CONSTRAINT exclude_offer_timespan EXCLUDE USING gist ("offerId" WITH =, timespan WITH &&) NOT VALID]

@ogeber-pass ogeber-pass force-pushed the ogeber/pc-33423-add-status-to-headline-offer branch from d77ec93 to b79fb82 Compare December 24, 2024 11:07
@ogeber-pass ogeber-pass force-pushed the ogeber/pc-33423-add-status-to-headline-offer branch from 1d1c699 to 33a1c57 Compare December 24, 2024 15:09
@ogeber-pass ogeber-pass changed the base branch from master to PC-33385-get-offerer-headline-offer December 24, 2024 15:09
Base automatically changed from PC-33385-get-offerer-headline-offer to master December 24, 2024 15:25
@ogeber-pass ogeber-pass force-pushed the ogeber/pc-33423-add-status-to-headline-offer branch from 33a1c57 to 433c8e3 Compare December 24, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgeoffray-pass mgeoffray-pass mgeoffray-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.
2 participants
@ogeber-pass @mgeoffray-pass