Project-Pro/orderPRO
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
main
orderPRO/.paul/codebase/CONCERNS.md

17 KiB
Raw Permalink Blame History

Codebase Concerns

Analysis Date: 2026-03-12

Tech Debt

[MEDIUM] SonarQube — 327 Open Issues (2026-03-12)

Listed in DOCS/todo.md:

  • 95x php:S112 — generic new \Exception instead of typed exceptions
  • 57x php:S1142 — excessive return statements per method
  • 40x php:S1192 — repeated string literals not extracted to constants
  • 31x php:S3776 — high cognitive complexity
  • 11x php:S1448 — classes with too many methods (god classes)
  • 4x php:S138 — methods over the allowed line limit

The most critical from a maintainability standpoint are the complexity and god-class violations.

[LOW] Duplicate Migration Number 000014

  • Issue: Two migration files share sequence number 000014:
    • 20260227_000014_create_product_integration_translations.sql
    • 20260301_000014_add_products_sku_format_setting.sql
  • File: database/migrations/
  • Impact: Depending on how the migrator detects "already run" (by filename vs. sequence number), one of these may silently never execute or may fail.
  • Fix approach: Rename one to 000014b or renumber all subsequent ones. Verify migrator uses full filename as key.

Security Concerns

[HIGH] No SSL Verification on cURL Calls

  • Issue: None of the AllegroApiClient, ShopproApiClient, ApaczkaApiClient, or AllegroOAuthClient set CURLOPT_SSL_VERIFYPEER or CURLOPT_SSL_VERIFYHOST. PHP's default is to verify SSL, but explicitly not setting it means behavior depends on the server's php.ini curl.cainfo configuration.
  • Files:
    • src/Modules/Settings/AllegroApiClient.php
    • src/Modules/Settings/AllegroOAuthClient.php
    • src/Modules/Settings/ShopproApiClient.php
    • src/Modules/Settings/ApaczkaApiClient.php
  • Impact: On environments without a properly configured CA bundle (common on shared hosting / Windows XAMPP), all API calls may proceed without SSL verification, enabling MITM attacks on OAuth tokens and API keys.
  • Fix approach: Explicitly set CURLOPT_SSL_VERIFYPEER => true and configure CURLOPT_CAINFO pointing to a known CA bundle. Document this in .env.example.

[MEDIUM] CSRF Token Is Never Rotated After Login

  • Issue: Csrf::token() in src/Core/Security/Csrf.php generates the token once per session and never regenerates it. The token is never rotated after a successful login, and it is never invalidated after a POST form is submitted.
  • File: src/Core/Security/Csrf.php
  • Impact: If a CSRF token leaks (e.g., via Referer header, browser history), it remains valid for the entire session lifetime.
  • Fix approach: Rotate the CSRF token on login. Optionally regenerate after each use (per-form tokens). At minimum, invalidate the session token on AuthService::logout().

[MEDIUM] Label File Served with file_get_contents($fullPath) — Partial Path Traversal Risk

  • Issue: In ShipmentController::label(), the file path is built from $package['label_path'] retrieved from the database. While basename() is used for the filename in the response header, $fullPath itself is derived by concatenating $storagePath . '/labels/' . $filename from the shipment service. There is no realpath() check confirming the file is actually within the storage directory before serving it.
  • File: src/Modules/Shipments/ShipmentController.php (lines 292–316)
  • Impact: Low risk currently (path comes from DB, not user input directly), but if label_path column is ever injectable or incorrectly set, arbitrary file reads become possible.
  • Fix approach: Validate realpath($fullPath) starts with realpath($storagePath) before calling file_get_contents().

[LOW] Allegro OAuth Callback Endpoint Has No Auth Middleware

  • Issue: GET /settings/integrations/allegro/oauth/callback is registered without $authMiddleware. The OAuth state parameter is validated via session, which does provide CSRF protection for the OAuth flow, but any authenticated check is absent.
  • File: routes/web.php (line 239)
  • Impact: An unauthenticated actor with knowledge of the state parameter could complete an OAuth token exchange. In practice, the state must match the session, limiting exploitability to session-fixation scenarios.
  • Fix approach: Add $authMiddleware to the callback route. Handle the "not authenticated yet" edge case by storing the session state before redirect.

