[INTERNAL] UI bugs fix#309
Conversation
Summary of ChangesHello @TLabutis, 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 significantly upgrades the SaferPay module's administrative configuration by introducing dynamic terminal management. It allows administrators to automatically retrieve and select payment terminals from SaferPay using their credentials, streamlining the setup process and reducing potential configuration errors. The changes encompass both backend logic for handling terminal data and a responsive frontend interface that provides clear feedback and flexibility for manual input when needed. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dynamic way to fetch SaferPay terminals in the admin settings page, which is a great improvement for user experience. The implementation involves a new AJAX endpoint in the controller, corresponding JavaScript to handle the frontend logic, and template updates.
My review has identified a few critical issues that need to be addressed:
- There is a fatal error due to a method signature mismatch when calling
getAvailableTerminals. - A debug
alert()is present in the JavaScript which will disrupt users. - There is a bug in a form submit handler in the JavaScript.
Additionally, there are several debug console.log statements that should be removed, and a small opportunity for code simplification in the JavaScript.
Please see the detailed comments for each point.
| /** @var SaferPayTerminalService $terminalService */ | ||
| $terminalService = $this->module->getService(SaferPayTerminalService::class); | ||
|
|
||
| $terminals = $terminalService->getAvailableTerminals($customerId, $username, $password, $isTestMode); |
There was a problem hiding this comment.
The call to getAvailableTerminals provides four arguments, but the method definition in SaferPayTerminalService only accepts one ($customerId). This will result in a fatal error.
To fix this, you need to update the getAvailableTerminals method in src/Service/SaferPayTerminalService.php to accept the customer ID, username, password, and test mode flag as arguments. This will also require updating the private methods it uses (getHeaders, getBaseRestUrl) to use these arguments instead of relying on global Configuration values. This change will make the service stateless and more robust.
Other calls to getAvailableTerminals in the codebase will also need to be updated to pass the required credentials.
| console.log('[DEBUG] Making AJAX request with params:', Object.assign({}, params, {password: '***'})); | ||
| // #endregion | ||
|
|
||
| alert('Customer ID before AJAX: ' + customerId); // Temporary debug alert |
There was a problem hiding this comment.
This alert() seems to be for debugging purposes. It's very disruptive to the user experience and must be removed before this code is merged.
|
|
||
| $('form').on('submit', function() { | ||
| if ($manualInput.val() && !$select.val()) { | ||
| var manualValue = $(this).val(); |
There was a problem hiding this comment.
In this form submit event handler, $(this) refers to the form element, not the manual input field. Therefore, $(this).val() will not return the value of the manual input. You should use $manualInput.val() instead to get the correct value.
var manualValue = $manualInput.val();| // #region agent log | ||
| console.log('[DEBUG] saferpay_settings.js loaded'); | ||
| // #endregion |
There was a problem hiding this comment.
This file contains multiple console.log statements (e.g., lines 25, 42, 54, 112, 125, 137, 171). These appear to be for debugging and should be removed from the production code to keep the browser console clean for users and other developers.
| if ($field.attr('type') === 'password') { | ||
| return $field.val() || ''; | ||
| } | ||
| return $field.val() || ''; |
There was a problem hiding this comment.
This if statement is redundant because both branches return the same value. It can be simplified to a single return statement. Note that this is safe to do after removing the debug console.log on line 54 which treats password fields specially.
return $field.val() || '';
TLabutis
force-pushed
the
release-2.0.3
branch
from
46d86f3 to
0c33f9d
Compare
1 participant
Self-Checks
JIRA task link
Summary
QA Checklist Labels
QA Checklist
Additional Context
Frontend Changes