feat: replace platform references with proposition terminology #398
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a new "Proposition" concept to replace the legacy "Platform" terminology throughout the codebase, while maintaining full backward compatibility. The change adds a new PropositionManager and Proposition facade as the preferred API, while keeping the Platform facade functional but deprecated.
Key changes:
- New Proposition API: Added PropositionManager, PropositionManagerInterface, Proposition facade, and Proposition class with constants
- Backward compatibility layer: Platform API remains functional via delegation, with deprecation notices added
- Context model updates: GlobalContext and DeliveryOptionsConfig now contain both
proposition(new) andplatform(legacy) properties with identical data - Comprehensive test coverage: New test files ensure parity between old and new APIs
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Proposition/PropositionManager.php | New manager implementing proposition-specific logic with runtime platform detection |
| src/Proposition/PropositionManagerInterface.php | Interface defining proposition operations (all, get, getPropositionName, getCarriers) |
| src/Facade/Proposition.php | New facade providing clean API for proposition operations |
| src/Account/Proposition.php | New constants class for proposition IDs and names |
| src/Platform/PlatformManager.php | Added deprecated getPropositionName() method delegating to getPlatform() |
| src/Facade/Platform.php | Added deprecation notice to docblock |
| src/Context/Model/GlobalContext.php | Added proposition property alongside platform for dual support |
| src/Context/Model/DeliveryOptionsConfig.php | Added proposition property alongside platform for dual support |
| config/pdk-services.php | Registered PropositionManagerInterface service binding |
| tests/Unit/Proposition/PropositionManagerTest.php | Comprehensive test suite with parity tests |
| tests/Unit/Context/PropositionContextTest.php | Tests for context models with dual property support |
| tests/snapshots/*.json | Updated snapshots reflecting new proposition properties |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * 1. New 'proposition' properties work correctly | ||
| * 2. Legacy 'platform' properties remain functional | ||
| * 3. Both properties contain identical data (parity) |
There was a problem hiding this comment.
[nitpick] Consider using proper punctuation. The comment block should end with a colon followed by proper list formatting.
| * 1. New 'proposition' properties work correctly | |
| * 2. Legacy 'platform' properties remain functional | |
| * 3. Both properties contain identical data (parity) | |
| * | |
| * 1. New 'proposition' properties work correctly | |
| * 2. Legacy 'platform' properties remain functional | |
| * 3. Both properties contain identical data (parity) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #398 +/- ##
============================================
- Coverage 95.21% 95.16% -0.05%
- Complexity 1890 1901 +11
============================================
Files 352 354 +2
Lines 6202 6230 +28
============================================
+ Hits 5905 5929 +24
- Misses 297 301 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Add proposition references to Account model and UserAgent concern - Maintain platform references for legacy support - Add comprehensive tests INT-1085
NabDevs
force-pushed
the
feature/refactor-platform-naming-to-proposition
branch
from
4dc4a99 to
1aea3be
Compare
|
|
||
| // Check for SendMyParcel (BE) | ||
| // Note: Platform constants use 2, but we support both 2 and 3 during transition | ||
| if (Platform::SENDMYPARCEL_ID === $platformId || 3 === $platformId) { |
There was a problem hiding this comment.
Dit hoeft toch niet meer, omdat Platform is geüpdatet met de eerdere fix (die corrigeerde voor de verkeerd ingetypte id’s)? (1/2)
| public const MYPARCEL_NAME = 'myparcel'; | ||
|
|
||
| /** | ||
| * Note: Platform constants use 2, but platformId 3 is also supported during transition |
There was a problem hiding this comment.
Hoeft niet meer toch? (2/2)
| public function getPropositionName(): string | ||
| { | ||
| // First check if platform is explicitly configured in PDK | ||
| $configuredPlatform = Pdk::get('platform'); |
There was a problem hiding this comment.
Waarom is deze toegevoegd (tov de eerdere implementatie in platformManager)? Bij mijn weten wordt dit niet meer geplaatst in de plugins (als ze platform ‘agnostisch’ zijn), dus dan is het alleen maar ruis / afleiding, of mis ik iets?
| expect($propositionName)->toBe(Platform::MYPARCEL_NAME); | ||
| }); | ||
|
|
||
| it('uses configured platform over account settings', function () { |
There was a problem hiding this comment.
Check even of dit echt moet, volgens mij niet namelijk. Ik denk dat het eerder tot problemen kan leiden (platform dat niet klopt met de api key bv)
| * Test support for both platformId 2 and 3 during transition | ||
| */ | ||
|
|
||
| it('supports both platformId 2 and 3 for SendMyParcel', function (int $platformId) { |
There was a problem hiding this comment.
Hoeft dus volgens mij ook niet.
| })->with('platforms'); | ||
|
|
||
| it('returns proposition name based on account', function () { | ||
| MockPdkFactory::create(['platform' => Platform::MYPARCEL_NAME]); |
There was a problem hiding this comment.
Deze slaat de propositie niet op in de account maar in de config, volgens mij, dus wordt niet getest wat die zegt dat ie test, volgens mij.
| // GlobalContext Tests | ||
|
|
||
| it('GlobalContext has both proposition and platform properties', function () { | ||
| MockPdkFactory::create(['platform' => Platform::MYPARCEL_NAME]); |
There was a problem hiding this comment.
Het is slechts een testfile, maar ik zou toch niet de deprecated Platform gebruiken hier. Is er een courante manier om deze constante op te halen?
INT-1085