-
Notifications
You must be signed in to change notification settings - Fork 56
Introduce Client#call_adyen_api_url method that accepts a full URL and HTTP method instead of relying on service & action parameters
#320
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @Korri, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Adyen client's API interaction capabilities by introducing a new, more flexible method for making API calls. The new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
f80b73d to
cacb867
Compare
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.
Code Review
This pull request introduces a flexible call_adyen_api_url method and refactors the existing call_adyen_api to use it, which is a great improvement. The refactoring of the request logic using a case statement significantly enhances readability and robustness. The test coverage for the new functionality is comprehensive. I've provided a couple of suggestions: one critical fix to ensure the new public method validates authentication correctly, and another to improve the maintainability of the new tests by reducing code duplication.
d56ffaf to
a071925
Compare
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.
Code Review
This pull request introduces a new Client#call_adyen_api_url method for making API calls with a full URL, and refactors the existing Client#call_adyen_api to use it. The changes are well-structured, significantly reduce code duplication, and include comprehensive test coverage for both the new and modified methods. My main feedback is to address a redundant authentication check that occurs when call_adyen_api calls call_adyen_api_url. I've provided suggestions to centralize the validation logic, which should improve clarity and efficiency.
|
/gemini review |
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.
Code Review
This pull request introduces a new call_adyen_api_url method, which is a great addition for flexibility. The existing call_adyen_api method has been nicely refactored to use this new method, and other parts of the client, like the HTTP request dispatching and authentication logic, have been cleaned up and simplified. The test coverage added for these changes is excellent and thorough. I have one suggestion regarding an inefficient and commented part of the code that seems to be a leftover from a previous implementation.
5a0d71e to
9696c00
Compare
This method allows passing in a full URL for users that do not want to use the whole concept of services and actions.
The service specific validation for `PaymentSetupAndVerification` was useless as this service is not a valid service in `service_url_base`. Removing it means that we where able to simplify the validation as it is not service dependent anymore.
9696c00 to
4b03d8f
Compare
|



Description
This introduces the
Client#call_adyen_api_urlmethod, that can be used to make an API call by specifying a full URL and HTTP method, while still taking care of authentication & basic error handling.Tested scenarios
I added test coverage for both the existing
Client#call_adyen_apimethod as this modifies it, and the newClient#call_adyen_api_url, I tried to stick to mocking practices uses in the rest of the repository.