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-32974)[API] fix: broken date filters on offer's page #15014

Conversation

xordoquy-pass
Copy link
Contributor

@xordoquy-pass xordoquy-pass commented Nov 13, 2024

But de la pull request

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

The join on Offer broke things since it missed the filters on it because there was already an alias used on offer.
It looks like the second join got all the stocks all the time, making the filters on date useless.

The test did not detect it because it was only making sure the offers were OK within the time range but was not checking if the offers were correctly ignored when outside the time range.

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

The join on Offer broke things since it missed the filters on it because
there was already an alias used on offer.
It looks like the second join got all the stocks all the time, making the
filters on date useless.

The test did not detect it because it was only making sure the offers
were OK within the time range but was not checking if the offers were
correctly ignored when outside the time range.
Copy link
Contributor

@lmaubert-pass lmaubert-pass left a comment

Choose a reason for hiding this comment

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

C'est bon pour moi =)

)
stock_query = stock_query.outerjoin(
offerers_models.OffererAddress,
offer_alias.offererAddressId == offerers_models.OffererAddress.id,
Copy link
Contributor

@tcoudray-pass tcoudray-pass Nov 13, 2024

Choose a reason for hiding this comment

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

On n'est d'accord que le isouter était inutile parce que l'on utilise la méthode outerjoin et non join ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C'est ce que je comprends aussi sur les parametres de https://docs.sqlalchemy.org/en/13/orm/query.html#sqlalchemy.orm.query.Query.join

isouter=False – If True, the join used will be a left outer join, just as if the Query.outerjoin() method were called.

@xordoquy-pass xordoquy-pass merged commit fb4161a into master Nov 13, 2024
24 checks passed
@xordoquy-pass xordoquy-pass deleted the PC-32974-pro-page-offres-et-reservations-a-verifier-le-filtre-par-date-est-ko branch November 13, 2024 15:55
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.

3 participants