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.

Sign up for GitHub

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

[16.0] shopinvader_api_address: improve schema data conv #1536

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from
Open

[16.0] shopinvader_api_address: improve schema data conv #1536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions shopinvader_api_address/routers/address_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def create_invoicing_address(
Create invoicing address
Raise error since invoicing address is the authenticated partner
"""
vals = data.to_res_partner_vals()
vals = data.to_res_partner_vals(partner.env)
address = partner._create_shopinvader_invoicing_address(vals)
return InvoicingAddress.from_res_partner(address)

Expand All @@ -77,7 +77,7 @@ def update_invoicing_address(
Update invoicing address of authenticated user
invoicing address corresponds to authenticated partner
"""
vals = data.to_res_partner_vals()
vals = data.to_res_partner_vals(partner, address_id)
# sudo() is needed because some addons override the write
# function of res.partner to do some checks before writing.
# These checks need more rights than what we are giving to
Expand Down Expand Up @@ -125,7 +125,7 @@ def create_delivery_address(
"""
Create delivery address of authenticated user
"""
vals = data.to_res_partner_vals()
vals = data.to_res_partner_vals(partner.env)
address = partner._create_shopinvader_delivery_address(vals)

return DeliveryAddress.from_res_partner(address)
Expand All @@ -140,7 +140,7 @@ def update_delivery_address(
"""
Update delivery address of authenticated user
"""
vals = data.to_res_partner_vals()
vals = data.to_res_partner_vals(partner, address_id)
# sudo() is needed because some addons override the write
# function of res.partner to do some checks before writing.
# These checks need more rights than what we are giving to
Expand Down
16 changes: 10 additions & 6 deletions shopinvader_api_address/schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@

from extendable_pydantic import StrictExtendableBaseModel

from odoo.api import Environment

from odoo.addons.base.models.res_partner import Partner as ResPartner


class AddressCreate(StrictExtendableBaseModel, extra="ignore"):
"""
Expand All @@ -21,7 +25,7 @@ class AddressCreate(StrictExtendableBaseModel, extra="ignore"):
state_id: int | None = None
country_id: int | None = None

def to_res_partner_vals(self) -> dict:
def to_res_partner_vals(self, env: Environment) -> dict:
vals = {
"name": self.name,
"street": self.street,
Expand Down Expand Up @@ -55,7 +59,7 @@ class AddressUpdate(StrictExtendableBaseModel, extra="ignore"):
state_id: int | None = None
country_id: int | None = None

def to_res_partner_vals(self) -> dict:
def to_res_partner_vals(self, rec: ResPartner, address_id: int) -> dict:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the use of the 'address_id' parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, the record you pass is not the real record you deal with. In fact, the method on the model is taking care of filtering the address on the current partner using this ID.
Hence, if you need to do something based on the real record value you need that.
The alternative would be to pass the address directly but yeah.... I've simply chosen to respect the same args that are passed over to _update_shopinvader_$type_address.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed addresses are filtered in _update_* methods: we pass the address identifier (address_id) to which this specific address is linked.. I still don't see why we should pass this information at this stage since you've the odoo record which is most probably a child of this address_id. Strictly speaking, when it comes to understanding the code, adding this parameter to this method makes no sense at all, and anyone reading the code should know that this parameter has a precise meaning only in a very specific context of use (this meaning is nowhere to be found here). I'd prefer to keep the general meaning of 'from' / 'to' methods and avoid making them polymorphic depending on the presence or absence of a parameter that should be interpreted in a precise sense for a specific use case.

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 agree w/ you, I'm trying to stay consistent w/ what I see 😄
Then... instead of passing the current partner I can browse the address right away.
And once I do this, I wonder why don't we pass the same record all around instead of the 2 args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lmignon ping :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still prefer to not add a new parameter.

fields = [
"name",
"street",
Expand Down Expand Up @@ -84,8 +88,8 @@ class InvoicingAddressCreate(AddressCreate):

vat: str | None = None

def to_res_partner_vals(self) -> dict:
vals = super().to_res_partner_vals()
def to_res_partner_vals(self, env: Environment) -> dict:
vals = super().to_res_partner_vals(env)

vals["vat"] = self.vat

Expand All @@ -99,8 +103,8 @@ class InvoicingAddressUpdate(AddressUpdate):

vat: str | None = None

def to_res_partner_vals(self) -> dict:
vals = super().to_res_partner_vals()
def to_res_partner_vals(self, rec: ResPartner, address_id: int) -> dict:
vals = super().to_res_partner_vals(rec, address_id)

vals["vat"] = self.vat

Expand Down
Loading