Skip to content

(PC-32268)[API] feat: Return individual or collective statistics only if offerer is concerned #14968

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

Closed
wants to merge 2 commits into from
Closed

(PC-32268)[API] feat: Return individual or collective statistics only if offerer is concerned #14968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
86dfa7e
(PC-32268)[API] feat: Return individual or collective statistics only…
pcharlet-pass Nov 7, 2024
d547187
(PC-32268)[API] feat: Return individual or collective statistics only…
pcharlet-pass Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 36 additions & 11 deletions api/src/pcapi/connectors/clickhouse/queries/yearly_revenue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,25 @@ class Config:
extra = "forbid"


class IndividualOnlyRevenue(pydantic_v1.BaseModel):
total: Decimal
individual: Decimal

class Config:
extra = "forbid"


class CollectiveOnlyRevenue(pydantic_v1.BaseModel):
total: Decimal
collective: Decimal

class Config:
extra = "forbid"


class AggregatedRevenue(pydantic_v1.BaseModel):
revenue: Revenue
expected_revenue: Revenue
revenue: Revenue | IndividualOnlyRevenue | CollectiveOnlyRevenue
expected_revenue: Revenue | IndividualOnlyRevenue | CollectiveOnlyRevenue

class Config:
extra = "forbid"
Expand All @@ -34,16 +50,25 @@ class Config:


class YearlyAggregatedRevenueQuery(BaseQuery[YearlyAggregatedRevenueModel]):
def __init__(self, venues_have_individual: bool, venues_have_collective: bool) -> None:
self.venues_have_individual = venues_have_individual
self.venues_have_collective = venues_have_collective
super().__init__()

def _format_result(self, results: list) -> dict:
return {
"incomeByYear": {
result.year: {
"revenue": json.loads(result.revenue),
"expectedRevenue": json.loads(result.expected_revenue),
}
for result in results
}
}
income_by_year = {}
for result in results:
revenue = json.loads(result.revenue)
expected_revenue = json.loads(result.expected_revenue)
if not self.venues_have_individual:
revenue.pop("individual", None)
expected_revenue.pop("individual", None)
if not self.venues_have_collective:
revenue.pop("collective", None)
expected_revenue.pop("collective", None)
income_by_year.update({result.year: {"revenue": revenue, "expectedRevenue": expected_revenue}})

return {"incomeByYear": income_by_year}

@property
def raw_query(self) -> str:
Expand Down
11 changes: 11 additions & 0 deletions api/src/pcapi/core/offers/repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1396,3 +1396,14 @@ def merge_products(to_keep: models.Product, to_delete: models.Product) -> models
db.session.delete(to_delete)

return to_keep


def venues_have_individual_and_collective_offers(venue_ids: list[int]) -> tuple[bool, bool]:
return (
db.session.query(offers_model.Offer.query.filter(offers_model.Offer.venueId.in_(venue_ids)).exists()).scalar(),
db.session.query(
educational_models.CollectiveOffer.query.filter(
educational_models.CollectiveOffer.venueId.in_(venue_ids)
).exists()
).scalar(),
)
5 changes: 2 additions & 3 deletions api/src/pcapi/core/users/repository.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

query = db.session.query(query.filter(*filters).exists()).scalar()

return query
return have_access_to_all_venues


def get_newly_eligible_age_18_users(since: date) -> list[models.User]:
Expand Down
6 changes: 5 additions & 1 deletion api/src/pcapi/routes/pro/statistics.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from flask_login import login_required

from pcapi.connectors.clickhouse import queries as clickhouse_queries
from pcapi.core.offers.repository import venues_have_individual_and_collective_offers
from pcapi.models.api_errors import ApiErrors
from pcapi.routes.apis import private_api
from pcapi.routes.serialization.statistics_serialize import StatisticsModel
Expand All @@ -26,5 +27,8 @@ def get_statistics(query: StatisticsQueryModel) -> StatisticsModel:
status_code=422,
)
check_user_has_access_to_venues(current_user, venue_ids)
Copy link

@damien-ramelet damien-ramelet Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ça n'est pas directement en lien avec ta PR, mais d'après git tu l'as implémenté. J'ai l'impression qu'il y a un soucis d'implémentation, cette fonction renvoit True à partir du moment où au moins un id de venue_ids appartient à l'utilisateur. Je pense (en tout cas le naming et la logique métier laissent à penser) qu'elle devrait plutôt renvoyer True si et seulement si tous les id de venue_ids appartiennent à l'utilisateur.

Le soucis se situe au niveau du exists, où on vérifie qu'il y a au moins un user_offerer correspondant au user passé en paramètre et à la liste de venue_id, on devrait plutôt vérifier que tous existent non ?

