fix: Allow to update custom field data #6613
Conversation
- Add OrganizationNotification model for tracking read status per organization
- Add organization notification endpoints: GET/POST /organizations/{id}/notifications
- Add send_to_organization() method to notification service
- Add database migrations for organization_notifications table and index
- Maintain backward compatibility with existing user-level notifications
Closes polarsource#6598
This reverts commit c65acb2.
|
@amankitsingh is attempting to deploy a commit to the polar-sh Team on Vercel. A member of the Team first needs to authorize it. |
This reverts commit 9cacda2.
|
Can you do the following to make sure that the PR is ready for review:
|
rishi-raj-jain
changed the title
|
|
@frankie567 , @psincraian , kindly review this PR |
|
can you please share the video of it's working in your local setup? can be through a CLI based set up as well to validate the code's working. |
There was a problem hiding this comment.
A few things to fix, but that overall answers the need.
That said, it’s quite clear this PR was generated using an LLM. I’m not against that—these are extremely useful tools I use myself.
The problem here is that:
- It’s not clear if you actually set up the dev environment and ran the code
- The LLM added a lot of things that had nothing to do with the PR
It’s great to see people like you contributing to the project; but it’s also important for us as maintainers that we don’t end up spending more time reviewing and fixing a PR than it took the AI to generate it.
| 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.
We have CustomFieldDataInputMixin in polar/custom_field/data.py that should be used to include that field with all its characteristics in the model.
| @@ -425,10 +426,23 @@ async def update( | |||
| if errors: | |||
| raise PolarRequestValidationError(errors) | |||
|
|
|||
| # Handle custom field data validation and update | |||
| 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.
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 get method:
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.
@frankie567 are you suggesting this because of the performance, so that update function doesnt lazy load the data?
| 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.
That refresh is not needed at all
There was a problem hiding this comment.
@frankie567 is this because, in get method we are eagerly loading the product relationship which the modification?
| customer=customer, | ||
| custom_field_data={"text": "original", "select": "a"}, | ||
| ) | ||
| await save_fixture(order) |
| customer=customer, | ||
| billing_name="Original Name", | ||
| ) | ||
| await save_fixture(order) |
| order.invoice_path = "/path/to/invoice.pdf" # Invoice already generated | ||
| await save_fixture(order) | ||
|
|
||
| from polar.exceptions import PolarRequestValidationError |
There was a problem hiding this comment.
Imports should be at the top
| assert errors[0]["loc"] == ("body", "billing_name") | ||
| assert "cannot be updated after the invoice is generated" in errors[0]["msg"] | ||
|
|
||
| async def test_update_custom_field_data_with_invoice_generated( |
There was a problem hiding this comment.
I get the idea of the test but well, I think we can remove it's not that useful
| @pytest.mark.auth( | ||
| AuthSubjectFixture(scopes={Scope.web_write}), | ||
| ) | ||
| async def test_update_billing_name( |
There was a problem hiding this comment.
Not relevant for those changes
| @pytest.mark.auth( | ||
| AuthSubjectFixture(scopes={Scope.web_write}), | ||
| ) | ||
| async def test_update_billing_address( |
There was a problem hiding this comment.
Not relevant for those changes
|
@frankie567 Yes, ChatGPT was used to refine the test case, as said earlier as well. |
fix #6168
Allow merchants to update custom field data for orders via API