Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions controllers/admin/AdminSaferPayOfficialSettingsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
use Invertus\SaferPay\Service\SaferPayRestrictionCreator;
use Invertus\SaferPay\Exception\Api\SaferPayApiException;
use Invertus\SaferPay\Exception\Restriction\RestrictionException;
use Invertus\SaferPay\Logger\LoggerInterface;

require_once dirname(__FILE__) . '/../../vendor/autoload.php';

Expand Down Expand Up @@ -209,8 +210,8 @@ public function ajaxProcessSaveCredentials()

// Auto-detect license features from Saferpay Management API
$suffix = $isTestMode ? SaferPayConfig::TEST_SUFFIX : '';
$licenseMessage = '';
$hasBusinessLicense = false;
$licenseFetchFailed = false;

if (!empty($activeUsername) && !empty($activePassword) && !empty($activeCustomerId)) {
try {
Expand All @@ -227,18 +228,29 @@ public function ajaxProcessSaveCredentials()
$configuration->set(SaferPayConfig::BUSINESS_LICENSE . $suffix, $hasBusinessLicense ? 1 : 0);
} catch (\Exception $e) {
$configuration->set(SaferPayConfig::BUSINESS_LICENSE . $suffix, 0);
$licenseFetchFailed = true;

$licenseMessage = ' ' . $this->module->l('Could not retrieve license information. Please verify your credentials.', self::FILE_NAME);
/** @var LoggerInterface $logger */
$logger = $this->module->getService(LoggerInterface::class);
$logger->error('License fetch failed on credentials save: ' . $e->getMessage(), [
'context' => ['exception_class' => get_class($e)],
]);
}
} else {
$configuration->set(SaferPayConfig::BUSINESS_LICENSE . $suffix, 0);
}

$message = $licenseFetchFailed
? $this->module->l('Settings saved, but Saferpay Fields availability could not be confirmed. Please try again later or check the module Logs for details.', self::FILE_NAME)
: $this->module->l('Settings saved successfully.', self::FILE_NAME);

$this->ajaxResponse(
true,
$this->module->l('Settings saved successfully.', self::FILE_NAME) . $licenseMessage,
$message,
[
'hasBusinessLicense' => $hasBusinessLicense,
'testHasBusinessLicense' => (bool) $configuration->get(SaferPayConfig::BUSINESS_LICENSE . SaferPayConfig::TEST_SUFFIX),
'liveHasBusinessLicense' => (bool) $configuration->get(SaferPayConfig::BUSINESS_LICENSE),
'warning' => $licenseFetchFailed,
]
);
}
Expand Down Expand Up @@ -534,8 +546,9 @@ private function collectSettingsData()
'liveFieldAccessToken' => (string) $configuration->get(SaferPayConfig::FIELDS_ACCESS_TOKEN),
'liveFieldJsUrl' => (string) $configuration->get(SaferPayConfig::FIELDS_LIBRARY),

// License (auto-detected)
'hasBusinessLicense' => (bool) $configuration->get(SaferPayConfig::BUSINESS_LICENSE . SaferPayConfig::getConfigSuffix()),
// License (auto-detected, per environment)
'testHasBusinessLicense' => (bool) $configuration->get(SaferPayConfig::BUSINESS_LICENSE . SaferPayConfig::TEST_SUFFIX),
'liveHasBusinessLicense' => (bool) $configuration->get(SaferPayConfig::BUSINESS_LICENSE),

// Payment Processing
'paymentBehavior' => (int) $configuration->get(SaferPayConfig::PAYMENT_BEHAVIOR),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export function ApiCredentials() {
const fieldJsUrl = isTest ? settings.testFieldJsUrl : settings.liveFieldJsUrl

const hasCredentials = username.length > 0 && password.length > 0
const hasBusinessLicense = isTest ? settings.testHasBusinessLicense : settings.liveHasBusinessLicense

const setField = (field: string, value: string | boolean) => {
updateSettings({ [`${prefix}${field.charAt(0).toUpperCase() + field.slice(1)}`]: value } as Record<string, string | boolean>)
Expand Down Expand Up @@ -145,17 +146,23 @@ export function ApiCredentials() {
{/* Username & Password */}
<div className="sp-grid sp-gap-5 md:sp-grid-cols-2">
<div className="sp-flex sp-flex-col sp-gap-2">
<Label htmlFor="api-username">{t('jsonApiUsername')}</Label>
<Label htmlFor="api-username">
{t('jsonApiUsername')} <span className="sp-text-destructive" aria-hidden="true">*</span>
</Label>
<Input
id="api-username"
type="text"
placeholder={t('enterApiUsername', envLabel.toLowerCase())}
value={username}
onChange={(e) => setField('username', e.target.value)}
required
aria-required="true"
/>
Comment on lines 157 to 160

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The required and aria-required attributes prevent the form from being submitted if the field is empty. This prevents users from clearing credentials to reset or disable the module configuration. Since the backend already handles empty values and performs validation, these restrictions should be removed to allow for more flexibility in managing settings.

Suggested change
onChange={(e) => setField('username', e.target.value)}
required
aria-required="true"
/>
onChange={(e) => setField('username', e.target.value)}
/>

</div>
<div className="sp-flex sp-flex-col sp-gap-2">
<Label htmlFor="api-password">{t('jsonApiPassword')}</Label>
<Label htmlFor="api-password">
{t('jsonApiPassword')} <span className="sp-text-destructive" aria-hidden="true">*</span>
</Label>
<div className="sp-relative">
<Input
id="api-password"
Expand All @@ -164,6 +171,8 @@ export function ApiCredentials() {
value={password}
onChange={(e) => setField('password', e.target.value)}
className="sp-pr-10"
required
aria-required="true"
/>
Comment on lines 173 to 176

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Similar to the username field, the required and aria-required attributes here prevent clearing the password. It is recommended to remove them to allow users to clear credentials if needed.

Suggested change
className="sp-pr-10"
required
aria-required="true"
/>
className="sp-pr-10"
/>

<button
type="button"
Expand Down Expand Up @@ -242,7 +251,7 @@ export function ApiCredentials() {
</CardContent>
</Card>

{settings.hasBusinessLicense && <Card>
{hasBusinessLicense && <Card>
<CardHeader>
<CardTitle className="sp-text-base sp-font-semibold">{t('saferpayFields')}</CardTitle>
<CardDescription>{t('saferpayFieldsDescription')}</CardDescription>
Expand Down Expand Up @@ -330,7 +339,12 @@ export function ApiCredentials() {
</Card>}

<div className="sp-flex sp-justify-end">
<Button className="sp-min-w-[120px]" onClick={saveCredentials} disabled={saving} aria-label={saving ? t('saving') : undefined}>
<Button
className="sp-min-w-[120px]"
onClick={saveCredentials}
disabled={saving || !hasCredentials || credentialStatus === 'invalid' || credentialStatus === 'checking'}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The disabled condition is too restrictive. Blocking the save when credentialStatus === 'invalid' or !hasCredentials prevents users from saving their settings if the background validation fails (e.g., due to transient network issues) or if they wish to clear the credentials. Since the backend performs its own validation upon saving, the frontend should ideally only provide feedback rather than blocking the action entirely.

Suggested change
disabled={saving || !hasCredentials || credentialStatus === 'invalid' || credentialStatus === 'checking'}
disabled={saving || credentialStatus === 'checking'}

aria-label={saving ? t('saving') : undefined}
>
{saving ? <Loader2 className="sp-h-4 sp-w-4 sp-animate-spin" /> : t('saveChanges')}
</Button>
</div>
Expand Down
15 changes: 10 additions & 5 deletions views/js/admin/settings-app/src/context/settings-context.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,16 @@ export function SettingsProvider({ children }: { children: React.ReactNode }) {
}, [])

const handleSave = useCallback(async (
saveFn: () => Promise<{ success: boolean; message?: string }>,
saveFn: () => Promise<{ success: boolean; message?: string; warning?: boolean }>,
label: string,
section: SavingSection,
) => {
setSavingSections((prev) => new Set(prev).add(section))
try {
const result = await saveFn()
if (result.success) {
toast({ title: result.message || t('savedSuccessfully', label), variant: 'default' })
const variant = result.warning ? 'warning' : 'default'
toast({ title: result.message || t('savedSuccessfully', label), variant })
} else {
toast({ title: result.message || t('failedToSave', label), variant: 'destructive' })
}
Expand Down Expand Up @@ -91,10 +92,14 @@ export function SettingsProvider({ children }: { children: React.ReactNode }) {
liveFieldJsUrl: currentSettings.liveFieldJsUrl,
})
const data = result as unknown as Record<string, unknown>
if (result.success && typeof data.hasBusinessLicense === 'boolean') {
setSettings((prev) => ({ ...prev, hasBusinessLicense: data.hasBusinessLicense as boolean }))
if (result.success) {
setSettings((prev) => ({
...prev,
...(typeof data.testHasBusinessLicense === 'boolean' ? { testHasBusinessLicense: data.testHasBusinessLicense as boolean } : {}),
...(typeof data.liveHasBusinessLicense === 'boolean' ? { liveHasBusinessLicense: data.liveHasBusinessLicense as boolean } : {}),
}))
}
return result
return { ...result, warning: data.warning === true }
}, 'API Credentials', 'credentials')
}, [handleSave])

Expand Down
5 changes: 3 additions & 2 deletions views/js/admin/settings-app/src/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ export interface SaferpaySettingsData {
liveFieldAccessToken: string
liveFieldJsUrl: string

// License (read-only, auto-detected from API)
hasBusinessLicense: boolean
// License (read-only, auto-detected from API, per environment)
testHasBusinessLicense: boolean
liveHasBusinessLicense: boolean

// Payment Processing
paymentBehavior: number
Expand Down
Loading