Conversation
…ity checks - isReleaseCurrent() now populates releases cache if empty before comparing - checkSystemUpdateAvailable() adds debug logging for version comparisons - refresh-upgrade-info endpoint recalculates availability and updates session keys - upgrade page fetches fresh release info on load and recomputes update state
…e duplication Create new service layer for upgrade operations to enable code reuse across admin and potential future public API endpoints: New Service: - src/ChurchCRM/Service/UpgradeAPIService.php - downloadLatestRelease() - download upgrade from GitHub - doUpgrade() - apply upgrade with SHA1 verification - refreshUpgradeInfo() - refresh from GitHub and update session state - Wraps ChurchCRMReleaseManager for API endpoint use Admin API Route (refactored to use service): - src/admin/routes/api/upgrade.php (moved from src/api/routes/system/system-upgrade.php) - Comprehensive endpoint documentation in comments - All three endpoints now delegate to UpgradeAPIService - Consistent error handling and response formats - Still protected by AdminRoleAuthMiddleware at router level Frontend JavaScript: - src/skin/js/CRMJSOM.js: Added window.CRM.AdminAPIRequest() wrapper - Automatically prefixes with /admin/api/ for admin endpoints - Eliminates double /api/ nesting issue - webpack/upgrade-wizard-app.js: Updated all endpoints to use AdminAPIRequest - upgrade/download-latest-release - upgrade/do-upgrade - upgrade/refresh-upgrade-info Updated Imports: - src/admin/index.php: Added new upgrade route import - src/api/index.php: Removed old system-upgrade import Benefits: - Single source of truth for upgrade API logic (UpgradeAPIService) - Easy to expose same endpoints publicly if needed (just add new route file) - Reduces code duplication across API routes - Better separation of concerns (service vs HTTP handling) - Git history preserved (file moved, not recreated)
Move ChurchCRMReleaseManager import from inline fully-qualified class names to top-level use statement, following PHP 8.2+ best practices for namespaced code. Changes: - Add 'use ChurchCRM\Utils\ChurchCRMReleaseManager' at top - Replace inline \ChurchCRM\Utils\ChurchCRMReleaseManager:: with ChurchCRMReleaseManager:: - Improves code readability and follows codebase conventions
There was a problem hiding this comment.
Pull request overview
This PR refactors the system upgrade API endpoints by consolidating them under the new admin API infrastructure. The upgrade endpoints are moved from /api/systemupgrade to /admin/api/upgrade, following the established pattern for admin-only APIs introduced in previous refactorings.
Key Changes:
- Introduces
UpgradeAPIServiceto encapsulate upgrade operations following the service layer pattern - Migrates three upgrade endpoints to the new admin API route structure with proper documentation
- Updates frontend JavaScript to use
AdminAPIRequestinstead ofAPIRequestfor admin-specific endpoints
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
webpack/upgrade-wizard-app.js |
Updated API calls to use AdminAPIRequest with new /admin/api/upgrade paths |
src/skin/js/CRMJSOM.js |
Added new AdminAPIRequest wrapper function for admin API endpoints |
src/api/index.php |
Removed old system-upgrade.php route registration |
src/admin/routes/system.php |
Added fresh release check when loading upgrade page |
src/admin/routes/api/upgrade.php |
Migrated upgrade endpoints with enhanced documentation |
src/admin/index.php |
Registered new upgrade API route file |
src/ChurchCRM/dto/ChurchCRMReleaseManager.php |
Added debug logging for version comparison troubleshooting |
src/ChurchCRM/Service/UpgradeAPIService.php |
New service class wrapping ChurchCRMReleaseManager for API routes |
Comments suppressed due to low confidence (2)
src/admin/routes/api/upgrade.php:42
- Missing input validation for required parameters. The
$input['fullPath']and$input['sha1']are accessed directly without checking if they exist in the parsed body. If these fields are missing, this will generate PHP warnings/errors.
Add validation before calling the service:
$input = $request->getParsedBody();
if (!isset($input['fullPath']) || !isset($input['sha1'])) {
return SlimUtils::renderJSON($response, [
'message' => 'Missing required parameters: fullPath and sha1'
], 400);
}
UpgradeAPIService::doUpgrade($input['fullPath'], $input['sha1']);webpack/upgrade-wizard-app.js:177
- Inconsistent parameter name. The function uses
type: 'GET'instead ofmethod: 'GET', which is inconsistent with the other API calls in this file (lines 250, 347, 403) that usemethod. While jQuery's$.ajax()accepts both, the codebase should use consistent naming.
Change to:
window.CRM.AdminAPIRequest({
method: 'GET',
path: 'upgrade/download-latest-release',
}) * Auto-download update when step is shown
*/
function autoDownloadUpdate() {
| if (!options.method) { | ||
| options.method = "GET"; | ||
| } else { | ||
| options.dataType = "json"; | ||
| } |
There was a problem hiding this comment.
The conditional logic is incorrect. When a method is provided, dataType should be set to "json", but the current logic only sets it when a method is NOT provided. This means GET requests won't have dataType: "json" but POST/PUT requests will.
This should be:
if (!options.method) {
options.method = "GET";
}
options.dataType = "json";The same bug exists in the original APIRequest function above (lines 6-10).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
What Changed
Fixes #
Type
Testing
Screenshots
Security Check
Code Quality
Pre-Merge