My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 894: Patch for /tags/3.8.8-beta/wpsc-includes/coupons.class.php
7 people starred this issue and may be notified of changes. Back to list
Status:  Fixed
Owner:  ----
Closed:  Aug 2012


Sign in to add a comment
 
Reported by thomas@6www.net, Jan 2, 2012
If the buyer buy 2 products the discount should be also doubled. Isn't it ? + Type Error
coupons.class.php.patch
382 bytes   View   Download
Feb 12, 2012
Project Member #1 zaowebde...@gmail.com
This seems legitimate.  Haven't checked code, but this should make sure that "Apply to All Products" is checked.
Status: Accepted
Labels: Milestone-3.8.future
Feb 13, 2012
Project Member #3 leewilli...@gmail.com
That patch doesn't make sense to me.

My understanding of the "Applies to all products" was that if it wasn't set then it would apply to the first, and only the first matching item - hence: 

return $item->discount;

would be the desired behaviour?

Isn't the correct patch to change:

if($this->every_product == 1){
 	$return += $item->discount;

To: 

if($this->every_product == 1){
 	$return +=  ( $item->discount * $item->quantity );

[ie fix the "apply to all products" case?]
Feb 14, 2012
Project Member #4 benjamin...@gmail.com
I think Lee's take on this makes more sense.
Feb 15, 2012
Project Member #5 g...@instinct.co.nz
I also think Lee's right. Committing this.
Labels: -Milestone-3.8.future Milestone-3.8.9
Feb 15, 2012
Project Member #6 g...@instinct.co.nz
Actually, there's something that I need confirmation from other devs.

So, the code after being fixed will look like this:

if ( $this->every_product == 1 )
	$return += $item->discount * $item->quantity;
else
	return $item->discount;

What I'm concerned about is the "return $item->discount" line. Shouldn't it be "$return += $item->discount;" instead? We're looping through cart items, and this looks like a premature return to me.
Feb 15, 2012
Project Member #7 g...@instinct.co.nz
(No comment was entered for this change.)
Labels: -Type-Patch Workflow-HasPatch
Jul 23, 2012
Project Member #8 miche...@instinct.co.nz
(No comment was entered for this change.)
Labels: Workflow-NeedsReview
Jul 30, 2012
Project Member #9 gary...@garyc40.com
Here's what I think the intended behavior should be, so please correct me if I understand anything wrong:

- When "All product" is not checked, the discount amount is for the whole cart. That's what we've been doing all along.

- When "All product" is checked, the discount amount should also take into account the quantity of items. Currently we're not taking into account the quantity. Hence the patch attached below.

Please confirm that I got this right.
894.diff
9.3 KB   View   Download
Labels: -Workflow-NeedsReview Workflow-NeedsTesting
Jul 30, 2012
Project Member #10 miche...@instinct.co.nz
Hey Gary

"- When "All product" is not checked, the discount amount is for the whole cart. That's what we've been doing all along.

- When "All product" is checked, the discount amount should also take into account the quantity of items. Currently we're not taking into account the quantity. Hence the patch attached below. "

How is that different? the discount for the whole cart would be the same as the discount for each item ordered?

I agree with what Lee Said - All products should be all products and their quantity. If its not checked then it should just apply to the first item. Thats how I would expect it to work.
Jul 30, 2012
Project Member #11 gary...@garyc40.com
What I meant by "applied to the whole cart" is , if it's a percentage, it's the percentage of the whole cart. If it's a value, that value won't be multiplied with each item. The patch I attached above doesn't change any code related to this.

When "applied to each product" is checked, the discount amount will be multiplied with the number of products. However I think it should be multiplied with the total quantity. So if we have 2 products, but one of them has a quantity of two. The total discount amount should be multiplied by three.
Jul 30, 2012
Project Member #12 gary...@garyc40.com
Wait, I made a mistake in that last comment. If the discount amount is a percentage and "applied to all products" is not checked, then the amount would be the percentage of the first item, not of the whole cart.
Jul 30, 2012
Project Member #13 gary...@garyc40.com
Alright, looking at the current code without the patch, it's a bit confusing.

So, when the coupon does **not** have any conditions:

- If "Apply On All Products" is not checked:

   + if discount is percentage, return the percentage of the **subtotal** (ok, makes sense)

   + if discount is value, return the value, not multiplied by anything (ok, no issue with this)

- If "Apply On All Products" is checked:

   + if discount is percentage, return the percentage of the **subtotal** (ok, that's consistent)
   
   + if discount is value, return the value multiplied by **total quantity of all products in cart** (ok)

Now, if the coupon has conditions, then discount is calculated differently:

- If "Apply On All Products" is not checked:

   + if discount is percentage, return the percentage of the **first item** (woah, why only the first item and not the subtotal?)

   + if discount is value, return the value, not multiplied by anything (ok)

- If "Apply On All Products" is checked:

   + if discount is percentage, return the percentage of subtotal (ok, that's cool)

   + if discount is value, return the value multiplied by **total number of products**, not the total quantity (again, not good, not consistent!)

So in short, when there's no condition, the code seems good. But when there is condition, it's not consistent.
Jul 30, 2012
Project Member #15 gary...@garyc40.com
After discussing with Michelle, here's what we should do:

When the coupon does **not** have any conditions:

- If "Apply On All Products" is not checked:

   + if discount is percentage, we should return the percentage of the **first product** instead of the percentage of subtotal

When the coupon has conditions:

- If "Apply On All Products" is checked:
   + if discount is value, return the value multiplied by total quantity instead of total number of products


Does that make sense to everyone else?
Jul 30, 2012
Project Member #16 zaowebde...@gmail.com
Couple thoughts - 

- Rather than applying discounts to the first product (which is pretty arbitrary) - we should have a filterable sorting mechanism, defaulting to either the least or most expensive product.

- If there is a condition, the discount should probably only apply to the products that match that condition, right?

Other than that, I think we're on the right track here.
Labels: -Workflow-NeedsTesting Workflow-NeedsReview
Jul 31, 2012
Project Member #17 gary...@garyc40.com
First pass at fixing these coupon inconsistencies. Tested and working as expected for me.

I ended up refactoring the calculate_discount() function and break it down into multiple smaller functions. Code is easier to read, understand and debug now.

Instead of arbitrarily select the first item in the cart, we're defaulting to the lowest unit price product on the cart.

Also fixed a bug with free shipping not taking into account conditions, as well as "begins" and "ends" conditions don't work.

Please test thoroughly with all sorts of crazy conditions.
Labels: -Workflow-NeedsReview Workflow-NeedsTesting
Jul 31, 2012
Project Member #18 gary...@garyc40.com
(No comment was entered for this change.)
894.1.diff
24.9 KB   View   Download
Jul 31, 2012
Project Member #19 gary...@garyc40.com
Two more issues I found with coupons:

- Can't delete coupon conditions. Sometimes it works, sometimes it doesn't.

- Infinite redirect loop when deleting a coupon.

These will be addressed in an upcoming patch, in the mean time, let's just test 894.1.diff and make sure the discount amount works.
Jul 31, 2012
Project Member #20 gary...@garyc40.com
Update patch to handle item name condition properly now.
894.2.diff
24.8 KB   View   Download
Jul 31, 2012
Project Member #21 gary...@garyc40.com
Fix wrong comment.
894.3.diff
24.8 KB   View   Download
Jul 31, 2012
Project Member #22 gary...@garyc40.com
 Issue 1017  has been merged into this issue.
Aug 1, 2012
Project Member #23 miche...@instinct.co.nz
FTW - Great patch Gary works for me tried a million conditions :)

1 more tester to commit!
Aug 1, 2012
Project Member #24 miche...@instinct.co.nz
Ok wait one - so I can't recreate the delete errors your having. but just noticed this small UI thing as discussed - when you create a coupon for first time you have a link to add new conditions when you edit a current condition the link is gone see screen shot attached.. lets restore this and then call it done ill keep trying to create the delete infinite loop
Screen shot 2012-08-02 at 2.14.32 PM.png
65.2 KB   View   Download
Aug 8, 2012
Project Member #25 edw...@instinct.co.nz
I'm taking screen shots
[8/8/12 11:18:01 PM] Edward O'Rourke: Screen shot of the rule
http://dl.getdropbox.com/u/151891/screenshots/2012-08-08_2310.png

Screenshot of the result
http://dl.getdropbox.com/u/151891/screenshots/2012-08-08_2316.png
http://dl.getdropbox.com/u/151891/screenshots/2012-08-08_2317.png

discount is for $100 price should be $700 in the end
Labels: -Workflow-HasPatch -Workflow-NeedsTesting Workflow-NeedsReview
Aug 8, 2012
Project Member #26 edw...@instinct.co.nz
Previous comment links to this topic http://getshopped.org/forums/topic/discount-for-entire-order/
Aug 12, 2012
Project Member #27 g...@instinct.co.nz
Attached is a patch that fixes Edward's issue.

Regarding the missing "Add New Condition" for existing coupon, it seems like it has always been that way. There has never been that link. You just have to click "Update Coupon" every time you want to add a new one. We're redoing the coupon UI anyways, so that's not a big deal now.
894.4.diff
29.4 KB   View   Download
Labels: -Workflow-NeedsReview Workflow-NeedsTesting Workflow-HasPatch
Aug 13, 2012
Project Member #28 edw...@instinct.co.nz
Some failed - this is on a clean fresh download. 

patch -p0 -i /Users/efo/Desktop/894.4.diff 
patching file wpsc-admin/admin-form-functions.php
Hunk #2 FAILED at 145.
Hunk #4 FAILED at 263.
Hunk #5 succeeded at 342 with fuzz 2 (offset 11 lines).
Hunk #6 FAILED at 359.
Hunk #7 succeeded at 388 (offset 11 lines).
Hunk #8 FAILED at 440.
Hunk #9 succeeded at 455 with fuzz 2 (offset 11 lines).
4 out of 9 hunks FAILED -- saving rejects to file wpsc-admin/admin-form-functions.php.rej
can't find file to patch at input line 284
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: wpsc-admin/init.php
|===================================================================
|--- wpsc-admin/init.php	(revision 1931)
|+++ wpsc-admin/init.php	(working copy)
--------------------------
admin-form-functions.php.rej
11.6 KB   View   Download
Aug 14, 2012
Project Member #29 g...@instinct.co.nz
Hi Edward,

Don't apply the patch on a clean download. Use SVN, switch to branch 3.8 and make sure you have checked out the latest version of branch 3.8. Then apply the patch.

That worked fine for me.

But I refreshed the patch just in case and attached it here again.
894.5.diff
29.2 KB   View   Download
Aug 16, 2012
Project Member #30 gary...@garyc40.com
This issue was closed by revision r1977.
Status: Fixed
Sign in to add a comment

Powered by Google Project Hosting