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

[REM] *: remove calls to `api.one` and adapt code #29511

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
@Elkasitu
Copy link
Contributor

commented Dec 13, 2018

Adapt all code that was using api.one to recordset-style method and
remove any and all calls to api.one in preparation for its removal.

Related to odoo/enterprise#3277

@robodoo robodoo added the seen 🙂 label Dec 13, 2018

@pedrobaeza

This comment has been minimized.

Copy link
Collaborator

commented Dec 13, 2018

Isn't better for reducing diff to leave the method as is renaming it with for example _one suffix, and then create a new method for making the iteration?

Full example:
Original

    @api.one
    @api.constrains('currency_id', 'default_credit_account_id', 'default_debit_account_id')
    def _check_currency(self):
        if self.currency_id:
            if self.default_credit_account_id and not self.default_credit_account_id.currency_id.id == self.currency_id.id:
                raise ValidationError(_('The currency of the journal should be the same than the default credit account.'))
            if self.default_debit_account_id and not self.default_debit_account_id.currency_id.id == self.currency_id.id:
                raise ValidationError(_('The currency of the journal should be the same than the default debit account.'))

Proposition

-    @api.one
     @api.constrains('currency_id', 'default_credit_account_id', 'default_debit_account_id')
     def _check_currency(self):
+        for record in self:
+            record._check_currency_one()

+    def _check_currency_one(self):
         if self.currency_id:
             if self.default_credit_account_id and not self.default_credit_account_id.currency_id.id == self.currency_id.id:
                 raise ValidationError(_('The currency of the journal should be the same than the default credit account.'))
             if self.default_debit_account_id and not self.default_debit_account_id.currency_id.id == self.currency_id.id:
                 raise ValidationError(_('The currency of the journal should be the same than the default debit account.'))

This is going to be even bigger on large methods.

@robodoo robodoo added the CI 🤖 label Dec 13, 2018

@C3POdoo C3POdoo added the RD label Dec 13, 2018

@Elkasitu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2018

@pedrobaeza What you're suggesting is to keep @api.one's functionality but changing the api to be more obscure, at which point might as well keep the current api.

The goal of the PR is to completely remove @api.one because methods decorated with it will return a list and not a recordset, this can be annoying/confusing/unexpected when calling a method on a recordset

@Yenthe666

This comment has been minimized.

Copy link
Collaborator

commented Dec 13, 2018

Yeah I'd vote for fully removing them too, although the diff is bigger. Plenty of people are confused by api.one

@mreficent

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

The problem with this "big difference" approach is that fixes from previous versions may be badly forward ported.

@pedrobaeza

This comment has been minimized.

Copy link
Collaborator

commented Dec 13, 2018

Yeah, that's why I suggested to keep it that way, but it's up to you to decide what is the best approach.

@Yenthe666

This comment has been minimized.

Copy link
Collaborator

commented Dec 13, 2018

It is against master though. Since V13 is still 9 months away I'd assume that pretty much all of them should be out before we get it publicly available. So the 'pain' should be at Odoo S.A. if some are not correctly forwarded.

Show resolved Hide resolved addons/account/models/chart_template.py Outdated
@wtaferner
Copy link
Contributor

left a comment

Go for it, but please try to make code more readable (variable naming) and add some standard doc strings if you do such a big diff in order to have at least some improvement in the code instead of just search and replace stuff ;-)

addons/account/models/account.py Outdated
raise ValidationError(_('The currency of the journal should be the same than the default credit account.'))
if self.default_debit_account_id and not self.default_debit_account_id.currency_id.id == self.currency_id.id:
raise ValidationError(_('The currency of the journal should be the same than the default debit account.'))
for rec in self:

This comment has been minimized.

Copy link
@wtaferner

wtaferner Dec 14, 2018

Contributor

May I ask you to use proper variable/record naming for the iteration?
journal instead of rec in this method otherwise it is just a big diff in favor of removing without winning a clearer code.

This comment has been minimized.

Copy link
@nhomar

