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][MIG] payment_cielo: Migration to 14.0 #3423

Open
wants to merge 54 commits into
base: 14.0
Choose a base branch
from

Conversation

corredato
Copy link
Contributor

No description provided.

@corredato corredato force-pushed the 14.0-mig-payment_cielo branch 4 times, most recently from 42889b3 to 350994e Compare October 9, 2024 16:51
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.

pode aproveitar para passar os docstrings no estilo imperativo em vez da terceira pessoa, assim fica consistente com o resto da OCA

https://stackoverflow.com/a/72805661

@corredato corredato changed the title [14.0][MIG] paymet_cielo: Migration to 14.0 [14.0][MIG] payment_cielo: Migration to 14.0 Oct 10, 2024
@corredato corredato force-pushed the 14.0-mig-payment_cielo branch from 350994e to cdcb1d7 Compare October 10, 2024 18:01
@rvalyi
Copy link
Member

rvalyi commented Oct 10, 2024

@corredato vi que vc arrumou os docstrings ficou legal. Ficou pronto para revisar? (se sim, tem que clicar no botão "ready for review").

@corredato
Copy link
Contributor Author

@corredato vi que vc arrumou os docstrings ficou legal. Ficou pronto para revisar? (se sim, tem que clicar no botão "ready for review").

Valeu! Vou abrir pra revisão sim, tinha esquecido

@corredato corredato marked this pull request as ready for review October 10, 2024 22: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.

tel alguns warnings nos tests:

 2024-10-10 18:12:55,510 536 INFO odoo odoo.addons.payment_cielo.models.payment_transaction: cielo_s2s_void_transaction: Values received:
{'AuthorizationCode': '189477',
 'Links': [{'Href': 'https://apiquerysandbox.cieloecommerce.cielo.com.br/1/sales/42e731d0-8cef-4716-921e-62991244b788',
            'Method': 'GET',
            'Rel': 'self'}],
 'ProofOfSale': '938973',
 'ProviderReturnCode': '0',
 'ProviderReturnMessage': 'Operation Successful',
 'ReasonCode': 0,
 'ReasonMessage': 'Successful',
 'ReturnCode': '0',
 'ReturnMessage': 'Operation Successful',
 'Status': 10,
 'Tid': '1124020251914'} 
2024-10-10 18:12:55,511 536 WARNING odoo odoo.addons.payment.models.payment_acquirer: Processed tx with abnormal state (ref: test_ref_10_c, target state: cancel, previous state done, expected previous states: ('draft', 'authorized')) 
2024-10-10 18:12:55,517 536 INFO odoo odoo.addons.payment_cielo.tests.test_cielo: Starting CieloTest.test_20_cielo_s2s ... 
2024-10-10 18:12:55,533 536 INFO odoo odoo.addons.payment_cielo.models.payment_token: _cielo_tokenize: Sending values to URL https://apisandbox.cieloecommerce.cielo.com.br/1/card 
2024-10-10 18:12:57,419 536 WARNING odoo odoo.addons.payment_cielo.tests.test_cielo: Expecting value: line 1 column 1 (char 0) 
2024-10-10 18:12:57,419 536 WARNING odoo odoo.addons.payment_cielo.tests.test_cielo: local variable 'tx' referenced before assignment 

https://github.com/OCA/l10n-brazil/actions/runs/11279464506/job/31370252976?pr=3423#step:9:2338

melhor resolver já porque depois a gente sabe que ninguem vai se dar o trabalho de limpar mais. Sem falar que na v18 warnings vão fazer os testes falhar...

No método test_cielo pelo jeito se tiver uma falha no try a variável tx fica indefinida me parece, o restante não olhei muito mas seria bom resolver.

@corredato corredato force-pushed the 14.0-mig-payment_cielo branch from cdcb1d7 to 65c7037 Compare October 11, 2024 01:52
@corredato
Copy link
Contributor Author

tel alguns warnings nos tests:

 2024-10-10 18:12:55,510 536 INFO odoo odoo.addons.payment_cielo.models.payment_transaction: cielo_s2s_void_transaction: Values received:
{'AuthorizationCode': '189477',
 'Links': [{'Href': 'https://apiquerysandbox.cieloecommerce.cielo.com.br/1/sales/42e731d0-8cef-4716-921e-62991244b788',
            'Method': 'GET',
            'Rel': 'self'}],
 'ProofOfSale': '938973',
 'ProviderReturnCode': '0',
 'ProviderReturnMessage': 'Operation Successful',
 'ReasonCode': 0,
 'ReasonMessage': 'Successful',
 'ReturnCode': '0',
 'ReturnMessage': 'Operation Successful',
 'Status': 10,
 'Tid': '1124020251914'} 
