Conversation
WalkthroughRefactors module statistics to report active module counts segmented by audience (users vs admins) instead of inactive/total, and updates the KPI language string capitalization for "New this month". Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14 +/- ##
============================================
- Coverage 19.23% 18.05% -1.18%
- Complexity 7584 7759 +175
============================================
Files 621 665 +44
Lines 40085 42700 +2615
============================================
Hits 7709 7709
- Misses 32376 34991 +2615 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/themes/modern/modern.php`:
- Around line 247-257: The code computes active_modules_admin by subtracting
active_modules_user from active_modules, which is incorrect because hasmain and
hasadmin are independent; instead build a new CriteriaCompo (like the existing
$criteria) or clone/reset it, add Criteria('isactive', 1) and
Criteria('hasadmin', 1) and call $module_handler->getCount(...) to get the
correct active_modules_admin value, then assign that result to
$tpl->assign('active_modules_admin', ...); ensure you reference the same
module_handler, CriteriaCompo and Criteria symbols when locating the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f8292ff1-be42-4aab-9a0e-bd3ba95815f6
📒 Files selected for processing (3)
htdocs/modules/system/themes/modern/language/english/main.phphtdocs/modules/system/themes/modern/modern.phphtdocs/modules/system/themes/modern/xotpl/xo_dashboard.tpl
There was a problem hiding this comment.
Pull request overview
Updates the Modern admin theme dashboard KPIs to provide a clearer breakdown of active modules by role, along with minor label text consistency improvements.
Changes:
- Restricts dashboard rendering to the admin homepage using
$xoops_page. - Splits “Active Modules” KPI details into user-side vs admin-side counts.
- Updates/enhances English language strings for the dashboard KPI labels.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| htdocs/modules/system/themes/modern/xotpl/xo_dashboard.tpl | Updates dashboard visibility condition and displays split active-module KPI details. |
| htdocs/modules/system/themes/modern/modern.php | Adjusts module statistics assigned to the dashboard template (user/admin split). |
| htdocs/modules/system/themes/modern/language/english/main.php | Updates capitalization and adds new language constants for the split KPI labels. |
| $criteria = new CriteriaCompo(); | ||
| $criteria->add(new Criteria('hasmain', 1)); | ||
| // active modules | ||
| $criteria->add(new Criteria('isactive', 1)); | ||
|
|
||
| $active_modules = $module_handler->getCount($criteria); | ||
|
|
||
| $criteria_all = new Criteria('dirname', '', '!='); | ||
| $total_modules = $module_handler->getCount($criteria_all); | ||
| // active modules for user side | ||
| $criteria->add(new Criteria('hasmain', 1)); | ||
| $active_modules_user = $module_handler->getCount($criteria); | ||
|
|
||
| $tpl->assign('active_modules', $active_modules); | ||
| $tpl->assign('total_modules', $total_modules); | ||
| $tpl->assign('inactive_modules', $total_modules - $active_modules); | ||
| $tpl->assign('active_modules_user', $active_modules_user); | ||
| $tpl->assign('active_modules_admin', $active_modules - $active_modules_user); |
There was a problem hiding this comment.
active_modules_admin is currently derived as active_modules - active_modules_user (i.e., active modules without hasmain). That doesn't actually represent “modules for admins”: modules that have both user side (hasmain=1) and an admin side (hasadmin=1) are excluded from the admin count, and modules with neither hasmain nor hasadmin would be included. Consider either (a) counting admin modules with an explicit hasadmin=1 criteria (separate CriteriaCompo) or (b) renaming the label/variable to reflect that it's counting “admin-only / no-user-side” modules and also filtering hasadmin=1 to avoid counting modules with no admin UI.
|
The counting logic is correct — the subtraction works because every active module has at least The only suggestion is on label clarity. "Active Modules For Admins" reads like "all modules with an admin panel" (which is almost every module), not "modules that only appear in admin" (which is the actual metric). Suggested label update in define('_MODERN_ACTIVE_MODULES_USERS', 'User-Facing Modules');
define('_MODERN_ACTIVE_MODULES_ADMINS', 'Admin-Only Modules');And a short comment in // Modules without a frontend entry (hasmain=0) are admin-only by XOOPS design.
// Every active module has at least hasmain=1 or hasadmin=1, so this subtraction
// equals querying isactive=1 AND hasmain=0 without an extra DB round-trip.
$tpl->assign('active_modules_admin', $active_modules - $active_modules_user);That way the dashboard reads:
Just a suggestion — your call @ggoffy. |
Summary by CodeRabbit
New Features
Updates