feat: introduce shipment query, delete, return and multi-collo services#596
feat: introduce shipment query, delete, return and multi-collo services#596
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## v11 #596 +/- ##
==========================================
+ Coverage 7.63% 8.36% +0.73%
- Complexity 22999 23054 +55
==========================================
Files 595 601 +6
Lines 56201 56387 +186
==========================================
+ Hits 4289 4717 +428
+ Misses 51912 51670 -242 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
myparcel-bot
bot
force-pushed
the
feat/v11-int-1032-shipment-services
branch
from
b4563e6 to
b557f34
Compare
myparcel-bot
bot
force-pushed
the
feat/v11-int-1032-shipment-services
branch
from
f251f29 to
cd5ca0f
Compare
Migrates remaining MyParcelCollection functionality into dedicated service classes using the generated OpenAPI client as source of truth. New services: - ShipmentQueryService (find, findMany, query, findByReferenceId) - ShipmentDeleteService (deleteMany) - ReturnShipmentService (createRelated, createUnrelated) - MultiColloShipmentService (splitShipment) Refactored services: - ShipmentTrackTraceService: hybrid approach for street deserialization bug - ShipmentPrintService: migrated from MyParcelRequest to generated client - ShipmentLabelsService: removed MyParcelRequest dependency Services using hybrid approach (request builder + manual send) include @todo markers for reverting once upstream spec/codegen fixes land.
myparcel-bot
bot
force-pushed
the
feat/v11-int-1032-shipment-services
branch
from
cd5ca0f to
0512442
Compare
| public function splitShipment(Shipment $shipment, int $amount): Shipment | ||
| { | ||
| if ($amount < 2) { | ||
| throw new InvalidArgumentException('Multi-collo requires at least 2 shipments'); |
There was a problem hiding this comment.
Following offline discussion: we do not have to validate this here.
| $secondary = new SecondaryShipmentRequest(); | ||
| $secondary->setReferenceIdentifier($referenceId); | ||
|
|
||
| $carrier = $main->getCarrier(); |
There was a problem hiding this comment.
I would move this assignment to outside the for loop just like weightPerCollo and referenceId. No reason for rejection just an observation.
| public function deleteMany(array $shipmentIds): void | ||
| { | ||
| if (empty($shipmentIds)) { | ||
| throw new InvalidArgumentException('At least one shipment ID is required'); |
There was a problem hiding this comment.
I think you can just return here as well, re: descussion about throwing errors. For consideration.
| { | ||
| $shipments = $this->findManyByReferenceId([$referenceIdentifier]); | ||
|
|
||
| return $shipments[0] ?? null; |
There was a problem hiding this comment.
I think findManyByReferenceId may return multiple shipments for the same referenceId, which is a scenario that can happen in the real world I think. Should we not return an array of shipments as well here and let the consuming app decide what to do (error / log / present results) what happens when multiple shipments exist with the same referenceId?
| $id = $shipment->getId(); | ||
|
|
||
| if (null === $id) { | ||
| $merged[] = $shipment; |
There was a problem hiding this comment.
I would consider this a bug, is it possible that a $shipment without an id is returned? If so, we are now silently assigning an id to it here in the $merged array because the keys are id’s, for instance 225 if the previous shipment had id 224, what happens when a shipment with id 225 is also in the loop?
| $shipmentId = $tracktrace->getShipmentId(); | ||
|
|
||
| $result[(int) $trackTrace->getShipmentId()] = $trackTrace; | ||
| if (null !== $shipmentId) { |
There was a problem hiding this comment.
This is a different approach from the ShipmentQueryService, now if there is no shipmentId we just ignore it. Is there a reason why these are different?
| ]); | ||
|
|
||
| $service = new ShipmentPrintService($this->getApiKey(), $api, $httpClient); | ||
| $service->setUserAgentForProposition('Magento', '2.4.7'); |
There was a problem hiding this comment.
Sanity check: is this useragent necessary for the test, and if so is a more generic one maybe clearer about the intent (for instance 'appname', 'v1.0.0')
3 participants
Migrates remaining MyParcelCollection functionality into dedicated service classes using the generated OpenAPI client as source of truth.
New services:
Refactored services:
Services using hybrid approach (request builder + manual send) include @todo markers for reverting once upstream spec/codegen fixes land.
INT-1032