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

fastapi: add base service for exposing search/get #382

Closed
wants to merge 2 commits into from

Conversation

sebastienbeau
Copy link
Member

Add generic service that can be used to expose odoo model

@OCA-git-bot
Copy link
Contributor

Hi @lmignon,
some modules you are maintaining are being modified, check this out!

Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

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

In general I wonder if this would not be best implemented as helper functions rather than an abstract class.
This may lead to a more linear flow that could be easier to read.

def _convert_search_params_to_domain(self, params):
return []

def _search(self, paging, params=None):
Copy link
Member

Choose a reason for hiding this comment

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

In this case would it make sense to pass the domain instead of params?

Copy link
Contributor

@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.

In my opinion, this approach lower the readability of the code. I would like to avoid to ends up with complex class inheritance where it becomes hard to know the responsibility of each layer and to understand the behavior of your final model.
You could implement the same functionality by defining for example a specialized adapter class.

from odoo import models
from odoo.osv import expressions
from extendable.main import ExtendableMeta


class FilteredDomainAdapter( metaclass=ExtendableMeta):

    def __init__(self, model: models.BaseModel, base_domain: list[tuple[]]):
        self._model = model
        self._base_domain = base_domain

    def get(self, record_id: int) -> BaseModel:
        return self._model.browse(record_id).filtered_domain(self._base_domain)

    def search_with_count(self, domain:list[tuple[], limit, offset]):
        final_domain = expressions.AND(
            [
                self._base_model,
               domain
            ]
        )
        ....

In your code (service or ...)

sale_model = FilteredDomainAdapter(self.env["sale.order"],  [("type", "=", "delivery")])

my_order = sale_model.get(23)
count, orders = sale_model.search([("state", "in", ["cancel", "done"] )])

I'm not sure that FilteredDomainAdapter should be modifiable by inheritance. It should probably be a simple python class.

@simahawk
Copy link
Contributor

Well, I guess we have 2 topics here:

  1. provide default endpoints to services
  2. avoid duplicated logic for simple tasks

As for point 1, IMO is up to the service to explicitly declare its endpoints. You can define your own mixins if needed.

As for point 2, IMO having something like the FilteredDomainAdapter I see in the example makes it even harder/more complex than using components. If we have to provide such helper, I agree it can be a simple python class.

@sebastienbeau
Copy link
Member Author

Thanks for your feedback.
Indeed implementing a Helper can make it more readable and still avoid to duplicate the code.
I am going to test this approach.
Thanks

@sebastienbeau
Copy link
Member Author

Also regarding some good practice, I would like to have your feedback on this :

shopinvader/odoo-shopinvader#1427
shopinvader/odoo-shopinvader#1426

Thanks

@sebastienbeau
Copy link
Member Author

@@ -0,0 +1,27 @@
# Copyright 2023 Akretion (https://www.akretion.com).
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this code has nothing to do with fastapi....

@lmignon
Copy link
Contributor

lmignon commented Nov 13, 2023

@sebastienbeau I close this one this the code is now provided into odoo-shopinvader

@lmignon lmignon closed this Nov 13, 2023
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.

5 participants