-
-
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] l10n_es_account_invoice_sequence: Migration to 9.0 #418
[9.0][MIG] l10n_es_account_invoice_sequence: Migration to 9.0 #418
Conversation
pedrobaeza
commented
Dec 24, 2016
- Tests completos
- Todos los flujos cubiertos
- Arreglo de los diarios existentes en la instalación
e0c64be
to
2f100b9
Compare
return re | ||
# Include the invoice reference on the created journal item | ||
# This is done for displaying the number on the conciliation | ||
inv.move_id.ref = ( |
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.
@jalzaga con esto, ya no tendrás el problema de que no te aparece el nº de factura en account_followup o en el asistente de conciliació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.
Revisión de código, algunos comentarios no críticos.
'l10n_es.account_chart_template_assoc', | ||
'l10n_es.account_chart_template_pymes', | ||
'l10n_es.account_chart_template_full', | ||
] |
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.
Normalmente prefiero que estas cosas las devuelvan métodos @api.model
, por posibilitar sobrecargas. Podrías usar ormcache + un consejillo que te pongo más abajo para tener mejor rendimiento. De todas formas, no es crítico.
# © 2011 NaN Projectes de Programari Lliure, S.L. | ||
# © 2014 Ángel Moya (Domatix) | ||
# © 2014 Roberto Lizana (Trey) | ||
# © 2013-2016 Pedro M. Baeza |
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.
OCA/maintainer-tools#197, aquí y en otros lugares del PR.
for inv in self: | ||
if not inv.invoice_number: | ||
sequence = inv.journal_id.invoice_sequence_id | ||
if inv.type in ['out_refund', 'in_refund']: |
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.
Como solo son 2 elementos, tampoco es muy crítico, pero igual te interesa saberlo: http://stackoverflow.com/a/2831242/1468388
if not vals.get('company_id') or vals.get('sequence_id'): | ||
return super(AccountJournal, self).create(vals) | ||
spanish_charts = reduce( | ||
operator.add, [self.env.ref(x) for x in SPANISH_CHARTS], |
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.
Interesante método de generar un recordset 😉
Si en lugar de [...]
usas (...)
, tendrás mejor rendimiento. Tampoco parece muy crítico porque son creo que 5 loops o así...
journal = self.search([('company_id', '=', company.id)], limit=1) | ||
if journal: | ||
vals['sequence_id'] = journal.sequence_id.id | ||
vals['refund_sequence'] = False |
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.
Casi seguro que me estoy confundiendo, pero pregunto: ¿aquí estás hardcodeando que no se trate de una secuencia de facturas rectificativas?
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, se le dice que la secuencia de las rectificativas es diferente. El campo es: "Dedicated Refund Sequence"
https://github.com/OCA/OCB/blob/9.0/addons/account/models/account.py#L245
@yajo, mejoras realizadas |
5162d8b
to
c7390f2
Compare
@api.multi | ||
@tools.ormcache() | ||
def is_spanish_chart(self): | ||
return self[:1] in reduce( |
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.
Creo que return self < reduce(
sería más fiable.
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.
Cuidadín que reduce() está eliminado en python3.x
def is_spanish_chart(self): | ||
return self[:1] in reduce( | ||
operator.add, | ||
(self.env.ref(x) for x in self._get_spanish_charts_xml_ids()), |
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 think it would be better to have a self._get_spanish_charts
method that does this inside and returns a recordset, just a matter of taste I guess.
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.
Demasiados métodos para algo tan complicado. Para empezar, esto en la vida se va a extender, pero si acaso hiciera falta, es tan fácil como comprobar otra condición, y si no se da, devolver super.
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, me refería reemplazando el _get_spanish_charts_xml_ids
.
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.
Puf, da más o menos igual, la verdad...
Estoy revisando el error de Travis que no me daba en local. |
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.
Excepto mis 2 comentarios y que en las cabeceras pones 2016 cuando debería de ser 2017 lo veo perfecto.
👍
@@ -1,13 +1,14 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<openerp> | |||
<data noupdate="1"> | |||
<odoo> |
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 debería de ser odoo noupdate="1" ??
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.
He preferido que sea así, porque si no, alguien puede modificar la secuencia por ejemplo cambiándole la compañía asociada y que eso estropee la creación de un nuevo plan contable. De todas formas, ésta es solo la plantilla, así que siempre se puede modificar la secuencia en concreto una vez generado el plan.
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.
👍
@api.multi | ||
@tools.ormcache() | ||
def is_spanish_chart(self): | ||
return self[:1] in reduce( |
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.
Cuidadín que reduce() está eliminado en python3.x
66a5290
to
3cc45b8
Compare
* Full tests * All flows covered * Journal fix on install
3cc45b8
to
8fe865f
Compare