Conversation
WalkthroughThis PR refactors the CollectionSource trait's event code derivation logic to shift from title-based lookups to sourceTypeName/subtypeId-based approaches, removes title persistence to the database, and enhances observability through CiviCRM logger calls. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
Code Review SummaryCritical Issues[CollectionSource.php:162] The code no longer saves the generated camp code to the database. The previous implementation had: EckEntity::update('Collection_Camp')
->addWhere('id', '=', $sourceId)
->addValue('title', $newTitle)
->execute();This has been removed. While Suggestions[CollectionSource.php:156] The logic change from using
[CollectionSource.php:158] The debug log includes all intermediate variables ( What Looks Good
Required ActionPlease restore the database save operation or confirm that |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php`:
- Around line 160-164: The generated title stored into $objectRef['title'] in
the CollectionSource trait is only an in-memory change for the after-write hook
and is not persisted; update the DB explicitly after generating $newTitle (use
the record id from $objectRef['id']) so Collection_Camp.title is saved (e.g.,
call the appropriate API/DAO update for the Collection_Camp entity rather than
just mutating $objectRef), ensure you check the id exists and log/report any
update errors alongside the existing \Civi::log() calls.
- Around line 155-158: The mapping lookup for event codes can return 'UNKNOWN'
and the code continues, which can corrupt downstream identifiers; in the block
using getSourceTypeName($subtypeId) and $config['event_codes'], validate that
$config['event_codes'][$sourceTypeTitle] exists and is not empty before
proceeding—if missing, abort generation by throwing an exception or returning an
error (do not silently set $eventCode = 'UNKNOWN'), and log a clear error
including $sourceTypeName/$sourceTypeTitle to help debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2eff2d9d-ee7a-4fa2-81ac-5a60c99adc09
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php
| $sourceTypeName = self::getSourceTypeName($subtypeId); | ||
| $sourceTypeTitle = str_replace('_', ' ', $sourceTypeName); | ||
| $eventCode = $config['event_codes'][$sourceTypeTitle] ?? 'UNKNOWN'; | ||
| \Civi::log()->debug('[CodeGen] eventCode', ['sourceTypeName' => $sourceTypeName, 'sourceTypeTitle' => $sourceTypeTitle, 'eventCode' => $eventCode]); |
There was a problem hiding this comment.
Avoid silently generating event codes with UNKNOWN.
If subtype-to-event mapping fails, the code still proceeds and produces a title with UNKNOWN, which can corrupt identifier semantics downstream. Abort generation when mapping is missing.
Suggested hardening
- $sourceTypeName = self::getSourceTypeName($subtypeId);
- $sourceTypeTitle = str_replace('_', ' ', $sourceTypeName);
- $eventCode = $config['event_codes'][$sourceTypeTitle] ?? 'UNKNOWN';
- \Civi::log()->debug('[CodeGen] eventCode', ['sourceTypeName' => $sourceTypeName, 'sourceTypeTitle' => $sourceTypeTitle, 'eventCode' => $eventCode]);
+ $sourceTypeName = self::getSourceTypeName((string) $subtypeId);
+ if (!$sourceTypeName) {
+ \Civi::log()->debug('[CodeGen] Aborting: sourceTypeName missing', ['subtypeId' => $subtypeId]);
+ return;
+ }
+ $sourceTypeTitle = str_replace('_', ' ', $sourceTypeName);
+ $eventCode = $config['event_codes'][$sourceTypeTitle] ?? NULL;
+ if (!$eventCode) {
+ \Civi::log()->debug('[CodeGen] Aborting: eventCode mapping missing', ['sourceTypeTitle' => $sourceTypeTitle]);
+ return;
+ }
+ \Civi::log()->debug('[CodeGen] eventCode', ['sourceTypeName' => $sourceTypeName, 'sourceTypeTitle' => $sourceTypeTitle, 'eventCode' => $eventCode]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php`
around lines 155 - 158, The mapping lookup for event codes can return 'UNKNOWN'
and the code continues, which can corrupt downstream identifiers; in the block
using getSourceTypeName($subtypeId) and $config['event_codes'], validate that
$config['event_codes'][$sourceTypeTitle] exists and is not empty before
proceeding—if missing, abort generation by throwing an exception or returning an
error (do not silently set $eventCode = 'UNKNOWN'), and log a clear error
including $sourceTypeName/$sourceTypeTitle to help debugging.
| $newTitle = $year . '/' . $stateCode . '/' . $eventCode . '/' . $sourceId; | ||
| error_log('newTitle:' . print_r($newTitle, TRUE)); | ||
| \Civi::log()->info('[CodeGen] newTitle', ['newTitle' => $newTitle]); | ||
| $objectRef['title'] = $newTitle; | ||
|
|
||
| // Save the updated title back to the Collection Camp entity. | ||
| EckEntity::update('Collection_Camp') | ||
| ->addWhere('id', '=', $sourceId) | ||
| ->addValue('title', $newTitle) | ||
| ->execute(); | ||
| \Civi::log()->info('[CodeGen] Camp code generated', ['sourceId' => $sourceId, 'newTitle' => $newTitle]); |
There was a problem hiding this comment.
Generated title is not persisted in an after-write hook.
Line 162 only mutates $objectRef['title']. Since this hook runs after DB write, the generated code is unlikely to be saved to Collection_Camp.title unless explicitly updated.
Suggested persistence fix
$newTitle = $year . '/' . $stateCode . '/' . $eventCode . '/' . $sourceId;
\Civi::log()->info('[CodeGen] newTitle', ['newTitle' => $newTitle]);
$objectRef['title'] = $newTitle;
+ if (($sourceTitle ?? NULL) !== $newTitle) {
+ EckEntity::update('Collection_Camp', FALSE)
+ ->addValue('title', $newTitle)
+ ->addWhere('id', '=', $sourceId)
+ ->execute();
+ }
\Civi::log()->info('[CodeGen] Camp code generated', ['sourceId' => $sourceId, 'newTitle' => $newTitle]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wp-content/civi-extensions/goonjcustom/Civi/Traits/CollectionSource.php`
around lines 160 - 164, The generated title stored into $objectRef['title'] in
the CollectionSource trait is only an in-memory change for the after-write hook
and is not persisted; update the DB explicitly after generating $newTitle (use
the record id from $objectRef['id']) so Collection_Camp.title is saved (e.g.,
call the appropriate API/DAO update for the Collection_Camp entity rather than
just mutating $objectRef), ensure you check the id exists and log/report any
update errors alongside the existing \Civi::log() calls.
1 participant
Collection camp code bug
Summary by CodeRabbit
Release Notes