Performance Concerns

[HIGH] N+1 Correlated Subqueries on Every Order List Row

  • Issue: The OrdersRepository::paginate() list SQL includes three correlated subqueries per row: items_count, items_qty, shipments_count, and documents_count. These fire once per order in the result set.
  • File: src/Modules/Orders/OrdersRepository.php (lines 124–127)
  • Impact: For a page of 50 orders: 4 × 50 = 200 additional subquery executions per page load. Degrades significantly as the orders table grows.
  • Fix approach: Replace correlated subqueries with LEFT JOIN + GROUP BY aggregates, or preload counts in a single separate query (SELECT order_id, COUNT(*) ... GROUP BY order_id WHERE order_id IN (...)).

[HIGH] canResolveMappedMedia() Queries information_schema.COLUMNS on Every Request

  • Issue: OrdersRepository::canResolveMappedMedia() runs a query against information_schema.COLUMNS to check whether optional product mapping tables exist. It caches the result in a private property, but this property is instance-scoped — the check fires once per HTTP request that instantiates OrdersRepository.
  • File: src/Modules/Orders/OrdersRepository.php (lines 604–648)
  • Impact: information_schema queries are notoriously slow on MySQL. This fires on every order-related page load (list, detail, shipment).
  • Fix approach: Cache the result in a static class property or in the app-level cache/session so it survives across requests. Alternatively, remove the check entirely — if the schema is managed by migrations, the tables either exist or don't.

[MEDIUM] AllegroStatusSyncService Fetches Up to 50 Orders Then Re-Imports Each One Fully

  • Issue: AllegroStatusSyncService::findOrdersNeedingStatusSync() returns up to 50 Allegro orders. For each, importSingleOrder() is called, which makes a full Allegro API call per order (checkout-form fetch + optional shipments fetch + optional offer image fetch per line item). This is 50+ API calls per cron run.
  • File: src/Modules/Settings/AllegroStatusSyncService.php
  • Impact: Rate-limit exhaustion on Allegro API. Slow cron runs.
  • Fix approach: Use a lighter-weight API call to fetch only status fields for multiple orders (e.g., listCheckoutForms with a filter) rather than a full re-import per order.
  • Note (2026-03-13): Plan 02-02 dodał kursor last_status_checked_at — eliminuje re-import zamówień bez zmiany statusu. Full API call per order pozostaje osobnym concernem.

[MEDIUM] No Database Indexes Verified for Key Queries

  • Issue: Queries frequently filter and sort on orders.source, orders.external_status_id, orders.ordered_at, order_items.order_id, and allegro_order_status_mappings.allegro_status_code. Whether indexes exist for these columns is not documented.
  • Files: src/Modules/Orders/OrdersRepository.php, database/migrations/20260302_000018_create_orders_tables_and_schedule.sql
  • Impact: Full table scans as data grows.
  • Fix approach: Audit the migration that creates orders and related tables for index definitions. Add missing indexes for (source, external_status_id), (ordered_at), and allegro_status_code.

Architectural Concerns

[HIGH] Settings Module Is a God Module — Contains Unrelated Concerns

  • Issue: The src/Modules/Settings/ directory contains 30+ classes covering: Allegro integration, Apaczka integration, InPost integration, shopPRO integration, company settings, cron settings, order statuses, carrier mappings, delivery method mappings, and OAuth. These are logically distinct domains crammed into a single module namespace.
  • Files: All files in src/Modules/Settings/
  • Impact: The module boundary is meaningless. New integrations will continue to pile into Settings. AllegroIntegrationController (923 lines) and ShopproIntegrationsController (901 lines) are god classes by SonarQube rules (S1448).
  • Fix approach: Gradually decompose into purpose-built modules: src/Modules/Integrations/Allegro/, src/Modules/Integrations/Shoppro/, src/Modules/Integrations/Apaczka/, src/Modules/Integrations/Inpost/.