nhomar Dec 14, 2018

Collaborator

@Elkasitu .. @wtaferner got a point

This comment has been minimized.

Copy link
@Elkasitu

Elkasitu Dec 14, 2018

Author Contributor

I used a vim macro, doing this manually would take too long (and be too boring).

This comment has been minimized.

Copy link
@gdgellatly

gdgellatly Dec 14, 2018

Contributor

but it leads to clearly poor code.

e.g. rec.env[model] - not really an idiom that is used anywhere unless it is specifically required, always self.env. In any case normally they are declared outside of a loop.
Or for rec ins self: rec.x == 'y'. Should just be
self.write()

or sql queries that are now iterated when a groupby is really whats needed.
or constants that are now initialised multiple times inside a loop.
or functions that clearly require ensure_one() now (like loading a chart of accounts for current company).

sometimes dev is boring.

This comment has been minimized.

Copy link
@Yenthe666

Yenthe666 Dec 14, 2018

Collaborator

Euhh this is disturbing. You shouldn't refactor over 1.000 lines of code with a macro. This is bound to fail and blow up at some points.. Sometimes a dev needs to do the boring things and take time for these kind of things, although they're not technically challenging. 😉

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo Dec 18, 2018

Member

@Yenthe666 don't worry, this was not a blind "apply macro then commit" 😉

This comment has been minimized.

Copy link
@Yenthe666

Yenthe666 Dec 18, 2018

Collaborator

@rco-odoo gotcha! So macro and then testing? Then it is good. Thanks for the feedback!

Show resolved Hide resolved addons/delivery/models/stock_picking.py Outdated
addons/account/models/account.py Outdated
if self.default_debit_account_id and not self.default_debit_account_id.currency_id.id == self.currency_id.id:
raise ValidationError(_('The currency of the journal should be the same than the default debit account.'))
for rec in self:
if rec.currency_id:

This comment has been minimized.

Copy link
@nhomar

nhomar Dec 14, 2018

Collaborator

code looks better is you return early/. and even better if you do before a filtered for the self.

This comment has been minimized.

Copy link
@Elkasitu

Elkasitu Dec 14, 2018

Author Contributor

what do you mean? there is no return here

addons/event/models/event_mail.py Outdated
if not rec.mail_sent and (rec.interval_type != 'before_event' or rec.event_id.date_end > now):
rec.event_id.mail_attendees(rec.template_id.id)
rec.write({'mail_sent': True})
return True

This comment has been minimized.

Copy link
@Elkasitu

Elkasitu Dec 14, 2018

Author Contributor

probably for xmlrpc, might be OK to just return a single True at the end

Show resolved Hide resolved addons/stock/models/stock_picking.py Outdated
addons/website_sale_delivery/models/sale_order.py Outdated
super(SaleOrder, self)._compute_website_order_line()
self.website_order_line = self.website_order_line.filtered(lambda l: not l.is_delivery)
for rec in self:
super(SaleOrder, self)._compute_website_order_line()

This comment has been minimized.

Copy link
@Elkasitu

Elkasitu Dec 14, 2018

Author Contributor

before: super call for every record
now: super call once with recordset? or keep old behavior? 🤔

@Elkasitu Elkasitu force-pushed the odoo-dev:master-rem-api-one-v2-adt branch Dec 14, 2018

@robodoo robodoo removed the CI 🤖 label Dec 14, 2018

@Elkasitu Elkasitu force-pushed the odoo-dev:master-rem-api-one-v2-adt branch 2 times, most recently Dec 14, 2018

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Dec 14, 2018

@Elkasitu Elkasitu changed the title [WIP][REM] *: remove calls to `api.one` and adapt code [REM] *: remove calls to `api.one` and adapt code Dec 17, 2018

@Elkasitu Elkasitu requested a review from rco-odoo Dec 17, 2018

@rco-odoo

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

Dear all, the purpose of this task is not to make the code nicer.
The point is to make changes as simple and safe as possible.