2024-10-10 18:12:55,511 536 WARNING odoo odoo.addons.payment.models.payment_acquirer: Processed tx with abnormal state (ref: test_ref_10_c, target state: cancel, previous state done, expected previous states: ('draft', 'authorized')) 
2024-10-10 18:12:55,517 536 INFO odoo odoo.addons.payment_cielo.tests.test_cielo: Starting CieloTest.test_20_cielo_s2s ... 
2024-10-10 18:12:55,533 536 INFO odoo odoo.addons.payment_cielo.models.payment_token: _cielo_tokenize: Sending values to URL https://apisandbox.cieloecommerce.cielo.com.br/1/card 
2024-10-10 18:12:57,419 536 WARNING odoo odoo.addons.payment_cielo.tests.test_cielo: Expecting value: line 1 column 1 (char 0) 
2024-10-10 18:12:57,419 536 WARNING odoo odoo.addons.payment_cielo.tests.test_cielo: local variable 'tx' referenced before assignment 

https://github.com/OCA/l10n-brazil/actions/runs/11279464506/job/31370252976?pr=3423#step:9:2338

melhor resolver já porque depois a gente sabe que ninguem vai se dar o trabalho de limpar mais. Sem falar que na v18 warnings vão fazer os testes falhar...

No método test_cielo pelo jeito se tiver uma falha no try a variável tx fica indefinida me parece, o restante não olhei muito mas seria bom resolver.

Não sabia que na v18 o warning ia falhar os testes... Mas beleza, vou dar um jeito o mais rápido possivel

@corredato corredato force-pushed the 14.0-mig-payment_cielo branch 4 times, most recently from 5f44e05 to 46d7f07 Compare October 11, 2024 02:57
@rvalyi
Copy link
Member

rvalyi commented Oct 11, 2024

@corredato tb um dos problemas recorrentes que tem na KMEE e que ninguém ensinou vocês a usar o git direito e ai vcs costumam fazer uma caralhada de commits onde vocês deveriam fazer squashes e rebases. Ai qdo a pessoa olha os contribuidores do projeto, tem pessoas mega iniciantes que parecem contribuidores importantes apenas porque nunca aprenderam a usar o git direito, fica complicado isso na OCA...
Então se puder dar um squash para que cada um desses 2 grupos de commits ficam apenas 1 so ficaria melhor tb (vc pode dar git rebase -i e depois escolher s ou squash para que ter apenas 2 commits nesses ai). Sei que a culpa não é sua, mas nunca é tarde para ajeitar o que foi enfiado no projeto meio que de qualquer jeito (para vc ter noção qdo o Luis fez o merge desse modulo, ele logava o numero, nome e código de segurança dos cartões nos logs do Odoo...)

2024-10-11_02-58

@corredato
Copy link
Contributor Author

@corredato tb um dos problemas recorrentes que tem na KMEE e que ninguém ensinou vocês a usar o git direito e ai vcs costumam fazer uma caralhada de commits onde vocês deveriam fazer squashes e rebases. Ai qdo a pessoa olha os contribuidores do projeto, tem pessoas mega iniciantes que parecem contribuidores importantes apenas porque nunca aprenderam a usar o git direito, fica complicado isso na OCA... Então se puder dar um squash para que cada um desses 2 grupos de commits ficam apenas 1 so ficaria melhor tb (vc pode dar git rebase -i e depois escolher s ou squash para que ter apenas 2 commits nesses ai). Sei que a culpa não é sua, mas nunca é tarde para ajeitar o que foi enfiado no projeto meio que de qualquer jeito (para vc ter noção qdo o Luis fez o merge desse modulo, ele logava o numero, nome e código de segurança dos cartões nos logs do Odoo...)

