[10.0] [MIG] account_balance_reporting/l10n_es_account_balance_report: Migration to 10.0 #417
Conversation
| ('name', operator, name), | ||
| ] + args, | ||
| limit=limit | ||
| ).name_get() |
There was a problem hiding this comment.
Creo que sería más normal hacer:
args += [...]
return super(...).name_search(name, args, operator, limit)
There was a problem hiding this comment.
No, porque entonces no aplicaría el OR, si no un AND
There was a problem hiding this comment.
Porque en args sólo vienen los AND extra, pero la búsqueda principal por el campo name no va ahí, y por tanto, no se puede hacer medio OR.
There was a problem hiding this comment.
¿Hmmm podrías comprobarlo? Según dice la documentación, y según veo en la implementación, args no es más que un dominio adicional a la búsqueda por nombre. No tiene ninguna limitación salvo lo habitual en los dominios, y el OR que tú estás poniendo está dentro de un AND en realidad.
Vamos, que name_search hace exactamente lo mismo que estás haciendo aquí, por eso mejor DRY y llama a super().
There was a problem hiding this comment.
Vale, ahora entiendo lo que dices. 😉
Sin embargo, si no llamas a super(), ¿no se rompería la cadena de herencia? Me refiero a que con 2 addons que sobrecarguen este método, podría pasar que uno no obtuviera el resultado esperado porque normalmente siempre debería llamarse a super()...
¿Se te ocurre alguna forma de evitar eso?
There was a problem hiding this comment.
Sí, pero en este caso no es crítico, ya que el módulo define el modelo. Sólo perderíamos las posibles modificaciones globales, pero tampoco es problema, ya que precisamente estamos sobreescribiendo ese comportamiento para obtener éste en concreto. A ver, siempre podemos llamar al super y luego añadir resultados, pero con el parámetro limit, se hace más complicado. No sé, ¿tú qué prefieres?: ¿el muy improbable error de comportamiento o no heredabilidad?
There was a problem hiding this comment.
Buff, difícil decidirse 😁
A ver, se me ocurre una opción. Podemos usar un operador falso y luego reemplazarlo.
return super(AccountBalanceReportingLine, self.with_context(patched_operator=operator)).name_search(..., operator="fake")
Luego:
def _search(self, args...):
patched_operator = self.env.context.get("patched_operator")
if patched_operator:
for arg in args:
if arg[1] == "fake":
arg[1] = patched_operator
return super(...)
Es código conceptual, eh; habría que tener presente la inmutabilidad de las tuplas, pero quizás con este bicho raro se arreglaría todo... ¿Cómo lo ves?
There was a problem hiding this comment.
Ya tengo la solución perfecta: normalizar el dominio y así, se puede incluir sin problemas el operador '|' al principio. Lo implemento aquí.
There was a problem hiding this comment.
Bueno, al final la implementación no sale tan limpia como si se hace en el search (en el que todo el domino va en args - Ejemplo: https://github.com/OCA/l10n-spain/pull/410/files#diff-e9a3089cf23a87b0b9820f3fa06ec8d3R18), así que la he dejado dependiente de la implementación actual, y si cambia, tenemos un test que lo delatará...
| ('name', operator, name), | ||
| ] + args, | ||
| limit=limit | ||
| ).name_get() |
pedrobaeza
force-pushed
the
10.0-l10n_es_account_balance_report
branch
from
11873df
to
c560ad0
Compare
* Optimized wizard * Changed headers * Correct match of accounts with * * Menus organization * Tests
pedrobaeza
force-pushed
the
10.0-l10n_es_account_balance_report
branch
from
c560ad0
to
3bb3302
Compare
|
Mergeando. Cuando #452 se complete, haré cherry-pick del cambio. |
No hay apenas cambios. El commit relevante es 3bb3302
cc @Tecnativa @yajo @cubells @rafaelbn