Making code more readable is refactoring, and that's a good thing, but it comes with a risk of breaking things. That's why it should be a separate task.

@wtaferner

This comment has been minimized.

Copy link
Contributor

commented Dec 18, 2018

@rco-odoo
Sorry, that I have to oppose here, and of course this task is not about making code nicer or really refactor, but honestly if you touch so many code lines at once it is important to NOT use a search and replace approach but if you feel the need to be efficient, write an iterator wrapper instead like @pedrobaeza suggested and wait for refactoring to get rid of the singleton methods. If you want to do it with search and replace the minimum you should do afterwards is to adapt the variable names to make the code readable like you normally would do it in case you write meaningful code for a purpose. Most of the code is business logic code, so it is not just the same and deserves more attention than a VIM macro which can only be a helper to get started.

And, it is not some individual customer implementation...it is the benchmark for all implementations on top of it (Odoo SA should be the role model), so I can of course expect better code, if it is touched like that as an outcome (master is the future stable).

If you manage that all methods are taken care afterwards or within the same branch by the teams, fine for me, but mostly it just don't happen. It is like removing tests and waiting for others to write new and better ones at some point.

In all respect that I have for the task, your work and the people moving forward 🙏

addons/account/models/partner.py Outdated
for rec in self:
if rec.zip_from > rec.zip_to:
raise ValidationError(_('Invalid "Zip Range", please configure it properly.'))
return True

This comment has been minimized.

Copy link
@rco-odoo

rco-odoo Dec 18, 2018

Member

Remove the return statement in this case.

@rco-odoo

This comment has been minimized.

Copy link
Member

commented Dec 18, 2018

@wtaferner you have been heard, we will have a closer look at the replaced methods.

@robodoo robodoo removed the CI 🤖 label Dec 19, 2018

@Elkasitu Elkasitu force-pushed the odoo-dev:master-rem-api-one-v2-adt branch Dec 19, 2018

@Elkasitu Elkasitu force-pushed the odoo-dev:master-rem-api-one-v2-adt branch 2 times, most recently Dec 20, 2018

@robodoo robodoo added the CI 🤖 label Dec 20, 2018

@Elkasitu Elkasitu force-pushed the odoo-dev:master-rem-api-one-v2-adt branch Dec 20, 2018

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Dec 20, 2018

addons/account/models/account.py Outdated
@api.constrains('journal_id')
def _check_journal_id(self):
if len(self.journal_id) > 1:
raise ValidationError(_('A bank account can belong to only one journal.'))
for journal in self:

This comment has been minimized.

Copy link
@fmdl

fmdl Dec 21, 2018

Contributor

for bank in self

@Elkasitu Elkasitu force-pushed the odoo-dev:master-rem-api-one-v2-adt branch Jan 7, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 7, 2019

@Elkasitu Elkasitu force-pushed the odoo-dev:master-rem-api-one-v2-adt branch Jan 23, 2019

@robodoo robodoo removed the CI 🤖 label Jan 23, 2019

[REM] *: remove calls to `api.one` and adapt code
Adapt all code that was using `api.one` to recordset-style method and
remove any and all calls to `api.one` in preparation for its removal.

@Elkasitu Elkasitu force-pushed the odoo-dev:master-rem-api-one-v2-adt branch Jan 23, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jan 23, 2019

Elkasitu added some commits Dec 17, 2018

[REM] api: remove helpers one and aggregate
`api.one` has been deprecated since v9 because it often makes the code
less clear and behaves in ways developers and readers may not expect
since functions decorated with it usually expect a `list` of record(s)
instead of the recordset, whereas most modern Odoo code expects `self`
to be a recordset.

The function `aggregate` was solely being used by `api.one` thus it has
also been removed since there doesn't seem to be any other use for it
thus far.

@Elkasitu Elkasitu force-pushed the odoo-dev:master-rem-api-one-v2-adt branch to 14cb94d Jan 25, 2019

@robodoo robodoo removed the CI 🤖 label Jan 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.