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

Feat/make account id optional on payment #29

Closed
wants to merge 2 commits into from

Conversation

brunoekferrari
Copy link

Updated the register_payment method to make account_id optional.

- Updated the `register_payment` method to make `account_id` optional by setting its default value to `nil`.
- Added test cases for optional `account_id`
Copy link

@gabrielpessoa1 gabrielpessoa1 left a comment

Choose a reason for hiding this comment

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

nao sei nada de ruby mas parece certo


it_behaves_like 'receiving one of the required tokens without account_id', :request_token
it_behaves_like 'receiving one of the required tokens without account_id', :installation_id
it_behaves_like 'receiving one of the required tokens without account_id', :session_token
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of declaring a new block/shared example without account id (that is now an optional argument), you could reuse the below shared example "when receiving any other optional arguments".

      it_behaves_like 'receiving optional args', 'account_id' do
        let(:opts) { { account_d: 'account-id' } }
      end

And you should also remove the account_id: account_id from the stub and register_payment call.

account_id: account_id
}.merge(opts),
headers: {
'Content-Type' => 'application/json', 'Authorization' => /Bearer.*/
}
)
api.register_payment(
request_token: request_token,
account_id: account_id,

@@ -67,7 +67,7 @@ def register_feedback(event:, occurred_at: nil, expires_at: nil, timestamp: nil,
response.success?
end

def register_payment(account_id:, request_token: nil, **opts)
def register_payment(account_id: nil, request_token: nil, **opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can let the account_id here to encourage its usage. Considering the lib pattern, we should remove it and the user could inform it through opts.

Thoughts, @ottony ?

Copy link
Member

@ottony ottony Oct 17, 2024

Choose a reason for hiding this comment

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

As it's specific for one client, why don't we just say to call the method with account_id: nil and do not change anything here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Noice.

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.

4 participants