I want to be able to add fields to my shipments using the Entity API. In my specific example I would like to be able to add a file field so I can upload signed packing slips to go with their associated shipments.

CommentFileSizeAuthor
#2 uc_shipping-entities-0.patch57.86 KBSilviuChingaru
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

SilviuChingaru’s picture

Assigned: Unassigned » SilviuChingaru
SilviuChingaru’s picture

Version: 7.x-3.2 » 7.x-3.x-dev
Status: Active » Needs review
FileSize
57.86 KB
  • The patch is not final and I barely test it.
  • The code also needs cleanup.
  • There are also known bugs (see TODO in code)

I submit it here because of the complexity of the patch and I realy need your feedback so far.

SilviuChingaru’s picture

According to #2094489: DatabaseTransaction::rollback() should check if already rolled back before trying to rollback comment #1 our implementation is correct so we should pass $transaction object to parent function to make sure all data has been written correctly to database and we've don't get partial save on shipment or package save.

At least I hope I understood right. :-)

SilviuChingaru’s picture

Marked #1791042: Edit shipment form doesn't save package weights or e-mail addresses as duplicate of this issue because it should be fixed here.

SilviuChingaru’s picture

Status: Needs review » Needs work

Intended features (suggestions are welcomed / needed):
Create fieldable entities:
uc_shipping_shipment (will include packages and packaged products entities)
uc_shipping_package (will include packaged products)
uc_shipping_packaged_product
uc_shipping_single_packaged_product (maybe)

Making all those entities fieldable will allow user to put any type of info on any of them. Like for packages a phisical weight of the package, or tare weight like DanZ suggested in a past issue.

For uc_shipping_packaged_product we will create one entity per each piece (1 qty = 1 entity, 2 qty = 2 entities) in order to be able to attach distinct field(s) per order product. For instance if we have 3 ordered televisions (same SKU) and we want to add serial number of each product and we send 2 in one package and 1 in another. With this implementation if we send those 2 packages with two distinct carriers or even with the same carrier and one is damaged or lost, we know for sure which one was lost or damaged and which was delivered to client. Or we could attach scans of warranty certificates and send them via email using rules for example when shipment is saved or display them to the client in a view with relationship.

Or we could create a sub-entity of packaged product on which to attach fields, something like uc_shipping_single_packaged_product? I don't think this is quite needed because there is no need for fields on uc_shipping_packaged_products then and way keep them both? For consistency maybe but it seems will be an useless table in db and a waste of space there. The best way i think is to create one instance of uc_shipping_packaged_product per each unit (piece). Waiting for suggestions here.

At package creation the form will be multi-step if user attached fields for packaged products an additional form step will be displayed with attached fields form. If user didn't attached fields form will remain the same like in current implementation.

Modules now can react on each entity hooks (load, presave, save, delete, etc). For example when a product is (un)packed, when a package is created/edited/deleted or a shipment is saved/deleted (or on all of them if needed).

I think with this implementation freedom will be endless and uc_shpping module will not need lot of work to be ported on D8 when the time will come. Also module should keep compatibility until then.

If you see any limitation or inconvenient please post it here.

I also intend to create some tests for uc_shipping module.

longwave’s picture

I haven't tested this, but reading the patch this is looking good - thank you for working on it. As this is already quite a big patch can we commit it in parts? I would like to get the package and shipment entities working alone first, then think about adding packaged products as entities in a followup.

Regarding uc_shipping_packaged_product vs uc_shipping_single_packaged_product, maybe this should be an option in the module? If uc_shipping_packaged_product has a qty field, then there could be an option to either keep packaged products with their full quantity, or split them into multiple entities each with a quantity of 1? I can't see a case when you would need both entity types at the same time, but some stores might want to split as in your example, while others might not need or want to?

Tests would be very welcome, as we don't have any for uc_shipping - some basic user interface and API tests would be a great addition. Ideally we would have some basic UI tests that work with the existing page callbacks and that also work with this patch applied (or with minimal changes after applying this patch), as that would help prove that we aren't breaking any existing shipping or packaging functionality.

A quick review of the patch and TODO items:

  * Displays shipment details.
+ *
+ * @TODO There is no need to pass the $order here. We should remove it but
+ *       we let it here for compability reasons.

I don't have a problem with breaking compatibility and removing $order here; this is a page callback and as we are changing behaviour anyway, anyone calling this will likely need to change their code.

+class UcShipping extends EntityAPIController {
+class UcShippingPackage extends UcShipping {
+class UcShippingShipment extends UcShipping {

I think "UcShipping" should maybe be renamed UcShippingBaseController, as it only contains the overridden delete() method. The other two should become UcShippingPackageController and UcShippingShipmentController as they are controller classes, not actual containers.

+    // @TODO Almost all package fields should be real fields not pseudofields
+    //       so this part should be removed in future. For now we mimic a real
+    //       field.

I actually think pseudofields are okay in some cases, specifically when you know the field must exist, and you never want the user to alter the properties/metadata of the field in any way. Drupal 8 is continuing this with "nonconfigurable fields". However I guess a lot of the package fields are debatable as to whether they are needed on all sites, and making them configurable might be useful. Migrating existing data might be difficult, though.

+          // @TODO Why we need to keep package info in uc_order_products data?
+          //       Now, uc_order_products are entities so we should react on
+          //       entity load, view, save etc. not keep data in data field.
+          //       Is this data used by any module?

No idea why we keep the package info in uc_order_products, it doesn't seem very useful. This is probably legacy code from Drupal 5 that has never been reviewed!

+      // @TODO I think we should add created too.

Adding this makes sense to me, almost all entities have creation and modification timestamps.

+      // @TODO Modules should now invoke hook_entity_insert or hook_entity_update
+      //       instead of this, we leave it here for now for compatibility
+      //       reasons but we should remove it in future.

+  // @TODO Should we implement this on entities?
+  //       At this time is used only by uc_google_checkout_uc_shipment() and
+  //       maybe some custom modules.

We should keep the uc_shipment_ hooks for now but remove them in D8. I think hook_uc_shipment_load can be moved from uc_shipping_shipment_load() to the controller class?

+/**
  * Displays the details of a package.
+ *
+ * @TODO This should be moved to uc_shipping.admin.inc or
+ *       uc_shipping_shipment_view() should be moved here.
  */
 function uc_shipping_package_view($package) {

I think this is safe to move to uc_shipping.admin.inc as part of this patch.

SilviuChingaru’s picture

Thank you for your review I'll take it in consideration on the next patch (I think I'll post it tommorow with a interdif).

SilviuChingaru’s picture

Issue summary: View changes

#802372: Allow Products to have Multiple Packages

Packaged product entity should be allowed to have unlimited pacakage_ids. We could implement this by adding another table with only 2 keys: packaged_product_id and package_id and on UcShippingPackagedProductController::buildQuery() add a key like packages to packaged product entity where we assemble all packages that contains this product.

P.S.: It took me a year to get back on this, but now I'm on it. :-)

SilviuChingaru’s picture

Assigned: SilviuChingaru » Unassigned
TR’s picture

Version: 7.x-3.x-dev » 8.x-4.x-dev
Component: Shipping » Fulfillment