Improve demo-import: ORM-only group import, demo config import, feature toggles#7633
Improve demo-import: ORM-only group import, demo config import, feature toggles#7633
Conversation
DawoudIO
requested review from
DAcodedBEAT,
MrClever,
bigtigerku and
grayeul
There was a problem hiding this comment.
Pull request overview
This PR refactors the demo data import system to be more robust and self-contained. It introduces automatic system configuration import from a new config.json file, adds feature toggle support for Sunday School and Finance modules based on API flags, and reimplements group import using Propel ORM directly without external service dependencies. The refactoring removes event and financial import logic (keeping only core congregation data), fixes duplicate email addresses in demo data, and increases the Cypress test timeout for more stable CI runs.
Key Changes
- ORM-only group import: Refactored
importGroups()to use Propel ORM directly, removing dependency onGroupServiceand implementing explicit role creation based onpeopleTypesdefinitions - System configuration import: Added
importSystemConfig()method to automatically load demo settings fromconfig.jsonand set feature toggles (bEnabledSundaySchool,bEnabledFinance) based on API parameters - Streamlined import flow: Removed event, calendar, pledge, and financial import methods, focusing exclusively on families, people, groups, and notes
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/admin/routes/api/demo.php | Added includeSundaySchool parameter, fixed typo on line 20 where $includeEvents was incorrectly assigned to $force, improved logging |
| src/admin/demo/people.json | Fixed 6 duplicate email addresses (dorothy.johnson2, mary.williams2, kenneth.lee2, david.rodriguez2, sarah.hall2, mary.hernandez2) to prevent database constraint violations |
| src/admin/demo/groups.json | New comprehensive group definitions file with 12 groups (6 Sunday School classes, 6 regular groups) including member rosters and role definitions |
| src/admin/demo/config.json | New demo system configuration file with church details, location coordinates, timezone, and contact information |
| src/ChurchCRM/Service/GroupService.php | Added addUserToGroupInternal() method for auth-bypass group membership creation (unused in current implementation) |
| src/ChurchCRM/Service/DemoDataService.php | Major refactoring: added constructor with logger, new importSystemConfig() and importGroups() methods, renamed importFromSimplified() to importCongregation(), removed event/financial imports, consolidated warning handling |
| locale/messages.po | Updated translation strings for demo import UI and removed obsolete entries |
| cypress/e2e/xReset/admin.system-demo.spec.js | Added includeSundaySchool: true parameter and increased wait timeout from 3s to 15s for CI stability |
| private function importFromSimplified(string $filePath): void | ||
| private function importCongregation(): array | ||
| { | ||
| $logger = LoggerUtils::getAppLogger(); |
There was a problem hiding this comment.
The importCongregation method creates a local $logger variable on line 170, but the class already has $this->logger initialized in the constructor. This local variable should be removed and all references to $logger in this method should use $this->logger instead for consistency.
| $force = isset($body['force']) ? (bool)$body['force'] : false; | ||
|
|
||
| if (!$force) { | ||
| $peopleCount = PersonQuery::create()->count(); |
There was a problem hiding this comment.
Inconsistent indentation: line 23 has incorrect indentation (should be indented to match line 24). The opening statement $peopleCount = PersonQuery::create()->count(); should be indented with 4 more spaces to align with the code inside the if (!$force) block.
| $peopleCount = PersonQuery::create()->count(); | |
| $peopleCount = PersonQuery::create()->count(); |
| private function importSystemConfig(bool $includeSundaySchool, bool $includeFinancial): void | ||
| { | ||
| $this->importResult['startTime'] = microtime(true); | ||
| $logger = LoggerUtils::getAppLogger(); |
There was a problem hiding this comment.
The importSystemConfig method creates a local $logger variable on line 111, but the class already has $this->logger initialized in the constructor (line 45). This local variable should be removed and all references to $logger in this method should use $this->logger instead for consistency.
| if ($isSS && isset($this->importResult['imported']['sunday_schools'])) { | ||
| $this->importResult['imported']['sunday_schools']++; |
There was a problem hiding this comment.
The code attempts to increment $this->importResult['imported']['sunday_schools'] on line 431, but this counter is never initialized in the $importResult array (see lines 23-35). The isset() check will always fail, and this counter will never be incremented. Either add 'sunday_schools' => 0 to the initialization array on line 26-29, or remove this conditional increment if the counter is not needed.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
What Changed
Refactors and hardens the demo-data import flow and demo dataset to be more self-contained and robust.
Adds automatic import of demo system configuration from config.json.
Ensures feature toggles for Sunday School and Finance are set according to API flags passed to the demo import endpoint.
Reworks group import to use Propel ORM directly (no external GroupService dependency), respect group types, and only create roles explicitly defined in the demo data.
Updates the Cypress demo test wait timeout for more stable CI runs.
Type
Testing
Screenshots
Security Check
Code Quality
Pre-Merge