-
Notifications
You must be signed in to change notification settings - Fork 437
fix: Allow to update custom field data #6613
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
base: main
Are you sure you want to change the base?
Changes from all commits
9cacda2
5b081d7
92315f1
e8d1207
cecd05b
ec5d4c4
86a76ae
64a8c35
0ccf563
e62a0c5
d7bc98e
1dfd331
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import datetime | ||
from typing import Annotated | ||
|
||
from babel.numbers import format_currency | ||
|
@@ -179,15 +180,23 @@ class Order(CustomFieldDataOutputMixin, MetadataOutputMixin, OrderBase): | |
|
||
class OrderUpdateBase(Schema): | ||
billing_name: str | None = Field( | ||
default=None, | ||
description=( | ||
"The name of the customer that should appear on the invoice. " | ||
"Can't be updated after the invoice is generated." | ||
) | ||
), | ||
) | ||
billing_address: Address | None = Field( | ||
default=None, | ||
frankie567 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
description=( | ||
"The address of the customer that should appear on the invoice. " | ||
"Can't be updated after the invoice is generated." | ||
), | ||
) | ||
custom_field_data: dict[str, str | int | bool | datetime.datetime | None] | None = ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have |
||
Field( | ||
default=None, | ||
description="Key-value object storing custom field values. Can be updated by merchants to correct errors.", | ||
) | ||
) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ | |
from polar.checkout.eventstream import CheckoutEvent, publish_checkout_event | ||
from polar.checkout.repository import CheckoutRepository | ||
from polar.config import settings | ||
from polar.custom_field.data import validate_custom_field_data | ||
from polar.customer.repository import CustomerRepository | ||
from polar.customer_portal.schemas.order import ( | ||
CustomerOrderPaymentConfirmation, | ||
|
@@ -425,10 +426,23 @@ async def update( | |
if errors: | ||
raise PolarRequestValidationError(errors) | ||
|
||
# Handle custom field data validation and update | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unneeded comment |
||
update_dict = order_update.model_dump(exclude_unset=True) | ||
|
||
if "custom_field_data" in update_dict: | ||
# Validate custom field data against the product's attached custom fields | ||
custom_field_data = validate_custom_field_data( | ||
order.product.attached_custom_fields, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will likely raise an eager loading error at runtime (this is a common shortcomings of our tests which usually don't catch that). That's also why @rishi-raj-jain asked for a video showing it working. For simplicity, you should add that eager load instruction above in the async def get(
self,
session: AsyncSession,
auth_subject: AuthSubject[User | Organization],
id: uuid.UUID,
) -> Order | None:
repository = OrderRepository.from_session(session)
statement = (
repository.get_readable_statement(auth_subject)
.options(
*repository.get_eager_options(
customer_load=contains_eager(Order.customer),
product_load=joinedload(Order.product).options(
joinedload(Product.organization),
selectinload(Product.attached_custom_fields),
)
)
)
.where(Order.id == id)
)
return await repository.get_one_or_none(statement) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @frankie567 are you suggesting this because of the performance, so that update function doesnt lazy load the data? |
||
update_dict["custom_field_data"], | ||
validate_required=False, # Allow merchants to update even if required fields are missing | ||
) | ||
update_dict["custom_field_data"] = custom_field_data | ||
|
||
repository = OrderRepository.from_session(session) | ||
order = await repository.update( | ||
order, update_dict=order_update.model_dump(exclude_unset=True) | ||
) | ||
order = await repository.update(order, update_dict=update_dict) | ||
|
||
# Refresh the order to get the updated data, including the product relationship | ||
await session.refresh(order, {"product"}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That refresh is not needed at all There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @frankie567 is this because, in get method we are eagerly loading the product relationship which the modification? |
||
|
||
await self.send_webhook(session, order, WebhookEventType.order_updated) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,10 +5,17 @@ | |
from httpx import AsyncClient | ||
|
||
from polar.auth.scope import Scope | ||
from polar.models import Customer, Order, Product, UserOrganization | ||
from polar.enums import SubscriptionRecurringInterval | ||
from polar.models import Customer, Order, Organization, Product, UserOrganization | ||
from polar.models.custom_field import CustomFieldType | ||
from tests.fixtures.auth import AuthSubjectFixture | ||
from tests.fixtures.database import SaveFixture | ||
from tests.fixtures.random_objects import create_order | ||
from tests.fixtures.random_objects import ( | ||
create_custom_field, | ||
create_customer, | ||
create_order, | ||
create_product, | ||
) | ||
|
||
|
||
@pytest_asyncio.fixture | ||
|
@@ -166,6 +173,234 @@ async def test_custom_field( | |
assert json["custom_field_data"] == {"test": None} | ||
|
||
|
||
@pytest.mark.asyncio | ||
class TestUpdateOrder: | ||
async def test_anonymous(self, client: AsyncClient, orders: list[Order]) -> None: | ||
response = await client.patch( | ||
f"/v1/orders/{orders[0].id}", | ||
json={"custom_field_data": {"test": "updated"}}, | ||
) | ||
|
||
assert response.status_code == 401 | ||
|
||
@pytest.mark.auth | ||
async def test_not_existing(self, client: AsyncClient) -> None: | ||
response = await client.patch( | ||
f"/v1/orders/{uuid.uuid4()}", | ||
json={"custom_field_data": {"test": "updated"}}, | ||
) | ||
|
||
assert response.status_code == 404 | ||
|
||
@pytest.mark.auth | ||
async def test_user_not_organization_member( | ||
self, client: AsyncClient, orders: list[Order] | ||
) -> None: | ||
response = await client.patch( | ||
f"/v1/orders/{orders[0].id}", | ||
json={"custom_field_data": {"test": "updated"}}, | ||
) | ||
|
||
assert response.status_code == 404 | ||
|
||
@pytest.mark.auth( | ||
AuthSubjectFixture(scopes={Scope.web_write}), | ||
AuthSubjectFixture(scopes={Scope.orders_write}), | ||
) | ||
async def test_user_valid( | ||
self, | ||
save_fixture: SaveFixture, | ||
client: AsyncClient, | ||
user_organization: UserOrganization, | ||
organization: Organization, | ||
) -> None: | ||
# Create a product with custom fields | ||
text_field = await create_custom_field( | ||
save_fixture, | ||
type=CustomFieldType.text, | ||
slug="text", | ||
organization=organization, | ||
) | ||
select_field = await create_custom_field( | ||
save_fixture, | ||
type=CustomFieldType.select, | ||
slug="select", | ||
organization=organization, | ||
properties={ | ||
"options": [{"value": "a", "label": "A"}, {"value": "b", "label": "B"}], | ||
}, | ||
) | ||
product = await create_product( | ||
save_fixture, | ||
organization=organization, | ||
recurring_interval=SubscriptionRecurringInterval.month, | ||
attached_custom_fields=[(text_field, False), (select_field, True)], | ||
) | ||
|
||
# Create an order with custom field data | ||
order = await create_order( | ||
save_fixture, | ||
product=product, | ||
customer=await create_customer(save_fixture, organization=organization), | ||
custom_field_data={"text": "original", "select": "a"}, | ||
) | ||
|
||
response = await client.patch( | ||
f"/v1/orders/{order.id}", | ||
json={"custom_field_data": {"text": "updated", "select": "b"}}, | ||
) | ||
|
||
assert response.status_code == 200 | ||
|
||
json = response.json() | ||
assert json["custom_field_data"] == {"text": "updated", "select": "b"} | ||
|
||
@pytest.mark.auth( | ||
AuthSubjectFixture(subject="organization", scopes={Scope.orders_write}), | ||
) | ||
async def test_organization( | ||
self, save_fixture: SaveFixture, client: AsyncClient, organization: Organization | ||
) -> None: | ||
# Create a product with custom fields | ||
text_field = await create_custom_field( | ||
save_fixture, | ||
type=CustomFieldType.text, | ||
slug="text", | ||
organization=organization, | ||
) | ||
select_field = await create_custom_field( | ||
save_fixture, | ||
type=CustomFieldType.select, | ||
slug="select", | ||
organization=organization, | ||
properties={ | ||
"options": [{"value": "a", "label": "A"}, {"value": "b", "label": "B"}], | ||
}, | ||
) | ||
product = await create_product( | ||
save_fixture, | ||
organization=organization, | ||
recurring_interval=SubscriptionRecurringInterval.month, | ||
attached_custom_fields=[(text_field, False), (select_field, True)], | ||
) | ||
|
||
# Create an order with custom field data | ||
order = await create_order( | ||
save_fixture, | ||
product=product, | ||
customer=await create_customer(save_fixture, organization=organization), | ||
custom_field_data={"text": "original", "select": "a"}, | ||
) | ||
|
||
response = await client.patch( | ||
f"/v1/orders/{order.id}", | ||
json={"custom_field_data": {"text": "updated", "select": "b"}}, | ||
) | ||
|
||
assert response.status_code == 200 | ||
|
||
json = response.json() | ||
assert json["custom_field_data"] == {"text": "updated", "select": "b"} | ||
|
||
@pytest.mark.auth( | ||
AuthSubjectFixture(scopes={Scope.web_write}), | ||
) | ||
async def test_update_existing_custom_field_data( | ||
self, | ||
save_fixture: SaveFixture, | ||
client: AsyncClient, | ||
user_organization: UserOrganization, | ||
organization: Organization, | ||
) -> None: | ||
# Create a product with custom fields | ||
text_field = await create_custom_field( | ||
save_fixture, | ||
type=CustomFieldType.text, | ||
slug="text", | ||
organization=organization, | ||
) | ||
select_field = await create_custom_field( | ||
save_fixture, | ||
type=CustomFieldType.select, | ||
slug="select", | ||
organization=organization, | ||
properties={ | ||
"options": [{"value": "a", "label": "A"}, {"value": "b", "label": "B"}], | ||
}, | ||
) | ||
product = await create_product( | ||
save_fixture, | ||
organization=organization, | ||
recurring_interval=SubscriptionRecurringInterval.month, | ||
attached_custom_fields=[(text_field, False), (select_field, True)], | ||
) | ||
|
||
# Create an order with custom field data | ||
order = await create_order( | ||
save_fixture, | ||
product=product, | ||
customer=await create_customer(save_fixture, organization=organization), | ||
custom_field_data={"text": "original", "select": "a"}, | ||
) | ||
|
||
response = await client.patch( | ||
f"/v1/orders/{order.id}", | ||
json={"custom_field_data": {"text": "updated", "select": "b"}}, | ||
) | ||
|
||
assert response.status_code == 200 | ||
|
||
json = response.json() | ||
assert json["custom_field_data"] == {"text": "updated", "select": "b"} | ||
|
||
@pytest.mark.auth( | ||
AuthSubjectFixture(scopes={Scope.web_write}), | ||
) | ||
async def test_update_billing_name( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not relevant for those changes |
||
self, | ||
client: AsyncClient, | ||
user_organization: UserOrganization, | ||
orders: list[Order], | ||
) -> None: | ||
response = await client.patch( | ||
f"/v1/orders/{orders[0].id}", | ||
json={"billing_name": "Updated Billing Name"}, | ||
) | ||
|
||
assert response.status_code == 200 | ||
|
||
json = response.json() | ||
assert json["billing_name"] == "Updated Billing Name" | ||
|
||
@pytest.mark.auth( | ||
AuthSubjectFixture(scopes={Scope.web_write}), | ||
) | ||
async def test_update_billing_address( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not relevant for those changes |
||
self, | ||
client: AsyncClient, | ||
user_organization: UserOrganization, | ||
orders: list[Order], | ||
) -> None: | ||
new_address = { | ||
"country": "US", | ||
"state": "CA", | ||
"line1": "123 Updated St", | ||
"city": "Updated City", | ||
"postal_code": "12345", | ||
} | ||
|
||
response = await client.patch( | ||
f"/v1/orders/{orders[0].id}", | ||
json={"billing_address": new_address}, | ||
) | ||
|
||
assert response.status_code == 200 | ||
|
||
json = response.json() | ||
assert json["billing_address"]["line1"] == "123 Updated St" | ||
assert json["billing_address"]["city"] == "Updated City" | ||
|
||
|
||
@pytest.mark.asyncio | ||
class TesGetOrdersStatistics: | ||
async def test_anonymous(self, client: AsyncClient) -> None: | ||
|
Uh oh!
There was an error while loading. Please reload this page.