[13.0][MIG] Module l10n_br_account_payment_order

[mbcosta]

Migration module l10n_br_account_payment_order to v13, #725 .

Migração do l10n_br_account_payment_order,

• Está pendente "WIP" o caso de uso do Modo de Pagamento que possui um "Produto Taxa" o campo "payment_mode_id" possui um "onchange" que deveria verificar e apagar caso exista na Fatura uma linha desse "Produto Taxa" e evitar também a duplicação da linha caso se selecione mais de uma vez o Modo de Pagto. No teste desse caso acontece algo estranho que é a alteração do Modo de Pagamento o que causa erro( precisa ser melhor investigado ). Eu comentei esse teste por enquanto e deixei um TODO para ter um retorno da cobertura de testes e poder ver onde não estaria passando e permitir outras refatorações sem esse problema, já que é uma caso bem especifico.

• Inclui um menu especifico para o CNAB, parece ter mais sentido ter um menu para acessar o LOG e a Importação do Retorno do que deixar isso no menu de Clientes ou Fornecedores já que o CNAB possui os casos de Pagamento também, mas se houver divergência sobre isso o "commit" é bem especifico e pode ser removido.

• Na v13 pelo o que vi ao Cancelar uma Fatura as Linhas de Lançamentos Contábeis não são mais apagadas, é preciso confirmar se teria algum caso onde isso acontece, deixei um TODO no código, e aguardo a revisão dos outros desenvolvedores para nos próximos dias ver de remover do ROADMAP essa questão e os campos que foram sobre escritos aqui para permitir o caso de um Cancelamento de Fatura quando possui uma Ordem de Debito/Pagamento já Confirmada

• Acabei nessa migração entendendo melhor a alteração no modulo account e que aquele erro de "Lançamentos Desbalanceados" que eu escrevi nos outros PRs de migração da v13 como sendo um problema do l10n_br_account na verdade podem não ter relação é preciso criar as linhas pelo campo "invoice_line_ids" ou chamar o método "_move_autocomplete_invoice_lines_values()" que os campos são preenchidos, vou rever esses PRs a partir disso

cc @renatonlima @rvalyi @marcelsavegnago @mileo @felipemotter @netosjb

[felipemotter]

Eu e o @netosjb estudamos bastante o que ocorre com esses onchange do produto taxa. Tivemos algumas evoluções para evitar retirar algo. Mesmo que achamos que essa funcionalidade um pouco bizarra e sem sentido.

Mesmo não concordando com a funcionalidade, achamos algumas soluçoes para evitar o 'retrocesso'. Praticamente isso é um problema da 13 em si, não da localização. Parece que sempre que a fatura é duplicada, o modo de pagamento padrão do cliente é forçado. Praticamente o erro é evitado ao contornar um determinado método que gera um cópia da fatura.

Em todo caso, ainda não acabamos e nem sabemos se isso é necessário. Mas amanhã pretendemos analisar essa pr e talvez somar nossas ideias.

[mbcosta]

Erro no Travis é o já relatado "Alteração da biblioteca usada parar gerar os certificado fake de PyOpenSSL para Cryptography" por isso o testes do travis estão dependendo do #1777

[mbcosta]

@felipemotter

Sobre "Mesmo que achamos que essa funcionalidade um pouco bizarra e sem sentido.

Mesmo não concordando com a funcionalidade,"

Eu não tenho esse caso de uso, isso é uma implementação antiga que veio antes da integração na OCA, pelo o que entendo é algo para atender uma empresa que queira associar uma Valor a um Modo de Pagamento ex.: "Taxa a mais se o cliente Pagar com Boleto X", a necessidade dessa implementação pode ser debatida e vocês podem propor a remoção mas acredito que teremos que confirmar com o pessoal da KMEE( talvez tenha sido feito por eles, não sei dizer ) sobre isso e a real necessidade.

Sobre esse problema técnico do "onchange" pode ter alguma relação com isso https://github.com/OCA/OCB/blob/13.0/addons/account/models/account_move.py#L1025

e é bom lembrar que implementação do Modo de Pagamento é feita por um modulo especifico https://github.com/OCA/bank-payment/tree/13.0/account_payment_mode mas esse problema nos "onchange" precisa realmente ser entendido melhor porque isso pode causar problemas mais sérios nos outros modulos que herdam o "account.move"

[felipemotter]

