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-32268)[API] feat: Return individual or collective statistics only if offerer is concerned - V2 #14992

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pcharlet-pass
Copy link
Contributor

But de la pull request

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

Deuxième version utilisant des modèle pydantic différent suivant les cas rencontré plutôt que de mettre à jour les résultat de la requête clickhouse et utiliser un modèle pydantic unique plus permissif

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

@pcharlet-pass pcharlet-pass force-pushed the pcharlet/pc-32268-statitics-none-zero-v2 branch 2 times, most recently from fdeabcf to ff31e23 Compare November 12, 2024 11:20
Copy link
Contributor

github-actions bot commented Nov 12, 2024

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

https://pc-pro-testing--pr14992-pcharlet-pc-32268-st-rnp58l9c.web.app

(expires Fri, 15 Nov 2024 16:10:03 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 032d233ee67e1c50d6af12e29c936c7076770eb1

Copy link
Contributor

@ogeber-pass ogeber-pass left a comment

Choose a reason for hiding this comment

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

Top, c'est mieux comme ça !

Et c'est validé côté data, les tables yearly_aggregated_venue_individual_revenue et yearly_aggregated_venue_collective_revenue ne sont pas vouées à changer au niveau du schéma (seulement la façon dont elles sont agrégées)

@@ -26,5 +27,11 @@ def get_statistics(query: StatisticsQueryModel) -> StatisticsModel:
status_code=422,
)
check_user_has_access_to_venues(current_user, venue_ids)

Choose a reason for hiding this comment

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

Je vois que tu as fait une seconde PR pour le même ticket, du coup je me permet de remettre ici le warning concernant cette fonction que j’ai soulevé dans la première

@pcharlet-pass pcharlet-pass force-pushed the pcharlet/pc-32268-statitics-none-zero-v2 branch from ff31e23 to ddbb612 Compare November 13, 2024 10:43
@@ -107,10 +107,9 @@ def has_access_to_venues(user: models.User, venue_ids: list[int]) -> bool:
offerers_models.UserOfferer.isValidated,
offerers_models.Venue.id.in_(venue_ids),
]
have_access_to_all_venues = query.filter(*filters).count() == len(venue_ids)
Copy link
Contributor

@dramelet-pass dramelet-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 aurait pu faire un peu plus efficace mais à priori un count sur venue ne posera pas de soucis en production

Ce qu’on cherche à faire c’est, est-ce que pour un user donné, la liste des venue_ids demandées sont toutes bien liées aux structures auquel appartient notre utilisateur ?
Dis autrement, est-ce que venue_ids est bien un subset des id de toutes les venues de toutes les structures de notre utilisateur, ce qui peut ce traduire comme ceci en SQL:

(données de sanbox)

pass_culture>
 SELECT ('{13, 14, 15}' <@ ARRAY_AGG(venue.id)) as has_all_venues FROM venue
 JOIN offerer ON offerer.id = venue."managingOffererId"
 JOIN user_offerer ON user_offerer."offererId" = offerer.id
 WHERE user_offerer."userId"=1
 GROUP BY user_offerer."userId";
+----------------+
| has_all_venues |
|----------------|
| True           |
+----------------+
SELECT 1
Time: 0.084s
pass_culture>
 SELECT ('{1, 13, 14, 15}' <@ ARRAY_AGG(venue.id)) as has_all_venues FROM venue
 JOIN offerer ON offerer.id = venue."managingOffererId"
 JOIN user_offerer ON user_offerer."offererId" = offerer.id
 WHERE user_offerer."userId"=1
 GROUP BY user_offerer."userId";
+----------------+
| has_all_venues |
|----------------|
| False          |
+----------------+
SELECT 1
Time: 0.008s

check_user_has_access_to_venues pourrait donc être modifiée comme ceci:

return db.session.execute(
    sa.select(
        sa.cast(postgresql.array(venue_ids), postgresql.ARRAY(postgresql.BIGINT)).contained_by(
            sa.func.array_agg(Venue.id)
        )
    )
    .select_from(Venue)
    .join(Offerer)
    .join(UserOfferer)
    .where(UserOfferer.userId == current_user)
    .group_by(UserOfferer.userId)
).scalar()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je me pose la question, est-ce qu'on a besoin de filtrer sur les offerers_models.UserOfferer.isValidated ? Tu l'as supprimé de la requête ci-dessus.

@pcharlet-pass pcharlet-pass force-pushed the pcharlet/pc-32268-statitics-none-zero-v2 branch from fdab1d3 to 29fe92e Compare November 13, 2024 15:53
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.

4 participants