Issue #247530 Fix : Resolved the issue while you try to edit notification it throws save error #176
Conversation
WalkthroughRefactors save() in src/com_tjnotifications/admin/models/notification.php to derive templateId consistently, validate backend entries and field structures, adjust timestamp handling for new vs existing backend configs, load existing configs by backend, and enhance error handling with backend-specific messages and template ID checks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin as Admin UI
participant Model as NotificationModel::save()
participant Template as TemplateTable
participant BackendCfg as BackendConfigTable
participant DB as Database
participant Err as Error Handler
Admin->>Model: submit notification data (template + backends)
Model->>Template: store/update template
Template-->>Model: templateId (existing or insertid)
alt missing templateId
Model->>Err: addError("Missing template ID")
Model-->>Admin: return false
else valid templateId
loop each backend entry
alt backend entry/fields invalid
Model->>Model: skip backend
else valid
Model->>BackendCfg: load by templateId + backend
alt existing record
BackendCfg->>BackendCfg: set updated_on (preserve created_on)
else new record
BackendCfg->>BackendCfg: set created_on & updated_on (now)
end
BackendCfg->>DB: save config
alt DB error
Model->>Err: addError("Backend save failed: <backend> + details")
Model-->>Admin: return false
end
end
end
Model-->>Admin: return success (true)
end
Note over Model,BackendCfg: Webhook validation (global vs custom URL) retained within per-backend flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/com_tjnotifications/admin/models/notification.php (1)
411-413: Use templateId (newly created) when fetching existing configsFor new records, $data['id'] is empty, causing lookups to fail. Use the resolved $templateId.
- $existingBackendConfigs = $this->getExistingTemplates($data['id'], $backend); + $existingBackendConfigs = $this->getExistingTemplates($templateId, $backend);
🧹 Nitpick comments (5)
src/com_tjnotifications/install.tjnotifications.php (1)
134-158: Column-add logic is fine; consider guarding with try/catch for resilienceOptional: wrap ALTERs in try/catch to prevent a single failure from aborting the whole update and log a warning.
- if (!array_key_exists('webhook_url', $columns)) { - $db->setQuery("ALTER TABLE `#__tj_notification_template_configs` ADD COLUMN `webhook_url` text DEFAULT NULL AFTER `body`;"); - $db->execute(); - } + try { + if (!array_key_exists('webhook_url', $columns)) { + $db->setQuery("ALTER TABLE `#__tj_notification_template_configs` ADD COLUMN `webhook_url` text DEFAULT NULL AFTER `body`;"); + $db->execute(); + } + } catch (\Throwable $e) { + // Optional: log warning + }src/com_tjnotifications/admin/models/notification.php (4)
417-433: Webhook validation — guard against missing ‘state’ and confirm language stringsUse isset/!empty for state to avoid notices. Ensure error strings exist in language files.
- if ($backend == 'webhook' && $data[$backend]['state']) + if ($backend == 'webhook' && !empty($data[$backend]['state'])) {
535-538: Store webhook_url as NULL when empty; avoid empty-string JSONAlign with schema defaults and avoid saving empty strings.
- $templateConfigTable->webhook_url = !empty($backendFieldValues['webhook_url']) ? json_encode($backendFieldValues['webhook_url']): ''; + $templateConfigTable->webhook_url = !empty($backendFieldValues['webhook_url']) + ? json_encode($backendFieldValues['webhook_url']) + : null;
386-387: Minor: unused $keyBackendIndex is unused; simplify foreach signature.
- foreach ($backendsArray as $keyBackend => $backend) + foreach ($backendsArray as $backend)
769-773: Set updated_on for new backend rows; avoid empty stringEmpty datetime can still cause issues under strict SQL modes. Prefer now() or NULL consistently.
- $templateConfigTable->created_on = Factory::getDate('now')->toSQL(); - $templateConfigTable->updated_on = ''; + $templateConfigTable->created_on = Factory::getDate('now')->toSQL(); + $templateConfigTable->updated_on = Factory::getDate('now')->toSQL();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (11)
src/com_tjnotifications/admin/config.xmlis excluded by!**/*.xmlsrc/com_tjnotifications/admin/language/en-GB.com_tjnotifications.iniis excluded by!**/language/**/*.inisrc/com_tjnotifications/admin/models/forms/global_config_webhookurl.xmlis excluded by!**/*.xmlsrc/com_tjnotifications/admin/models/forms/notification.xmlis excluded by!**/*.xmlsrc/com_tjnotifications/admin/models/forms/webhookfields.xmlis excluded by!**/*.xmlsrc/com_tjnotifications/admin/models/forms/webhookurl.xmlis excluded by!**/*.xmlsrc/com_tjnotifications/tjnotifications.xmlis excluded by!**/*.xmlsrc/plugins/actionlog/tjnotification/tjnotification.xmlis excluded by!**/*.xmlsrc/plugins/api/tjnotifications/tjnotifications.xmlis excluded by!**/*.xmlsrc/plugins/privacy/tjnotification/tjnotification.xmlis excluded by!**/*.xmlsrc/plugins/user/tjnotificationsmobilenumber/tjnotificationsmobilenumber.xmlis excluded by!**/*.xml
📒 Files selected for processing (9)
.coderabbit.yaml(1 hunks)src/com_tjnotifications/admin/defines.php(1 hunks)src/com_tjnotifications/admin/models/fields/backends.php(1 hunks)src/com_tjnotifications/admin/models/notification.php(6 hunks)src/com_tjnotifications/admin/models/notifications.php(2 hunks)src/com_tjnotifications/admin/sql/install.mysql.utf8.sql(2 hunks)src/com_tjnotifications/admin/sql/updates/mysql/2.1.0.sql(1 hunks)src/com_tjnotifications/admin/sql/updates/mysql/5.1.2.sql(1 hunks)src/com_tjnotifications/install.tjnotifications.php(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: "Review the PHP code for Joomla coding standards compliance, ensuring:
- Code follows Joomla's coding standards, validated using PHPCS with the Joomla standard.
- Adherence to Joomla’s MVC structure for extensions.
- Proper use of Joomla’s built-in functions (e.g., Input, Factory).
- SQL queries use Joomla’s database API to prevent SQL injection.
- Code is well-documented, with clear function and class descriptions."
Files:
src/com_tjnotifications/admin/models/notifications.phpsrc/com_tjnotifications/admin/models/fields/backends.phpsrc/com_tjnotifications/install.tjnotifications.phpsrc/com_tjnotifications/admin/models/notification.phpsrc/com_tjnotifications/admin/defines.php
🪛 PHPMD (2.15.0)
src/com_tjnotifications/admin/models/notification.php
386-386: Avoid unused local variables such as '$keyBackend'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (8)
.coderabbit.yaml (1)
11-35: Verify XML exclusion won’t hide critical Joomla manifests/formsExcluding all XML may skip reviewing manifest/form definitions which often carry schema, menu, and form changes.
Would you like to narrow the exclusion (e.g., only exclude media/layout XML) so manifest/form XML still get reviewed?
src/com_tjnotifications/admin/models/notifications.php (2)
321-329: Include newly required webhook fields in SELECT — LGTMSelecting provider_template_id, webhook_url, and use_global_webhook_url is correct and consistent.
340-348: Fallback SELECT updated with webhook fields — LGTMThe union branch matches the first SELECT; column order is consistent.
src/com_tjnotifications/admin/models/fields/backends.php (1)
45-52: New “webhook” option added — confirm language string existsEnsure COM_TJNOTIFICATIONS_BACKEND_WEBHOOK is present in the language ini.
src/com_tjnotifications/install.tjnotifications.php (1)
125-133: Calling addMissingColumns() during update — LGTMThis complements migrations and avoids duplicate ALTERs via column-existence checks.
src/com_tjnotifications/admin/sql/updates/mysql/5.1.2.sql (1)
4-11: Allowing NULL datetimes to fix save errors — LGTMThis addresses incorrect/empty datetime save failures on edit.
src/com_tjnotifications/admin/sql/updates/mysql/2.1.0.sql (1)
1-3: Webhook columns migration — LGTMColumns and defaults align with installer checks and model usage.
src/com_tjnotifications/admin/models/notification.php (1)
498-510: Datetime handling for configs — LGTMAlways setting created_on/updated_on appropriately resolves empty datetime save errors.
| if (!defined('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY')) | ||
| { | ||
| define('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY', "email,sms"); | ||
| define('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY', "email,sms,webhook"); | ||
| } |
There was a problem hiding this comment.
Backends constant misses ‘push’ and ‘whatsapp’ — causes data loss/skip
Forms expose push and whatsapp; model loops use this constant. Current value will skip saving/loading those backends.
Apply:
- define('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY', "email,sms,webhook");
+ define('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY', "email,sms,push,whatsapp,webhook");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!defined('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY')) | |
| { | |
| define('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY', "email,sms"); | |
| define('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY', "email,sms,webhook"); | |
| } | |
| if (!defined('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY')) | |
| { | |
| define('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY', "email,sms,push,whatsapp,webhook"); | |
| } |
🤖 Prompt for AI Agents
In src/com_tjnotifications/admin/defines.php around lines 13 to 16, the
TJNOTIFICATIONS_CONST_BACKENDS_ARRAY value omits "push" and "whatsapp", causing
those backends to be skipped when saving/loading; update the constant to include
push and whatsapp in the comma-separated list (e.g.
"email,sms,webhook,push,whatsapp") so the model loop will process them; ensure
the new values match form/backend identifiers used elsewhere.
| // Get global configs | ||
| $notificationsParams = ComponentHelper::getParams('com_tjnotifications'); | ||
| $webhookUrls = $notificationsParams->get('webhook_url'); | ||
| $webhookUrls = array_column((array) $webhookUrls, 'url'); | ||
|
|
There was a problem hiding this comment.
array_column on objects may yield empty results
Component params may return an array of objects; array_column won’t extract object properties reliably in all environments.
Use a robust extractor:
- $webhookUrls = $notificationsParams->get('webhook_url');
- $webhookUrls = array_column((array) $webhookUrls, 'url');
+ $webhookUrlsRaw = (array) $notificationsParams->get('webhook_url');
+ $webhookUrls = array_values(array_filter(array_map(
+ static function ($item) {
+ if (is_array($item)) {
+ return $item['url'] ?? null;
+ }
+ if (is_object($item)) {
+ return $item->url ?? null;
+ }
+ return null;
+ },
+ $webhookUrlsRaw
+ )));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Get global configs | |
| $notificationsParams = ComponentHelper::getParams('com_tjnotifications'); | |
| $webhookUrls = $notificationsParams->get('webhook_url'); | |
| $webhookUrls = array_column((array) $webhookUrls, 'url'); | |
| // Get global configs | |
| $notificationsParams = ComponentHelper::getParams('com_tjnotifications'); | |
| $webhookUrlsRaw = (array) $notificationsParams->get('webhook_url'); | |
| $webhookUrls = array_values( | |
| array_filter( | |
| array_map( | |
| static function ($item) { | |
| if (is_array($item)) { | |
| return $item['url'] ?? null; | |
| } | |
| if (is_object($item)) { | |
| return $item->url ?? null; | |
| } | |
| return null; | |
| }, | |
| $webhookUrlsRaw | |
| ) | |
| ) | |
| ); |
🤖 Prompt for AI Agents
In src/com_tjnotifications/admin/models/notification.php around lines 378 to
382, the call to array_column on
ComponentHelper::getParams('com_tjnotifications')->get('webhook_url') can fail
when entries are objects; replace array_column with a robust extractor that
iterates the returned value (handle null, string, array, object), for each entry
detect if it's an object or array and read the 'url' property/key (or the value
itself if it's a plain string), validate/trim non-empty strings and push them
into a new array, and assign that sanitized array to $webhookUrls.
| `created_on` datetime NOT NULL DEFAULT '0000-00-00 00:00:00', | ||
| `updated_on` datetime NOT NULL DEFAULT '0000-00-00 00:00:00', |
There was a problem hiding this comment.
Stop using 0000-00-00 00:00:00 defaults for datetime columns.
created_on/updated_on now default to '0000-00-00 00:00:00' and are NOT NULL. On MySQL 5.7+/MariaDB installations running in strict mode (NO_ZERO_DATE/NO_ZERO_IN_DATE), this default is rejected and the whole install/upgrade fails. Please keep these columns nullable (or use a valid default such as CURRENT_TIMESTAMP) so the schema can be applied everywhere.
- `created_on` datetime NOT NULL DEFAULT '0000-00-00 00:00:00',
- `updated_on` datetime NOT NULL DEFAULT '0000-00-00 00:00:00',
+ `created_on` datetime DEFAULT NULL,
+ `updated_on` datetime DEFAULT NULL,🤖 Prompt for AI Agents
In src/com_tjnotifications/admin/sql/install.mysql.utf8.sql around lines 35-36
the datetime columns use NOT NULL DEFAULT '0000-00-00 00:00:00', which fails
under strict MySQL/MariaDB modes; change these columns to either be nullable
with no default (allow NULLs) or use valid defaults such as DEFAULT
CURRENT_TIMESTAMP for created_on and DEFAULT CURRENT_TIMESTAMP ON UPDATE
CURRENT_TIMESTAMP for updated_on so the schema applies on strict servers.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/com_tjnotifications/admin/models/notification.php(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: "Review the PHP code for Joomla coding standards compliance, ensuring:
- Code follows Joomla's coding standards, validated using PHPCS with the Joomla standard.
- Adherence to Joomla’s MVC structure for extensions.
- Proper use of Joomla’s built-in functions (e.g., Input, Factory).
- SQL queries use Joomla’s database API to prevent SQL injection.
- Code is well-documented, with clear function and class descriptions."
Files:
src/com_tjnotifications/admin/models/notification.php
🔇 Additional comments (1)
src/com_tjnotifications/admin/models/notification.php (1)
379-382: array_column still drops object-based webhook URLsComponent params frequently return arrays of stdClass objects;
array_columnon that structure yields an empty array, so you’ll reject valid global webhook URLs and block saves. Please switch to an extractor that accepts both arrays and objects (as flagged earlier).- $webhookUrls = $notificationsParams->get('webhook_url'); - $webhookUrls = array_column((array) $webhookUrls, 'url'); + $webhookUrlsRaw = (array) $notificationsParams->get('webhook_url'); + $webhookUrls = array_values(array_filter(array_map( + static function ($item) { + if (is_array($item)) { + return !empty($item['url']) ? $item['url'] : null; + } + + if (is_object($item)) { + return !empty($item->url) ? $item->url : null; + } + + return is_string($item) && trim($item) !== '' ? $item : null; + }, + $webhookUrlsRaw + )));
| $templateConfigTable = Table::getInstance('Template', 'TjnotificationTable', array('dbo', $db)); | ||
| $templateConfigTable->load(array('template_id' => $templateId, 'backend' => $backendName)); | ||
| $isExistingRecord = $templateConfigTable->load(array('template_id' => $templateId, 'backend' => $backend)); | ||
|
|
||
| // Non-repeat data | ||
| $templateConfigTable->template_id = $templateId; | ||
| $templateConfigTable->backend = $backend; | ||
| $templateConfigTable->state = $data[$backend]['state']; | ||
| $templateConfigTable->created_on = $createdOn; | ||
| $templateConfigTable->updated_on = $updatedOn; | ||
|
|
||
| // Set datetime fields with proper values | ||
| // For existing records, only update the updated_on field | ||
| if ($isExistingRecord && !empty($templateConfigTable->created_on)) | ||
| { | ||
| $templateConfigTable->updated_on = $updatedOn; | ||
| // Keep existing created_on value | ||
| } | ||
| else | ||
| { | ||
| // For new records, set both created_on and updated_on | ||
| $templateConfigTable->created_on = $createdOn; | ||
| $templateConfigTable->updated_on = $updatedOn; | ||
| } |
There was a problem hiding this comment.
Do not preload the backend config with a generic template_id/backend lookup
Because $templateConfigTable->load() runs on every iteration with only template_id/backend, the table object keeps the first matching row’s primary key. When you later save a repeatable item whose id is empty, that stale primary key forces save() to overwrite an existing row instead of inserting a new one. Even when an id is present, the preserved created_on value now comes from whichever row happened to load first, so timestamps drift between language records. Please load by the actual repeatable row id (only when it’s provided) and keep the table reset for new rows so inserts stay inserts.
- $templateConfigTable = Table::getInstance('Template', 'TjnotificationTable', array('dbo', $db));
- $isExistingRecord = $templateConfigTable->load(array('template_id' => $templateId, 'backend' => $backend));
+ $templateConfigTable = Table::getInstance('Template', 'TjnotificationTable', array('dbo', $db));
+ $templateConfigTable->reset();
+
+ $isExistingRecord = false;
+
+ if (!empty($backendFieldValues['id']))
+ {
+ $isExistingRecord = $templateConfigTable->load((int) $backendFieldValues['id']);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $templateConfigTable = Table::getInstance('Template', 'TjnotificationTable', array('dbo', $db)); | |
| $templateConfigTable->load(array('template_id' => $templateId, 'backend' => $backendName)); | |
| $isExistingRecord = $templateConfigTable->load(array('template_id' => $templateId, 'backend' => $backend)); | |
| // Non-repeat data | |
| $templateConfigTable->template_id = $templateId; | |
| $templateConfigTable->backend = $backend; | |
| $templateConfigTable->state = $data[$backend]['state']; | |
| $templateConfigTable->created_on = $createdOn; | |
| $templateConfigTable->updated_on = $updatedOn; | |
| // Set datetime fields with proper values | |
| // For existing records, only update the updated_on field | |
| if ($isExistingRecord && !empty($templateConfigTable->created_on)) | |
| { | |
| $templateConfigTable->updated_on = $updatedOn; | |
| // Keep existing created_on value | |
| } | |
| else | |
| { | |
| // For new records, set both created_on and updated_on | |
| $templateConfigTable->created_on = $createdOn; | |
| $templateConfigTable->updated_on = $updatedOn; | |
| } | |
| $templateConfigTable = Table::getInstance('Template', 'TjnotificationTable', array('dbo', $db)); | |
| $templateConfigTable->reset(); | |
| $isExistingRecord = false; | |
| if (!empty($backendFieldValues['id'])) | |
| { | |
| $isExistingRecord = $templateConfigTable->load((int) $backendFieldValues['id']); | |
| } | |
| // Non-repeat data | |
| $templateConfigTable->template_id = $templateId; | |
| $templateConfigTable->backend = $backend; | |
| $templateConfigTable->state = $data[$backend]['state']; | |
| // Set datetime fields with proper values | |
| // For existing records, only update the updated_on field | |
| if ($isExistingRecord && !empty($templateConfigTable->created_on)) | |
| { | |
| $templateConfigTable->updated_on = $updatedOn; | |
| // Keep existing created_on value | |
| } | |
| else | |
| { | |
| // For new records, set both created_on and updated_on | |
| $templateConfigTable->created_on = $createdOn; | |
| $templateConfigTable->updated_on = $updatedOn; | |
| } |
🤖 Prompt for AI Agents
In src/com_tjnotifications/admin/models/notification.php around lines 490-510,
avoid preloading the backend config using only template_id/backend because that
preserves the first row’s primary key and created_on across repeatable
iterations; instead, only call ->load() with the specific repeatable row id when
that id is present, and for new repeatable rows ensure the Table object is reset
(clear id/primary key and unset created_on) so save() performs an INSERT; when
an existing id is loaded, update updated_on only and leave created_on intact.
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements