[16.0] shopinvader_api_address: refactor API (paginate, code improvement

[sebastienbeau]

Hi all

I have done some change for the address API. The main change are

• add pagination
• move logic in helper (instead of partner), so at the end the shopinvader_adress was empty, so I removed it
• remove useless response_model, fix inherit schema
• depend on the following PR for generic method : fastapi: add base service for exposing search/get OCA/rest-framework#382
• delivery address should return the main partner as address

Todo

• clean wording after deciding it : Convention wording #1426
• rework pydantic schema to vals : Convention converting a pydantic model to odoo vals #1427

[sebastienbeau]

@lmignon : I would like your feedback here : #1427

Maybe I should move this logic in the "service" ?

[lmignon]

@sebastienbeau Even if in this case we've a strict mapping between declared fields on the schema and fields on the res.partner model, we must avoid to return the result of the call to model_dump as parameters for a call to the write' method on the res.partnermodel. As discussed, this way you introduce a deep coupling between your schema and your model and this code will break if the schema is extended to add a field not defined on the odoo model. Nevertheless the call tomodel_dumpwith the exclude_unset=True` parameter could be good Idea to update only values set on the schema. But it's to use on purpose since you also lost default values defined on the schema (AFAIK).

fields = ["name", "street", "street2", "zip", "city", "phone", "email", "state_id", "country_id"]
values = self.model_dump(exclude_unset=True)
return {f: values[f] for f in fields if f in values }

[sbidoul]

This is purely mapping code, converting the result of the business logic (even if trivial in this case) to the data structure of the API. So this is good to have it in the API layer.

IMHO, mapping code is better explicit. This is may take a few seconds more to write, but it is much much easier to read afterwards. And we collectively spend much more time reading code that writing it :)

Also it is safer, because if we map fields with same names automatically, we risk accidentally exposing data when we add fields to the API data structure.

[sbidoul]

delivery address should return the main partner as address

I kind of remember we dicussed this in @AnizR's PR back then, and concluded to not do it for a good reason (that I don't remember).

[sbidoul]

Why removing shopinvader_address?

[sebastienbeau]

After starting to move code from "res.partner" to the "shopinvader_api_cart.address_router.helper".

I realize that everything was really related to the router, so at the end I had no more code on "res.partner". I only had an hesitation on the method "_ensure_address_not_used" that may stay on "res.partner"

So at the end the module shopinvader_address had no code, so I remove it.

But maybe I miss some point and some code was think to be reuse outside of the router.

[AnizR]

I realize that everything was really related to the router, so at the end I had no more code on "res.partner". I only had an hesitation on the method "_ensure_address_not_used" that may stay on "res.partner"

I respectfully express my dissenting opinion on this matter. From my perspective, the code previously residing in res.partner does not appear to be exclusively tied to the router. Consider a scenario where one opts for a different framework other than FastAPI or has an Odoo module utilizing 'shopinvader addresses' independently of 'shopinvader_api_address.' In such cases, it seems essential to retain all functions originally present in shopinvader_address for broader compatibility and flexibility. This approach ensures that the functionality remains accessible even in diverse usage contexts beyond the current implementation.

[sbidoul]

@sebastienbeau actually shopinvader_address is the logic and high level methods that defines what invoicing and delivery addresses mean for shopinvader. There is business logic in there that defines what the invoicing address is what deletion means (= archival) etc.

We can easily imagine that some backend code will want to manipulate such addresses, and that code would then depend on shopinvader_address to use these methods, and not shopinvader_api_address.

This is the principle that we want to avoid business logic in the API layer as much as possible, and in this case we concluded that the address logic was non-trivial enough to go in its own module.

[sbidoul]

I think we had decided to not do paging nor search because there would never be tons of addresses for a given customer, and filtering could be easily done in the frontend. Just a remark en passant, nothing blocking.

[sebastienbeau]

Indeed it make sens, but I would prefers to be consistent (a search always return a paginate).

I have a case where we plan to expose contact (it will done in shopinvader_api_contact, so we are going to expose a big list of contact ~200, so I will need paginatation. And it's look weird that on same object sometime we have pagination and sometime not)

[sbidoul]

As I said, not blocking for me but I'd argue adding paging and search to small lists makes it more difficult for the front-end dev (unless they ignore it and assumes there will only ever be one page, but that is opening the door to bugs).

[lmignon]

I would also prefer to not do paging nor search on the addresses by default. (We could add a dedicated endpoint if necessary to some uses)

[sebastienbeau]

delivery address should return the main partner as address

I kind of remember we dicussed this in @AnizR's PR back then, and concluded to not do it for a good reason (that I don't remember).

In odoo website, the main address can be used for invoicing and delivery, same in backoffice. And most of people use the same address for both. If you remember the reason, please share it.

[sbidoul]

delivery address should return the main partner as address

It's because, for instance, the main address cannot be deleted by the customer.
By not showing the main address as a delivery address, it is easy for the front to not confuse them.
And if the customer wants to deliver at the invoicing address, the front can display a "same as invoicing address" check box, or display the invoicing address in the list of delivery addresses but readonly.

#1361 is the PR where we distilled all this. There was a lot of thinking put into it as you see :) So please, pretty please, don't throw everything overboard.

[sebastienbeau]

@sbidoul thanks for pointing the PR, I will read the comment done.
Indeed this main address cause some trouble, I am going to check this with @thibaultrey

[sebastienbeau]

I close this PR : I just keep the rename : #1442