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.

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][FIX] l10n_br_account: account move on fiscal wiz #3090

Merged
merged 1 commit into from
May 24, 2024

Conversation

felipemotter
Copy link
Contributor

Com a entreda do PR #2761 , o documento fiscal passou a ser exibido ao consultar os detalhes fiscais, com isso a logica que existe no account não é mais chamada (por que ele verifica se o modelo ativo é o account.move) e por isso o move_id do wizard não é populado. Com isso, ao cancelar um documento, criar uma carta de cerreção ou invalidar uma numeração, a logica no move_id não é realizada.

Exemplo: se você for uma nfe confirmada e cancela-la (o governo permite em até 24h) a nfe estaria cancelada mas a fatura não, e o pior, devido ao status cancelado do documento, é impossível cancelar a fatura na visão dela.

Como o move_id não é algo que acho que seja comum para todos os wizards e é uma informação nem sempre necessária, decidi mudar um pouco a lógica para não ficar repetindo lógica e também só chamar os move_id quando necessário.

Está em validação. Provável que vou por outro commit para limpar um pouco mais de código morto.

@OCA-git-bot
Copy link
Contributor

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

@felipemotter felipemotter changed the title [FIX] l10n_br_account: account move on fiscal wiz [14.0][FIX] l10n_br_account: account move on fiscal wiz May 21, 2024
@felipemotter
Copy link
Contributor Author

Foquei a PR apenas na correção dos wizards... Acredito que o repactor com limpeza do código é um assunto delicado e precisa ser mais discutido.

@rvalyi
Copy link
Member

rvalyi commented May 23, 2024

Foquei a PR apenas na correção dos wizards...

Mott'er Foquer!

desculpa a piada precisava sair.
:-D

@felipemotter
Copy link
Contributor Author

Até que demorou pra sair essa piada kkkkk

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. Não testei :)

@marcelsavegnago
Copy link
Member

@rvalyi róla ?

rvalyi
rvalyi previously requested changes May 24, 2024
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.

no geral seu PR é legal, mas sera se precisa desse seu move_message_post como eu comentei?

l10n_br_account/models/fiscal_document.py Outdated Show resolved Hide resolved
Copy link
Member

@mileo mileo left a comment

Choose a reason for hiding this comment

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

Podemos ter a possibilidade também de somente um documento ser cancelado e os outros mantidos?

Mas creio que dessa forma teríamos que "zerar" o item relacionado da invoice ou pensar em alguma outra estratégia.

Isso com certeza aumenta o escopo do PR, mas se for o caso seria legal criar um issue pra resolvermos isso depois.

@felipemotter
Copy link
Contributor Author

felipemotter commented May 24, 2024

Podemos ter a possibilidade também de somente um documento ser cancelado e os outros mantidos?

Mas creio que dessa forma teríamos que "zerar" o item relacionado da invoice ou pensar em alguma outra estratégia.

Isso com certeza aumenta o escopo do PR, mas se for o caso seria legal criar um issue pra resolvermos isso depois.

Teria que ver a melhor forma de fazer isso, mas acredito que como você falou, valeria uma Issue para discutir melhor o assunto.

@rvalyi eu fiz a modificação que você solicitou.

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.

valeu @felipemotter ficou top. Eh que ja ta va pensando em esses wizards qdo fiz esse metodo... Foi mal a regressão com o cancel.

@rvalyi
Copy link
Member

rvalyi commented May 24, 2024

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 14.0-ocabot-merge-pr-3090-by-rvalyi-bump-minor, awaiting test results.

@rvalyi
Copy link
Member

rvalyi commented May 24, 2024

(nao precisa atualizar o banco mas botei merge minor para sinalizar que me parece uma mudança um pouco maior do que patch ja que remove arquivos que talvez alguem poderia herdar numa customizão).

@OCA-git-bot OCA-git-bot merged commit 3e64fe1 into OCA:14.0 May 24, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants