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

16.0 add shopinvader_api_sale #1421

Merged
merged 34 commits into from
Nov 8, 2023

Conversation

sebastienbeau
Copy link
Contributor

@sebastienbeau sebastienbeau commented Oct 7, 2023

Work in progress

  • add BaseSale schema (can be used by cart and sale)
  • add module shopinvader_api_sale
  • change the structure of the cart (TO DISCUSS)

@lmignon @sbidoul @thibaultrey

@sebastienbeau sebastienbeau changed the title 16.0 add api sale 16.0 add shopinvader_api_sale Oct 7, 2023
@sebastienbeau sebastienbeau marked this pull request as draft October 7, 2023 22:11
@@ -0,0 +1,5 @@
from . import shipping
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, a good practice is to always import defined models a module root. It eases code readability....

from odoo.addons.shopinvader_schema_sale.schemas import InvoicingInfo

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 for the tips

shopinvader_api_cart/schemas/cart.py Outdated Show resolved Hide resolved
@sebastienbeau
Copy link
Contributor Author

@lmignon I have a weird error with the PagedCollection and I don't know why, do you have an idea?

https://github.com/shopinvader/odoo-shopinvader/actions/runs/6450464419/job/17509863251?pr=1421#step:8:203

@lmignon
Copy link
Collaborator

lmignon commented Oct 11, 2023

@lmignon I have a weird error with the PagedCollection and I don't know why, do you have an idea?

https://github.com/shopinvader/odoo-shopinvader/actions/runs/6450464419/job/17509863251?pr=1421#step:8:203

@sebastienbeau Don't worry. It's just because you're mixing an extendable model Sale with a bare pydantic generic model PagedCollection...

I made a little test by defining the PagedCollection mode as follows

from typing import TypeVar, List, Generic
from odoo.addons.extendable_fastapi.schemas import StrictExtendableBaseModel

T = TypeVar("T")


class PagedCollection(StrictExtendableBaseModel, Generic[T]):
    total: int
    items: List[T]

and it works

I propose to add these generics models into the odoo.addons.extendable_fastapi.schemas module. I'll keep you informed once it's done.

@lmignon
Copy link
Collaborator

lmignon commented Oct 11, 2023

@sebastienbeau here it's OCA/rest-framework#380

@sebastienbeau
Copy link
Contributor Author

Regarding the security rule define here :
https://github.com/shopinvader/odoo-shopinvader/tree/16.0/shopinvader_api_cart/security

We need to define the same rule on api_sale, api_quotation, api_cart (we want to show the sale that belong to the authenticated_partner)

I see 3 possibility

  1. api_quotation, api_cart depend on api_sale (because you never will have a cart (and quotation) without sale history, and if you have the case, you still can not mount the endpoint "sales")
  2. we duplicate the rule and make them more accurate (by adding the typology on the rule), so the group cart only return cart. Note we also need in each api (cart, quotation, sale to filter on the right typo, so a cart can not read from sale endpoint)
  3. we create a base module api_sale_base that add the rule

@sebastienbeau
Copy link
Contributor Author

sebastienbeau commented Oct 13, 2023

There is also some "common" code for testing between the api_sale, api_quotation, api_cart

@sebastienbeau
Copy link
Contributor Author

@lmignon the api is ready.
We just need to decide for the rule, so I can change the dependency, and update the test

@sebastienbeau sebastienbeau force-pushed the 16.0-add-api-sale branch 2 times, most recently from 88beffe to 6049805 Compare October 15, 2023 11:20

def convert_to_sale_write(self):
vals = {}
data = self.model_dump(exclude_unset=True)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I have used the "self.model_dump(exclude_unset=True)" here to be able to support partial update

The API should be able to only push the invoicing info or the note, without needing to send every value

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sebastienbeau It's a valid and appropriate use. The most important here is that you use the result of the call to model_dump to create the result of the convert_to_sale_write method but not as result of the method.

@sebastienbeau
Copy link
Contributor Author

after merging api_address #1425

clean I will add this clean here akretion#8

Copy link
Collaborator

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

Thank you @sebastienbeau It's promising and easy to read. My remaining concerns:

  • Not comfortable with the definition of FilteredDomainAdapter into the fastapi addon (Could be also renamed FilteredModelAdapter???)
  • Do we've to put the logic to transform a schema into an odoo model dict or a domain into the schema? I've no clear args in favor or against?

Some little comments

__init__.py Outdated Show resolved Hide resolved
shopinvader_api_sale/routers/sales.py Outdated Show resolved Hide resolved
_description = "Shopinvader Api Sale Service Helper"

@property
def adapter(self):
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
def adapter(self):
def filtered_model(self):

shopinvader_schema_sale/schemas/sale_order.py Outdated Show resolved Hide resolved
shopinvader_api_sale/routers/sales.py Outdated Show resolved Hide resolved
shopinvader_api_sale/routers/sales.py Outdated Show resolved Hide resolved
@lmignon
Copy link
Collaborator

lmignon commented Nov 4, 2023

@sebastienbeau Here it's some fixes and improvements... akretion#9

@sebastienbeau
Copy link
Contributor Author

sebastienbeau commented Nov 6, 2023

@lmignon I have a weird issue on the test
If you install "shopinvader_api_cart" and run

odoo -i shopinvader_schema_sale --stop-after-init --test-enable

It fail on module shopinvader_api_cart

If you run

odoo -i shopinvader_api_cart --stop-after-init --test-enable

It's work

Seem there is an issue in "extendable"

@lmignon lmignon marked this pull request as ready for review November 7, 2023 14:28
@lmignon
Copy link
Collaborator

lmignon commented Nov 7, 2023

/ocabot merge minor

@shopinvader-git-bot
Copy link

@lmignon The merge process could not start, because of exception [Errno 13] Permission denied: '/app/run/.cache/oca-mqt'.

@sebastienbeau
Copy link
Contributor Author

/ocabot merge minor

@shopinvader-git-bot
Copy link

@sebastienbeau The merge process could not start, because of exception [Errno 13] Permission denied: '/app/run/.cache/oca-mqt'.

@sbidoul
Copy link
Member

sbidoul commented Nov 8, 2023

/ocabot merge minor

@shopinvader-git-bot
Copy link

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-1421-by-sbidoul-bump-minor, awaiting test results.

@shopinvader-git-bot shopinvader-git-bot merged commit bded3b0 into shopinvader:16.0 Nov 8, 2023
3 checks passed
@shopinvader-git-bot
Copy link

Congratulations, your PR was merged at b42cf29. Thanks a lot for contributing to shopinvader. ❤️

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

Successfully merging this pull request may close these issues.

None yet

4 participants