@mbcosta Sim, concordo que tenha que ser em outro lugar e irei tomar essa iniciativa depois dessa migração. Mas estou falando agora porque foi nesse momento que surgiu os problemas. Na minha opinião por ser uma funcionalidade bem específica, com pouca utilização e até contra o previsto por lei (https://raphaelgfaria.jusbrasil.com.br/artigos/848816195/taxa-de-emissao-de-boleto-e-ilegal#:~:text=Da%20simples%20leitura%20dos%20artigos,39.) acredito ser apenas mais uma dificuldade na hora da migração.

Esse erro específico do produto taxa era causado por algumas mudanças na forma do odoo criar as linhas da fatura(daqui a pouco o @netosjb vai comentar mais sobre isso e propor correções) e no fim até ajudou a gente a entender melhor a lógica por trás das novas versões. Mas acho que mudar ele para um módulo a parte, devido aos motivos acima, facilitaria nas próximas migrações. O móculo l10n_br_account_payment_order é algo considero importante na localização e essa divisão ajudaria a focar na verdadeira 'responsabilidade' do módulo.

Estou falando isso aqui só para poder reportar as dificuldades encontradas como argumento de uma possível alteração futura.

--

Outra coisa que nos surgiu nesse processo de migração foram os dados utilizados nos testes. Hoje utilizamos os dados demos para os testes, mas isso não parece ser correto. O que vimos em outras localizações foram dados criados diretamente para esse fim. Isso evita alguns conflitos desnecessários e acelera a migração.

Exemplo: hoje para executar os testes precisamos ficar instalando módulos adicionais que não estão nas dependências(por serem apenas para os testes) e também resetando o banco de dados toda vez que precisamos executar os testes novamente.

Não alteramos nada em relação a isso para evitar grandes mudanças na PR de migração, mas é um assunto legal a ser abordado depois(nos propomos a fazer isso).

[antoniospneto]

@mbcosta

Sobre o problema do "Produto Taxa", acabei de abrir uma solicitação com a correção: akretion#181

A partir do Odoo 13.0 todas as linhas da faturas (invoice_line_ids) precisam estar sincronizadas com os lançamentos contábeis (line_ids) é praticamente em tempo real, por isso, após criar a linha da fatura com o "produto taxa" precisamos chamar todos os "onchange" necessários que vão fazer o recompute dessas informações.

Antes sem chamar esses métodos, o Odoo percebia que as informações estavam incompletas e antes de salvar o modelo(write() do orm) era chamado o método _move_autocomplete_invoice_lines_write era esse método que estava alterando o payment_mode indevidamente, para chamar todas as computações necessárias, o método criava uma copia da invoice, pegava os dados gerados nesse copia e então enviava para invoice original, porém existe um bug na 13.0 , a duplicação da invoice não era fiel, o payment_mode que ele tava pegando era substituído pelo payment_mode configurado no cadastro do parceiro. Não precisamos se preocupar muito com isso, da forma que fiz a correção esse método nem chega mais a ser executado, pois toda informação necessária já é computado antes. Esse bug da duplicação também não acontece mais na 14.0 já fiz o teste funcional e está ok.

[mbcosta]

@felipemotter

Opa se incluir o "Produto Taxa" é contra o previsto por lei e pelo o que está no "link" isso vem desde de 2008 eu vou procurar remover do código o mais breve possível, existe um senso de Responsabilidade nos módulos da OCA sim, abri um "issue" a respeito, estou considerando remover na v12 e após o "merge" refazer os "commits" aqui já com isso removido. Obrigado por reportarem e como escrevi no "issue" se souberem de outro caso onde o código esteja permitindo a pratica de crimes previstos em lei por favor reportem que acredito que será tratado de forma séria por todos desenvolvedores do projeto.

Sobre:

• "Outra coisa que nos surgiu nesse processo de migração foram os dados utilizados nos testes. Hoje utilizamos os dados demos para os testes, mas isso não parece ser correto. O que vimos em outras localizações foram dados criados diretamente para esse fim. Isso evita alguns conflitos desnecessários e acelera a migração."

Existe um passo-a-passo na implementação e na migração de casos complexos( eu costumo fazer em todos os casos ):

1 - Verificar a compatibilidade com o que já existe no Core/Odoo e nos repositórios da OCA
2 - Verificar Módulos dependentes, objetos e campos herdados e visões, deixar o modulo instalável
3 - Cadastros, os objetos e campos necessários para implementação,

Aqui entra os dados de demonstração tanto para avaliar isso junto com o usuário quanto na migração para validar na tela mesmo antes dos testes estarem passando, quer dizer é uma etapa para saber algo como "Os cadastros estão certos ? " na migração "O que mudou?" , é importante também ter uma Referencia sobre como deve ser um Cadastro tanto para implementar como uma forma de outros desenvolvedores consultarem em caso de dúvidas, e como vocês viram os testes precisam chamar os "onchange" manualmente( em alguns testes estão usando o "Form" que parece simular isso, seria bom confirmar ) mas eu penso que é preferível ter essa etapa.

Por exemplo nesse caso do problema com o "onchange" do Modo de Pagto pelo o que vi a alteração do Modo de Pagamento acontecia nos testes mas não na tela.

Questões de Usabilidade também precisam ser vistas basicamente "O que o usuário vê?", "Os campos/botões/abas estão bem posicionados? De forma clara? O preenchimento tem um fluxo?"

Lembrando que os testes no "Runbot" usam os dados de demonstração, quer dizer se for removido quando alguém quiser testar lá vai ter que cadastrar isso na tela.

Os dados de demonstração também ativam e rodam os testes.

Também usamos para Comparar um o Banco de Dados Ideal(dados de demonstração) com um Banco de Dados de um cliente, já que assim é possível identificar se um Erro/BUG é causado por algo fora do modulo, existem implementações que usam muitos módulos e separar isso é fundamental.

E existe algo especifico do CNAB que é por um tempo foi pensando que existiria algo como um Padrão CNAB 240, os Códigos de Envio e Retorno estavam dentro do código como constantes e o que eu acabei descobrindo durante a implementação foi simplesmente que Não Existe Padrão mesmo no CNAB 240( gastei horas fazendo e refazendo até chegar a essa constatação da falta de padrão ), portanto os dados de demonstração do modulo também servem para demonstrar essa falta de padrão de forma a não ter dúvidas e justificar o porque das escolhas técnicas e implementações dos métodos e campos foram feitos da forma que estão hoje e deixar claro para quem for desenvolver/dar manutenção/expandir o modulo essa questão da falta de padrão entre os CNAB, quer dizer o modulo precisa ser pensando considerando essa questão fundamental.

4 - Funcionalidade/Métodos que deverão ser implementados, aqui entram os testes para ter no modulo validado os Casos de Uso definidos junto as especificações do modulo e cliente
5 - Com os testes rodando é possível refatorar o código eliminando ou melhorando o que foi feito e devido aos testes sem receio de quebrar algo já testado/homologado
6 - Pre-commit, pep8 e README

Esse passo-a-passo é importante para poder separar e identificar um problema, em módulos simples isso pode parecer irrelevante mas em casos complexos ou como aqui onde vários desenvolvedores podem trabalhar em um mesmo modulo eu penso que o entendimento e a visualização do processo na tela usando os dados de demonstração são fundamentais para validar a Usabilidade do que foi feito.

• "Exemplo: hoje para executar os testes precisamos ficar instalando módulos adicionais que não estão nas dependências(por serem apenas para os testes) e também resetando o banco de dados toda vez que precisamos executar os testes novamente."

Estou instalando e testando assim:
$ createdb test
$ odoo -d test -i l10n_br_base,l10n_br_account_due_list,account_payment_order --stop-after-init --workers 0

Crio um Template para se necessário não precisar rodar o comando acima e criar um novo BD a partir daqui
$ createdb -T test payment_order_base
Se necessário
$ dropdb test
$ createdb -T payment_order_base test

Instalo o l10n_br_account_payment_order
$ odoo -d test -i l10n_br_account_payment_order --stop-after-init --workers 0
Para testar
$ odoo -d test -u l10n_br_account_payment_order --stop-after-init --workers 0 --test-enable

Rodei 3 vezes seguidas o último comando sem erros com esse PR, o que acredito que causava o problema que levou vocês a recriar o BD era no XML dos Dados de Demostração o noupdate="1" pelo o que entendi precisa ser noupdate="True" isso foi feito no "commit" que eu coloquei você como autor 54602a7 pelo o que vi resolve.

@netosjb

Parabéns pela investigação do problema vou usar como referencia e isso vai ser importante na migração dos outros módulos, eu até testei usando o método self._move_autocomplete_invoice_lines_values() para preencher e conseguir salvar a Fatura sem o erro de "Lançamentos Desbalanceados" mas você identificou onde acontecia, e sobre "porém existe um bug na 13.0 , a duplicação da invoice não era fiel, o payment_mode que ele tava pegando era substituído pelo payment_mode configurado no cadastro do parceiro" é por isso que pretendemos deixar tudo funcional mas dependendo do problema o foco vai ser resolver na v14, bom como vocês colocaram essa questão da ilegalidade de adicionar o "Produto Taxa" acredito que vai acontecer o que já escrevi acima que é buscar remover isso na v12 e depois refazer essa "branch" já sem isso, mas essa investigação já valeu pelo entendimento sobre as alterações na Fatura a partir da v13.

[felipemotter]

Mas minha observação não é contra os dados demos. Eles são muito úteis.

O que falo, é que os testes podem criar diretamente os dados que eles precisam. Assim garantimos que eles vão ser exatamente o que esperamos deles. Pois não tem a chance de outro módulo ter alterado eles. Vimos isso em várias localizações .

Dessa forma temos os dados demos e os dados para testes de forma separada e mais direta.

[antoniospneto]

@mbcosta sobre os dados demonstração não somos contra, achamos muito importante também, nossa sugestão não é remove-los, mas sim limitar o seu proposito para teste funcional e demonstração.
Para os casos de teste python talvez fosse melhor eles terem os seus próprios dados, isso reduziria a dependência não declarada com os outros módulos na hora de executar os testes, outra coisa que pode acontecer com os dados demos é ter algum conflito, caso um módulo altere os dados de demonstração e um outro módulo for usar os mesmos dados, isso pode levar a erros inesperados, até a ordem que for executado os módulos pode levar a um erro.

Sei que isso não é crucial no momento , mas é um ponto interessante para ser discutido.

Obrigado pelas dicas também, vou testar com o noupdate="True" :D

[mbcosta]

Depends / Depende
#1777

[mbcosta]

@felipemotter @netosjb sobre os dados de demonstração entendo a proposta e acredito que o debate e o dialogo são fundamentais na construção de uma boa ferramenta, algumas considerações:

• Sim eu concordo que tanto os testes quanto os dados de demonstração podem ser refatorados mas como foi escrito existe uma questão de prioridade, hoje a prioridade é usar os dados de demonstração e testes para migrar e refatorar o código, pequenas alterações podem ocorrer nos testes e dados de demo durante esse processo mas acredito que uma grande alteração será melhor se for feita após a migração na v14 até para ter uma referencia do antes e depois do percentual de cobertura dos testes.

• "O que falo, é que os testes podem criar diretamente os dados que eles precisam." "Dessa forma temos os dados demos e os dados para testes de forma separada e mais direta." "nossa sugestão não é remove-los, mas sim limitar o seu proposito para teste funcional e demonstração."

Existe uma questão que é evitar tanto nos dados quanto nos códigos a duplicidade, quer dizer nesse caso não é o ideal ter dados de demonstração e nos testes a recriação dos mesmos dados, então a pergunta a ser feita é quais dados de demonstração deveriam se tornar apenas testes?

Eu uso na tela os dados do Unicred, um caso que foge do padrão e serve de referencia ao problema de falta de padronização do CNAB, testo impressão de boleto, geração de arquivo de remessa e importação do arquivo de retorno tanto com e sem Liquidação( dependendo do caso são abertas diferentes telas ), valor recebido a maior e a menor do que a Fatura, os dados da Caixa Econômica para testar o caso de Pagamento Por Fora do CNAB porque possui um código de Alteração de Valor e a Fatura do Cheque que é o caso de Não CNAB os outros dados eu penso que são reais demonstrações das diferenças entre CNABs para os desenvolvedores e servem de apresentação aos clientes sobre a cobertura de pelo menos alguns dos principais CNABs usados.

Durante a implementação eu acreditava que só seria possível ter os testes rodando de forma completa no l10n_br_account_payment_order através do modulo que implementa a biblioteca a ser usada, no caso o l10n_br_account_payment_brcobranca, porém o Luís Mileo encontrou uma forma que foi incluindo Códigos CNAB Genéricos https://github.com/OCA/l10n-brazil/blob/12.0/l10n_br_account_payment_order/demo/mov_instruction_code.xml , Metodo de Pagamento https://github.com/OCA/l10n-brazil/blob/12.0/l10n_br_account_payment_order/demo/account_payment_method.xml#L7 e Faturas Genéricas https://github.com/OCA/l10n-brazil/blob/12.0/l10n_br_account_payment_order/demo/account_invoice.xml#L244 , seriam esses dados que vocês gostariam de transformar apenas em python? Pelo o que vi esses dados poderiam ser apenas testes python. Uma outra questão é que eu criei testes principalmente de Alterações de Dados do CNAB no l10n_br_account_payment_brcobranca que depois da inclusão dos dados genéricos podem estar sendo feitos em ambos os módulos, em duplicidade, o que é desnecessário e devem ser removidos.

• "Para os casos de teste python talvez fosse melhor eles terem os seus próprios dados, isso reduziria a dependência não declarada com os outros módulos na hora de executar os testes, " "até a ordem que for executado os módulos pode levar a um erro."

Isso de "dependência não declarada" desconheço qual seria? Por enquanto como escrevi existe a necessidade de instalar o l10n_br_base,l10n_br_account_due_list,account_payment_order antes, mas não vi erro de algum modulo que não consta na lista de dependências( isso é algo importante para o modulo e mesmo para o projeto como um todo devido a ideia de permitir diversas combinações entre os módulos, por exemplo umas das especificações do l10n_br_account_payment_order é não ter dependência direta do l10n_br_account, caso encontrem algum caso desse é bom reportar ), sobre a ordem de instalação do modulo é um erro que precisa ser corrigido, se quiserem podem abrir um "issue" sobre( ideal abrir na v14 após a migração, quem sabe resolvemos até lá ), existe um aberto nesse sentido porém do modulo l10n_br_sale_stock #1197 .

• "outra coisa que pode acontecer com os dados demos é ter algum conflito, caso um módulo altere os dados de demonstração e um outro módulo for usar os mesmos dados, isso pode levar a erros inesperados,"

Isso acontece, vai acontecer e até onde entendo faz parte da migração porque alguns dados de demonstração são localizados para manter um fluxo de processos com os dados que vem de outros módulos, existem vários exemplos disso mesmo na localização, alterar isso na minha opinião acabaria colocando sobre cada modulo uma carga excessiva de dados de demonstração já que cada modulo teria que ter seus próprios dados. A forma de depurar e resolver os erros e bem parecida com outros arquivos XML.

• "Vimos isso em várias localizações ."

Eu também uso outros repositórios da OCA como referencia e é sempre bom buscar aderência as melhores praticas, podemos ver de ter alguns testes criando dados como apontei acima, mas não concordo com a ideia de separar todos os testes dos dados de demonstração, se quiserem podem abrir um "issue" para um debate especifico sobre isso.

Mas vejam existem outras formas de reduzir a necessidade de ter dados de demonstração e testes, por exemplo poderíamos propor no BRCobranca a padronização do JSON do Arquivo de Retorno( campos de Datas, Valor com ou Sem Tarifa, e outras diferenças ) assim o tratamento dessas diferenças que hoje está sendo feito dentro da Localização https://github.com/OCA/l10n-brazil/blob/12.0/l10n_br_account_payment_brcobranca/parser/cnab_file_parser.py seria feito antes já no BRCobranca e com isso uma vez testado um arquivo todos os outros deverão vir com isso já resolvido.

Bom se tiverem problemas com a implementação do CNAB e BRCobranca ou com dados de demonstração desse ou de outros módulos podem me marcar, que dentro do que souber posso ver de orientar.

[mbcosta]

@netosjb @felipemotter obrigado pela revisão feita, como vimos tinha o problema com o pyopenssl nos testes aqui do Travis e que foi resolvido por vocês agora o PR está verde, valeu

[OCA-git-bot]

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]

boa @mbcosta !

[rvalyi]

/ocabot merge nobump

[OCA-git-bot]

This PR looks fantastic, let's merge it!
Prepared branch 13.0-ocabot-merge-pr-1792-by-rvalyi-bump-nobump, awaiting test results.

[rvalyi]

servidor do banco central dando erro. Vou tentar o merge de novo...

[rvalyi]

/ocabot merge nobump

[OCA-git-bot]

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

[OCA-git-bot]

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