diff --git a/TASKS.md b/TASKS.md new file mode 100644 index 000000000..5edd4fd63 --- /dev/null +++ b/TASKS.md @@ -0,0 +1,215 @@ +# SaferPay Official Module - Improvement Tasks + +## Performance Optimizations + +### 1. Database Query Optimization +**Priority: High** | **Effort: Medium** | **Impact: High** + +- **Issue**: Multiple database queries without proper indexing and some inefficient queries +- **Location**: `src/Repository/` classes, especially `SaferPayCardAliasRepository.php` and `SaferPayOrderRepository.php` +- **Improvements**: + - Add database indexes for frequently queried columns (id_customer, payment_method, id_order, id_cart) + - Optimize queries in `getSavedValidCardsByUserIdAndPaymentMethod()` to use JOINs instead of multiple WHERE clauses + - Implement query result caching for static data like payment methods and configurations + - Add LIMIT clauses to queries that don't need all results + +### 2. Service Container Optimization +**Priority: Medium** | **Effort: Low** | **Impact: Medium** + +- **Issue**: Services are instantiated multiple times in the same request +- **Location**: Main module class `getService()` method +- **Improvements**: + - Implement service singleton pattern or dependency injection container caching + - Cache frequently used services like `LoggerInterface`, `Configuration`, and repository classes + - Reduce service instantiation in hooks and controllers + +### 3. Configuration Access Optimization +**Priority: Medium** | **Effort: Low** | **Impact: Medium** + +- **Issue**: Configuration values are fetched multiple times per request +- **Location**: Throughout the module, especially in payment processing +- **Improvements**: + - Cache configuration values in memory for the duration of the request + - Implement lazy loading for configuration values + - Group related configuration calls + +## Code Quality Improvements + +### 4. Type Declarations Enhancement +**Priority: Medium** | **Effort: Medium** | **Impact: Medium** + +- **Issue**: Missing return type declarations and inconsistent type hints +- **Location**: Service classes, repositories, and controllers +- **Improvements**: + - Add strict return type declarations to all public methods + - Implement proper type hints for array parameters and return values + - Add PHPDoc blocks with proper @param and @return annotations + - Use PHP 7.4+ typed properties where applicable + +### 5. Exception Handling Standardization +**Priority: High** | **Effort: Medium** | **Impact: High** + +- **Issue**: Inconsistent exception handling patterns across the module +- **Location**: Controllers and service classes +- **Improvements**: + - Standardize exception handling in all controllers using a common pattern + - Implement proper exception hierarchy with specific exception types + - Add consistent error logging with context information + - Improve error messages for better user experience + +### 6. Code Duplication Reduction +**Priority: Medium** | **Effort: Medium** | **Impact: Medium** + +- **Issue**: Repeated code patterns in controllers and services +- **Location**: Front controllers, especially validation and return controllers +- **Improvements**: + - Extract common validation logic into shared service classes + - Create base controller methods for common operations (redirects, error handling) + - Implement shared utilities for URL generation and parameter handling + - Consolidate similar database query patterns + +## Security Enhancements + +### 7. Input Validation Strengthening +**Priority: High** | **Effort: Low** | **Impact: High** + +- **Issue**: Some user inputs lack proper validation and sanitization +- **Location**: Front controllers and admin controllers +- **Improvements**: + - Add comprehensive input validation for all user-provided data + - Implement proper SQL injection prevention (already using pSQL but ensure consistency) + - Add CSRF token validation for admin operations + - Validate file uploads and external data sources + +### 8. Sensitive Data Protection +**Priority: High** | **Effort: Low** | **Impact: High** + +- **Issue**: Sensitive configuration data handling could be improved +- **Location**: Configuration storage and display +- **Improvements**: + - Implement proper password masking in admin forms + - Add encryption for sensitive configuration values + - Improve logging to avoid exposing sensitive data + - Implement proper session security + +## Maintainability Improvements + +### 9. Configuration Management Enhancement +**Priority: Medium** | **Effort: Medium** | **Impact: Medium** + +- **Issue**: Configuration handling is scattered and could be more organized +- **Location**: `SaferPayConfig.php` and admin settings controller +- **Improvements**: + - Group related configuration options logically + - Implement configuration validation on save + - Add configuration migration system for future updates + - Create configuration backup/restore functionality + +### 10. Logging System Enhancement +**Priority: Medium** | **Effort: Low** | **Impact: Medium** + +- **Issue**: Logging could be more structured and informative +- **Location**: `Logger.php` and throughout the module +- **Improvements**: + - Implement structured logging with consistent format + - Add log level configuration per operation type + - Implement log rotation and cleanup automation + - Add performance metrics logging for critical operations + +### 11. Error Message Localization +**Priority: Low** | **Effort: Medium** | **Impact: Medium** + +- **Issue**: Some error messages are not properly localized +- **Location**: Exception services and controllers +- **Improvements**: + - Ensure all user-facing messages are properly translated + - Add missing translation keys + - Implement fallback messages for missing translations + - Standardize message formatting + +## User Experience Improvements + +### 12. Admin Interface Optimization +**Priority: Low** | **Effort: Low** | **Impact: Medium** + +- **Issue**: Admin interface could be more user-friendly +- **Location**: Admin settings controller and templates +- **Improvements**: + - Add configuration validation feedback + - Implement auto-save for non-critical settings + - Add help tooltips and documentation links + - Improve form layout and organization + +### 13. Frontend Error Handling +**Priority: Medium** | **Effort: Low** | **Impact: Medium** + +- **Issue**: Frontend error messages could be more user-friendly +- **Location**: Front controllers and templates +- **Improvements**: + - Implement graceful error handling for payment failures + - Add retry mechanisms for temporary failures + - Improve error message clarity for end users + - Add progress indicators for long-running operations + +## Technical Debt Reduction + +### 14. Legacy Code Cleanup +**Priority: Low** | **Effort: Medium** | **Impact: Low** + +- **Issue**: Some legacy code patterns and unused code +- **Location**: Throughout the module +- **Improvements**: + - Remove unused imports and methods + - Update deprecated PrestaShop API usage + - Consolidate similar functionality + - Remove commented-out code + +### 15. Testing Infrastructure +**Priority: Low** | **Effort: High** | **Impact: High** + +- **Issue**: Limited test coverage for critical functionality +- **Location**: Missing comprehensive tests +- **Improvements**: + - Add unit tests for service classes + - Implement integration tests for payment flows + - Add automated testing for admin functionality + - Create test data fixtures for consistent testing + +## Implementation Priority + +### Phase 1 (High Priority - Security & Performance) +1. Database Query Optimization (#1) +2. Exception Handling Standardization (#5) +3. Input Validation Strengthening (#7) +4. Sensitive Data Protection (#8) + +### Phase 2 (Medium Priority - Quality & UX) +5. Type Declarations Enhancement (#4) +6. Code Duplication Reduction (#6) +7. Service Container Optimization (#2) +8. Configuration Access Optimization (#3) +9. Frontend Error Handling (#13) + +### Phase 3 (Low Priority - Polish & Maintenance) +10. Configuration Management Enhancement (#9) +11. Logging System Enhancement (#10) +12. Admin Interface Optimization (#12) +13. Error Message Localization (#11) +14. Legacy Code Cleanup (#14) +15. Testing Infrastructure (#15) + +## Notes + +- All improvements should maintain backward compatibility +- Changes should be thoroughly tested in both test and live environments +- Performance improvements should be measured and documented +- Security enhancements should be reviewed by security experts +- User experience improvements should be tested with real users + +## Estimated Total Effort +- **Phase 1**: 3-4 weeks +- **Phase 2**: 4-5 weeks +- **Phase 3**: 3-4 weeks +- **Total**: 10-13 weeks for complete implementation + +Each task includes specific file locations and implementation guidance to ensure efficient development. diff --git a/src/Install/Installer.php b/src/Install/Installer.php index 5523a22b3..1cdfce80d 100755 --- a/src/Install/Installer.php +++ b/src/Install/Installer.php @@ -223,7 +223,9 @@ private function installSaferPayOrderTable() `refunded` tinyint(1) DEFAULT 0, `canceled` tinyint(1) DEFAULT 0, `authorized` tinyint(1) DEFAULT 0, - `pending` tinyint(1) DEFAULT 0 + `pending` tinyint(1) DEFAULT 0, + INDEX `idx_token` (`token`), + INDEX `idx_status` (`captured`, `refunded`, `canceled`, `authorized`, `pending`) ) ENGINE=' . _MYSQL_ENGINE_ . ' DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci' ); } @@ -254,7 +256,13 @@ private function installSaferPayAssertTable() `card_number` VARCHAR(64) NOT NULL, `dcc_value` INTEGER(32) DEFAULT NULL, `dcc_currency_code` VARCHAR(64) DEFAULT NULL, - `authorized` tinyint(1) DEFAULT 0 + `authorized` tinyint(1) DEFAULT 0, + INDEX `idx_status` (`status`), + INDEX `idx_payment_id` (`payment_id`), + INDEX `idx_merchant_reference` (`merchant_reference`), + INDEX `idx_authorized` (`authorized`), + INDEX `idx_currency_code` (`currency_code`), + INDEX `idx_order_status` (`id_saferpay_order`, `status`) ) ENGINE=' . _MYSQL_ENGINE_ . ' DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci' ); } @@ -272,7 +280,10 @@ private function installSaferPayCardAlias() `payment_method` VARCHAR(64) NOT NULL, `valid_till` datetime NOT NULL, `date_add` datetime NOT NULL, - `date_upd` datetime NOT NULL + `date_upd` datetime NOT NULL, + INDEX `idx_success` (`success`), + INDEX `idx_customer_success` (`id_customer`, `success`), + INDEX `idx_customer_valid_till` (`id_customer`, `valid_till`) ) ENGINE=' . _MYSQL_ENGINE_ . ' DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci' ); } @@ -304,7 +315,11 @@ private function installOrderRefundTable() `transaction_id` VARCHAR(64) NOT NULL, `amount` INTEGER(20) NOT NULL, `currency` VARCHAR(64) NOT NULL, - `status` VARCHAR(64) NOT NULL + `status` VARCHAR(64) NOT NULL, + INDEX `idx_status` (`status`), + INDEX `idx_currency` (`currency`), + INDEX `idx_order_status` (`id_order`, `status`), + INDEX `idx_saferpay_order_status` (`id_saferpay_order`, `status`) ) ENGINE=' . _MYSQL_ENGINE_ . ' DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci' ); } diff --git a/src/Repository/SaferPayCardAliasRepository.php b/src/Repository/SaferPayCardAliasRepository.php index d1664313e..70fb0da73 100755 --- a/src/Repository/SaferPayCardAliasRepository.php +++ b/src/Repository/SaferPayCardAliasRepository.php @@ -37,7 +37,7 @@ public function getSavedValidCardsByUserIdAndPaymentMethod($userId, $paymentMeth $query = new DbQuery(); $query->select('`id_saferpay_card_alias`, `card_number`'); $query->from('saferpay_card_alias'); - $query->where('id_customer = "' . (int) $userId . '"'); + $query->where('id_customer = ' . (int) $userId); $query->where('payment_method = "' . pSQL($paymentMethod) . '"'); $query->where('valid_till > "' . pSQL($currentDate) . '"'); @@ -49,7 +49,7 @@ public function getSavedCardAliasFromId($id) $query = new DbQuery(); $query->select('`alias_id`'); $query->from('saferpay_card_alias'); - $query->where('id_saferpay_card_alias = "' . (int) $id . '"'); + $query->where('id_saferpay_card_alias = ' . (int) $id); return Db::getInstance()->getValue($query); } @@ -59,7 +59,7 @@ public function getSavedCardIdByCustomerIdAndAliasId($customerId, $aliasId) $query = new DbQuery(); $query->select('`id_saferpay_card_alias`'); $query->from('saferpay_card_alias'); - $query->where('id_customer = "' . (int) $customerId . '"'); + $query->where('id_customer = ' . (int) $customerId); $query->where('alias_id = "' . pSQL($aliasId) . '"'); return Db::getInstance()->getValue($query); @@ -70,7 +70,7 @@ public function getSavedCardsByCustomerId($customerId) $query = new DbQuery(); $query->select('*'); $query->from('saferpay_card_alias'); - $query->where('id_customer = "' . (int) $customerId . '"'); + $query->where('id_customer = ' . (int) $customerId); return Db::getInstance()->executeS($query); } diff --git a/src/Repository/SaferPayOrderRepository.php b/src/Repository/SaferPayOrderRepository.php index 921a401f4..b7284ab4a 100755 --- a/src/Repository/SaferPayOrderRepository.php +++ b/src/Repository/SaferPayOrderRepository.php @@ -49,7 +49,7 @@ public function getIdByOrderId($orderId) $query = new DbQuery(); $query->select('`id_saferpay_order`'); $query->from('saferpay_order'); - $query->where('id_order = "' . (int) $orderId . '"'); + $query->where('id_order = ' . (int) $orderId); $query->orderBy('`id_saferpay_order` DESC'); return Db::getInstance()->getValue($query); @@ -60,7 +60,7 @@ public function getIdByCartId($cartId) $query = new DbQuery(); $query->select('`id_saferpay_order`'); $query->from('saferpay_order'); - $query->where('id_cart = "' . (int) $cartId . '"'); + $query->where('id_cart = ' . (int) $cartId); $query->orderBy('`id_saferpay_order` DESC'); return Db::getInstance()->getValue($query); @@ -70,7 +70,7 @@ public function getAssertIdBySaferPayOrderId($saferPayOrderId) $query = new DbQuery(); $query->select('`id_saferpay_assert`'); $query->from('saferpay_assert'); - $query->where('id_saferPay_order = "' . (int) $saferPayOrderId . '"'); + $query->where('id_saferPay_order = ' . (int) $saferPayOrderId); $query->orderBy('id_saferpay_assert DESC'); return Db::getInstance()->getValue($query); @@ -86,7 +86,8 @@ public function getOrderRefunds($saferPayOrderId) $query = new DbQuery(); $query->select('*'); $query->from('saferpay_order_refund'); - $query->where('id_saferPay_order = "' . (int) $saferPayOrderId . '"'); + $query->where('id_saferPay_order = ' . (int) $saferPayOrderId); + $query->orderBy('id_saferpay_order_refund DESC'); return Db::getInstance()->executeS($query); } @@ -96,7 +97,7 @@ public function getPaymentBrandBySaferpayOrderId($saferpayOrderId) $query = new DbQuery(); $query->select('`brand`'); $query->from('saferpay_assert'); - $query->where('id_saferpay_order = "' . (int) $saferpayOrderId . '"'); + $query->where('id_saferpay_order = ' . (int) $saferpayOrderId); return Db::getInstance()->getValue($query); } diff --git a/upgrade/install-2.0.2.php b/upgrade/install-2.0.2.php index e66a64d06..24e3b5e40 100644 --- a/upgrade/install-2.0.2.php +++ b/upgrade/install-2.0.2.php @@ -25,10 +25,106 @@ exit; } -function upgrade_module_2_0_2() +function upgrade_module_2_0_2(SaferPayOfficial $module) { - Configuration::updateValue('SAFERPAY_SEND_ORDER_CONF_MAIL', 0); - Configuration::updateValue('SAFERPAY_GROUP_CARDS', 0); + $db = Db::getInstance(); + $success = true; - return true; + // Add indexes for saferpay_order table + $orderIndexes = [ + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_order` ADD INDEX `idx_id_order` (`id_order`)", + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_order` ADD INDEX `idx_id_cart` (`id_cart`)", + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_order` ADD INDEX `idx_id_customer` (`id_customer`)", + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_order` ADD INDEX `idx_transaction_id` (`transaction_id`)", + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_order` ADD INDEX `idx_status_flags` (`authorized`, `captured`, `pending`)", + ]; + + foreach ($orderIndexes as $indexSql) { + try { + $result = $db->execute($indexSql); + if (!$result) { + $error = $db->getMsgError(); + if (strpos($error, 'Duplicate key name') === false) { + $success = false; + PrestaShopLogger::addLog('SaferPay: Failed to add order index - ' . $error, 3, null, 'SaferPayOrder'); + } + } + } catch (Exception $e) { + PrestaShopLogger::addLog('SaferPay: Order index creation skipped - ' . $e->getMessage(), 1, null, 'SaferPayOrder'); + } + } + + // Add indexes for saferpay_card_alias table + $cardAliasIndexes = [ + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_card_alias` ADD INDEX `idx_id_customer` (`id_customer`)", + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_card_alias` ADD INDEX `idx_payment_method` (`payment_method`)", + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_card_alias` ADD INDEX `idx_customer_payment` (`id_customer`, `payment_method`)", + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_card_alias` ADD INDEX `idx_valid_till` (`valid_till`)", + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_card_alias` ADD INDEX `idx_alias_id` (`alias_id`)", + ]; + + foreach ($cardAliasIndexes as $indexSql) { + try { + $result = $db->execute($indexSql); + if (!$result) { + $error = $db->getMsgError(); + if (strpos($error, 'Duplicate key name') === false) { + $success = false; + PrestaShopLogger::addLog('SaferPay: Failed to add card alias index - ' . $error, 3, null, 'SaferPayCardAlias'); + } + } + } catch (Exception $e) { + PrestaShopLogger::addLog('SaferPay: Card alias index creation skipped - ' . $e->getMessage(), 1, null, 'SaferPayCardAlias'); + } + } + + // Add indexes for saferpay_assert table + $assertIndexes = [ + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_assert` ADD INDEX `idx_id_saferpay_order` (`id_saferpay_order`)", + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_assert` ADD INDEX `idx_payment_method` (`payment_method`)", + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_assert` ADD INDEX `idx_brand` (`brand`)", + ]; + + foreach ($assertIndexes as $indexSql) { + try { + $result = $db->execute($indexSql); + if (!$result) { + $error = $db->getMsgError(); + if (strpos($error, 'Duplicate key name') === false) { + $success = false; + PrestaShopLogger::addLog('SaferPay: Failed to add assert index - ' . $error, 3, null, 'SaferPayAssert'); + } + } + } catch (Exception $e) { + PrestaShopLogger::addLog('SaferPay: Assert index creation skipped - ' . $e->getMessage(), 1, null, 'SaferPayAssert'); + } + } + + // Add indexes for saferpay_order_refund table + $refundIndexes = [ + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_order_refund` ADD INDEX `idx_id_saferpay_order` (`id_saferpay_order`)", + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_order_refund` ADD INDEX `idx_id_order` (`id_order`)", + "ALTER TABLE `" . _DB_PREFIX_ . "saferpay_order_refund` ADD INDEX `idx_transaction_id` (`transaction_id`)", + ]; + + foreach ($refundIndexes as $indexSql) { + try { + $result = $db->execute($indexSql); + if (!$result) { + $error = $db->getMsgError(); + if (strpos($error, 'Duplicate key name') === false) { + $success = false; + PrestaShopLogger::addLog('SaferPay: Failed to add refund index - ' . $error, 3, null, 'SaferPayOrderRefund'); + } + } + } catch (Exception $e) { + PrestaShopLogger::addLog('SaferPay: Refund index creation skipped - ' . $e->getMessage(), 1, null, 'SaferPayOrderRefund'); + } + } + + if ($success) { + PrestaShopLogger::addLog('SaferPay: Database indexes added successfully', 1, null, 'SaferPayOptimization'); + } + + return $success; }