Phase 1 complete (2/2 plans): - Plan 01-01: Extract AllegroTokenManager — OAuth token logic centralized from 4 classes into dedicated manager class - Plan 01-02: Extract StringHelper — nullableString/normalizeDateTime/ normalizeColorHex extracted from 15+ classes into App\Core\Support\StringHelper; removed 19 duplicate private methods Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
299 lines
14 KiB
Markdown
299 lines
14 KiB
Markdown
---
|
||
phase: 01-tech-debt
|
||
plan: 01
|
||
type: execute
|
||
wave: 1
|
||
depends_on: []
|
||
files_modified:
|
||
- src/Modules/Settings/AllegroTokenManager.php
|
||
- src/Modules/Settings/AllegroOrderImportService.php
|
||
- src/Modules/Settings/AllegroOrdersSyncService.php
|
||
- src/Modules/Settings/AllegroStatusDiscoveryService.php
|
||
- src/Modules/Shipments/AllegroShipmentService.php
|
||
- routes/web.php
|
||
- src/Core/Application.php
|
||
- .paul/codebase/CONCERNS.md
|
||
- DOCS/ARCHITECTURE.md
|
||
autonomous: false
|
||
---
|
||
|
||
<objective>
|
||
## Goal
|
||
Extract the duplicated Allegro OAuth token-refresh logic from 4 service classes into a single `AllegroTokenManager` service, then delete the private methods from each consumer.
|
||
|
||
## Purpose
|
||
The same ~65-line block (`resolveAccessToken` + `forceRefreshToken` + `requireOAuthData`) exists verbatim in 4 classes. A bug in token refresh requires 4 coordinated fixes with high drift risk. This is the highest-severity tech debt item per CONCERNS.md.
|
||
|
||
## Output
|
||
- New class: `src/Modules/Settings/AllegroTokenManager.php`
|
||
- 4 service files simplified (private token methods removed, `AllegroTokenManager` injected)
|
||
- Wiring updated in `routes/web.php` and `src/Core/Application.php`
|
||
- CONCERNS.md entry for this issue removed
|
||
- ARCHITECTURE.md updated with new class
|
||
</objective>
|
||
|
||
<context>
|
||
## Project Context
|
||
@.paul/PROJECT.md
|
||
@.paul/STATE.md
|
||
|
||
## Affected Files
|
||
@src/Modules/Settings/AllegroOrderImportService.php
|
||
@src/Modules/Settings/AllegroOrdersSyncService.php
|
||
@src/Modules/Settings/AllegroStatusDiscoveryService.php
|
||
@src/Modules/Shipments/AllegroShipmentService.php
|
||
@routes/web.php
|
||
@src/Core/Application.php
|
||
</context>
|
||
|
||
<skills>
|
||
## Required Skills (from SPECIAL-FLOWS.md)
|
||
|
||
| Skill | Priority | When to Invoke | Loaded? |
|
||
|-------|----------|----------------|---------||
|
||
| /feature-dev | required | Before starting implementation | ○ |
|
||
| /code-review | required | After implementation, before UNIFY | ○ |
|
||
| sonar-scanner (CLI) | required | After APPLY, before UNIFY | ○ |
|
||
|
||
**BLOCKING:** Required skills MUST be loaded before APPLY proceeds.
|
||
|
||
## Skill Invocation Checklist
|
||
- [ ] /feature-dev loaded
|
||
- [ ] /code-review loaded (run after implementation)
|
||
- [ ] sonar-scanner run (run after implementation)
|
||
</skills>
|
||
|
||
<acceptance_criteria>
|
||
|
||
## AC-1: AllegroTokenManager extracting token resolution
|
||
```gherkin
|
||
Given the AllegroIntegrationRepository has valid OAuth credentials
|
||
When AllegroTokenManager::resolveToken() is called
|
||
Then it returns [accessToken: string, environment: string]
|
||
AND if the token is within 5 minutes of expiry it first calls refreshAccessToken()
|
||
AND if the token is absent it calls refreshAccessToken()
|
||
AND after refresh it saves updated tokens via integrationRepository->saveTokens()
|
||
```
|
||
|
||
## AC-2: No private token methods in the 4 consumer classes
|
||
```gherkin
|
||
Given the refactor is complete
|
||
When the 4 service files are read
|
||
Then none of them contain resolveAccessToken(), forceRefreshToken(), or requireOAuthData() methods
|
||
AND each has AllegroTokenManager injected in its constructor
|
||
```
|
||
|
||
## AC-3: Services retain correct constructor dependencies
|
||
```gherkin
|
||
Given the refactor is complete
|
||
When AllegroOrderImportService and AllegroOrdersSyncService are instantiated
|
||
Then they still inject AllegroIntegrationRepository (used for getActiveIntegrationId, getSettings)
|
||
AND also inject AllegroTokenManager
|
||
When AllegroStatusDiscoveryService and AllegroShipmentService are instantiated
|
||
Then they inject AllegroTokenManager instead of AllegroIntegrationRepository + AllegroOAuthClient separately
|
||
```
|
||
|
||
## AC-4: Wiring updated in both places
|
||
```gherkin
|
||
Given routes/web.php and src/Core/Application.php are read
|
||
When searching for instantiation of the 4 services
|
||
Then each instantiation passes an AllegroTokenManager instance
|
||
AND AllegroTokenManager is constructed with (allegroIntegrationRepository, allegroOAuthClient)
|
||
```
|
||
|
||
## AC-5: App boots without errors
|
||
```gherkin
|
||
Given the refactor is complete
|
||
When the application is loaded in a browser (or PHP -l is run on all changed files)
|
||
Then no PHP parse errors or fatal errors occur
|
||
```
|
||
|
||
</acceptance_criteria>
|
||
|
||
<tasks>
|
||
|
||
<task type="checkpoint:human-verify" gate="blocking">
|
||
<what-built>Before implementing, verify the required /feature-dev skill is loaded.</what-built>
|
||
<how-to-verify>
|
||
Run /feature-dev in this conversation before proceeding.
|
||
</how-to-verify>
|
||
<resume-signal>Type "feature-dev loaded" to continue with implementation</resume-signal>
|
||
</task>
|
||
|
||
<task type="auto">
|
||
<name>Task 1: Create AllegroTokenManager</name>
|
||
<files>src/Modules/Settings/AllegroTokenManager.php</files>
|
||
<action>
|
||
Create `src/Modules/Settings/AllegroTokenManager.php` in namespace `App\Modules\Settings`.
|
||
|
||
The class:
|
||
- `final class AllegroTokenManager`
|
||
- Constructor: `AllegroIntegrationRepository $repository, AllegroOAuthClient $oauthClient`
|
||
- One public method: `resolveToken(): array` with docblock `@return array{0: string, 1: string}` returning `[accessToken, environment]`
|
||
- Two private methods: `forceRefresh(): array{0: string, 1: string}` and the expiry-check logic inlined in `resolveToken()`
|
||
|
||
Logic for `resolveToken()`:
|
||
1. Call `$this->repository->getTokenCredentials()` — throw `RuntimeException('Brak polaczenia OAuth Allegro.')` if null
|
||
2. Extract `$accessToken`, `$tokenExpiresAt`, `$env` from the oauth array
|
||
3. If `$accessToken === ''` → call `forceRefresh()` and return
|
||
4. If `$tokenExpiresAt !== ''`:
|
||
- Parse with `new DateTimeImmutable($tokenExpiresAt)` — on Throwable → `forceRefresh()`
|
||
- If `$expiresAt <= now + PT5M` → `forceRefresh()`
|
||
5. Return `[$accessToken, $env]`
|
||
|
||
Logic for `forceRefresh()`:
|
||
1. Load oauth from `$this->repository->getTokenCredentials()` — throw if null
|
||
2. Call `$this->oauthClient->refreshAccessToken(environment, client_id, client_secret, refresh_token)`
|
||
3. Calculate `$expiresAt` (null if `expires_in` is 0)
|
||
4. Preserve old refresh_token if new one is empty
|
||
5. Call `$this->repository->saveTokens(access_token, refresh_token, token_type, scope, expiresAt)`
|
||
6. Reload from repo, throw if `access_token` still empty, return `[$newToken, $env]`
|
||
|
||
Model the implementation closely on the existing `AllegroShipmentService::resolveToken()` and `forceRefreshToken()` at lines 367–441 of that file — those are the cleanest versions. Use `declare(strict_types=1)`, proper imports, no unnecessary comments.
|
||
|
||
Avoid: introducing a public `forceRefresh()` method — keep it private.
|
||
</action>
|
||
<verify>Run: php -l "src/Modules/Settings/AllegroTokenManager.php" — should output "No syntax errors detected"</verify>
|
||
<done>AC-1 satisfied: AllegroTokenManager created with correct resolve/refresh logic</done>
|
||
</task>
|
||
|
||
<task type="auto">
|
||
<name>Task 2: Refactor 4 service classes to use AllegroTokenManager</name>
|
||
<files>
|
||
src/Modules/Settings/AllegroOrderImportService.php,
|
||
src/Modules/Settings/AllegroOrdersSyncService.php,
|
||
src/Modules/Settings/AllegroStatusDiscoveryService.php,
|
||
src/Modules/Shipments/AllegroShipmentService.php
|
||
</files>
|
||
<action>
|
||
For each of the 4 files:
|
||
|
||
**AllegroOrderImportService** (src/Modules/Settings/AllegroOrderImportService.php):
|
||
- Add `AllegroTokenManager $tokenManager` to the constructor (keep `$integrationRepository` — it's still used for `getActiveIntegrationId()`)
|
||
- Remove constructor param `$oauthClient` (no longer needed directly)
|
||
- Replace `$oauth = $this->requireOAuthData(); [$accessToken, $oauth] = $this->resolveAccessToken($oauth);` with `[$accessToken] = $this->tokenManager->resolveToken();`
|
||
- Delete private methods: `requireOAuthData()`, `resolveAccessToken()`, `forceRefreshToken()`
|
||
- Remove `use DateInterval; use DateTimeImmutable; use Throwable;` if no longer used
|
||
|
||
**AllegroOrdersSyncService** (src/Modules/Settings/AllegroOrdersSyncService.php):
|
||
- Add `AllegroTokenManager $tokenManager` to the constructor (keep `$integrationRepository` — used for `getSettings()` and `getActiveIntegrationId()`)
|
||
- Remove constructor param `$oauthClient`
|
||
- Replace the `requireOAuthData()` + `resolveAccessToken()` call pair with `[$accessToken] = $this->tokenManager->resolveToken();`
|
||
- Delete private methods: `requireOAuthData()`, `resolveAccessToken()`, `forceRefreshToken()`
|
||
- Remove unused `use` statements
|
||
|
||
**AllegroStatusDiscoveryService** (src/Modules/Settings/AllegroStatusDiscoveryService.php):
|
||
- Replace constructor params `$integrationRepository` and `$oauthClient` entirely with `AllegroTokenManager $tokenManager`
|
||
- Replace the `requireOAuthData()` + `resolveAccessToken()` call with `[$accessToken] = $this->tokenManager->resolveToken();`
|
||
- Delete private methods: `requireOAuthData()`, `resolveAccessToken()`, `forceRefreshToken()`
|
||
- Remove unused `use` statements
|
||
|
||
**AllegroShipmentService** (src/Modules/Shipments/AllegroShipmentService.php):
|
||
- Replace constructor params `$integrationRepository` and `$oauthClient` with `AllegroTokenManager $tokenManager`
|
||
- Replace `[$token, $env] = $this->resolveToken();` with `[$token, $env] = $this->tokenManager->resolveToken();`
|
||
- Delete private methods: `resolveToken()`, `forceRefreshToken()`
|
||
- Remove unused `use` statements (DateInterval, DateTimeImmutable, Throwable if not used elsewhere)
|
||
- Add `use App\Modules\Settings\AllegroTokenManager;` import
|
||
</action>
|
||
<verify>Run: php -l on each of the 4 files. All should report "No syntax errors detected"</verify>
|
||
<done>AC-2 and AC-3 satisfied: private token methods gone, AllegroTokenManager injected correctly</done>
|
||
</task>
|
||
|
||
<task type="auto">
|
||
<name>Task 3: Update wiring and remove concern entry</name>
|
||
<files>
|
||
routes/web.php,
|
||
src/Core/Application.php,
|
||
.paul/codebase/CONCERNS.md,
|
||
DOCS/ARCHITECTURE.md
|
||
</files>
|
||
<action>
|
||
**routes/web.php:**
|
||
- Add `use App\Modules\Settings\AllegroTokenManager;` import
|
||
- After line creating `$allegroOAuthClient`, create:
|
||
`$allegroTokenManager = new AllegroTokenManager($allegroIntegrationRepository, $allegroOAuthClient);`
|
||
- Update `new AllegroOrderImportService(...)`:
|
||
- Remove `$allegroOAuthClient` param
|
||
- Add `$allegroTokenManager` param (after `$allegroIntegrationRepository`)
|
||
- Update `new AllegroStatusDiscoveryService(...)`:
|
||
- Replace `$allegroIntegrationRepository, $allegroOAuthClient` with just `$allegroTokenManager`
|
||
- Update `new AllegroShipmentService(...)`:
|
||
- Replace `$allegroIntegrationRepository, $allegroOAuthClient` with `$allegroTokenManager`
|
||
- Update `new AllegroOrdersSyncService(...)` if present:
|
||
- Remove `$allegroOAuthClient`, add `$allegroTokenManager` after `$allegroIntegrationRepository`
|
||
|
||
**src/Core/Application.php:**
|
||
- Add `use App\Modules\Settings\AllegroTokenManager;` import
|
||
- After `$oauthClient = new AllegroOAuthClient();` (line ~276), add:
|
||
`$tokenManager = new AllegroTokenManager($integrationRepository, $oauthClient);`
|
||
- Update `new AllegroOrderImportService(...)`:
|
||
- Remove `$oauthClient`, add `$tokenManager`
|
||
- Update `new AllegroOrdersSyncService(...)`:
|
||
- Remove `$oauthClient`, add `$tokenManager`
|
||
|
||
**.paul/codebase/CONCERNS.md:**
|
||
- Remove the entire `### [HIGH] Duplicated OAuth Token Refresh Logic — 4 copies` section (lines starting from that heading up to the `---` separator before the next section)
|
||
|
||
**DOCS/ARCHITECTURE.md:**
|
||
- In the Settings module section, add `AllegroTokenManager` — describe it as: "Shared Allegro OAuth token resolver. Checks expiry and refreshes via AllegroOAuthClient when needed. Injected into all Allegro service classes."
|
||
</action>
|
||
<verify>
|
||
1. Run: php -l routes/web.php — no syntax errors
|
||
2. Run: php -l src/Core/Application.php — no syntax errors
|
||
3. Grep: grep -r "resolveAccessToken\|forceRefreshToken\|requireOAuthData" src/Modules/Settings/ src/Modules/Shipments/ — should return 0 results
|
||
</verify>
|
||
<done>AC-4 satisfied: wiring updated. Concern removed from CONCERNS.md.</done>
|
||
</task>
|
||
|
||
<task type="checkpoint:human-verify" gate="blocking">
|
||
<what-built>
|
||
AllegroTokenManager class created and 4 service classes refactored to use it.
|
||
Wiring updated in routes/web.php and Application.php.
|
||
</what-built>
|
||
<how-to-verify>
|
||
1. Open the application in a browser — verify it loads without a 500 error
|
||
2. Navigate to Settings > Allegro — verify the page loads
|
||
3. Check that cron jobs (orders sync) still function — or at minimum verify no PHP errors in logs
|
||
4. Run: grep -r "resolveAccessToken\|forceRefreshToken\|requireOAuthData" src/ — should return 0 results
|
||
</how-to-verify>
|
||
<resume-signal>Type "approved" if working, or describe the error if something broke</resume-signal>
|
||
</task>
|
||
|
||
</tasks>
|
||
|
||
<boundaries>
|
||
|
||
## DO NOT CHANGE
|
||
- `src/Modules/Cron/AllegroTokenRefreshHandler.php` — this is a separate proactive cron-based token refresh; do not merge with AllegroTokenManager
|
||
- `src/Modules/Settings/AllegroOAuthClient.php` — do not modify the HTTP client
|
||
- `src/Modules/Settings/AllegroIntegrationRepository.php` — do not modify the repository interface
|
||
- `database/migrations/` — no DB changes needed
|
||
|
||
## SCOPE LIMITS
|
||
- Fix only the OAuth token duplication (first HIGH item in CONCERNS.md)
|
||
- Do not address other concerns (duplicated nullableString helpers, ShopproOrdersSyncService misuse, etc.)
|
||
- Do not refactor any logic — only move existing code; do not change behavior
|
||
- Do not add tests in this plan
|
||
|
||
</boundaries>
|
||
|
||
<verification>
|
||
Before declaring plan complete:
|
||
- [ ] `php -l` passes on all 7 modified PHP files (0 syntax errors)
|
||
- [ ] `grep -r "resolveAccessToken\|forceRefreshToken\|requireOAuthData" src/` returns 0 results
|
||
- [ ] Application loads without fatal errors
|
||
- [ ] `AllegroTokenManager.php` exists at `src/Modules/Settings/AllegroTokenManager.php`
|
||
- [ ] CONCERNS.md no longer contains the OAuth duplication entry
|
||
</verification>
|
||
|
||
<success_criteria>
|
||
- All tasks completed
|
||
- All verification checks pass
|
||
- No behavior change — only structural extraction
|
||
- CONCERNS.md HIGH item #1 removed
|
||
</success_criteria>
|
||
|
||
<output>
|
||
After completion, create `.paul/phases/01-tech-debt/01-01-SUMMARY.md`
|
||
</output>
|