Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[14.0][IMP] l10n_br_account: cancel invoice on fiscal document denial #3272

Merged
merged 2 commits into from
Sep 23, 2024
Merged

[14.0][IMP] l10n_br_account: cancel invoice on fiscal document denial #3272

merged 2 commits into from
Sep 23, 2024

Conversation

DiegoParadeda
Copy link
Contributor

Adiciona cancelamento automático da fatura caso o documento fiscal seja denegado

@OCA-git-bot
Copy link
Contributor

Hi @renatonlima, @rvalyi,
some modules you are maintaining are being modified, check this out!

rvalyi
rvalyi requested changes Aug 15, 2024
View reviewed changes
l10n_br_account/models/fiscal_document.py Outdated Show resolved Hide resolved
@DiegoParadeda DiegoParadeda force-pushed the imp/inv-cancel-on-doc-deny branch from cfcb20f to 3300f88 Compare August 16, 2024 00:31
@DiegoParadeda DiegoParadeda force-pushed the imp/inv-cancel-on-doc-deny branch from 3300f88 to ca88bfa Compare August 16, 2024 16:39
@rvalyi
Copy link
Member

rvalyi commented Aug 21, 2024

eu queria uma mudança sim mas comento daqui pouco.

@rvalyi
Copy link
Member

rvalyi commented Aug 23, 2024

prometo explicar essa tarde.

@rvalyi
Copy link
Member

rvalyi commented Aug 28, 2024

nao esqueci desse PR. Ainda vou fazer uns ajustes pequenos nesse PR #3012 e explicar o que teria que fazer aqui.

@rvalyi
Copy link
Member

rvalyi commented Aug 28, 2024 •
edited
Loading

@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:
exec_after_SITUACAO_EDOC_DENEGADA (sem _) https://github.com/OCA/l10n-brazil/pull/3012/files#diff-f146a34ad7da33eb19b281e63a3d4dbe42724fb3c3e8c9eaa107c537f280b4a2R445
Assim evita que a localização vira um grande cocô onde tudo depende de tudo e ninguém não controla mais nada.

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

@rvalyi
Copy link
Member

rvalyi commented Sep 9, 2024

@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

@DiegoParadeda DiegoParadeda force-pushed the imp/inv-cancel-on-doc-deny branch from ca88bfa to e0a7a28 Compare September 13, 2024 19:05
rvalyi
rvalyi approved these changes Sep 13, 2024
View reviewed changes
Copy link
Member

@rvalyi rvalyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

marcelsavegnago
marcelsavegnago approved these changes Sep 13, 2024
View reviewed changes
Copy link
Member

@marcelsavegnago marcelsavegnago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@antoniospneto
Copy link
Contributor

@DiegoParadeda o que acha de fazer um pequeno teste unitário para esse caso?

@marcelsavegnago
Copy link
Member

@DiegoParadeda o que acha de fazer um pequeno teste unitário para esse caso?

boa... bora evitar regressões.

antoniospneto
antoniospneto approved these changes Sep 23, 2024
View reviewed changes
Copy link
Contributor

@antoniospneto antoniospneto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DiegoParadeda obrigado!

@antoniospneto
Copy link
Contributor

/ocabot merge patch

@rvalyi
Copy link
Member

rvalyi commented Sep 23, 2024

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-3272-by-rvalyi-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 294a6df into OCA:14.0 Sep 23, 2024
5 of 6 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 6f64e22. Thanks a lot for contributing to OCA. ❤️

@DiegoParadeda
Copy link
Contributor Author

@DiegoParadeda obrigado!

De nada, @antoniospneto

@antoniospneto antoniospneto mentioned this pull request Oct 14, 2024
Closed
This was referenced Oct 16, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marcelsavegnago marcelsavegnago marcelsavegnago approved these changes

@antoniospneto antoniospneto antoniospneto approved these changes

@rvalyi rvalyi rvalyi approved these changes

@mileo mileo Awaiting requested review from mileo

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@DiegoParadeda @OCA-git-bot @rvalyi @antoniospneto @marcelsavegnago @mileo