SL-353/SL-354: fields settings block logic#316
SL-353/SL-354: fields settings block logic#316GytisZum wants to merge 2 commits intoSL-350/overall-design-changesfrom
Conversation
Summary of ChangesHello, 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 enhances the Saferpay module's license management by implementing real-time license fetching and dynamically adjusting the user interface based on the retrieved license status. This ensures that the displayed features and configuration options accurately reflect the user's current Saferpay plan, improving user experience and reducing potential confusion regarding feature availability. 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. Changelog
Activity
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 logic to fetch and handle Saferpay license information, which controls the visibility of "Saferpay Fields" settings. The frontend changes correctly hide this section if the user does not have the required business license, improving the UI. The backend adds new services and DTOs to communicate with the Saferpay API for license fetching, including a fallback mechanism which is a nice touch.
However, I have identified a couple of areas for improvement. Firstly, a new block of code in AdminSaferPayOfficialSettingsController fetches the license status on every page load. This is a performance concern and it fails silently, which can lead to a confusing user experience with stale data. I've recommended removing it or implementing caching and proper error handling. Secondly, the new service classes use mixed as a return type in PHPDocs, which weakens type safety. I've suggested using more specific types like object|null to improve code clarity and maintainability.
| if (!empty($activeUsername) && !empty($activePassword) && !empty($activeCustomerId)) { | ||
| try { | ||
| /** @var SaferPayGetLicense $getLicense */ | ||
| $getLicense = $this->module->getService(SaferPayGetLicense::class); | ||
| $licenseInfo = $getLicense->fetchLicenseWithCredentials( | ||
| $activeUsername, | ||
| $activePassword, | ||
| $activeCustomerId, | ||
| $isTestMode | ||
| ); | ||
| $configuration->set( | ||
| SaferPayConfig::BUSINESS_LICENSE . $suffix, | ||
| $licenseInfo['hasBusinessLicense'] ? 1 : 0 | ||
| ); | ||
| } catch (\Exception $e) { | ||
| // Silently fall back to stored value | ||
| } | ||
| } |
There was a problem hiding this comment.
Making an API call on every page load of the settings page can negatively impact performance and user experience, especially if the external API is slow to respond. Additionally, silently catching all exceptions (catch (\Exception $e)) hides potential issues from the user, such as invalid credentials or network problems. This can lead to confusion when the license status displayed is stale and doesn't reflect the actual state.
Consider removing this automatic refresh. The license status is already updated when credentials are saved, which provides better user feedback on failure. If a manual refresh is desired, a dedicated button would be more efficient.
If you must keep the automatic refresh, please consider these improvements:
- Implement caching with a reasonable TTL (e.g., 1 hour) to avoid making an API call on every single page load.
- Handle exceptions gracefully by logging the error and displaying a non-intrusive message to the user indicating that the license status could not be refreshed.
GytisZum
changed the title
GytisZum
changed the title
1 participant
Self-Checks
JIRA task link
Summary
QA Checklist Labels
QA Checklist
Additional Context
Frontend Changes