-
-
Notifications
You must be signed in to change notification settings - Fork 554
[16.0][REF] l10n_es_aeat_mod190: Multiple fixes #3828
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
Conversation
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.
También tengo dudas del rendimiento de esto. Si tantas veces hay que hacer ese filtro, debería hacerse una vez en un solo sitio. Tal vez se puede tener un calculado a nivel de account.move
para ello.
): | ||
for item in self: | ||
if item.discapacidad and item.discapacidad != "0": | ||
item.percepciones_dinerarias = 0.0 |
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.
En lugar de esto, se puede hacer al principio de todo un self.percepciones_dinerarias = 0
tax_lines = item.report_id.tax_line_ids.filtered( | ||
lambda x: x.field_number in (11, 15) and x.res_id == item.report_id.id | ||
) | ||
value = 0.0 | ||
for move_line in tax_lines.move_line_ids: | ||
for move_line in tax_lines.move_line_ids.filtered(item._check_lines): |
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 debería llevar lambda
, y además estar en el objeto donde self
sea el elemento.
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.
En todos lados se usa el mismo filtro que encima es grande. Lo he pasado al final
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.
Sí, pero yo te digo de la forma en la que se debería implementar. Mira también lo del tema de rendimiento.
@etobella puedes hacer rebase y resolver el conflicto surgido tras fusionar #3884? Puedes también por favor revisar mis comentarios? @EmilioPascual, qué opinas sobre este PR? |
7ac7bb7
to
f67478c
Compare
A ver, desconozco si esto tendrá peor rendimiento o no, ya que no soy un experto en ello, pero si ponemos lambda tendremos: .filtered(
lambda line: line.partner_id == item.partner_id
and (
(line.aeat_perception_key_id or line.partner_id.aeat_perception_key_id)
== item.aeat_perception_key_id
)
and (
(line.aeat_perception_subkey_id)
or (
not line.aeat_perception_key_id
and line.partner_id.aeat_perception_subkey_id
)
or item.env["l10n.es.aeat.report.perception.subkey"]
)
== item.aeat_perception_subkey_id
) Cuando yo sugiero def _check_lines(self, line):
return (
line.partner_id == self.partner_id
and (
(line.aeat_perception_key_id or line.partner_id.aeat_perception_key_id)
== self.aeat_perception_key_id
)
and (
(line.aeat_perception_subkey_id)
or (
not line.aeat_perception_key_id
and line.partner_id.aeat_perception_subkey_id
)
or self.env["l10n.es.aeat.report.perception.subkey"]
)
== self.aeat_perception_subkey_id
) y usaremos Desconozco si a nivel de rendimiento será más rápido uno u lo otro, pero creo que debería ser negligible la diferencia. |
Si se va a cambiar la lógica de los compute de los campos sin incapacidad creo que se debería implementar la misma lógica para los compute de los campos con incapacidad. |
Perdonad que lo pregunte así, pero lo pregunto desde el desconocimiento técnico. ¿Por qué mejor con Desde mi punto de vista, no debería haber diferencias a nivel de velocidad y mi propuesta nos da mejoras a nivel de herencia y seguimiento (además de que son menos líneas y luego es menos probable equivocarse en una modificación futura). Por ejemplo, en v17, en estas lambdas , pre-commit nos pedirá que usemos |
@EmilioPascual En su dia evalué el PR que propones y la solución no es del todo correcta, ya que no separa por claves de percepción... |
Compute for all lines, otherwise, the tax is not correctly assigned.
f8f4441
to
7584f40
Compare
Bueno, lo he revisado y para optimizar de verdad lo he passado a 6 queries con read_group. Lo he probado en local y me ha ido bien, pero estaria bien probarlo en entornos con más datos. Además, he descubierto que la parte de |
and line.id in line.move_id.invoice_line_ids.ids | ||
and line.move_id.aeat_perception_key_id | ||
): | ||
if line.move_id.is_invoice() and line.move_id.aeat_perception_key_id: |
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.
Las secciones y notas se rellenarán con un clave de percepción, lo que no es correcto. No hay que quitar la comprobación de display_type
.
for item in self.filtered(lambda x: x.discapacidad and x.discapacidad != "0"): | ||
tax_lines = item.report_id.tax_line_ids.filtered( | ||
lambda x: x.field_number in (11, 15) and x.res_id == item.report_id.id | ||
ingresos_a_cuenta_efectuados = sum( |
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.
Aprovecha para renombrar las variables intermedias a inglés y si es posible, más sintentizado.
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.
/ocabot merge minor
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at c0dac78. Thanks a lot for contributing to OCA. ❤️ |
Gracias a todos, está dando guerra el 190 este año. Vamos a ir a mejorarlo o lo vamos a intentar para cubrir más casos. |
Sin este cambio, se mezclan todos los apuntes en todos los registros individuales. Con este cambio se ve todo. Además, había un compute que no estaba pasando correctamente los datos a los impuestos. Se ha modificado.
@victoralmau @pedrobaeza