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

Add agencia DV to bank account payload #270

Closed

Conversation

vagrantsn
Copy link

@vagrantsn vagrantsn commented Jun 6, 2018

Description

When creating a bank account instance like this:

$bankAccount = new \PagarMe\Sdk\BankAccount\BankAccount([
    "bank_code" => "55",
    "agencia" => "1111",
    "agencia_dv" => "9",
    "conta" => "54283",
    "type" => "conta_corrente",
    "conta_dv" => "3",
    "document_number" => "10833324063",
    "legal_name" => "Saitama"
]);

No request is made to Pagar.me API (does not generate an id) thus causing the agencia_dv property to use its default NULL value when used in a Recipient creation/update request. This PR adds the parameter in order to solve this problem.

Tests

  • Unit
  • Coverage
  • Integration

@murilohns
Copy link
Contributor

Se eu enviar agencia_dv = null, pode ocorrer algum erro?

@vagrantsn
Copy link
Author

@murilohns Não há problemas, dado que hoje o valor default é nulo (se o bankAccount for uma instância direta e não criado pelo método create de BankAccount) pela propriedade não ser informada:
https://github.com/pagarme/pagarme-php/blob/V3/lib/Recipient/Request/RecipientCreate.php#L112

Testei para ambos os métodos de criação e atualização.

@devdrops
Copy link
Contributor

Fala @vagnervst, tudo certo? Como estamos com essa issue? Ainda pretende seguir com ela? Precisa de review?

@vagrantsn
Copy link
Author

Fala @devdrops! Não mexi mais nela mas pretendo seguir com ela sim. É um fix bem simples, então preciso somente que mais alguém teste e deixe o review =)

Copy link
Contributor

@devdrops devdrops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Como o @murilohns disse:

Se eu enviar agencia_dv = null, pode ocorrer algum erro?

As alterações passam a inserir o campo, seja null ou não, mas os testes somente cobrem casos quando o campo tem valor diferente de null. Acho que seria bacana adicionar testes que inserem null na request, assim podemos ver ambos os cenários. O que acha @vagnervst?

@vagrantsn
Copy link
Author

@devdrops Feito!

Copy link
Contributor

@devdrops devdrops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fala @vagnervst! Ainda pretende seguir com este PR? Se quiser, é preciso atualizar com V3 antes disso, ok?

@vagrantsn vagrantsn force-pushed the fix/add-recipient-bank-account-data branch from f55f7b7 to 124b172 Compare August 14, 2018 01:53
@vagrantsn
Copy link
Author

Opa @devdrops pretendo seguir sim! Atualizei a branch de acordo com a V3.

@leonampd
Copy link
Contributor

@vagnervst pretende seguir com esse PR? O branch está desatualizado e vai precisar de um novo rebase

@vagrantsn vagrantsn force-pushed the fix/add-recipient-bank-account-data branch from 124b172 to 4e8379e Compare January 2, 2019 20:42
@vagrantsn
Copy link
Author

@leonampd rebase feito! porém quebraram dois testes que não consigo rodar novamente, acredito ser uma intermitência 🤔

@leonampd
Copy link
Contributor

Fala @vagnervst @murilohns que acham de fechar esse PR? Tá bem desatualizado. Eu n tenho mais os poderes haushaus

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.

None yet

4 participants