The premise is this: my client wants to be able to offer upgrade shipping such that if standard shipping fees are $7.95 and expedited shipping is $14.95, then expedited shipping would be available to the customer at a cost of $7.95.

Practically speaking, this requires the administrator to select a shipping service whose rate should be discounted and the shipping service it should be discounted to match. I'm still toying around with label names here, though I'll just use target and source as the machine-names (e.g. the target is expedited shipping and the source is standard shipping to signify that expedited shipping should be discounted to the price of standard shipping).

CommentFileSizeAuthor
#11 2560455-11.shipping_service_upgrade.patch17.81 KBrszrama
#10 2560455-10.shipping_service_upgrade.patch17.72 KBrszrama
#2 shipping-service-upgrade.png30.17 KBrszrama

Comments

rszrama created an issue. See original summary.

rszrama’s picture

rszrama commented
Title: Add a "Free shipping upgrade" offer type » Add a "Shipping service upgrade" offer type
StatusFileSize
new shipping-service-upgrade.png30.17 KB

Retitling the issue based on changes I made during implementation. Decided "Free shipping" followed by "Free shipping upgrade" would be a bit confusing.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

This seems like a good customer experience although very specific case.
Have you had this request more than once from clients or other clients mention they were interested in this feature?

It could be a bit easy to get down feature bloat (which muddies the UI on which option to choose). Even as it is head: 'Free shipping' could be the same as '100% off of shipping'.

It's nice the customer doesn't need to know the price of the shipping in this approach. Just questioning how important and used this feature would be (man it would be nice to have anonymous usage stats from a module like this so it could be tailored and cruft can be scraped off easier :) .)

rszrama’s picture

rszrama commented

Honestly, every project we take on asks for discount types that don't exist. The project I'm on is just the first time I've been driving the process and know how to take those things and make them part of the appropriate module. : D

I'd be happy for all of the shipping related discounts to be moved out of this module into Commerce Shipping, so I'm really just following the historical model here. I have another one coming to discount the price of one shipping service by the price of another (i.e. get "Free standard shipping or an equivalent amount off of expedited shipping.").

This module really offers value the more precise offers get as opposed to the more abstract.

rszrama’s picture

rszrama commented

Also, I will move this below the "% off" offer and change the label for the first select list to the shorter "Let customers select this service".

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I have an abstract offer that would really help;) Free shipping on "all services". Right now if I have $1000 order and we want free shipping everywhere I'd have to create a different Discount for each service.

Sometimes less specific provides it's own benefits.

rszrama’s picture

rszrama commented

Well, that's just a failure of the current shipping offers in my opinion. You shouldn't have to choose a shipping service or services to limit it to.

Dropping the price of one to the price of another or removing a variable amount from any shipping service is not so easily managed. Even just through Rules.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

Suggestion:

For the price of this one

to

Using the same price of this shipping service

Does that help clarify?

rszrama’s picture

rszrama commented

Yeah, I like that approach. Let me give it a few iterations. I'm also going to open an issue for the abstract shipping discount offer types to support optional shipping service restrictions.

rszrama’s picture

rszrama commented
Status: Active » Needs review
StatusFileSize
new 2560455-10.shipping_service_upgrade.patch17.72 KB

Alrighty, the patch attached adds this new offer type and the matching Rules action to enact it. It's pretty straightforward - how to present the options was more difficult than encoding the logic.

The only thing I'm a little unsure of is how to indicate the discount was applied to the customer. I followed the pattern of the % off action, but really that action is doing this incorrectly. It shouldn't be showing the discount label to the customer, because the label is only supposed to be shown to administrators (per the UI help text). It should be using the $discount->component_title instead. I'll file a separate issue for that.

(It's worth noting that there isn't much you can do here outside of using a new price component type. The default behavior of the shipping module is to only show the shipping amount in the order total formatted with components, not the shipping line item itself. This means the discount module's alteration of formatted components will not come into effect to swap in the discount's component title where it would otherwise.)

rszrama’s picture

rszrama commented
StatusFileSize
new 2560455-11.shipping_service_upgrade.patch17.81 KB

Saw I forgot to put a return value on the update function. Attached patch fixes that.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

This looks very good, I just reviewed the code only not testing it. Those fields look awfully tricky to maintain I wonder how we can refactor but still make them easy to see what's being built at a glance? (not needed for this patch just an observation)

  1. +++ b/commerce_discount.rules.inc
@@ -315,21 +333,21 @@ function commerce_discount_rules_event_info() {
-    'group' => t('Commerce - discount'),
+    'group' => t('Commerce Discount'),

    Bit unrelated but sure why not, cleanup abound! 

  2. +++ b/commerce_discount.rules.inc
@@ -930,7 +948,75 @@ function commerce_discount_percent_off_shipping_service($order, $discount_name)
+  // If the order has had shipping rates calculated for it.
+  if (!empty($order->shipping_rates)) {
+    // Load the discount to find the target and source services.
+    $discount = entity_load_single('commerce_discount', $discount_name);
...
+  }

    Just a note, it may be better to exit early to avoid the super long if statement. Just makes the function a bit easier to follow. 

  3. +++ b/commerce_discount.rules.inc
@@ -930,7 +948,75 @@ function commerce_discount_percent_off_shipping_service($order, $discount_name)
+    try {
...
+    catch (Exception $e) {
+      return;
+    }

    Should this be watchdog to the fact that their services is busted?

rszrama’s picture

rszrama commented

I don't mind adding the watchdog - sure. I'll do it for the "free or reduced" patch as well.

And sorry for the errant clean-up - I had used an event in this patch originally before realizing I really needed to use the same event as the percent off offer type to ensure the two types of discount offers could be sorted relative to one another. Made it match the action / event groups at that time. : P

rszrama’s picture

rszrama commented

@joelpittet My preference would be to clean-up all of the code related to field management in this module. Instead of managing all these arrays directly and creating fields in different locations than their instances, I'd bring 'em all inline - make an API function for creating at the same time the fields and instances we want.

Additionally, they should function as "Create or Update" commands, whereby we reset any critical settings in the event that a field or instance has been edited elsewhere via code in a way that we know breaks it. I've used this approach to great effect on other projects.

See for example:

  // Create the relevant fields on the vocabularies as necessary.
  field_info_cache_clear();

  // Field one, used to do X on Y.
  example_create_or_update_text_field_and_instance('example_bundle', 'example_field_one', t('Field one'), 1);

  // Field two, used to do A on B.
  example_create_or_update_text_field_and_instance('example_bundle', 'example_field_two', t('Field Two'), 1);

I love the CINC project for managing this, though in the project that pseudo-code is in I just managed the arrays directly via custom functions. Depends on whether or not you want to add the dependency. Additionally, you set it up so these functions are always called on module_enable() and they support retaining any allowed changes that may have been made to the fields (such as description changes).

rszrama’s picture

rszrama commented

Also, I wish the shipping related discounts / offer types were simply a part of Commerce Shipping. : P

rszrama’s picture

rszrama commented
Status: Needs review » Fixed

Alrighty, addressed your issues in #12. Unfortunately, it'd been two weeks since I worked on the patch, and I forgot I was managing two patches at the same time (this and #2560773: Add support for "Free or reduced shipping" offers) via separate branches. This means the clean-up suggested will actually be committed as I commit the follow-up.

  • rszrama committed 821d42a on 7.x-1.x 
    Issue #2560455 by rszrama: add a shipping service upgrade offer type.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.