Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REST API: Adding + removing coupons via API doesn't recalculate totals correctly #18236

Merged
merged 4 commits into from
Jan 3, 2018

Conversation

ryelle
Copy link
Member

@ryelle ryelle commented Dec 19, 2017

This PR is proof of an issue, rather than a fix in itself. When coupons are added or removed from an existing order via the API, the item totals are not updated. I've added two failing tests in this PR, one for adding & one for removing a coupon.

You can also replicate the issue manually, by sending requests to POST orders/:orderId

Add a coupon:

{
    "coupon_lines": [
        {
            "code": <coupon-code>
        }
    ]
}

Remove a coupon:

{
    "coupon_lines": [
        {
            "id": <couponLineId>,
            "code": null
        }
    ]
}

In both cases, the line items are not updated (the discount is not applied to line items, or it's not removed from line items), which means the order total is not updated again when calculate_totals is run. It looks like a fix might be to run WC_Order::recalculate_coupons if coupon_lines changes, but that's a protected function in the abstract class. An alternate approach could be to use the apply_coupon/remove_coupon functions directly for coupon_lines.

Feel free to take over this PR to fix this, or give me a pointer & I'll fix it. We'll probably end up porting whatever fix we end up with into https://github.com/woocommerce/wc-api-dev, so letting @justinshreve & @timmyc know this exists :)

@ryelle ryelle changed the title REST API: Can REST API: Adding + removing coupons via API doesn't recalculate totals correctly Dec 19, 2017
@ryelle ryelle self-assigned this Dec 19, 2017
@claudiulodro claudiulodro added the type: bug The issue is a confirmed bug. label Dec 20, 2017
@claudiulodro claudiulodro added this to the 3.2.7 milestone Dec 20, 2017
@claudiulodro
Copy link
Contributor

I think it needs to use WC_Order::apply_coupon/WC_Order::remove_coupon instead of manually setting/removing coupon order items. I'll open a PR to this PR.

@claudiulodro claudiulodro modified the milestones: 3.2.7, 3.3.0 Dec 22, 2017
Update unit tests for api coupon recalculating
@codecov
Copy link

codecov bot commented Jan 2, 2018

Codecov Report

Merging #18236 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18236      +/-   ##
============================================
+ Coverage     37.62%   37.64%   +0.02%     
  Complexity    12547    12547              
============================================
  Files           344      344              
  Lines         46812    46812              
============================================
+ Hits          17612    17621       +9     
+ Misses        29200    29191       -9
Impacted Files Coverage Δ Complexity Δ
includes/abstracts/abstract-wc-order.php 70.86% <0%> (+0.16%) 283% <0%> (ø) ⬇️
includes/class-wc-tax.php 80% <0%> (+0.26%) 132% <0%> (ø) ⬇️
includes/api/class-wc-rest-orders-controller.php 88.71% <0%> (+1.11%) 128% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5349ffb...555d882. Read the comment docs.

@mikejolley
Copy link
Member

@claudiulodro Merging these updated tests and logged a new issue for wc-api-dev here: woocommerce/wc-api-dev#78

@mikejolley mikejolley merged commit 13602cd into master Jan 3, 2018
@claudiosanches claudiosanches deleted the fix/rest-api-add-coupons-order branch January 24, 2018 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants