-
-
Notifications
You must be signed in to change notification settings - Fork 507
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
[9.0][MIG] account_balance_reporting y l10n_es_account_balance_report #416
[9.0][MIG] account_balance_reporting y l10n_es_account_balance_report #416
Conversation
pedrobaeza
commented
Dec 19, 2016
- Optimized wizard
- Changed headers
- Correct match of accounts with *
- Menus organization
- Tests
b322d24
to
97b50fb
Compare
👍 LGTM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in runbot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Algunos comentarios que puedes tomar en cuenta si te parecen interesantes. Revision de código y funcional.
def onchange_previous_date_range(self): | ||
if self.previous_date_range: | ||
self.previous_date_from = self.previous_date_range.date_start | ||
self.previous_date_to = self.previous_date_range.date_end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these just be related fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, porque tú puedes poner fechas sin tener rangos de fechas. Esto sólo sirve como "acelerador": seleccionar 2016 en lugar de teclear del 01/01/2016 a 31/12/2016, pero también puedes querer hacer un informe del 02/02/2016 al 17/07/2016 sin necesidad de tener un rango de fecha para ello.
@@ -58,25 +58,23 @@ class AccountBalanceReportingTemplate(models.Model): | |||
comodel_name='account.balance.reporting.template.line', | |||
inverse_name='template_id', string='Lines') | |||
|
|||
@api.model | |||
def copy(self, id, default=None): | |||
@api.one |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this should be multi instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, la definición en API vieja es con argumento id
, lo que equivale a un @api.one
: https://github.com/odoo/odoo/blob/c480ec46cc4860c030b4a54b978fd2aec108d9e4/openerp/models.py#L4976.
En v10 lo han hecho api.multi
, pero con un self.ensure_one()
, como el one está deprecated: https://github.com/odoo/odoo/blob/2455e17842e9d5114bb3e2aa161e2afa54106a34/odoo/models.py#L4354, pero por el momento hay que dejarlo así para que casen los argumentos. Voy a añadir en la guía de migración esta conversión...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bueno que pongan id
o ids
no importa, la api ya se encarga de eso. El problema es que el return del final te lo envolverá en una lista si no lo haces multi; si no pasa nada por eso, entonces 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Voy a hacer pruebas otra vez, pero recuerdo que la última vez que probé fallaba con @api.multi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parece que también funciona con api.multi
, así que lo dejo con eso. Esto puede ser que lo hayan corregido en v9 o a lo largo de la vida de la v8
|
||
<!--*** window action ******************************************* --> | ||
<!--*** window action ******************************************* --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are smart enough to read it without all the sparkles. 😋
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Esto es herencia del código original de hace mucho... pero se puede cambiar.
a0ddd3c
to
d9beb21
Compare
* Optimized wizard * Changed headers * Correct match of accounts with * * Menus organization * Tests
d9beb21
to
3fc7399
Compare