Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#2 | uc_shipping-entities-0.patch | 57.86 KB | SilviuChingaru |
Comments
Comment #1
SilviuChingaru CreditAttribution: SilviuChingaru commentedComment #2
SilviuChingaru CreditAttribution: SilviuChingaru commentedI submit it here because of the complexity of the patch and I realy need your feedback so far.
Comment #3
SilviuChingaru CreditAttribution: SilviuChingaru commentedAccording 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. :-)
Comment #4
SilviuChingaru CreditAttribution: SilviuChingaru commentedMarked #1791042: Edit shipment form doesn't save package weights or e-mail addresses as duplicate of this issue because it should be fixed here.
Comment #5
SilviuChingaru CreditAttribution: SilviuChingaru commentedIntended 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.
Comment #6
longwaveI 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:
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.
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.
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.
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!
Adding this makes sense to me, almost all entities have creation and modification timestamps.
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?
I think this is safe to move to uc_shipping.admin.inc as part of this patch.
Comment #7
SilviuChingaru CreditAttribution: SilviuChingaru commentedThank you for your review I'll take it in consideration on the next patch (I think I'll post it tommorow with a interdif).
Comment #8
SilviuChingaru CreditAttribution: SilviuChingaru commented#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. :-)
Comment #9
SilviuChingaru CreditAttribution: SilviuChingaru commentedComment #10
TR CreditAttribution: TR commented