-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
- Updated the `register_payment` method to make `account_id` optional by setting its default value to `nil`. - Added test cases for optional `account_id`
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
incognia-ruby/spec/incognia_spec.rb
Lines 343 to 352 in 573f91d
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) |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noice.
Updated the
register_payment
method to makeaccount_id
optional.