fix: fix price update endpoint (#227)

* Price update: use PATCH instead of PUT

* Better manage extra fields

* Update tests + extra test for None values

app/api.py

@@ -358,24 +358,27 @@ def create_price(
     return db_price
 
 
-@app.delete(
-    "/api/v1/prices/{price_id}",
-    status_code=status.HTTP_204_NO_CONTENT,
+@app.patch(
+    path="/api/v1/prices/{price_id}",
+    response_model=schemas.PriceFull,
+    status_code=status.HTTP_200_OK,
     tags=["Prices"],
 )
-def delete_price(
+def update_price(
     price_id: int,
+    new_price: schemas.PriceBasicUpdatableFields,
     current_user: schemas.UserCreate = Depends(get_current_user),
     db: Session = Depends(get_db),
 ):
     """
-    Delete a price.
+    Update a price.
 
     This endpoint requires authentication.
-    A user can delete only owned prices.
+    A user can update only owned prices.
     """
+    # fetch price with id = price_id
     db_price = crud.get_price_by_id(db, id=price_id)
-    # get price
+
     if not db_price:
         raise HTTPException(
             status_code=404,
@@ -387,32 +390,29 @@ def delete_price(
             status_code=status.HTTP_403_FORBIDDEN,
             detail="Price does not belong to current user",
         )
-    # delete price
-    crud.delete_price(db, db_price=db_price)
-    return
 
+    # updated price
+    return crud.update_price(db, db_price, new_price)
 
-@app.put(
-    path="/api/v1/prices/{price_id}",
-    response_model=schemas.PriceFull,
-    status_code=status.HTTP_200_OK,
+
+@app.delete(
+    "/api/v1/prices/{price_id}",
+    status_code=status.HTTP_204_NO_CONTENT,
     tags=["Prices"],
 )
-def update_price(
+def delete_price(
     price_id: int,
-    new_price: schemas.PriceBasicUpdatableFields,
     current_user: schemas.UserCreate = Depends(get_current_user),
     db: Session = Depends(get_db),
 ):
     """
-    Update a price.
+    Delete a price.
 
     This endpoint requires authentication.
-    A user can update only owned prices.
+    A user can delete only owned prices.
     """
-    # fetch price with id = price_id
     db_price = crud.get_price_by_id(db, id=price_id)
-
+    # get price
     if not db_price:
         raise HTTPException(
             status_code=404,
@@ -424,9 +424,9 @@ def update_price(
             status_code=status.HTTP_403_FORBIDDEN,
             detail="Price does not belong to current user",
         )
-
-    # updated price
-    return crud.update_price(db, db_price, new_price)
+    # delete price
+    crud.delete_price(db, db_price=db_price)
+    return
 
 
 # Routes: Proofs

app/crud.py

@@ -333,8 +333,9 @@ def delete_price(db: Session, db_price: PriceFull) -> bool:
 
 
 def update_price(db: Session, price: Price, new_values: PriceBasicUpdatableFields):
-    for [key, value] in new_values:
-        setattr(price, key, value)
+    new_values_cleaned = new_values.model_dump(exclude_unset=True)
+    for key in new_values_cleaned:
+        setattr(price, key, new_values_cleaned[key])
     db.commit()
     db.refresh(price)
     return price

app/schemas.py

@@ -364,12 +364,15 @@ def check_price_discount(self):
 
 
 class PriceBasicUpdatableFields(BaseModel):
-    price: float | None
-    price_is_discounted: bool | None
-    price_without_discount: float | None
-    price_per: PricePerEnum | None
-    currency: CurrencyEnum | None
-    date: datetime.date | None
+    price: float | None = None
+    price_is_discounted: bool | None = None
+    price_without_discount: float | None = None
+    price_per: PricePerEnum | None = None
+    currency: CurrencyEnum | None = None
+    date: datetime.date | None = None
+
+    class Config:
+        extra = "forbid"
 
 
 class PriceFull(PriceCreate):

tests/integration/test_api.py

@@ -11,7 +11,6 @@
 from app.models import Session as SessionModel
 from app.schemas import (
     LocationCreate,
-    PriceBasicUpdatableFields,
     PriceCreate,
     ProductCreate,
     ProofFilter,
@@ -102,6 +101,8 @@ def override_get_db():
     product_name="PATE NOCCIOLATA BIO 700G",
     # category="en:tomatoes",
     price=3.5,
+    price_is_discounted=True,
+    price_without_discount=5,
     currency="EUR",
     location_osm_id=123,
     location_osm_type="NODE",
@@ -583,81 +584,89 @@ def test_get_prices_orders(db_session, user_session: SessionModel, clean_prices)
     assert (response.json()["items"][0]["date"]) == "2023-10-31"
 
 
-def test_delete_price(db_session, user_session: SessionModel, clean_prices):
+def test_update_price(db_session, user_session: SessionModel):
     db_price = crud.create_price(db_session, PRICE_1, user_session.user)
+    new_price = 5.5
+    PRICE_UPDATE_PARTIAL = {"price": new_price}
     # without authentication
-    response = client.delete(f"/api/v1/prices/{db_price.id}")
+    response = client.patch(f"/api/v1/prices/{db_price.id}")
     assert response.status_code == 401
     # with authentication but not price owner
     user_1_session, *_ = crud.create_session(db_session, USER_1.user_id, USER_1.token)
-    response = client.delete(
+    response = client.patch(
         f"/api/v1/prices/{db_price.id}",
         headers={"Authorization": f"Bearer {user_1_session.token}"},
+        json=jsonable_encoder(PRICE_UPDATE_PARTIAL),
     )
     assert response.status_code == 403
     # with authentication but price unknown
-    response = client.delete(
-        f"/api/v1/prices/{db_price.id + 1}",
+    response = client.patch(
+        f"/api/v1/prices/{db_price.id+1}",
         headers={"Authorization": f"Bearer {user_session.token}"},
+        json=jsonable_encoder(PRICE_UPDATE_PARTIAL),
     )
     assert response.status_code == 404
     # with authentication and price owner
-    response = client.delete(
+    response = client.patch(
         f"/api/v1/prices/{db_price.id}",
         headers={"Authorization": f"Bearer {user_session.token}"},
+        json=jsonable_encoder(PRICE_UPDATE_PARTIAL),
     )
-    assert response.status_code == 204
+    assert response.status_code == 200
+    assert response.json()["price"] == new_price
+    assert response.json()["price_is_discounted"] == PRICE_1.price_is_discounted
+    assert response.json()["price_without_discount"] == PRICE_1.price_without_discount
+    # with authentication and price owner
+    PRICE_UPDATE_PARTIAL_MORE = {
+        **PRICE_UPDATE_PARTIAL,
+        "price_is_discounted": False,
+        "price_without_discount": None,
+    }
+    response = client.patch(
+        f"/api/v1/prices/{db_price.id}",
+        headers={"Authorization": f"Bearer {user_session.token}"},
+        json=jsonable_encoder(PRICE_UPDATE_PARTIAL_MORE),
+    )
+    assert response.status_code == 200
+    assert response.json()["price"] == new_price
+    assert (
+        response.json()["price_is_discounted"] != PRICE_1.price_is_discounted
+    )  # False
+    assert response.json()["price_without_discount"] is None
+    # with authentication and price owner but extra fields
+    PRICE_UPDATE_PARTIAL_WRONG = {**PRICE_UPDATE_PARTIAL, "proof_id": 1}
+    response = client.patch(
+        f"/api/v1/prices/{db_price.id}",
+        headers={"Authorization": f"Bearer {user_session.token}"},
+        json=jsonable_encoder(PRICE_UPDATE_PARTIAL_WRONG),
+    )
+    assert response.status_code == 422
 
 
-def test_update_price(db_session, user_session: SessionModel):
+def test_delete_price(db_session, user_session: SessionModel, clean_prices):
     db_price = crud.create_price(db_session, PRICE_1, user_session.user)
     # without authentication
-    response = client.put(f"/api/v1/prices/{db_price.id}")
+    response = client.delete(f"/api/v1/prices/{db_price.id}")
     assert response.status_code == 401
-
     # with authentication but not price owner
     user_1_session, *_ = crud.create_session(db_session, USER_1.user_id, USER_1.token)
-    response = client.put(
+    response = client.delete(
         f"/api/v1/prices/{db_price.id}",
         headers={"Authorization": f"Bearer {user_1_session.token}"},
     )
     assert response.status_code == 403
     # with authentication but price unknown
-    response = client.put(
-        f"/api/v1/prices/{db_price.id+1}",
+    response = client.delete(
+        f"/api/v1/prices/{db_price.id + 1}",
         headers={"Authorization": f"Bearer {user_session.token}"},
     )
     assert response.status_code == 404
-
     # with authentication and price owner
-
-    # if any field which is not is basic updatable field provided
-    # it should throw an error
-    price_fields = {
-        "proof_id": 1,
-        "new_price": 5.5,
-    }
-
-    response = client.put(
-        f"/api/v1/prices/{db_price.id}",
-        headers={"Authorization": f"Bearer {user_session.token}"},
-        json=jsonable_encoder(price_fields),
-    )
-
-    assert response.status_code == 422  # Unprocessable Entity
-
-    new_price = 5.5
-    price_updatable_fields = PriceBasicUpdatableFields(price=new_price)
-
-    response = client.put(
+    response = client.delete(
         f"/api/v1/prices/{db_price.id}",
         headers={"Authorization": f"Bearer {user_session.token}"},
-        json=jsonable_encoder(price_updatable_fields),
     )
-
-    assert response.status_code == 200
-
-    assert response.json()["price"] == new_price
+    assert response.status_code == 204
 
 
 # Test proofs