[14.0] l10n_br_base, l10n_br_account: Cadastro de chaves PIX no Parceiro. #2123
Conversation
|
Hi @rvalyi, @renatonlima, |
antoniospneto
force-pushed
the
14.0-add-partner-pix
branch
2 times, most recently
from
c2b4dd9
to
5900857
Compare
|
testes e dados demos incluído ✔️ |
|
ah esqueci de comentar, os commits da PR #2124 estão incluídos aqui, ao fazer o merge dele eu faço o rebase aqui pra eles sumirem. |
antoniospneto
requested review from
felipemotter and
marcos-mendez
and removed request for
felipemotter and
marcos-mendez
|
Eu acho tranquilo fazer o merge do #2124 porem para esse eu acho bom eu e o @renatonlima revisar com calma. Mas valeu pelo trabalho, vai servir isso ja temos certeza. |
There was a problem hiding this comment.
[APROVADO] Testes Funcionais, com uma sugestão de melhoria no final deste review.
Screenshots:
Seleção de n chaves:
Utilização de uma tabela com relação:
Validação de CPF/CNPJ e email:
SUGESTÃO:
Incluir um campo de seleção de qual é a chave Pix PADRÃO para o parceiro. Para que assim seja utilizado em uma execução de pagamento.
|
Obrigado pela revisão @douglascstd ! Sobre a sugestão, essa implementação já existe, funciona assim: a listagem ela é ordenável, vc pode arrastar pra cima ou pra baixo, a chave que estiver em primeiro será o preferencial. Vou incluir uma observação sobre isso no help do campo para deixar claro ;) |
|
Itens para considerar:
|
Esse é o X da questão que eu queria avaliar melhor. Talvez vale a pena no mesmo modulo, mas no passado a gente acabou dando mole demais para a enfiação de funcionalidades e por examplo no modulo fiscal ficou abusado e hoje ainda temos trabalho para limpar. Então temos que pensar com calma... |
|
Sobre por em um módulo separado:
Particularmente eu prefiro ter o modelo bem definido, outros meios de pagamento podem ter seu próprio modelo, pois as regras de negócio podem mudar bastante, por exemplo no PIX temos os tipos, validações e o uso bem especifico.
Eu pensei nessas informações como algo básico, assim como temos as contas bancárias no parceiro, podemos ter as chaves pix, mesmo que seja para uso manual, por exemplo um usuário pode consultar no cadastro quando precisar fazer um deposito, mesmo que seja direto no banco dele de forma manual. |
Show.. acho valido considerar como um módulo separado fica mais fácil porque de certa forma fica opcional.. mas vamos ver o que dizem os colegas. |
|
As vezes não é nem pela questão de ficar opcional, mas sim de deixar as camadas de código separadas, de forma que fica explícito o que depende de que em vez de ficar uma hidra gigantesca. Mas vou olhar melhor ainda logo que eu puder. |
|
Certo, obrigado pela revisão! aguardo o feedback, se precisar separar em um outro módulo faço sem problemas ;) |
|
|
||
| @api.model | ||
| def create(self, vals): | ||
| self.check_vals(vals) |
There was a problem hiding this comment.
Ao invés de chamar o check_vals nos métodos create e write você pode adicionar a notação @api.constrains no método check_vals
There was a problem hiding this comment.
@netosjb, você chegou a ver essa mudança? Daria para usar o api.constraints() no método check_vals e não sobre escrever os método create e write...
There was a problem hiding this comment.
que estranho, eu tinha respondido essa questão, mas meu comentário sumiu.
Sim, eu tentei fazer com a notação, porém no meu uso não deu certo, é que além de fazer a validação eu faço a normalização dos campos que altera o valor do mesmo. pelo fato do valor está sendo alterado, com o api.constraints() o método entra em um loop infinito, fica chamando de forma recursiva até atingir o maximo e estourar uma exception.
|
@netosjb, @rvalyi e @marcelsavegnago, Eu não vejo problema nas informações de pix ficarem no l10n_br_base porque no módulo base do core do Odoo já existe informações bancárias como a lista de bancos (res.bank) e das contas bancárias (res.partner.bank). Só se justificaria criar outro módulo se nós não quiséssemos deixar a dependência externa do phonenumbers e email_validator" no l10n_br_base, que na minha opinião não vejo problema já que se tratam de dependências de baixa complexidade. Sobre a criação de um módulo mais genérico a nível da OCA como account_instant_payment, é válido pensar no caso a médio prazo, porque pesquisando rapidamente sobre pagamento instantâneo em outros países, eu acredito que deveríamos observar se vai ser implementado algo pelo banco central europeu (que provavelmente seria compatível com os países da união europeia), mas como eu falei no inicio deveríamos pensar nisso a medio prazo talvez fazer esse trabalho na 16.0 e na 14.0 implementar algo na nível da localização mesmo. |
|
@netosjb uma precaução: vc não iria se opor caso a gente for propor de mudar a licença do modulo l10n_br_base para LGPL-3 no futuro como explicamos aqui #1374 ? Seria bom opinar aqui ou isso poderia ser sim um motivo de deixar separado. Feito essa observação eu ainda não olhei bem o código mas eu pretendo fazer logo... |
|
Pelo que pude verificar o protótipo do mileo visa a implementação para receber pagamentos via pix, qr code, pix conbranca, integração com o pagseguro, Que aí realmente nesse caso seria melhor um módulo separado. A implementação que estou propondo nesta pr é apenas para armazenar as informações de chaves pix nos cadastros dos parceiros, com as validações devidas. @mileo ficarei grato pela revisão. |
|
sim @netosjb foi assim que entendemos tb na Akretion por isso vemos essa PR de forma positiva, mas valeu a pena vc ter explicitado. |
|
Dei uma olhada rápida não testei. Vocês cogitaram a possibilidade de implementar dentro do modelo res.partner.bank e não implementar o modelo res.partner.pix? Isso facilitaria a integração com outros módulos que usam o campo conta bancária, obviamente os dados como banco, agencia e outros ficariam em branco. Mas por exemplo na integração com o account_payment_order funcionaria sem muitos problemas. @netosjb |
antoniospneto
force-pushed
the
14.0-add-partner-pix
branch
from
52e3ca6
to
f4642b2
Compare
Pensamos nessa possibilidade, mas no fim não ajuda muito, com os objetos separado fica melhor de fazer os tratamentos do fluxo e as validações, além que na visão fica um pouco estranho uma conta bancária sem banco e sem numero. Eu já tenho o tratamento quase pronto para lidar com o PIX no account_payment_order, pretendo submeter uma PR logo após a aprovação desta. |
Outro detalhe, a chave pix pode ser referenciada dentro da conta bancária, porém é opcional, qualquer coisa é só tornar essa relação obrigatória. |
|
E vale lembrar que uma das opções de pix é o uso da conta/agencia do destinatário |
|
@netosjb @felipemotter por mim esta aprovado, mas fiz um PR para incluir o @netosjb nos contribuidores do modulo Engenere#11 . Logo apos o merge aprovo aqui. |
|
/ocabot merge major |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
(os testes falharam sem motivo por cause do JS nos testes Pagseguro, aqueles bugs inconsistentes, nada de blocar o merge) |
|
Congratulations, your PR was merged at 9c5b38c. Thanks a lot for contributing to OCA. ❤️ |
|
Pessoal tem um detalhe que parece não ter sido visto é que nos novos arquivos incluídos está faltando o cabeçalho de licença e o autor, acredito que isso é um padrão e consta na maioria dos arquivos python, exceção os init.py, em alguns casos até mesmo nos arquivos XML, por isso @netosjb minha opinião é que isso deve ser corrigido nos seguintes arquivos: https://github.com/OCA/l10n-brazil/blob/14.0/l10n_br_base/models/res_partner_pix.py#L1 Um exemplo com o formato pode ser visto aqui https://github.com/OCA/l10n-brazil/blob/14.0/l10n_br_account_nfe/models/document_workflow.py#L1 Copyright (C) 2022-Today - Engenere (https://engenere.one). |
|
opa! vou fazer a correção, obrigado @mbcosta |
Feito #2148 |
| @@ -176,3 +188,13 @@ def _address_fields(self): | |||
| @api.onchange("city_id") | |||
| def _onchange_city_id(self): | |||
| self.city = self.city_id.name | |||
|
|
|||
| def _compute_show_l10n_br(self): | |||
There was a problem hiding this comment.
@antoniospneto Estou com dúvida sobre este método já que a principio por default os parceiros não tem um company_id definido. Talvez possamos incrementar testando se o pais do parceiro é diferente de BR. Fora isso, acredita que seja válido considerar a empresa selecionada no momento também?
There was a problem hiding this comment.
Realmente, a validação que fiz na época não é uma boa.
Acho que o código do país do parceiro vai ser melhor 👍🏻
Permite o cadastro de chaves PIX no Parceiro.
O cadastro fica dentro do cadastro do Parceiro, na aba invoicing a baixo do cadastro das contas bancárias.
As chaves são tratadas e validadas conforme o seu tipo.