Skip to content
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

[ADD][16.0] address shipping note #1374 #1553

Open
wants to merge 4 commits into
base: 16.0
Choose a base branch
from

Conversation

benwillig
Copy link
Contributor

Simply a cherry-pick of relevant commits of #1374

cc @AnizR @sebastienbeau

@sebastienbeau sebastienbeau added this to the 16.0 milestone Jun 4, 2024
@@ -49,7 +49,7 @@ def setUpClass(cls) -> None:
cls.default_fastapi_authenticated_partner = cls.test_partner
cls.default_fastapi_router = address_router

def test_get_invoicing_address(self):
def test_get_billing_address(self):
Copy link
Member

Choose a reason for hiding this comment

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

Why renaming this test from invoicing to billing? Because it does a get of /addresses/invoicing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted it

@benwillig benwillig force-pushed the 16.0-address_shipping_note_rebase branch from 3d901bd to 3a59539 Compare June 4, 2024 16:18
Copy link
Contributor

@sebastienbeau sebastienbeau left a comment

Choose a reason for hiding this comment

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

LGTM (code review)

return res


class ShippingAddressNoteCreate(DeliveryAddressCreate, extends=DeliveryAddressCreate):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
class ShippingAddressNoteCreate(DeliveryAddressCreate, extends=DeliveryAddressCreate):
class ShippingAddressNoteCreate(DeliveryAddressCreate, extends=True):

return vals


class ShippingAddressNoteUpdate(DeliveryAddressUpdate, extends=DeliveryAddressUpdate):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
class ShippingAddressNoteUpdate(DeliveryAddressUpdate, extends=DeliveryAddressUpdate):
class ShippingAddressNoteUpdate(DeliveryAddressUpdate, extends=True):

Comment on lines 94 to 97
with self._create_test_client(router=address_router) as test_client:
response: Response = test_client.get(
"/addresses/delivery",
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

to avoid to always have to pass the router on each call to _create_test_client you should assign the address_router to the property default_fastapi_router into your setUpClass


def to_res_partner_vals(self) -> dict:
vals = super().to_res_partner_vals()
shipping_note = self.shipping_note
Copy link
Collaborator

Choose a reason for hiding this comment

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

With partial update, it's not required to set the field into the json you pass to odoo when updating the address. With your code, you' don't make the difference between to cases:

  • The field is not set into the json -> value is None in python and we don't want to update it
  • The field is set to null into the json -> value is None in python and we want to update it...

To covers both cases you must call the model_dump to retrieve fields really given to instanciate the model:

values = self.model_dump(exclude_unset=True, include=["shipping_note"])
if "shipping_note" in values:
    vals["shipping_note"] = values["shipping_note"
return vals]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I updated the code

@benwillig benwillig force-pushed the 16.0-address_shipping_note_rebase branch from 239b573 to c64193a Compare June 6, 2024 10:38
Copy link
Contributor

@paradoxxxzero paradoxxxzero left a comment

Choose a reason for hiding this comment

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

LGTM (code + functional review), requested changes have been fixed, requested a very minor not blocking change.

def _call_test_client(self, url, http_code=status.HTTP_200_OK, **kwargs):
method = kwargs.pop("method", "get")
with self._create_test_client(
router=self.default_fastapi_router
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not necessary as cls.default_fastapi_router is already setting the default router: https://github.com/OCA/rest-framework/blob/16.0/fastapi/tests/common.py#L146

Suggested change
router=self.default_fastapi_router

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants