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][IMP] l10n_br_fiscal_edi: add _document_qrcode() on _exec_before_SITUACAO_EDOC_A_ENVIAR method #3508

Conversation

marcelsavegnago
Copy link
Member

@marcelsavegnago marcelsavegnago commented Nov 26, 2024

Incluindo no workflow a chamada ao metodo generico para geração do qrcode.

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.

parece OK, mas me parece que a montagem do qrcode da NFCe deveria ser unificado com esse API. ver https://github.com/OCA/l10n-brazil/blob/14.0/l10n_br_nfe/models/document.py#L1227
Tou errado?

cc @antoniospneto @DiegoParadeda @mileo

@marcelsavegnago
Copy link
Member Author

parece OK, mas me parece que a montagem do qrcode da NFCe deveria ser unificado com esse API. ver https://github.com/OCA/l10n-brazil/blob/14.0/l10n_br_nfe/models/document.py#L1227 Tou errado?

cc @antoniospneto @DiegoParadeda @mileo

Sim.. estava pensando nisso agora pouco.. vou ajustar nesta PR para o NFCe usar este document_qrcode

@marcelsavegnago
Copy link
Member Author

ping @rvalyi b294aa9

@marcelsavegnago marcelsavegnago force-pushed the l10n_br_fiscal_edi-imp-add-documnet_qrcode-to-workflow branch from b294aa9 to 69115ec Compare December 2, 2024 15:13
@marcelsavegnago marcelsavegnago force-pushed the l10n_br_fiscal_edi-imp-add-documnet_qrcode-to-workflow branch from 69115ec to 537e46a Compare December 3, 2024 14:49
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.

eu posso estar enganado, mas me parece que seria melhor detonar o método _prepare_nfce_send e la no método _eletronic_document_send chamaria direitamente o _prepare_payments_for_nfce ficando assim:


    def _eletronic_document_send(self):
        super()._eletronic_document_send()
        for record in self.filtered(filter_processador_edoc_nfe):
            if record.xml_error_message:
                return  # Skip
            if record.state_edoc not in ["enviada", "a_enviar"]:
                return  # Skip
            if record.document_type == MODELO_FISCAL_NFCE:
                record._prepare_payments_for_nfce()
            if record.state_edoc == "enviada":
                record._nfe_consult_receipt()
            if record.state_edoc == "a_enviar":
                record._nfe_send_for_authorization()

Ou então conservando o método _prepare_nfce_send e botando o código do método _prepare_payments_for_nfce dentro dele.

Não sei dizer qual seria o melhor, mas eu queria apenas insistir que ter um método que fica apenas de 1 linha para chamar outro método é um tal de "leaky abstraction" que temos que evitar. Por isso inclusive que não é legal deixar iniciante mexer no código da OCA porque depois os API ficam tudo cagados de dar medo e tem que ficar mudando anos passando imagem de que a OCA não tem código estável. Sem falar que seria melhor "electronic_..." do que "eletronic_..." (em ingles) mas isso pode até ser tratado depois...

@marcelsavegnago marcelsavegnago force-pushed the l10n_br_fiscal_edi-imp-add-documnet_qrcode-to-workflow branch 5 times, most recently from 0c87bf1 to dd0e930 Compare December 3, 2024 20:40
@marcelsavegnago marcelsavegnago force-pushed the l10n_br_fiscal_edi-imp-add-documnet_qrcode-to-workflow branch from d516b7e to b83eb7a Compare December 4, 2024 12:45
@marcelsavegnago
Copy link
Member Author

Pessoal, estou separando o ajuste no l10n_br_fiscal_edi e o ajuste no modulo l10n_br_nfe. No caso da alteracao da geracao do qrcode do NFCE cai em um caso que nao tenho muito conhecimento para saber se esta certo ou nao. Aparantemente no update do documento para um documento de contingencia, ocorre alguma duplicacao.. nao sei se eh soh no teste mas seria melhor alguem da Kmee avaliar. Esta alteracao joguei na PR #3522 ..

cc @mileo @rvalyi @antoniospneto

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@rvalyi
Copy link
Member

rvalyi commented Dec 4, 2024

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 14.0-ocabot-merge-pr-3508-by-rvalyi-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 1963949 into OCA:14.0 Dec 4, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 85e3e82. 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.

4 participants