(testable facilement en local avec les données de sandbox)

Copy link

Choose a reason for hiding this comment

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

(je ne voulais pas approve, j'ai misclick : )

Copy link

Choose a reason for hiding this comment

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

On peut facilement écrire un test de non regression, comme ceci:

     @patch("pcapi.connectors.clickhouse.testing_backend.TestingBackend.run_query")
     def test_get_statistics_from_multiple_venues(self, run_query, client):
+        venue_not_belonging_to_user = offerers_factories.VenueFactory()
         user = users_factories.UserFactory()
         offerer = offerers_factories.OffererFactory()
         offerers_factories.UserOffererFactory(user=user, offerer=offerer)
         venue2 = offerers_factories.VenueFactory(managingOfferer=offerer)
         venue_id = venue.id
         venue2_id = venue2.id
+        foreign_venue = venue_not_belonging_to_user.id
         test_client = client.with_session_auth(email=user.email)
         num_queries = testing.AUTHENTICATION_QUERIES
         num_queries += 1  # select Offerer
         with testing.assert_num_queries(num_queries):
             run_query.return_value = fixtures.YEARLY_AGGREGATED_VENUE_REVENUE
-            response = test_client.get(f"/get-statistics/?venue_ids={venue_id}&venue_ids={venue2_id}")
+            response = test_client.get(f"/get-statistics/?venue_ids={venue_id}&venue_ids={venue2_id}&venue_ids={foreign_venue}")
             assert response.status_code == 200

On ne devrait pas avoir une 200 dans ces conditions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En effet ! Merci d'avoir trouvé ça ! (corrigé sur les 2 PR)

Copy link

Choose a reason for hiding this comment

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

Merci à toi !

result = clickhouse_queries.YearlyAggregatedRevenueQuery().execute(tuple(venue_ids))
venues_have_individual, venues_have_collective = venues_have_individual_and_collective_offers(venue_ids)
result = clickhouse_queries.YearlyAggregatedRevenueQuery(venues_have_individual, venues_have_collective).execute(
tuple(venue_ids)
)
return StatisticsModel.from_query(income_by_year=result.income_by_year)
2 changes: 1 addition & 1 deletion api/tests/connectors/clickhouse/clickhouse_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def test_get_yearly_revenue(self):

with mock.patch("pcapi.connectors.clickhouse.testing_backend.TestingBackend.run_query") as mock_run_query:
mock_run_query.return_value = fixtures.YEARLY_AGGREGATED_VENUE_REVENUE
result = clickhouse_queries.YearlyAggregatedRevenueQuery().execute(venue_ids)
result = clickhouse_queries.YearlyAggregatedRevenueQuery(True, True).execute(venue_ids)

assert result.income_by_year["2024"].revenue.total == Decimal("24.24")
assert result.income_by_year["2024"].revenue.individual == Decimal("12.12")
Expand Down
89 changes: 86 additions & 3 deletions api/tests/routes/pro/get_statistics_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
import pytest

from pcapi.core import testing
import pcapi.core.educational.factories as educational_factories
import pcapi.core.offerers.factories as offerers_factories
import pcapi.core.offers.factories as offers_factories
import pcapi.core.users.factories as users_factories

from tests.connectors.clickhouse import fixtures
Expand All @@ -18,10 +20,14 @@ def test_get_statistics_from_one_venue(self, run_query, client):
offerers_factories.UserOffererFactory(user=user, offerer=offerer)
venue = offerers_factories.VenueFactory(managingOfferer=offerer)
venue_id = venue.id
educational_factories.CollectiveOfferFactory(venue=venue)
offers_factories.OfferFactory(venue=venue)

test_client = client.with_session_auth(email=user.email)
num_queries = testing.AUTHENTICATION_QUERIES
num_queries += 1 # select Offerer
num_queries += 1 # select Offer
num_queries += 1 # select CollectiveOffer
with testing.assert_num_queries(num_queries):
run_query.return_value = fixtures.YEARLY_AGGREGATED_VENUE_REVENUE
response = test_client.get(f"/get-statistics/?venue_ids={venue_id}")
Expand All @@ -44,10 +50,14 @@ def test_get_statistics_from_multiple_venues(self, run_query, client):
venue2 = offerers_factories.VenueFactory(managingOfferer=offerer)
venue_id = venue.id
venue2_id = venue2.id
educational_factories.CollectiveOfferFactory(venue=venue)
offers_factories.OfferFactory(venue=venue)

test_client = client.with_session_auth(email=user.email)
num_queries = testing.AUTHENTICATION_QUERIES
num_queries += 1 # select Offerer
num_queries += 1 # select Offer
num_queries += 1 # select CollectiveOffer
with testing.assert_num_queries(num_queries):
run_query.return_value = fixtures.YEARLY_AGGREGATED_VENUE_REVENUE
response = test_client.get(f"/get-statistics/?venue_ids={venue_id}&venue_ids={venue2_id}")
Expand All @@ -70,10 +80,14 @@ def test_get_statistics_multiple_years(self, run_query, client):
venue2 = offerers_factories.VenueFactory(managingOfferer=offerer)
venue_id = venue.id
venue2_id = venue2.id
educational_factories.CollectiveOfferFactory(venue=venue)
offers_factories.OfferFactory(venue=venue)

test_client = client.with_session_auth(email=user.email)
num_queries = testing.AUTHENTICATION_QUERIES
num_queries += 1 # select Offerer
num_queries += 1 # select Offer
num_queries += 1 # select CollectiveOffer
with testing.assert_num_queries(num_queries):
run_query.return_value = fixtures.YEARLY_AGGREGATED_VENUE_REVENUE_MULTIPLE_YEARS
response = test_client.get(f"/get-statistics/?venue_ids={venue_id}&venue_ids={venue2_id}")
Expand All @@ -92,16 +106,84 @@ def test_get_statistics_multiple_years(self, run_query, client):
}
}