[HIGH] AllegroIntegrationController and ShopproIntegrationsController Are God Classes (900+ lines)

  • Issue: Each controller handles: displaying settings, saving credentials, OAuth flow, status mapping CRUD, delivery method mapping CRUD, and import triggering. Each is 900+ lines.
  • Files:
    • src/Modules/Settings/AllegroIntegrationController.php (923 lines)
    • src/Modules/Settings/ShopproIntegrationsController.php (901 lines)
  • Impact: Violates SRP. Methods average far more than 50 lines. Cognitive complexity is high (SonarQube S3776, 31 violations).
  • Fix approach: Split each into focused controllers per tab/concern: e.g., AllegroCredentialsController, AllegroStatusMappingController, AllegroDeliveryMappingController.

[HIGH] ShopproOrdersSyncService Is 1192 Lines — Largest Single File

  • Issue: ShopproOrdersSyncService handles the full import lifecycle for shopPRO orders including: pagination, cursor management, order mapping, address building, item building, image resolution, payment mapping, shipment mapping, status history, and activity logging — all in one class.
  • File: src/Modules/Settings/ShopproOrdersSyncService.php
  • Impact: Highest complexity class in the codebase. Hard to test, hard to modify, prone to regression.
  • Fix approach: Extract a ShopproOrderMapper (mapping logic), a ShopproImageResolver (image fetch), and keep ShopproOrdersSyncService as the orchestrator. Mirror the pattern of AllegroOrderImportService which is better separated.

[MEDIUM] No Dependency Injection Container — Full Object Graph in routes/web.php

  • Issue: routes/web.php manually constructs all services and controllers, including multiple separate instantiations of identical objects (e.g., new AllegroApiClient() appears 3 times, new OrdersRepository($app->db()) appears 4 times).
  • File: routes/web.php
  • Impact: Multiple object instances where singletons are implied. Adding new dependencies requires editing this file, which is already 257 lines. Risk of silently passing the wrong instance.
  • Fix approach: Introduce a lightweight DI container or service locator. At minimum, hoist shared instances ($allegroApiClient, $ordersRepository) to named variables and reuse them.

Incomplete Features

[HIGH] 5 Stub Buttons with No Actions in Order Detail View

  • Issue: The order detail page (resources/views/orders/show.php, lines 48–53) contains 5 buttons that render in the UI but have no href, no form action, and no JavaScript handler: "Strefa klienta", "Platnosc", "Drukuj", "Pakuj", "Edytuj".
  • File: resources/views/orders/show.php
  • Impact: Users see clickable but non-functional UI elements. The "Pakuj" (pack) button is styled as the primary CTA (btn--primary).
  • Fix approach: Either implement the features or hide the buttons until they are ready.

[HIGH] orderpro_to_allegro Status Sync Direction Not Implemented

  • Issue: The setting UI offers two sync directions: Allegro → orderPRO and orderPRO → Allegro. The second direction explicitly returns early with "Kierunek orderPRO -> Allegro nie jest jeszcze wdrozony" ("not yet implemented").
  • File: src/Modules/Settings/AllegroStatusSyncService.php (lines 38–45)
  • Impact: Users who configure the "push status to Allegro" direction get silent no-op behavior with no clear user-facing indication in the cron log.
  • Fix approach: Either implement the feature or disable the orderpro_to_allegro direction option in the UI until it is ready.

[HIGH] TODO in DOCS/todo.md — UI Issues Open

The following items from DOCS/todo.md are marked incomplete:

  • Item 12: Status change sync with Allegro after manual user change
  • Item 14: Input/select/textarea border color needs to be darker
  • Item 15: Source and ID display order should be reversed (source first, then ID)
  • Item 16: Order list statuses should be colored according to settings
  • Item 17: Source display should show specific integration name instead of "shopPRO"; ID label missing
  • Files: resources/views/orders/list.php, resources/views/orders/show.php

