From bcc6b9d0a8a70d02853945d3214641d100b52f68 Mon Sep 17 00:00:00 2001 From: Jeremie Baudet Date: Thu, 17 Oct 2024 20:51:39 +0200 Subject: [PATCH] (PC-31975)[API] feat: log stock update changes --- api/src/pcapi/core/offers/api.py | 84 ++++++++++++++----- api/src/pcapi/routes/pro/offers.py | 1 + api/src/pcapi/routes/pro/stocks.py | 10 ++- .../public/individual_offers/v1/events.py | 7 +- .../public/individual_offers/v1/products.py | 1 + api/tests/core/offers/test_api.py | 12 +-- api/tests/routes/pro/post_stocks_test.py | 30 ++++++- .../individual_offers/v1/patch_date_test.py | 47 +++++++---- 8 files changed, 141 insertions(+), 51 deletions(-) diff --git a/api/src/pcapi/core/offers/api.py b/api/src/pcapi/core/offers/api.py index 4b5bf7634b2..2283b9eb0d6 100644 --- a/api/src/pcapi/core/offers/api.py +++ b/api/src/pcapi/core/offers/api.py @@ -10,6 +10,7 @@ from psycopg2.errorcodes import CHECK_VIOLATION from psycopg2.errorcodes import UNIQUE_VIOLATION from psycopg2.extras import DateTimeRange +from pydantic import v1 as pydantic_v1 import sentry_sdk import sqlalchemy as sa from sqlalchemy import func @@ -742,6 +743,23 @@ def create_stock( return created_stock +class FieldUpdateLog(pydantic_v1.BaseModel): + old: typing.Any + new: typing.Any + + +class StockUpdateLog(pydantic_v1.BaseModel): + stock_id: int + offer_id: int + user_id: int | None + api_key_id: int | None + changes: dict[str, FieldUpdateLog] + + @property + def is_beginning_updated(self) -> bool: + return "beginningDatetime" in self.changes + + def edit_stock( stock: models.Stock, *, @@ -752,7 +770,9 @@ def edit_stock( editing_provider: providers_models.Provider | None = None, price_category: models.PriceCategory | None | T_UNCHANGED = UNCHANGED, id_at_provider: str | None | T_UNCHANGED = UNCHANGED, -) -> tuple[models.Stock | None, bool]: + current_user_id: int | None = None, + api_key_id: int | None = None, +) -> tuple[models.Stock | None, StockUpdateLog | None]: """If anything has changed, return the stock and whether the "beginning datetime" has changed. Otherwise, return `(None, False)`. """ @@ -760,7 +780,7 @@ def edit_stock( old_price = stock.price old_quantity = stock.quantity - modifications: dict[str, typing.Any] = {} + modifications = {} if beginning_datetime is not UNCHANGED or booking_limit_datetime is not UNCHANGED: changed_beginning = beginning_datetime if beginning_datetime is not UNCHANGED else stock.beginningDatetime @@ -771,12 +791,14 @@ def edit_stock( validation.check_required_dates_for_stock(stock.offer, changed_beginning, changed_booking_limit) if price is not UNCHANGED and price is not None and price != stock.price: - modifications["price"] = price + modifications["price"] = FieldUpdateLog(old=stock.price, new=price) validation.check_stock_price(price, stock.offer, old_price=stock.price) if price_category is not UNCHANGED and price_category is not None and price_category is not stock.priceCategory: - modifications["priceCategory"] = price_category - modifications["price"] = price_category.price + modifications["priceCategory"] = FieldUpdateLog(old=stock.priceCategory, new=price_category) + modifications["price"] = FieldUpdateLog( + old=stock.priceCategory.price if stock.priceCategory is not None else None, new=price_category.price + ) validation.check_stock_price( price_category.price, stock.offer, @@ -784,18 +806,20 @@ def edit_stock( ) if quantity is not UNCHANGED and quantity != stock.quantity: - modifications["quantity"] = quantity + modifications["quantity"] = FieldUpdateLog(old=stock.quantity, new=quantity) validation.check_stock_quantity(quantity, stock.dnBookedQuantity) if booking_limit_datetime is not UNCHANGED and booking_limit_datetime != stock.bookingLimitDatetime: - modifications["bookingLimitDatetime"] = booking_limit_datetime + modifications["bookingLimitDatetime"] = FieldUpdateLog( + old=stock.bookingLimitDatetime, new=booking_limit_datetime + ) validation.check_activation_codes_expiration_datetime_on_stock_edition( stock.activationCodes, booking_limit_datetime, ) if beginning_datetime not in (UNCHANGED, stock.beginningDatetime): - modifications["beginningDatetime"] = beginning_datetime + modifications["beginningDatetime"] = FieldUpdateLog(old=stock.beginningDatetime, new=beginning_datetime) if id_at_provider not in (UNCHANGED, stock.idAtProviders): if id_at_provider is not None: @@ -804,14 +828,14 @@ def edit_stock( id_at_provider, # type: ignore[arg-type] stock.id, ) - modifications["idAtProviders"] = id_at_provider + modifications["idAtProviders"] = FieldUpdateLog(old=stock.idAtProviders, new=id_at_provider) if not modifications: logger.info( "Empty update of stock", extra={"offer_id": stock.offerId, "stock_id": stock.id}, ) - return None, False # False is for `"beginningDatetime" in modifications` + return None, None if stock.offer.isFromAllocine: updated_fields = set(modifications) @@ -819,7 +843,7 @@ def edit_stock( stock.fieldsUpdated = list(set(stock.fieldsUpdated) | updated_fields) for model_attr, value in modifications.items(): - setattr(stock, model_attr, value) + setattr(stock, model_attr, value.new) if "beginningDatetime" in modifications: finance_api.update_finance_event_pricing_date(stock) @@ -837,26 +861,38 @@ def edit_stock( "stock_dnBookedQuantity": stock.dnBookedQuantity, } - if (new_price := modifications.get("price", UNCHANGED)) is not UNCHANGED: + if (price := modifications.get("price", UNCHANGED)) is not UNCHANGED: # type: ignore log_extra_data["old_price"] = old_price - log_extra_data["stock_price"] = new_price + log_extra_data["stock_price"] = price.new # type: ignore - if (new_quantity := modifications.get("quantity", UNCHANGED)) is not UNCHANGED: + if (quantity := modifications.get("quantity", UNCHANGED)) is not UNCHANGED: # type: ignore log_extra_data["old_quantity"] = old_quantity - log_extra_data["stock_quantity"] = new_quantity + log_extra_data["stock_quantity"] = quantity.new # type: ignore logger.info("Successfully updated stock", extra=log_extra_data, technical_message_id="stock.updated") - return stock, "beginningDatetime" in modifications + update_log = StockUpdateLog( + stock_id=stock.id, offer_id=stock.offerId, user_id=current_user_id, api_key_id=api_key_id, changes=modifications + ) + return stock, update_log -def handle_stocks_edition(edited_stocks: list[tuple[models.Stock, bool]]) -> None: - for stock, is_beginning_datetime_updated in edited_stocks: - if is_beginning_datetime_updated: + +def handle_stocks_edition(edited_stocks: list[tuple[models.Stock, StockUpdateLog]]) -> None: + for stock, modifications in edited_stocks: + if modifications and modifications.is_beginning_updated: bookings = bookings_repository.find_not_cancelled_bookings_by_stock(stock) _notify_pro_upon_stock_edit_for_event_offer(stock, bookings) _notify_beneficiaries_upon_stock_edit(stock, bookings) + logger.info( + "Stock updated with beginning datetime updated", + extra={ + "bookings_count": len(bookings), + **modifications.dict(), + }, + ) + def _format_publication_date(publication_date: datetime.datetime | None, timezone: str) -> datetime.datetime | None: if publication_date is None: @@ -1604,6 +1640,8 @@ def edit_price_category( price: decimal.Decimal | T_UNCHANGED = UNCHANGED, editing_provider: providers_models.Provider | None = None, id_at_provider: str | None | T_UNCHANGED = UNCHANGED, + current_user_id: int | None = None, + api_key_id: int | None = None, ) -> models.PriceCategory: validation.check_price_category_is_updatable(price_category, editing_provider) @@ -1626,7 +1664,13 @@ def edit_price_category( stocks_to_edit = [stock for stock in offer.stocks if stock.priceCategoryId == price_category.id] for stock in stocks_to_edit: - edit_stock(stock, price=price_category.price, editing_provider=editing_provider) + edit_stock( + stock, + price=price_category.price, + editing_provider=editing_provider, + current_user_id=current_user_id, + api_key_id=api_key_id, + ) return price_category diff --git a/api/src/pcapi/routes/pro/offers.py b/api/src/pcapi/routes/pro/offers.py index a30ea5004f2..e37589fff77 100644 --- a/api/src/pcapi/routes/pro/offers.py +++ b/api/src/pcapi/routes/pro/offers.py @@ -610,6 +610,7 @@ def post_price_categories( price_category=existing_price_categories_by_id[data["id"]], label=data.get("label", offers_api.UNCHANGED), price=data.get("price", offers_api.UNCHANGED), + current_user_id=current_user.id, ) return offers_serialize.GetIndividualOfferResponseModel.from_orm(offer) diff --git a/api/src/pcapi/routes/pro/stocks.py b/api/src/pcapi/routes/pro/stocks.py index 7a29b0aa57f..544824a2153 100644 --- a/api/src/pcapi/routes/pro/stocks.py +++ b/api/src/pcapi/routes/pro/stocks.py @@ -1,4 +1,5 @@ import logging +import typing from flask_login import current_user from flask_login import login_required @@ -135,7 +136,7 @@ def upsert_stocks( price_categories = {price_category.id: price_category for price_category in offer.priceCategories} upserted_stocks = [] - edited_stocks_with_update_info: list[tuple[offers_models.Stock, bool]] = [] + edited_stocks_with_update_info: list[tuple[offers_models.Stock, offers_api.StockUpdateLog]] = [] try: with transaction(): if stocks_to_edit: @@ -149,7 +150,7 @@ def upsert_stocks( offers_validation.check_stock_has_price_or_price_category(offer, stock_to_edit, price_categories) - edited_stock, is_beginning_updated = offers_api.edit_stock( + edited_stock, modifications = offers_api.edit_stock( existing_stocks[stock_to_edit.id], price=stock_to_edit.price, quantity=stock_to_edit.quantity, @@ -164,10 +165,13 @@ def upsert_stocks( else None ), price_category=price_categories.get(stock_to_edit.price_category_id, None), + current_user_id=current_user.id, ) if edited_stock: upserted_stocks.append(edited_stock) - edited_stocks_with_update_info.append((edited_stock, is_beginning_updated)) + edited_stocks_with_update_info.append( + (edited_stock, typing.cast(offers_api.StockUpdateLog, modifications)) + ) for stock_to_create in stocks_to_create: offers_validation.check_stock_has_price_or_price_category(offer, stock_to_create, price_categories) diff --git a/api/src/pcapi/routes/public/individual_offers/v1/events.py b/api/src/pcapi/routes/public/individual_offers/v1/events.py index c8de4817e24..03789f8e74b 100644 --- a/api/src/pcapi/routes/public/individual_offers/v1/events.py +++ b/api/src/pcapi/routes/public/individual_offers/v1/events.py @@ -1,4 +1,5 @@ import copy +import typing from flask_login import current_user import sqlalchemy as sqla @@ -404,6 +405,7 @@ def patch_event_price_category( ), id_at_provider=update_body.get("id_at_provider", offers_api.UNCHANGED), editing_provider=current_api_key.provider, + api_key_id=current_api_key.id, ) except (offers_exceptions.OfferEditionBaseException, offers_exceptions.PriceCategoryCreationBaseException) as error: raise api_errors.ApiErrors(error.errors, status_code=400) @@ -624,7 +626,7 @@ def patch_event_stock( ) quantity = serialization.deserialize_quantity(update_body.get("quantity", offers_api.UNCHANGED)) - edited_stock, is_beginning_updated = offers_api.edit_stock( + edited_stock, modifications = offers_api.edit_stock( stock_to_edit, quantity=quantity + stock_to_edit.dnBookedQuantity if isinstance(quantity, int) else quantity, price_category=price_category, @@ -632,8 +634,9 @@ def patch_event_stock( beginning_datetime=update_body.get("beginning_datetime", offers_api.UNCHANGED), id_at_provider=update_body.get("id_at_provider", offers_api.UNCHANGED), editing_provider=current_api_key.provider, + api_key_id=current_api_key.id, ) - offers_api.handle_stocks_edition([(stock_to_edit, is_beginning_updated)]) + offers_api.handle_stocks_edition([(stock_to_edit, typing.cast(offers_api.StockUpdateLog, modifications))]) except ( offers_exceptions.OfferCreationBaseException, offers_exceptions.OfferEditionBaseException, diff --git a/api/src/pcapi/routes/public/individual_offers/v1/products.py b/api/src/pcapi/routes/public/individual_offers/v1/products.py index 202e3417bbd..8ffa5f60735 100644 --- a/api/src/pcapi/routes/public/individual_offers/v1/products.py +++ b/api/src/pcapi/routes/public/individual_offers/v1/products.py @@ -872,6 +872,7 @@ def _upsert_product_stock( price=finance_utils.to_euros(price) if price != offers_api.UNCHANGED else offers_api.UNCHANGED, booking_limit_datetime=stock_update_body.get("booking_limit_datetime", offers_api.UNCHANGED), editing_provider=provider, + api_key_id=current_api_key.id, ) diff --git a/api/tests/core/offers/test_api.py b/api/tests/core/offers/test_api.py index 299a277292f..c9fc936a7a1 100644 --- a/api/tests/core/offers/test_api.py +++ b/api/tests/core/offers/test_api.py @@ -383,13 +383,13 @@ def test_edit_stock(self): existing_stock = factories.StockFactory(price=10) # When - edited_stock, update_info = api.edit_stock(stock=existing_stock, price=5, quantity=7) + edited_stock, modifications = api.edit_stock(stock=existing_stock, price=5, quantity=7) # Then assert edited_stock == models.Stock.query.filter_by(id=existing_stock.id).first() assert edited_stock.price == 5 assert edited_stock.quantity == 7 - assert update_info is False + assert not modifications.is_beginning_updated @pytest.mark.parametrize( "init_value,edit_value", @@ -421,7 +421,7 @@ def test_edit_beginning_datetime(self): ) # When - edited_stock, update_info = api.edit_stock( + edited_stock, modifications = api.edit_stock( stock=existing_stock, price=12, quantity=77, @@ -435,7 +435,7 @@ def test_edit_beginning_datetime(self): assert edited_stock.quantity == 77 assert edited_stock.beginningDatetime == new_beginning assert edited_stock.bookingLimitDatetime == previous_booking_limit - assert update_info is True + assert modifications.is_beginning_updated def test_edit_event_without_beginning_update(self): # Given @@ -447,7 +447,7 @@ def test_edit_event_without_beginning_update(self): ) # When - edited_stock, update_info = api.edit_stock( + edited_stock, modifications = api.edit_stock( stock=existing_stock, price=10, quantity=7, @@ -461,7 +461,7 @@ def test_edit_event_without_beginning_update(self): assert edited_stock.quantity == 7 assert edited_stock.beginningDatetime == beginning assert edited_stock.bookingLimitDatetime == new_booking_limit - assert update_info is False + assert not modifications.is_beginning_updated def test_update_fields_updated_on_allocine_stocks(self): allocine_provider = providers_factories.AllocineProviderFactory() diff --git a/api/tests/routes/pro/post_stocks_test.py b/api/tests/routes/pro/post_stocks_test.py index fb560241807..dce95bcf5b6 100644 --- a/api/tests/routes/pro/post_stocks_test.py +++ b/api/tests/routes/pro/post_stocks_test.py @@ -1,6 +1,7 @@ import dataclasses import datetime from decimal import Decimal +import logging from unittest import mock from unittest.mock import patch @@ -342,7 +343,7 @@ def test_edit_one_event_stock_using_price_category(self, mocked_async_index_offe assert offers_models.PriceCategoryLabel.query.count() == 1 @patch("pcapi.core.search.async_index_offer_ids") - def test_edit_one_event_stock_created_with_price_category(self, mocked_async_index_offer_ids, client): + def test_edit_one_event_stock_created_with_price_category(self, mocked_async_index_offer_ids, client, caplog): venue = offerers_factories.VenueFactory() offer = offers_factories.EventOfferFactory( isActive=False, @@ -355,10 +356,10 @@ def test_edit_one_event_stock_created_with_price_category(self, mocked_async_ind ) existing_stock = offers_factories.StockFactory(offer=offer, price=10, priceCategory=old_price_category) - offerers_factories.UserOffererFactory( + user = offerers_factories.UserOffererFactory( user__email="user@example.com", offerer=offer.venue.managingOfferer, - ) + ).user beginning = datetime.datetime.utcnow() + relativedelta(days=10) stock_data = { @@ -372,13 +373,34 @@ def test_edit_one_event_stock_created_with_price_category(self, mocked_async_ind } ], } - client.with_session_auth("user@example.com").post("/stocks/bulk/", json=stock_data) + with caplog.at_level(logging.INFO): + response = client.with_session_auth(user.email).post("/stocks/bulk/", json=stock_data) + created_stock = Stock.query.first() assert offer.id == created_stock.offerId assert len(Stock.query.all()) == 1 assert created_stock.priceCategory == new_price_category assert created_stock.price == 25 + target_log_message = "Stock updated with beginning datetime updated" + log = next(record for record in caplog.records if record.message == target_log_message) + + assert log.extra["user_id"] == user.id + + changes = log.extra["changes"] + + expected_updated_field = {"beginningDatetime", "bookingLimitDatetime", "priceCategory", "price"} + assert expected_updated_field <= set(changes) + + assert changes["beginningDatetime"]["old"] != changes["beginningDatetime"]["new"] + assert changes["beginningDatetime"]["new"] == beginning + + assert changes["bookingLimitDatetime"]["old"] != changes["bookingLimitDatetime"]["new"] + assert changes["bookingLimitDatetime"]["new"] == beginning + + assert changes["priceCategory"]["old"] != changes["priceCategory"]["new"] + assert changes["priceCategory"]["new"] == new_price_category + def test_create_one_stock_with_activation_codes(self, client): # Given offer = offers_factories.DigitalOfferFactory(url="https://chartreu.se") diff --git a/api/tests/routes/public/individual_offers/v1/patch_date_test.py b/api/tests/routes/public/individual_offers/v1/patch_date_test.py index 7eab85a38a2..8904ca4cfc8 100644 --- a/api/tests/routes/public/individual_offers/v1/patch_date_test.py +++ b/api/tests/routes/public/individual_offers/v1/patch_date_test.py @@ -1,6 +1,7 @@ import dataclasses import datetime import decimal +import logging from unittest import mock import pytest @@ -73,7 +74,7 @@ def test_should_raise_404_because_venue_provider_is_inactive(self, client: TestC assert response.status_code == 404 @mock.patch("pcapi.core.search.async_index_offer_ids") - def test_update_all_fields_on_date_with_price(self, mocked_async_index_offer_ids, client): + def test_update_all_fields_on_date_with_price(self, mocked_async_index_offer_ids, client, caplog): plain_api_key, venue_provider = self.setup_active_venue_provider() event, stock = self.setup_base_resource(venue=venue_provider.venue, provider=venue_provider.provider) price_category = offers_factories.PriceCategoryFactory(offer=event) @@ -82,21 +83,21 @@ def test_update_all_fields_on_date_with_price(self, mocked_async_index_offer_ids twenty_four_day_from_now = datetime.datetime.utcnow().replace(second=0, microsecond=0) + datetime.timedelta( days=24 ) - response = client.with_explicit_token(plain_api_key).patch( - self.endpoint_url.format(event_id=event.id, stock_id=stock.id), - json={ - "beginningDatetime": date_utils.utc_datetime_to_department_timezone( - twenty_four_day_from_now, None - ).isoformat(), - "bookingLimitDatetime": date_utils.utc_datetime_to_department_timezone( - one_week_from_now, departement_code=None - ).isoformat(), - "priceCategoryId": price_category.id, - "quantity": 24, - "id_at_provider": "hey you !", - }, - ) - + with caplog.at_level(logging.INFO): + response = client.with_explicit_token(plain_api_key).patch( + self.endpoint_url.format(event_id=event.id, stock_id=stock.id), + json={ + "beginningDatetime": date_utils.utc_datetime_to_department_timezone( + twenty_four_day_from_now, None + ).isoformat(), + "bookingLimitDatetime": date_utils.utc_datetime_to_department_timezone( + one_week_from_now, departement_code=None + ).isoformat(), + "priceCategoryId": price_category.id, + "quantity": 24, + "id_at_provider": "hey you !", + }, + ) assert response.status_code == 200, response.json assert stock.bookingLimitDatetime == one_week_from_now assert stock.beginningDatetime == twenty_four_day_from_now @@ -106,6 +107,20 @@ def test_update_all_fields_on_date_with_price(self, mocked_async_index_offer_ids assert stock.idAtProviders == "hey you !" mocked_async_index_offer_ids.assert_called_once() + log_message = "Stock updated with beginning datetime updated" + log = next(record for record in caplog.records if record.message == log_message) + + assert log.extra["api_key_id"] + assert not log.extra["user_id"] + + changes = log.extra["changes"] + + assert changes["priceCategory"]["old"] != changes["priceCategory"]["new"] + assert changes["priceCategory"]["new"].id == price_category.id + + assert changes["idAtProviders"]["old"] != changes["idAtProviders"]["new"] + assert changes["idAtProviders"]["new"] == "hey you !" + def test_sends_email_if_beginning_date_changes_on_edition(self, client): plain_api_key, venue_provider = self.setup_active_venue_provider() event, stock = self.setup_base_resource(venue=venue_provider.venue, provider=venue_provider.provider)