@patch("pcapi.connectors.clickhouse.testing_backend.TestingBackend.run_query")
def test_get_statistics_only_collective(self, run_query, client):
user = users_factories.UserFactory()
offerer = offerers_factories.OffererFactory()
offerers_factories.UserOffererFactory(user=user, offerer=offerer)
venue = offerers_factories.VenueFactory(managingOfferer=offerer)
venue_id = venue.id
educational_factories.CollectiveOfferFactory(venue=venue)

test_client = client.with_session_auth(email=user.email)
num_queries = testing.AUTHENTICATION_QUERIES
num_queries += 1 # select Offerer
num_queries += 1 # select Offer
num_queries += 1 # select CollectiveOffer
with testing.assert_num_queries(num_queries):
run_query.return_value = fixtures.YEARLY_AGGREGATED_VENUE_REVENUE_MULTIPLE_YEARS
response = test_client.get(f"/get-statistics/?venue_ids={venue_id}")
assert response.status_code == 200
assert response.json == {
"incomeByYear": {
"2022": {
"expectedRevenue": {"collective": 22.12, "total": 44.24},
"revenue": {"collective": 22.12, "total": 44.24},
},
"2023": {},
"2024": {
"expectedRevenue": {"collective": 13.12, "total": 26.24},
"revenue": {"collective": 12.12, "total": 24.24},
},
}
}

@patch("pcapi.connectors.clickhouse.testing_backend.TestingBackend.run_query")
def test_get_statistics_only_individual(self, run_query, client):
user = users_factories.UserFactory()
offerer = offerers_factories.OffererFactory()
offerers_factories.UserOffererFactory(user=user, offerer=offerer)
venue = offerers_factories.VenueFactory(managingOfferer=offerer)
venue_id = venue.id
offers_factories.OfferFactory(venue=venue)

test_client = client.with_session_auth(email=user.email)
num_queries = testing.AUTHENTICATION_QUERIES
num_queries += 1 # select Offerer
num_queries += 1 # select Offer
num_queries += 1 # select CollectiveOffer
with testing.assert_num_queries(num_queries):
run_query.return_value = fixtures.YEARLY_AGGREGATED_VENUE_REVENUE_MULTIPLE_YEARS
response = test_client.get(f"/get-statistics/?venue_ids={venue_id}")
assert response.status_code == 200
assert response.json == {
"incomeByYear": {
"2022": {
"expectedRevenue": {"individual": 22.12, "total": 44.24},
"revenue": {"individual": 22.12, "total": 44.24},
},
"2023": {},
"2024": {
"expectedRevenue": {"individual": 13.12, "total": 26.24},
"revenue": {"individual": 12.12, "total": 24.24},
},
}
}

def test_get_statistics_empty_result(self, client):
user = users_factories.UserFactory()
offerer = offerers_factories.OffererFactory()
offerers_factories.UserOffererFactory(user=user, offerer=offerer)
venue = offerers_factories.VenueFactory(managingOfferer=offerer)
venue_id = venue.id
educational_factories.CollectiveOfferFactory(venue=venue)
offers_factories.OfferFactory(venue=venue)

