if ($this->shouldRepack($order, $shipments)) {
$first_shipment = reset($shipments);
$shipping_profile = $first_shipment->getShippingProfile();
list($shipments, $removed_shipments) = $this->packerManager->packToShipments($order, $shipping_profile, $shipments);
// @todo Save only the modified shipments.
foreach ($shipments as $shipment) {
$shipment->save();
}
Which means that we need a way to ask the shipment whether it was modified. Probably by having populateFromProposedShipment() set a flag.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 2854034-4.patch | 819 bytes | googletorp |
| #3 | commerce_shipping-save-shipment-only-if-updated-2854034-3.patch | 3.39 KB | jsacksick |
| #2 | commerce_shipping-save-shipment-only-if-updated-2854034-2.patch | 1.71 KB | jsacksick |
Comments
Comment #2
jsacksick commentedI decided to give it a shot, I flag the shipment (I set _updated to TRUE) if a change has been detected.
However, I detected an issue while doing some tests locally, the test I'm relying on for detecting a change will always return TRUE because the default packer will return a list of proposed shipments that don't have the "package_type_id" property set.
Comment #3
jsacksick commentedSince the package type is populated in preSave, the package type set in
populateFromProposedShipmentwas always set to NULL causing the comparison to fail...As a temporary fix, I don't update the package type if empty.
Comment #4
googletorp commentedI believe ::hasTranslationChanges should give us what we need. It detects changes for a specific translation which is basically what we want to test.
Comment #5
bojanz commentedInteresting, didn't know about this method.
We need a test to prove that it works as promised (especially since Jonathan mentioned that package_type is kinda special)
Comment #6
googletorp commentedI was hoping we already would have a test for this, I need to dig into the module a bit more to really get a bearing of it again.
@bojanz Maybe we can hook up on IRC and talk about how to make the test, I'm still a bit fussy for some the architecture.
Comment #8
bojanz commentedDecided to proceed without tests, since we're using a core function here (and we've successfully used it in Profile since then)