Issue #239557 Fix : IT is SHOWING error when we try to save I have just added attendee name tag so that in mail attendee name should display. #178
Conversation
…st added attendee name tag so that in mail attendee name should display.
WalkthroughThis pull request adds webhook backend support to the notification system. Changes include a new review configuration file, updates to backend constants and options, enhanced notification model logic with webhook validation, database schema modifications to support webhook URLs and global webhook settings, SQL migrations, and installer schema augmentation. Changes
Sequence DiagramsequenceDiagram
actor Admin
participant UI as Admin Form
participant Model as Notification Model
participant DB as Database
participant Validator as Validation Layer
Admin->>UI: Configure Notification (select webhook backend)
UI->>Model: save(data)
rect rgb(200, 220, 255)
Note over Model: Backend Validation
Model->>Validator: Validate backend config
alt Email Backend
Validator->>Validator: Check subject present
else Webhook Backend
Validator->>Model: Fetch global webhook URLs
Validator->>Validator: Check custom URLs or global flag
end
Validator-->>Model: Validation result
end
alt Validation passes
Model->>DB: Insert/Update template config
Model->>DB: Store webhook_url & use_global_webhook_url
DB-->>Model: Template ID (insertid/data[id])
Model->>Model: Update model state with template ID
Model-->>UI: Success
else Validation fails
Model->>Model: Log error
Model-->>UI: Error response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
📜 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 (15)
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/emailfields.xmlis excluded by!**/*.xmlsrc/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/pushfields.xmlis excluded by!**/*.xmlsrc/com_tjnotifications/admin/models/forms/smsfields.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/admin/models/forms/whatsappfields.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 (8)
.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(8 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/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/fields/backends.phpsrc/com_tjnotifications/admin/defines.phpsrc/com_tjnotifications/admin/models/notification.phpsrc/com_tjnotifications/admin/models/notifications.phpsrc/com_tjnotifications/install.tjnotifications.php
🔇 Additional comments (1)
src/com_tjnotifications/admin/sql/install.mysql.utf8.sql (1)
1-74: PR description mismatch: title mentions "attendee name tag" but schema shows webhook support.The PR title states the fix is for adding an "attendee name tag" to mail display, but this SQL schema file only shows:
- Webhook URL columns being added
- NULL→NOT NULL constraints being tightened
- No new "attendee name" or related columns visible
Please clarify:
- Is the "attendee name tag" actually being added in this PR? If so, which column represents it?
- Are the webhook-related changes the primary intent, or is this an auto-generated summary mismatch?
- Should there be other files in this PR that handle the attendee name logic?
| 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.
Don't drop existing backends from TJNOTIFICATIONS_CONST_BACKENDS_ARRAY
Line 15 now limits TJNOTIFICATIONS_CONST_BACKENDS_ARRAY to email,sms,webhook. Every place we iterate this constant (for example the foreach in src/com_tjnotifications/admin/models/notification.php) will now completely ignore previously supported backends such as push and whatsapp, so their configs will neither load nor persist. That’s a regression for existing installations. Please restore the prior entries and just append webhook to the list.
Apply this diff to keep the existing backends and add webhook:
- define('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY', "email,sms,webhook");
+ define('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY', "email,push,sms,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.
| define('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY', "email,sms,webhook"); | |
| define('TJNOTIFICATIONS_CONST_BACKENDS_ARRAY', "email,push,sms,whatsapp,webhook"); |
🤖 Prompt for AI Agents
In src/com_tjnotifications/admin/defines.php around line 15, the
TJNOTIFICATIONS_CONST_BACKENDS_ARRAY constant was overwritten to
"email,sms,webhook", which removes previously supported backends (e.g., push,
whatsapp) and breaks config loading/persistence; restore the original list of
backends and ensure webhook is appended instead of replacing entries — update
the constant value to include all prior backend identifiers plus "webhook"
(preserving order/duplicates as before).
| $notificationsParams = ComponentHelper::getParams('com_tjnotifications'); | ||
| $webhookUrls = $notificationsParams->get('webhook_url'); | ||
| $webhookUrls = array_column((array) $webhookUrls, 'url'); | ||
|
|
There was a problem hiding this comment.
Guard array_column() against non-array params
On Lines 378-381 we do array_column((array) $webhookUrls, 'url'). On a fresh install ComponentHelper::getParams('com_tjnotifications')->get('webhook_url') returns an empty string, and casting that to an array still leaves a scalar element. array_column() then emits array_column(): Value of type string is not array at runtime. Please normalise the value before calling array_column() (e.g. convert traversables to arrays and bail out with an empty array when the param isn’t an array yet).
Apply this diff to harden the code:
- $notificationsParams = ComponentHelper::getParams('com_tjnotifications');
- $webhookUrls = $notificationsParams->get('webhook_url');
- $webhookUrls = array_column((array) $webhookUrls, 'url');
+ $notificationsParams = ComponentHelper::getParams('com_tjnotifications');
+ $webhookUrlsParam = $notificationsParams->get('webhook_url');
+
+ if ($webhookUrlsParam instanceof \Traversable)
+ {
+ $webhookUrlsParam = iterator_to_array($webhookUrlsParam);
+ }
+
+ $webhookUrls = array();
+
+ if (is_array($webhookUrlsParam))
+ {
+ $webhookUrls = array_column($webhookUrlsParam, 'url');
+ }📝 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.
| $notificationsParams = ComponentHelper::getParams('com_tjnotifications'); | |
| $webhookUrls = $notificationsParams->get('webhook_url'); | |
| $webhookUrls = array_column((array) $webhookUrls, 'url'); | |
| $notificationsParams = ComponentHelper::getParams('com_tjnotifications'); | |
| $webhookUrlsParam = $notificationsParams->get('webhook_url'); | |
| if ($webhookUrlsParam instanceof \Traversable) | |
| { | |
| $webhookUrlsParam = iterator_to_array($webhookUrlsParam); | |
| } | |
| $webhookUrls = array(); | |
| if (is_array($webhookUrlsParam)) | |
| { | |
| $webhookUrls = array_column($webhookUrlsParam, 'url'); | |
| } |
🤖 Prompt for AI Agents
In src/com_tjnotifications/admin/models/notification.php around lines 378-381,
the code currently casts the webhook param to an array then calls array_column,
which fails when the param is an empty string or otherwise not an array of
arrays; instead, normalize the param first: if it's a Traversable convert to
array, if it's not an array after that (or is an empty string/NULL/scalar) set
$webhookUrls to an empty array, and only then call array_column($webhookUrls,
'url'); this ensures array_column always receives an array of arrays and
prevents the runtime warning.
| `subject` text NOT NULL, | ||
| `body` text NOT NULL, | ||
| `webhook_url` text DEFAULT NULL, | ||
| `use_global_webhook_url` TINYINT(1) NOT NULL DEFAULT '1' COMMENT 'Use Global Config Webhook URLs', | ||
| `params` text NOT NULL, | ||
| `state` int(11) NOT NULL, | ||
| `created_on` datetime NOT NULL DEFAULT '0000-00-00 00:00:00', | ||
| `updated_on` datetime NOT NULL DEFAULT '0000-00-00 00:00:00', | ||
| `is_override` int(1) NOT NULL, | ||
| `provider_template_id` varchar(255) DEFAULT NULL, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify NULL→NOT NULL schema changes are covered by migrations for existing installations.
Multiple columns in __tj_notification_template_configs were changed from nullable to NOT NULL without explicit DEFAULT values (state, is_override). Additionally, critical columns (subject, body, params) changed from DEFAULT NULL to NOT NULL. If this PR targets existing installations with data, these changes will fail on upgrade unless handled by a migration script.
While this is acceptable for fresh installations, please verify that:
- A migration/update SQL file exists (e.g.,
2.1.0.sqlmentioned in AI summary) that handles existing NULL data before enforcing NOT NULL constraints. - Any PHP installer logic conditionally adds these columns/constraints for existing installations.
- INSERT operations in the codebase always provide explicit values for these NOT NULL columns.
You can verify this by running:
🏁 Script executed:
#!/bin/bash
# Check for migration files that might handle this schema transition
fd -e sql -type f | grep -E "(update|migration|2\.1\.0)" | head -20
# Check if installer has conditional column addition logic
grep -r "webhook_url\|use_global_webhook_url" src/com_tjnotifications/install* 2>/dev/null | head -10Length of output: 787
I can see partial results from the migration logic. Let me gather more comprehensive information to verify the schema change handling.
Now let me verify the migration files and INSERT operations in the codebase:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find migration/update SQL files
find src/com_tjnotifications -name "*.sql" -type f 2>/dev/null
# Check manifest file for update schema path configuration
find src/com_tjnotifications -name "*.xml" -type f 2>/dev/null | head -5
# Show full context of the installer logic from grep output
grep -r "array_key_exists.*webhook" src/com_tjnotifications/install* 2>/dev/null -A 3 -B 1
Length of output: 1581
Let me examine the migration file and compare schema versions:
---
🏁 Script executed:
```shell
#!/bin/bash
# Show content of 2.1.0.sql migration file
cat src/com_tjnotifications/admin/sql/updates/mysql/2.1.0.sql
# Show content of an earlier migration for comparison
echo "=== 2.0.1.sql ==="
cat src/com_tjnotifications/admin/sql/updates/mysql/2.0.1.sql 2>/dev/null || echo "Not found"
# Search for INSERT statements on tj_notification_template_configs
grep -r "INSERT.*tj_notification_template_configs" src/ 2>/dev/null
Length of output: 528
Let me trace the schema evolution to understand when these columns were created and their original nullability:
---
🏁 Script executed:
```shell
#!/bin/bash
# Check all migration files in order to trace schema evolution
for file in src/com_tjnotifications/admin/sql/updates/mysql/*.sql; do
echo "=== $(basename $file) ==="
cat "$file"
echo ""
done
# Also check install file for full schema
echo "=== install.mysql.utf8.sql (full file) ==="
cat src/com_tjnotifications/admin/sql/install.mysql.utf8.sql
Length of output: 12714
install.mysql.utf8.sql schema is inconsistent with migration path and will cause INSERT failures on fresh installations.
The current fresh-install schema differs critically from the migrated production schema:
-
Missing DEFAULT values on NOT NULL columns: The install file defines
subject,body,paramsasNOT NULLwithout DEFAULT, while 3.0.0.sql migration addsDEFAULT NULLfor these columns on upgrades. Fresh installations cannot safely INSERT rows without explicit values for every column. -
Inconsistent state/is_override defaults: 3.0.0.sql adds
DEFAULT 0tostateandis_override, but install.mysql.utf8.sql has neither—these should beint(11) NOT NULL DEFAULT 0andint(1) NOT NULL DEFAULT 0respectively. -
Invalid datetime defaults: Columns
created_onandupdated_onuse'0000-00-00 00:00:00'which is disabled in MySQL 5.7.4+ strict mode; 3.0.0.sql correctly changed these toNULL DEFAULT NULL.
Fix required: Update install.mysql.utf8.sql to match the final migrated schema from 3.0.0.sql:
- Change
subject text NOT NULL→subject text DEFAULT NULL - Change
body text NOT NULL→body text DEFAULT NULL - Change
params text NOT NULL→params text DEFAULT NULL - Change
state int(11) NOT NULL→state int(11) NOT NULL DEFAULT 0 - Change
is_override int(1) NOT NULL→is_override int(1) NOT NULL DEFAULT 0 - Change
created_on datetime NOT NULL DEFAULT '0000-00-00 00:00:00'→created_on datetime NULL DEFAULT NULL - Change
updated_on datetime NOT NULL DEFAULT '0000-00-00 00:00:00'→updated_on datetime NULL DEFAULT NULL
🤖 Prompt for AI Agents
In src/com_tjnotifications/admin/sql/install.mysql.utf8.sql around lines 29-38,
the schema differs from the migrated 3.0.0 schema and will cause INSERT failures
(NOT NULL text columns without defaults, missing DEFAULT 0 for
state/is_override, and invalid '0000-00-00 00:00:00' datetime defaults); update
the file to match the migration by changing subject, body, and params to "text
DEFAULT NULL", setting state to "int(11) NOT NULL DEFAULT 0" and is_override to
"int(1) NOT NULL DEFAULT 0", and changing created_on/updated_on to "datetime
NULL DEFAULT NULL".
| CREATE TABLE IF NOT EXISTS `#__tj_notification_logs` ( | ||
| `id` int(11) NOT NULL AUTO_INCREMENT, | ||
| `key` varchar(100) NOT NULL DEFAULT '', | ||
| `client` varchar(100) NOT NULL DEFAULT '', | ||
| `backend` varchar(50) NOT NULL DEFAULT '', | ||
| `subject` varchar(250) NOT NULL DEFAULT '', | ||
| `title` varchar(100) NOT NULL DEFAULT '', | ||
| `body` text DEFAULT NULL, | ||
| `from` varchar(500) NOT NULL DEFAULT '', | ||
| `to` text DEFAULT NULL, | ||
| `cc` text DEFAULT NULL, | ||
| `bcc` text DEFAULT NULL, | ||
| `date` datetime NULL DEFAULT NULL, | ||
| `state` tinyint(2) NOT NULL DEFAULT 0, | ||
| `params` text DEFAULT NULL, | ||
| `priority`int(11) NOT NULL DEFAULT 0, | ||
| `message` text DEFAULT NULL, | ||
| `category` text DEFAULT NULL, | ||
| `key` varchar(100) NOT NULL, | ||
| `client` varchar(100) NOT NULL, | ||
| `backend` varchar(50) NOT NULL, | ||
| `subject` varchar(250) NOT NULL, | ||
| `title` varchar(100) NOT NULL, | ||
| `body` text NOT NULL, | ||
| `webhook_url` text DEFAULT NULL, | ||
| `from` varchar(500) NOT NULL, | ||
| `to` text NOT NULL, | ||
| `cc` text NOT NULL, | ||
| `bcc` text NOT NULL, | ||
| `date` datetime NOT NULL, | ||
| `state` tinyint(2) NOT NULL, | ||
| `params` text NOT NULL, | ||
| `priority`int(11) NOT NULL, | ||
| `message` text NOT NULL, | ||
| `category` text NOT NULL, |
There was a problem hiding this comment.
🧩 Analysis chain
Address risky NULL→NOT NULL column constraints in __tj_notification_logs table.
The __tj_notification_logs table has many columns changed from DEFAULT '' or DEFAULT NULL to NOT NULL without explicit defaults:
- key, client, backend, subject, title, body, from, to, cc, bcc, date, state, params, priority, message, category
This creates insert/update hazards if not handled properly in migrations. Additionally, columns like to, cc, and bcc may legitimately be empty in some notifications, making NOT NULL without a default semantically problematic.
Ensure that:
- A migration file handles existing NULL/empty values before constraints are enforced.
- All code paths that insert into this table provide values for all NOT NULL columns.
- The NOT NULL constraint for email fields (to, cc, bcc) is intentional and aligns with business logic.
Run this verification:
🏁 Script executed:
#!/bin/bash
# Search for INSERT statements into tj_notification_logs table
rg -n "INSERT INTO.*tj_notification_logs" src/ --type sql --type php -C2 | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find all references to tj_notification_logs in the codebase
rg "tj_notification_logs" --type-list | head -20Length of output: 389
🏁 Script executed:
#!/bin/bash
# Search for all migration files
fd -e sql -e php -e mysql | grep -i migrat | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look at the SQL install file directly
fd -n "install.mysql.utf8.sql" Length of output: 230
🏁 Script executed:
#!/bin/bash
# Broader search for any INSERT/UPDATE operations on tj_notification_logs (case-insensitive)
rg -i "tj_notification_logs" -n | head -40Length of output: 3281
🏁 Script executed:
#!/bin/bash
# Search for any INSERT operations in PHP files
rg -n "INSERT" --type php | grep -i "notification\|insert" | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the log.php table class for save/bind/insert handling
cat src/com_tjnotifications/admin/tables/log.phpLength of output: 623
🏁 Script executed:
#!/bin/bash
# Look at the 3.0.0.sql migration file in full
cat src/com_tjnotifications/admin/sql/updates/mysql/3.0.0.sqlLength of output: 5305
🏁 Script executed:
#!/bin/bash
# Find all SQL update files to understand version progression
find src -name "*.sql" | grep -E "updates|install" | sortLength of output: 463
🏁 Script executed:
#!/bin/bash
# Check install.mysql.utf8.sql for the actual full table definition
cat src/com_tjnotifications/admin/sql/install.mysql.utf8.sqlLength of output: 4003
Update install.mysql.utf8.sql to include DEFAULT values matching migration 3.0.0.sql.
The migration 3.0.0.sql properly addresses existing data by adding DEFAULT values before enforcing NOT NULL constraints. However, fresh installs at v3.0.0+ using the current install.mysql.utf8.sql will encounter insert failures if all NOT NULL columns aren't provided.
Key issues:
key,client,backend,subject,title,from,state,priority: Should haveDEFAULT ''orDEFAULT 0to,cc,bcc,body,params,message,category: Should haveDEFAULT NULL(migration 3.0.0 already corrects this for upgrades)date: Should haveDEFAULT NULL
Align install.mysql.utf8.sql with the defaults already applied in migration 3.0.0.sql to ensure consistency across fresh installs and upgrades.
| `date` datetime NOT NULL, | ||
| `state` tinyint(2) NOT NULL, | ||
| `params` text NOT NULL, | ||
| `priority`int(11) NOT NULL, |
There was a problem hiding this comment.
Fix syntax error: missing space before type declaration.
Line 70 has a formatting error with missing space between column name and type.
Apply this diff:
- `priority`int(11) NOT NULL,
+ `priority` int(11) NOT NULL,📝 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.
| `priority`int(11) NOT NULL, | |
| `priority` int(11) NOT NULL, |
🤖 Prompt for AI Agents
In src/com_tjnotifications/admin/sql/install.mysql.utf8.sql around line 70, the
column definition "`priority`int(11) NOT NULL," is missing a space between the
column name and the type; update it to include a space so it reads "`priority`
int(11) NOT NULL," (ensure spacing matches surrounding definitions and save the
file).
1 participant
Summary by CodeRabbit
New Features
Improvements