BUGFIX: validate Merchant Emails field on save#329
Conversation
Frontend (api-credentials.tsx): show inline error and disable Save when any comma-separated entry fails an email regex. Backend (AdminSaferPayOfficialSettingsController): reject save when testMerchantEmails or liveMerchantEmails contains a value that fails Validate::isEmail() — guards against curl/devtools bypass.
![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 frontend and backend validation for merchant email addresses to ensure only valid, comma-separated lists are saved. Key changes include a new validation helper in the PHP controller, updated translation strings, and React UI enhancements to display error messages and disable the save button. Feedback suggests optimizing the backend validation order to check local inputs before making API calls and improving the frontend logic to validate both test and live email fields simultaneously to prevent UX inconsistencies.
| $testMerchantEmails = $this->getStringValue($data, 'testMerchantEmails'); | ||
| $liveMerchantEmails = $this->getStringValue($data, 'liveMerchantEmails'); | ||
| $invalidEmail = $this->findInvalidEmail($testMerchantEmails) ?: $this->findInvalidEmail($liveMerchantEmails); | ||
| if ($invalidEmail !== null) { | ||
| $this->ajaxResponse(false, sprintf( | ||
| $this->module->l('Invalid merchant email address: %s', self::FILE_NAME), | ||
| $invalidEmail | ||
| )); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The email validation logic should be moved before the API credentials validation (lines 166-180). Validating local input data first is more efficient as it avoids unnecessary and potentially slow remote API calls if the merchant emails are already invalid.
| const invalidEmails = merchantEmails | ||
| .split(',') | ||
| .map(e => e.trim()) | ||
| .filter(e => e.length > 0 && !EMAIL_RE.test(e)) | ||
| const merchantEmailsInvalid = invalidEmails.length > 0 |
There was a problem hiding this comment.
The merchantEmailsInvalid flag currently only validates the emails for the active environment (Test or Live). However, the backend validates both fields upon saving. This can lead to a confusing UX where the 'Save' button is enabled but the save operation fails due to invalid data in the hidden tab. Consider validating both testMerchantEmails and liveMerchantEmails to determine the disabled state of the Save button, while keeping the inline error message specific to the current tab.
1 participant
Summary
demo@p§1312312EWRWrestashop.com) and the controller persisted it verbatim. Notifications would silently bounce / never be sent.api-credentials.tsx): on every change, split by,, trim, validate each part against an email regex. When any entry is invalid → input shows red border + inline error listing the bad values; Save button disabled.AdminSaferPayOfficialSettingsController::ajaxProcessSaveCredentials): newfindInvalidEmail()helper walks bothtestMerchantEmailsandliveMerchantEmailsand rejects the save with a clear message ifValidate::isEmail()fails. Guards against curl / devtools bypass.invalidMerchantEmailsadded.[2.0.2].Test plan
foo@bar.com, not-an-emailshows red border + "Invalid email address: not-an-email"; Save disabled.?action=saveCredentialswithtestMerchantEmails="demo@p§1.com"returns{"success":false,"message":"Invalid merchant email address: demo@p§1.com"}; DB unchanged.a@x.com, b@y.com) saves successfully.