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

[16.0][l10n_br_account_payment_order][l10n_br_account_payment_brcobranca] Oca port 14.0 to 16.0 2e18e4 #3537

Open
wants to merge 23 commits into
base: 16.0
Choose a base branch
from

Conversation

@OCA-git-bot
Copy link
Contributor

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

@rvalyi rvalyi changed the title [16.0][l10n_br_account_payment_order] Oca port l10n br account payment order 14.0 to 16.0 2e18e4 [16.0][l10n_br_account_payment_order][l10n_br_account_payment_brcobranca] Oca port 14.0 to 16.0 2e18e4 Dec 9, 2024
@rvalyi rvalyi force-pushed the oca-port-l10n_br_account_payment_order-14.0-to-16.0-2e18e4 branch 2 times, most recently from 406b4bc to 4d60a42 Compare December 9, 2024 21:21
@rvalyi rvalyi marked this pull request as draft December 9, 2024 21:40
@rvalyi rvalyi force-pushed the oca-port-l10n_br_account_payment_order-14.0-to-16.0-2e18e4 branch from 4d60a42 to 4b3e146 Compare December 11, 2024 03:47
@rvalyi rvalyi force-pushed the oca-port-l10n_br_account_payment_order-14.0-to-16.0-2e18e4 branch from 4b3e146 to 3a0ec98 Compare December 11, 2024 04:10
@mbcosta mbcosta force-pushed the oca-port-l10n_br_account_payment_order-14.0-to-16.0-2e18e4 branch 2 times, most recently from dfe95f8 to 0e11af7 Compare December 17, 2024 02:39
@antoniospneto
Copy link
Contributor

@rvalyi agora tá pronto para revisão?

@mbcosta
Copy link
Contributor

mbcosta commented Dec 17, 2024

Pessoal, @rvalyi @antoniospneto @marcelsavegnago , acabei subindo PR ontem tarde da noite e faltou explicar o que foi feito, segue:

  • Adaptei o os Scripts de Migração

O principal foi o referente a Separação da Configuração do CNAB do Modo de Pagamento onde usei parte do que foi refatorado pelo Antonio, incluindo ele como autor, mas modifiquei a parte onde eram apagados os Dados de Demonstração para apenas deixar False os campos Sequencias que causam erro, com isso é possível simular a Migração com esses Dados, porém para fazer esse Teste é necessário comentar a parte do código que avalia se vai ou não ser feita a migração

https://github.com/akretion/l10n-brazil/blob/oca-port-l10n_br_account_payment_order-14.0-to-16.0-2e18e4/l10n_br_account_payment_order/migrations/16.0.3.0.0/post-migration.py#L216

    #if is_cnab_config_already_exist:
    #    return

Dessa forma o script vai duplicar as Configurações CNAB mas isso permite confirmar a efetividade do Script de Migração, e para testar o caso onde os Dados já foram migrados basta simular uma nova versão por exemplo a 16.0.6.0.0 e copiar esse script.

Nos outros casos pareceu ser desnecessário adaptações e os scripts rodaram sem erros aparentes, fiz somente correções na numeração das Versões, mas pretendo rever para garantir essa questão.

  • É preciso adaptar a Visão das Configurações CNAB para incluir o colspan="2" quando necessário para mostrar os campos de forma correta 517b178

  • Acredito que houve um erro em parte do código onde deveria ser cnab_config_id estava somente cnab_config 938f16e

  • Refatorei o método que que verifica se a Sequencia Nosso Número ou a Sequencia CNAB está sendo usada porque ao tentar testar esses casos é possível ver que o código em determinada situações retornava sempre a primeira mensagem de erro, que é referente ao Nosso Número mesmo quando o problema era na Sequencia CNAB, um problema de lógica, pode ser visto na v14

https://github.com/OCA/l10n-brazil/blob/14.0/l10n_br_account_payment_order/models/l10n_br_cnab_config.py#L126

    def _check_sequences(self):
        for record in self:
            already_in_use = self.search(
                [
                    ("id", "!=", record.id),
                    "|",
                    ("own_number_sequence_id", "=", record.own_number_sequence_id.id),
                    ("cnab_sequence_id", "=", record.cnab_sequence_id.id),
                ],
                limit=1,
            )

            if already_in_use.own_number_sequence_id:
                raise ValidationError(
                    _(
                        "Sequence Own Number already in use by %(cnab_config)s!",
                        cnab_config=already_in_use.name,
                    )
                )

            if already_in_use.cnab_sequence_id:
                raise ValidationError(
                    _(
                        "Sequence CNAB Sequence already in use by %(cnab_config)s!",
                        cnab_config=already_in_use.name,
                    )
                )