![2024-10-11_02-58](https://private-user-images.githubusercontent.com/16926/375612776-1ae2849a-1c2f-4f24-922c-925e5c66280c.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mjg2MTY4NTUsIm5iZiI6MTcyODYxNjU1NSwicGF0aCI6Ii8xNjkyNi8zNzU2MTI3NzYtMWFlMjg0OWEtMWMyZi00ZjI0LTkyMmMtOTI1ZTVjNjYyODBjLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDEwMTElMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQxMDExVDAzMTU1NVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTNmMjQzMjA5MGRhMjlkOGRjYzkyOTk4YjQ5MDA2MDJlNmU4NWY1Yzg5M2NkZWM2NTUyMDQxYzFmNzYxZGQ0NDUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.C34DiiAImHhVHhSFN7AG8H-

Entendo a frustração, de fato sou iniciante quando se trata da OCA, muitas vezes erro ao seguir diretrizes e, em todas essas vezes, fui corrigido por pessoas mais experientes (principais menções: Luis, você e o @antoniospneto) que estavam enxergando algo que eu não vi e sou muito grato por isso. Mas, de certa forma, sou contra esse tipo de crítica genérica que você fez sobre a KMEE e sobre o Luis por três motivos: 1. Ela parece ter sido feita com o intuito de eu me sentir melhor com um erro que foi exclusivamente meu. 2. Talvez isso afaste aos poucos qualquer tipo de relação positiva que possamos ter daqui pra frente, ou pelo menos é assim que eu penso e 3. Não é como se você também tivesse começado sabendo de tudo e não tivesse errado, e errar não muda o fato de que você e o Luis tem contribuições importantíssimas no projeto.

Resumindo: A crítica com certeza vai ser mais bem-vinda se você não a fizer em uma PR pública.

Assim que eu decidir que é hora de terminar de me estressar com os testes desse módulo eu vou resolver esse problema no histórico de commits e parar de fuder com vcs.

@corredato corredato force-pushed the 14.0-mig-payment_cielo branch 2 times, most recently from 8e4e040 to 0a72b98 Compare October 11, 2024 04:37
@corredato corredato force-pushed the 14.0-mig-payment_cielo branch from 0a72b98 to 808d135 Compare October 11, 2024 04:48
@rvalyi
Copy link
Member

rvalyi commented Oct 11, 2024

@corredato o problema é que faz 10 anos que temos muitos problemas com as contribuições dirigidas pelo @mileo... A gente reclama e continua, entao é bem complicado... Por examplo recentemente o @mileo foi experimentar com a ferramenta oca-port e ferrou o historico das branches 15.0 e 16.0 do projeto, perdendo uns 500 commits com os PR que ele fez para migrar o módulo l10n_br_base, o módulo mais facil de migrar... A gente vai ter que corrigir com force push mais uma vez e prejudicar geral... ai depois ele vem aconselhar vc de usar o oca-port tb quem sabe de forma errada, ja que ele nem percebeu e aí complica né...

O problema é que a gente gasta muito tempo depois para arrumar depois (posso te passar uma lista absurda se tiver duvidas). Temos semanalmente contribuições das empresas Escodoo e Engenere por examplo que chegaram muitos anos depois e nunca tivemos esses problemas... Là eles tambem tem inicantes mas eles recebem uma mentoria correta.

Por ser iniciante eu entendo que vc nao percebe, mas até por isso eu falo para dessa vez as coisas começam de uma forma correta que não sugar a energia de quem mantem o projeto. Realmente nada pessoal, mas é apenas para você se ligar. Ninguém obriga iniantes a contribuir e no caso suas contribuições são bem vindas. Mas o importante é que as pessoas com pouca experiencia se tocam tambem (nem digo isso para vc saca)...

@corredato
Copy link
Contributor Author

@rvalyi O único warning que está aparecendo agora eu não tenho ideia de como resolver, tem algum palpite? Parece que o json da api não tá sendo decodificado

2024-10-11 04:59:13,008 535 WARNING odoo odoo.addons.payment_cielo.tests.test_cielo: Expecting value: line 1 column 1 (char 0)

https://github.com/OCA/l10n-brazil/actions/runs/11286501028/job/31390950411?pr=3423#step:9:2325

@corredato
Copy link
Contributor Author

@corredato o problema é que faz 10 anos que temos muitos problemas com as contribuições dirigidas pelo @mileo... A gente reclama e continua, entao é bem complicado... Por examplo recentemente o @mileo foi experimentar com a ferramenta oca-port e ferrou o historico das branches 15.0 e 16.0 do projeto, perdendo uns 500 commits com os PR que ele fez para migrar o módulo l10n_br_base, o módulo mais facil de migrar... A gente vai ter que corrigir com force push mais uma vez e prejudicar geral... ai depois ele vem aconselhar vc de usar o oca-port tb quem sabe de forma errada, ja que ele nem percebeu e aí complica né...

O problema é que a gente gasta muito tempo depois para arrumar depois (posso te passar uma lista absurda se tiver duvidas). Temos semanalmente contribuições das empresas Escodoo e Engenere por examplo que chegaram muitos anos depois e nunca tivemos esses problemas... Là eles tambem tem inicantes mas eles recebem uma mentoria correta.

Por ser iniciante eu entendo que vc nao percebe, mas até por isso eu falo para dessa vez as coisas começam de uma forma correta que não sugar a energia de quem mantem o projeto. Realmente nada pessoal, mas é apenas para você se ligar. Ninguém obriga iniantes a contribuir e no caso suas contribuições são bem vindas. Mas o importante é que as pessoas com pouca experiencia se tocam tambem (nem digo isso para vc saca)...

Claro, nada pessoal também, o foda é que quando você coloca essas questões em publico você me faz pagar o pato de estar em uma posição desconfortável, beleza, você tá me informando dos problemas e qualquer conhecimento é sempre bem-vindo, mas não sou eu que tenho poder pra resolver essa parada

@rvalyi
Copy link
Member

rvalyi commented Oct 11, 2024

Eu tb não sei de cara como resolver esse warning, vai ter que olhar mas com certeza deve indicar algo errado...

Sobre jogar o assunto no publico. Infelizmente passamos o toque inúmeras vezes pro @mileo então não adianta ele vir se fazer de bobo, de uma forma geral, já não é questão de não saber e do nosso lado a gente não sabe muito mais o que fazer fora explanar ou levar o assunto no board da OCA.

Eu comentei sobre os commits porque é sim mesmo um problema totalmente recorrente da KMEE mesmo (infelizmente). Veja aqui a lista dos "maiores" contribuidores do projeto:
2024-10-11_05-44

Esses 3 ex das KMEE, por exemplo, longe de ser contribuidores extremamente produtivos como pode parecer, é tudo iniciante que nunca usou o git direito bem naquele molde que eu comentei antes, basta ver o histórico para ver... Eu sou sim um dos "principais" contribuidores na OCA então eu conheço (fui co-criador da OCA e tenho direito de commits em todos repos por examplo). E posso afirmar: dos 240 repos da OCA, não tem um outro onde foi feito esse tipo de extrapolo e sempre vem da mesma pessoa que nunca quis se dar o trabalho de aprender as bases então infelizmente temos que explanar até que isso mude quem sabe. Vc talvez não sabe, mas depois fazemos backports e forward ports dos PRs entre as branches (idealmente) e esse lance de multiplicar os commit atrapalha demais, ou tb na hora de rastrear uma regressão.

E sobre o lance de corrigir os warning. Veja que quando esse modulo payment_cielo entrou na v12, aquela falha de segurança de logar os cartões de credito nos logs (#1562) ficou no ar um ano direito... Ou seja, bastava alguém usar uma vez em prod para se tocar da falha mas não aconteceu, enfiaram módulos que nem usavam obviamente (ou senão é muito pior).

Novamente entendo que vc não tem nada ver como isso e eu até acho que vc faz contribuições boas no caso. Mas do nosso lado temos que pelo menos dar o toque para que as coisas mudam...

@rvalyi
Copy link
Member

rvalyi commented Oct 20, 2024

Existe tb suspeita que o gateway de vocês com Pagseguro teve tb merge na v14 sem funcionar... Aqui teve um PR #2309 para tentar rssolver e que ainda não recebeu retorno nenhum de vocês...

cc @marcelsavegnago @mileo @antoniospneto @renatonlima

@marcelsavegnago
Copy link
Member

Existe tb suspeita que o gateway de vocês com Pagseguro teve tb merge na v14 sem funcionar... Aqui teve um PR #2309 para tentar rssolver e que aimda não recebeu retorno nenhum de vocês...

cc @marcelsavegnago @mileo @antoniospneto @renatonlima

de fato precisava de atenção, houveram mudanças no pagseguro já faz um bom tempo

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.

9 participants