test_client = client.with_session_auth(email=user.email)
num_queries = testing.AUTHENTICATION_QUERIES
num_queries += 1 # select Offerer
num_queries += 1 # select Offer
num_queries += 1 # select CollectiveOffer
with testing.assert_num_queries(num_queries):
response = test_client.get(f"/get-statistics/?venue_ids={venue_id}")
assert response.status_code == 200
Expand Down Expand Up @@ -129,17 +211,18 @@ def test_get_statistics_with_no_venue_id_should_fail(self, client):
class Returns403Test:
def test_get_statistics_from_not_owned_venue_should_fail(self, client):
user = users_factories.UserFactory()
user2 = users_factories.UserFactory()
offerer = offerers_factories.OffererFactory()
offerers_factories.UserOffererFactory(user=user2, offerer=offerer)
offerers_factories.UserOffererFactory(user=user, offerer=offerer)
venue = offerers_factories.VenueFactory(managingOfferer=offerer)
venue_not_belonging_to_user = offerers_factories.VenueFactory()
venue_id = venue.id
foreign_venue = venue_not_belonging_to_user.id

test_client = client.with_session_auth(email=user.email)
num_queries = testing.AUTHENTICATION_QUERIES
num_queries += 1 # select Offerer
with testing.assert_num_queries(num_queries):
response = test_client.get(f"/get-statistics/?venue_ids={venue_id}")
response = test_client.get(f"/get-statistics/?venue_ids={venue_id}&venue_ids={foreign_venue}")
assert response.status_code == 403
assert response.json["global"] == [
"Vous n'avez pas les droits d'accès suffisants pour accéder à cette information."
Expand Down
2 changes: 2 additions & 0 deletions pro/src/apiClient/v1/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export type { CollectiveOfferTemplateBodyModel } from './models/CollectiveOfferT
export type { CollectiveOfferTemplateResponseIdModel } from './models/CollectiveOfferTemplateResponseIdModel';
export { CollectiveOfferType } from './models/CollectiveOfferType';
export type { CollectiveOfferVenueBodyModel } from './models/CollectiveOfferVenueBodyModel';
export type { CollectiveOnlyRevenue } from './models/CollectiveOnlyRevenue';
export type { CollectiveStockCreationBodyModel } from './models/CollectiveStockCreationBodyModel';
export type { CollectiveStockEditionBodyModel } from './models/CollectiveStockEditionBodyModel';
export type { CollectiveStockResponseModel } from './models/CollectiveStockResponseModel';
Expand Down Expand Up @@ -140,6 +141,7 @@ export type { GetVenueResponseModel } from './models/GetVenueResponseModel';
export type { GetVenuesOfOffererFromSiretResponseModel } from './models/GetVenuesOfOffererFromSiretResponseModel';
export type { HasInvoiceQueryModel } from './models/HasInvoiceQueryModel';
export type { HasInvoiceResponseModel } from './models/HasInvoiceResponseModel';
export type { IndividualOnlyRevenue } from './models/IndividualOnlyRevenue';
export type { InviteMemberQueryModel } from './models/InviteMemberQueryModel';
export type { InvoiceListV2QueryModel } from './models/InvoiceListV2QueryModel';
export type { InvoiceListV2ResponseModel } from './models/InvoiceListV2ResponseModel';
Expand Down
6 changes: 4 additions & 2 deletions pro/src/apiClient/v1/models/AggregatedRevenue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
/* istanbul ignore file */
/* tslint:disable */
/* eslint-disable */
import type { CollectiveOnlyRevenue } from './CollectiveOnlyRevenue';
import type { IndividualOnlyRevenue } from './IndividualOnlyRevenue';
import type { Revenue } from './Revenue';
export type AggregatedRevenue = {
expectedRevenue: Revenue;
revenue: Revenue;
expectedRevenue: (Revenue | IndividualOnlyRevenue | CollectiveOnlyRevenue);
revenue: (Revenue | IndividualOnlyRevenue | CollectiveOnlyRevenue);
};

9 changes: 9 additions & 0 deletions pro/src/apiClient/v1/models/CollectiveOnlyRevenue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/* generated using openapi-typescript-codegen -- do not edit */
/* istanbul ignore file */
/* tslint:disable */
/* eslint-disable */
export type CollectiveOnlyRevenue = {
collective: number;
total: number;
};

9 changes: 9 additions & 0 deletions pro/src/apiClient/v1/models/IndividualOnlyRevenue.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/* generated using openapi-typescript-codegen -- do not edit */
/* istanbul ignore file */
/* tslint:disable */
/* eslint-disable */
export type IndividualOnlyRevenue = {
individual: number;
total: number;
};

Loading