Acredito que o objetivo era validar ao mesmo tempo se uma determinada Sequencia estava sendo usada por outra Configuração CNAB/Modo de Pagto tanto no campo da Sequencia Nosso Número quanto na Sequencia CNAB mas da forma que estava acabava retornando o erro no Nosso Número mesmo quando havia casos que não era, bom os testes agora verificam isso e da forma que está no PR parece atender essa necessidade, se acharem necessário posso ver de extrair, commit d589e90

  • Refatorei os Testes, apesar de muito volume é algo simples de ser explicado, ao invés de repetir os mesmos códigos diversas vezes para os diversos testes eu crie métodos que fazem isso, então basta olhar o arquivo l10n_br_account_payment_order/tests/test_base_class.py para ver esse novos métodos e no l10n_br_account_payment_brcobranca criei um novo arquivo 10n_br_account_payment_brcobranca/tests/common.py que herda o que existe no test_base_class.py e adiciona métodos especifico do l10n_br_account_payment_brcobranca assim reduzindo código repetido, posso ver de extrair isso, mas acredito ser desnecessário por ser apenas Testes e é possível avaliar pelo percentual de "Cobertura dos Testes/Code Coverage" e agora em caso de problemas basta ver esses métodos e não ver caso a caso como estava antes 273cc60 e9f0d43

  • Por fim atualizei o README, foi preciso alterar do rst para o md e também atualizei os links para a v16, pelo o que vi parece tudo certo, mas talvez algo tenha passado é bom uma Revisão

Com isso eu acredito que o status do PR pode ser alterado para Pronto para Revisão/Ready to Review, se tiver problemas é fácil de resolver via cherry-pick

@rvalyi rvalyi marked this pull request as ready for review December 17, 2024 19:46
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.

@mbcosta acho que houve um equivoco na alteração do README, pois está sendo alterado no módulo l10n_br_account

@mbcosta mbcosta force-pushed the oca-port-l10n_br_account_payment_order-14.0-to-16.0-2e18e4 branch from 0e11af7 to 94de4b6 Compare December 17, 2024 20:38
@mbcosta
Copy link
Contributor

mbcosta commented Dec 17, 2024

valeu @antoniospneto realmente teve esse problema, já corrigido

@mbcosta mbcosta force-pushed the oca-port-l10n_br_account_payment_order-14.0-to-16.0-2e18e4 branch from 94de4b6 to ca0f72f Compare December 18, 2024 15:22
@mbcosta
Copy link
Contributor

mbcosta commented Dec 18, 2024

Force-Push para:

  • Incluir no Script de migração do Código de Desconto a verificação se já houve migração
  • Na verificação da Sequencia é preciso desconsiderar o caso False para evitar o erro
/l10n_br_account_payment_order/models/l10n_br_cnab_config.py", line 138, in _check_sequence_already_in_use
    raise ValidationError(
odoo.exceptions.ValidationError: Sequence False already in use by Banco Bradesco - CNAB 400 (inbound)!

Como havia comentado é possível simular a migração com os Dados de Demonstração, mas para isso é preciso comentar as verificações se já houve migração em:

https://github.com/akretion/l10n-brazil/blob/oca-port-l10n_br_account_payment_order-14.0-to-16.0-2e18e4/l10n_br_account_payment_order/migrations/16.0.3.0.0/post-migration.py#L216

    #if is_cnab_config_already_exist:
    #    return

e em https://github.com/akretion/l10n-brazil/blob/oca-port-l10n_br_account_payment_order-14.0-to-16.0-2e18e4/l10n_br_account_payment_order/migrations/16.0.4.0.0/post-migration.py#L41

    #if is_already_migrated:
    #    return

A partir daí é possível validar a migração:

O Script vai duplicar as Configurações CNAB porém a que foi Migrada e que simula a migração vai ter os campos Sequencia Nosso Número e Sequencia CNAB a outra é a que foi criada pelos Dados de Demonstração

image

Caso que simula a Migração

image

image

image

image

image

Ligação com o Modo de Pagamento

image

Caso carregado pelos Dados de Demonstração não tem os campos Sequencias preenchidos que é o que causa erro

image

image

Migração do campo CNAB Processor que é feita no módulo l10n_br_account_payment_brcobranca

image

Migração do Campo Desconto

image

Aqui parece que houve algum problema no cherry-pick do 85b5d7f porque acabou não copiando a alteração na Visão, por isso o último commit

image

@rvalyi eu busquei não alterar o que havia sido feito antes, apenas removi o commit que comentava o teste, mas se achar melhor posso rever esse cherry-pick que faltou copiar a Visão do Desconto

Então apenas confirmando que os Scripts de Migração parecem estar funcionando como o esperado e que agora resta apenas avaliar se seria melhor rever algum cherry-picking

@rvalyi rvalyi requested a review from antoniospneto December 31, 2024 17:32
@rvalyi
Copy link
Member Author

rvalyi commented Dec 31, 2024

Não posso aprovar porque sou eu que criei o PR. Porem eu apenas resolvi os conflitos dos primeiros commits. Os commits são do @mbcosta mesmo e aprovo agora com essas ultimas correções que ele fez.

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

Successfully merging this pull request may close these issues.

6 participants