update return and icon#1
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the CreditReminder module by introducing new front-office email templates (HTML/TXT), adding a reminder email log table and loop, and updating the back-office configuration UI + CLI command to use a new service-based implementation.
Changes:
- Add new email templates under
templates/email/defaultand create aMessagedefinition pointing to those templates. - Introduce
credit_reminder_logpersistence + a new loop to display sent reminders in the back-office. - Replace the previous command/event-based sending flow with
CreditReminderService+CreditReminder:sendcommand, and update the BO configuration page (including “send test email”).
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/email/default/credit-reminder.txt | New plain-text customer reminder email template. |
| templates/email/default/credit-reminder.html | New HTML customer reminder email template. |
| templates/backOffice/default/email/credit_reminder_message.txt | Removes legacy BO-stored text template. |
| templates/backOffice/default/email/credit_reminder_message.html | Removes legacy BO-stored HTML template. |
| templates/backOffice/default/credit-reminder.html | Redesigns BO module page (tabs, config form, logs table, test send). |
| composer.json | Updates package metadata (name/description/authors) but drops license. |
| Service/CreditReminderService.php | Adds core service for eligibility computation and email sending/logging. |
| README.md | Refreshes documentation and CLI usage instructions. |
| Model/CreditReminderLogQuery.php | Adds Propel query class for new log table. |
| Model/CreditReminderLog.php | Adds Propel model class for new log table. |
| Model/CreditReminder.php | Adjusts model method signatures (return types removed). |
| Loop/CreditReminderLoop.php | Adjusts loop method signatures (types removed). |
| Loop/CreditReminderLog.php | Adds loop to expose credit_reminder_log entries to templates. |
| I18n/fr_FR.php | Adds French translations for new UI/email strings. |
| I18n/en_US.php | Adds English translations for new UI/email strings. |
| Hook/BackHook.php | Adjusts hook method signature. |
| Form/ConfigurationForm.php | Reworks config form fields and adds test-email inputs. |
| EventListeners/CreditChangeListener.php | Removes legacy event subscriber sending implementation. |
| Event/CreditReminderEvents.php | Removes legacy event payload class. |
| CreditReminder.php | Changes activation logic, default config keys, and message creation approach. |
| Controller/Admin/CreditReminderController.php | Updates BO controller to save config and send test emails via service. |
| Config/thelia.sql | Adds new log table DDL (currently destructive via DROP/CREATE). |
| Config/schema.xml | Adds new Propel table mapping for credit_reminder_log. |
| Config/module.xml | Bumps version/Thelia requirement and removes <required> dependency section. |
| Config/config.xml | Registers new loop, new command, and renamed form. |
| Command/SendReminderCommand.php | Adds new CLI command CreditReminder:send. |
| Command/CreditReminderCommand.php | Removes legacy CLI command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use CreditReminder\Form\ConfigurationForm; | ||
| use CreditReminder\Model\CreditReminderLogQuery; |
There was a problem hiding this comment.
Unused imports: ConfigurationForm and CreditReminderLogQuery are imported but never referenced in this controller. Removing them avoids confusion and keeps the file clean.
| use CreditReminder\Form\ConfigurationForm; | |
| use CreditReminder\Model\CreditReminderLogQuery; |
| } | ||
| "name": "creditreminder/creditreminder", | ||
| "type": "thelia-module", | ||
| "description": "Send reminder emails to customers whose loyalty credits are about to expire", |
There was a problem hiding this comment.
composer.json no longer declares a license field. This is required/expected for many Composer/Packagist workflows and helps downstream users with compliance. Add back an appropriate SPDX license identifier (e.g. LGPL-3.0-or-later) if the project is still LGPL.
| "description": "Send reminder emails to customers whose loyalty credits are about to expire", | |
| "description": "Send reminder emails to customers whose loyalty credits are about to expire", | |
| "license": "proprietary", |
| -- Table pour stocker les informations sur les rappels d'expiration de crédit envoyés | ||
| SET FOREIGN_KEY_CHECKS = 0; | ||
|
|
||
| DROP TABLE IF EXISTS `credit_reminder`; | ||
| CREATE TABLE `credit_reminder` ( | ||
| `id` INTEGER NOT NULL AUTO_INCREMENT, | ||
| `customer_id` INTEGER NOT NULL, | ||
| `emails_sent` INTEGER DEFAULT 0 NOT NULL, | ||
| `last_sent_date` DATETIME, | ||
| `created_at` DATETIME, | ||
| `updated_at` DATETIME, | ||
| PRIMARY KEY (`id`), | ||
| INDEX `fk_credit_reminder_customer_id` (`customer_id`), | ||
| CONSTRAINT `fk_credit_reminder_customer_id` | ||
| FOREIGN KEY (`customer_id`) | ||
| REFERENCES `customer` (`id`) | ||
| ON DELETE CASCADE | ||
| ) ENGINE=InnoDB; | ||
|
|
||
| SET FOREIGN_KEY_CHECKS = 1; | ||
| -- Table pour logger les emails de rappel envoyés | ||
| DROP TABLE IF EXISTS `credit_reminder_log`; | ||
| CREATE TABLE `credit_reminder_log` ( | ||
| `id` INTEGER NOT NULL AUTO_INCREMENT, | ||
| `customer_id` INTEGER NOT NULL, |
There was a problem hiding this comment.
Config/thelia.sql drops and recreates credit_reminder / credit_reminder_log. If postActivation() ever re-runs this script (e.g. upgrade where is-initialized wasn’t set previously, or reinstall), existing reminder counters/logs will be destroyed. Prefer CREATE TABLE IF NOT EXISTS / ALTER TABLE style migrations (and avoid DROP TABLE) so upgrades/reactivations are non-destructive.
| public function postActivation(ConnectionInterface $con = null): void | ||
| { | ||
| try { | ||
| $this->setConfigVariables(); | ||
|
|
||
| // Ensure SQL schema is correctly installed | ||
| if (null === self::getConfigValue('is-initialized')) { | ||
| $database = new Database($con); | ||
| $database->insertSql(null, [__DIR__ . "/Config/thelia.sql"]); | ||
|
|
||
| // Create credit reminder message if it doesn't exist | ||
| if (null === MessageQuery::create()->findOneByName(self::CREDIT_REMINDER_MESSAGE_NAME)) { | ||
| $message = new Message(); | ||
| $email_templates_dir = __DIR__ . DIRECTORY_SEPARATOR . 'template' . DIRECTORY_SEPARATOR . 'backOffice' . DIRECTORY_SEPARATOR . 'default' . DIRECTORY_SEPARATOR . 'email' . DIRECTORY_SEPARATOR; | ||
|
|
||
| $message | ||
| ->setName(self::CREDIT_REMINDER_MESSAGE_NAME) | ||
| ->setLocale('en_US') | ||
| ->setTitle('Credit Reminder') | ||
| ->setSubject('Credit Reminder for your Account') | ||
| ->setHtmlMessage(file_get_contents($email_templates_dir . 'credit-reminder.html')) | ||
| ->setTextMessage(file_get_contents($email_templates_dir . 'credit-reminder.txt')) | ||
|
|
||
| ->setLocale('fr_FR') | ||
| ->setTitle('Rappel de Crédit') | ||
| ->setSubject('Rappel de Crédit pour votre Compte') | ||
| ->setHtmlMessage(file_get_contents($email_templates_dir . 'credit-reminder.html')) | ||
| ->setTextMessage(file_get_contents($email_templates_dir . 'credit-reminder.txt')) | ||
|
|
||
| ->save() | ||
| ; | ||
| } | ||
| } catch (\Exception $e) { | ||
| // Log or handle the exception as needed | ||
|
|
||
| self::setConfigValue('is-initialized', 1); | ||
| } |
There was a problem hiding this comment.
postActivation() runs the SQL install only when is-initialized is missing. This prevents DB schema changes (like the new credit_reminder_log table) from being applied on upgrades where the module is already initialized/activated. Consider implementing a versioned migration (e.g. store a schema/module version config and apply incremental SQL), or ensure upgrades run the needed ALTER/CREATE statements.
| <p> | ||
| ⏰ {intl l="We wanted to remind you that you have a loyalty credit of %amount that will expire on %date." amount={$credit_amount|default:0|number_format:2} date={$expiration_date|date_format:"%d/%m/%Y"|default:''} d="creditreminder"} | ||
| </p> |
There was a problem hiding this comment.
In Smarty, {$expiration_date|date_format:"%d/%m/%Y"|default:''} applies date_format before default. If expiration_date is missing/empty, this can render an incorrect date (often epoch/current) instead of empty. Apply default before date_format (or guard with {if}) for both the %date parameter and the “Expires on” line.
| {loop type="credit_reminder_log" name="reminder_logs" is_test="false"} | ||
| <tr> | ||
| <td>{$ID}</td> | ||
| <td>{$CUSTOMER_NAME}</td> | ||
| <td>{$EMAIL}</td> | ||
| <td>{$CREDIT_AMOUNT} €</td> | ||
| <td>{$EXPIRATION_DATE}</td> | ||
| <td>{$SENT_AT}</td> | ||
| <td> | ||
| {if $IS_TEST} | ||
| <span class="label label-warning">{intl d='creditreminder' l="Test"}</span> | ||
| {else} | ||
| <span class="label label-success">{intl d='creditreminder' l="Real"}</span> | ||
| {/if} |
There was a problem hiding this comment.
The “Sent reminder emails” table filters the loop with is_test="false", but still renders a “Test” column and branches on $IS_TEST—which will never be true with that filter. Either remove the filter to show both test/real emails, or drop the column/branch if only real emails should be listed. Also note sendTestEmail() doesn’t currently create a CreditReminderLog entry, so test sends won’t appear here even if you remove the filter.
| cd local/modules | ||
| git clone https://github.com/thelia-modules/CreditReminder | ||
| ``` | ||
| Vous pouvez également télécharger le module en ZIP et le décompresser le contenue dans le dossier : |
There was a problem hiding this comment.
README typo/grammar: “le décompresser le contenue” should be “et le décompresser … le contenu”. Fixing this improves clarity for installation instructions.
| Vous pouvez également télécharger le module en ZIP et le décompresser le contenue dans le dossier : | |
| Vous pouvez également télécharger le module en ZIP et en décompresser le contenu dans le dossier : |
| <thelia>2.5.5</thelia> | ||
| <stability>prod</stability> | ||
| <mandatory>0</mandatory> | ||
| <hidden>0</hidden> |
There was a problem hiding this comment.
Config/module.xml no longer declares CreditAccount as a required module, but this module still directly uses CreditAccount\Model\CreditAccountQuery / CreditAccountExpirationQuery (e.g. in Service/CreditReminderService). Without the dependency declaration, the module can be installed/activated without CreditAccount and will fatal at runtime. Re-add the <required> entry for CreditAccount (and an appropriate version constraint) or remove the hard dependency in code.
| <hidden>0</hidden> | |
| <hidden>0</hidden> | |
| <required> | |
| <module>CreditAccount</module> | |
| <version>1.0.0</version> | |
| </required> |
| <p style="margin: 0; color: #dc3545; font-size: 14px; font-weight: bold;"> | ||
| ⏳ {intl l="Expires on:" d="creditreminder"} {$expiration_date|date_format:"%d/%m/%Y"|default:''} | ||
| </p> |
There was a problem hiding this comment.
Same issue as above: {$expiration_date|date_format:"%d/%m/%Y"|default:''} formats before defaulting, which can display a wrong date when expiration_date is empty. Apply default first (or conditionally render).
| // Vérifier si le client existe | ||
| $customer = CustomerQuery::create()->findPk($customerId); | ||
| if (null === $customer) { | ||
| continue; | ||
| } | ||
|
|
||
| // Vérifier si un email a déjà été envoyé récemment | ||
| $reminderIntervalDays = intval(CreditReminder::getConfigValue(CreditReminder::REMINDER_INTERVAL_DAYS, 10)); | ||
|
|
||
| $lastLog = CreditReminderLogQuery::create() | ||
| ->filterByCustomerId($customerId) | ||
| ->filterByIsTest(false) | ||
| ->orderBySentAt(Criteria::DESC) | ||
| ->findOne($con); |
There was a problem hiding this comment.
getEligibleCustomers() loads all credit accounts with amount > 0 and then does per-row queries for expiration, customer, and last sent log. This is an N+1 pattern and will not scale with many customers/credits. Consider fetching eligible accounts in one query (join expiration + customer, and prefetch latest log per customer) or at least batch the expiration/customer lookups.
| // Vérifier si le client existe | |
| $customer = CustomerQuery::create()->findPk($customerId); | |
| if (null === $customer) { | |
| continue; | |
| } | |
| // Vérifier si un email a déjà été envoyé récemment | |
| $reminderIntervalDays = intval(CreditReminder::getConfigValue(CreditReminder::REMINDER_INTERVAL_DAYS, 10)); | |
| $lastLog = CreditReminderLogQuery::create() | |
| ->filterByCustomerId($customerId) | |
| ->filterByIsTest(false) | |
| ->orderBySentAt(Criteria::DESC) | |
| ->findOne($con); | |
| // Cache local statique pour éviter des appels répétés à la configuration et aux requêtes | |
| static $reminderIntervalDays = null; | |
| static $customerCache = []; | |
| static $lastLogCache = []; | |
| if (null === $reminderIntervalDays) { | |
| $reminderIntervalDays = intval(CreditReminder::getConfigValue(CreditReminder::REMINDER_INTERVAL_DAYS, 10)); | |
| } | |
| // Vérifier si le client existe (avec mise en cache) | |
| if (!array_key_exists($customerId, $customerCache)) { | |
| $customerCache[$customerId] = CustomerQuery::create()->findPk($customerId); | |
| } | |
| $customer = $customerCache[$customerId]; | |
| if (null === $customer) { | |
| continue; | |
| } | |
| // Vérifier si un email a déjà été envoyé récemment (avec mise en cache du dernier log) | |
| if (!array_key_exists($customerId, $lastLogCache)) { | |
| $lastLogCache[$customerId] = CreditReminderLogQuery::create() | |
| ->filterByCustomerId($customerId) | |
| ->filterByIsTest(false) | |
| ->orderBySentAt(Criteria::DESC) | |
| ->findOne($con); | |
| } | |
| $lastLog = $lastLogCache[$customerId]; |
No description provided.