[MEDIUM] InPost Integration — Settings Page Exists But No Shipment Provider

  • Issue: InpostIntegrationController and InpostIntegrationRepository exist and the settings page at /settings/integrations/inpost is functional. However, there is no InpostShipmentService implementing ShipmentProviderInterface. InPost labels are currently routed through allegro_wza as a workaround (provider_code === 'inpost' is remapped to 'allegro_wza' in ShipmentController::create()).
  • Files:
    • src/Modules/Settings/InpostIntegrationController.php
    • src/Modules/Settings/InpostIntegrationRepository.php
    • src/Modules/Shipments/ShipmentController.php (line 165–166)
  • Impact: InPost credentials are stored but not used by their own provider. The remap to allegro_wza means InPost shipments are actually created via Allegro WZA. This breaks if the user has only InPost credentials and no Allegro integration.
  • Fix approach: Implement InpostShipmentService implements ShipmentProviderInterface using the stored InPost API token.

[MEDIUM] database/drafts/ Contains Uncommitted Schema SQL

  • Issue: database/drafts/20260302_orders_schema_v1.sql exists in the drafts subdirectory. This is not a migration — it will not be executed by the migrator.
  • File: database/drafts/20260302_orders_schema_v1.sql
  • Impact: Unclear whether this represents schema that has not yet been migrated or schema that was migrated by other means. Creates confusion about source of truth.
  • Fix approach: Either move to database/migrations/ with a proper sequence number, or delete it if already superseded.

Known Bugs

Test Coverage Gaps

[HIGH] No Integration or Functional Tests Exist

  • Issue: The tests/ directory contains only bootstrap.php. No test files exist. The archive/ directory contains unit tests from a previous reset, but they are not in active use.
  • Files: tests/bootstrap.php, archive/2026-03-02_users-only-reset/tests/
  • Risk: Zero automated coverage for any critical path: order import, shipment creation, OAuth flow, cron sync, status updates.
  • Priority: High

[HIGH] Allegro OAuth Token Refresh Logic Has No Tests

  • Issue: The token refresh logic is centralized in AllegroTokenManager but untested. It includes complex edge cases: token expiry within 5-minute window, empty refresh token fallback, write-then-re-read pattern.
  • Files: src/Modules/Settings/AllegroTokenManager.php
  • Risk: Token refresh failures cause complete import failure. Silent breakage possible.
  • Priority: High

[MEDIUM] No Error Path Tests for Order Import

  • Issue: AllegroOrderImportService::importSingleOrder() and ShopproOrdersSyncService::sync() have complex error handling (401 retry, silent Throwable catch blocks). None of this is tested.
  • Files:
    • src/Modules/Settings/AllegroOrderImportService.php
    • src/Modules/Settings/ShopproOrdersSyncService.php
  • Risk: Error paths that swallow exceptions silently (multiple catch (Throwable) {} blocks in OrdersRepository) may hide data integrity issues.
  • Priority: Medium

Fragile Areas

[HIGH] AllegroStatusSyncService — orderpro_to_allegro Silent No-Op

  • Files: src/Modules/Settings/AllegroStatusSyncService.php
  • Why fragile: Returns ok: true when direction is orderpro_to_allegro with zero processing, no log, no user feedback. Cron logs will show "success" even though nothing happened.
  • Safe modification: Do not add code after the early return without understanding why it was left as a no-op. Consider returning ok: false or a warning until the feature is implemented.
  • Test coverage: None.

[MEDIUM] Web Cron Throttling Uses Session Timestamp ($_SESSION['cron_web_last_run_at'])

  • Issue: Application::isWebCronThrottled() reads and writes a cron timestamp from $_SESSION. In stateless or load-balanced deployments, different users hit different sessions, meaning the throttle is per-session (per-user), not global.
  • File: src/Core/Application.php (lines 363–375)
  • Why fragile: With two active sessions, the cron may run twice within the throttle window. The DB-level GET_LOCK is the real protection, but this creates confusing interactions.
  • Fix approach: Move last-run timestamp to the database (e.g., an app_settings key) instead of the session.

Concerns audit: 2026-03-12

Reference in New Issue View Git Blame Copy Permalink