-
-
Notifications
You must be signed in to change notification settings - Fork 246
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
[14.0][IMP] l10n_br_account: cancel invoice on fiscal document denial #3272
Conversation
Hi @renatonlima, @rvalyi, |
cfcb20f
to
3300f88
Compare
3300f88
to
ca88bfa
Compare
eu queria uma mudança sim mas comento daqui pouco. |
prometo explicar essa tarde. |
nao esqueci desse PR. Ainda vou fazer uns ajustes pequenos nesse PR #3012 e explicar o que teria que fazer aqui. |
@DiegoParadeda entao o lance eh que tem que estancar essa sangria do modulo l10n_br_fiscal chegar na obesidade morbida... Já são mais de 20k linhas, diga-se alias muito por causa de adições do @mileo que poderiam ter sido feitas em módulos extras. Uma parte já limpamos quando extraiamos os módulos l10n_br_fiscal_dfe, l10n_br_fiscal_closing e l10n_br_fiscal_certificate no inicio da v14. Porem ta na hora de extrair ainda esse l10n_br_fiscal_edi pelo menos na v16. Ai vem a questão se não é melhor encarar isso na v14 ainda para não matar a sinergia entre a v14 e as versões mais novas. Sou dos que pensam que deixar a cagada para limpar depois vai custar ainda 10x mais do que limpar quanto antes... Nisso a ideia seria proceder com o merge de #3012 primeiro. E depois fazer um override desse novo hook: Pode ser debatido quais hooks teria ou não porem, o urgente eh estruturar melhor a localização e extrair este modulo l10n_br_fiscal_edi antes de impactar mais gente e mais módulos com essa mudança... cc @renatonlima @mbcosta @marcelsavegnago @antoniospneto @felipemotter @kaynnan @douglascstd |
@DiegoParadeda fizemos o merge de #3012 para não perder a sinergia com a v16, vc consegue dar um rebase e fazer o que eu sugeri no comentário antes? cc @mileo |
ca88bfa
to
e0a7a28
Compare
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.
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.
LGTM
@DiegoParadeda o que acha de fazer um pequeno teste unitário para esse caso? |
boa... bora evitar regressões. |
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.
@DiegoParadeda obrigado!
/ocabot merge patch |
/ocabot merge patch |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at 6f64e22. Thanks a lot for contributing to OCA. ❤️ |
De nada, @antoniospneto |
Adiciona cancelamento automático da fatura caso o documento fiscal seja denegado