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

Amend store visibility of fields in Tax admin panel #10102

Closed

Conversation

gpcrocker
Copy link

@gpcrocker gpcrocker commented Jun 29, 2017

Original Description

In the admin panel, setting Stores -> Configuration -> Tax - > Calculation setting -> Apply Customer Tax from After Discount to Before Discount had no affect.

In \vendor\magento\module-tax\Model\Calculation\UnitBaseCalculator.php:45
$applyTaxAfterDiscount = $this->config->applyTaxAfterDiscount($this->storeId); kept returning true inspite of setting the above correctly.

Description

Fields that are being pulled at the store level from Magento Tax Config Model are not available to be set in the Configuration panel. This fix makes them visible to allow the user to for example set the Apply Customer Tax_ from After Discount to Before Discount

Manual testing scenarios

  1. Stores -> Configuration -> Tax - > Calculation setting -> Apply Customer Tax from After Discount to Before Discount
  2. Breakpoint on \vendor\magento\module-tax\Model\Calculation\UnitBaseCalculator.php:45 should return false
  3. Stores -> Configuration -> Tax - > Calculation setting -> Apply Customer Tax from Before Discount to After Discount
  4. Breakpoint on \vendor\magento\module-tax\Model\Calculation\UnitBaseCalculator.php:45 should return true

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Jun 29, 2017

CLA assistant check
All committers have signed the CLA.

@okorshenko okorshenko self-assigned this Jun 29, 2017
@okorshenko okorshenko added this to the June 2017 milestone Jun 29, 2017
@gpcrocker
Copy link
Author

gpcrocker commented Jun 30, 2017

Test failed, because it is expecting the scope to be 'store' and yet in adminhtml/system.xml it is clearly not set to show in store. If it is meant to show in store then I can revert the commit and and change it at magento-deploy\magento2\app\code\Magento\Tax\etc\adminhtml\system.xml

<field id="discount_tax" translate="label comment" type="select" sortOrder="50" showInDefault="1" showInWebsite="1" showInStore="0" canRestore="1"> <label>Apply Discount On Prices</label> <source_model>Magento\Tax\Model\System\Config\Source\PriceType</source_model> <backend_model>Magento\Tax\Model\Config\Notification</backend_model> <comment>Warning: To apply the discount on prices including tax and apply the tax after discount, set Catalog Prices to “Including Tax”.</comment> </field>

Note that there are 11 other fields which may have their scope expected to be store (not website) and are set not to show in store.

@gpcrocker gpcrocker changed the title Fix Apply Customer Tax Before Discount configuration Fix Apply Customer Tax Before Discount field Jun 30, 2017
@gpcrocker gpcrocker changed the title Fix Apply Customer Tax Before Discount field Fix scope of calculation setting fields in Admin panel Jun 30, 2017
@gpcrocker
Copy link
Author

Failure:

  1. Magento\Tax\Test\Unit\Model\ConfigTest::testScopeConfigMethods with data set Can you commit to repository a folder dev/tests/static ? #1 ('applyTaxAfterDiscount', 'tax/calculation/apply_after_discount', true, true)
    Expectation failed for method name is equal to string:getValue when invoked 1 time(s)
    Parameter 1 for invocation Magento\Framework\App\Config\ScopeConfigInterface::getValue('tax/calculation/apply_after_discount', 'website', null) does not match expected value.
    Failed asserting that two strings are equal.
    --- Expected
    +++ Actual
    @@ @@
    -'store'
    +'website'

@gpcrocker
Copy link
Author

In rethinking this - Tax calculation settings need to be on a Store view scope as the test expects, because a client can use a Store view for different countries/languages so this PR needs to change to include tax calculation. http://docs.magento.com/m1/ce/user_guide/store-operations/stores-multiple.html

So what needs to be changed is the adminhtml/system.xml

gpcrocker and others added 2 commits June 30, 2017 10:30
Functions in Tax Model get values from the store scope thus these fields should be visible at the store view level.
@gpcrocker gpcrocker changed the title Fix scope of calculation setting fields in Admin panel Amend store visibility of fields in Tax admin panel Jun 30, 2017
@okorshenko okorshenko modified the milestones: June 2017, July 2017 Jul 2, 2017
@vherasymenko
Copy link

@gpcrocker Hi.
I can`t reproduce bug described in your ticket.
Try on default store and on created.

Steps which I execute:

  1. Install Magento from develop branch
  2. Create second store in "Stores > All Stores"
  3. Create some product in "Catalog > Products" with price 100
  4. Create category and asigne created proudct to that category. "Catalog > Categories"
  5. Create Tax Rule for US "Stores > Tax Rule" (8.25 %)
  6. Create Customer on frontend with settings which can by applied for TAX which was created early.
  7. Try create "Cart price rule" coupon code. (for example 20% discount)
    8 . In Settings "Stores > Configurations > Sales > Tax > Calculation Settings > Apply Customer Tax" set to "Before Discount"
  8. Add created product to cart on frontend and add coupon code for order.
  9. Click on Cart and click "View and Edit Card" Verify TAX price in right side.
  • Displayed 8.25$ Tax. (It`s correct behavior)
  1. Try verify on checkout page
  • Displayed 8.25$ Tax. (It`s correct behavior)
  1. Go to "Admin > Stores >Configuration > Sales > Tax > Calculation Settings > Apply Customer Tax" set to "After discount"
  2. Add created product to cart
  3. Click on Cart and click "View and Edit Card" Verify TAX price in right side.
  • Displayed 6.6$ Tax. (It`s correct behavior)
  1. Try verify on checkout page
  • Displayed 6.6$ Tax. (It`s correct behavior)

Also I tried with "Catalog price rule" and "Cart price rule" where added some conditions and discount 20%. In this case item price was decresed to 80$ and when add that item to cart tax calucation starts from 80$.

The same situation with Tier price for product.

  • It`s correct behavior.

Maybe I miss some steps for reproducing bug which was described in ticket?

@gpcrocker
Copy link
Author

gpcrocker commented Jul 11, 2017

@k7triton It was reported to us that on a £10 order, if a 10% discount was applied they got £1.20 off instead of £1.00. (Note that this is UK Tax 20%)

We wanted to configure our store at the store level (and not the website level) so if 10% is taken off 10, then the user has a new cart total of 9. Instead the 10% discount was being calculated on the VAT separately. We amended the configuration from After Discount to Before Discount to no avail.

I dug through core code and noticed that Apply Customer Tax configuration was being brought in at the store level, but in app/code/Magento/Tax/etc/adminhtml/system.xml this setting was hidden in adminhtml at the store level. In the Database the setting for apply customer tax was actually null, so I am not sure how this can be made to work using our website-store structure.

When I find the right time at work, I will go through the same steps you described on our store and my local magento 2 instance.

Thanks for taking the time to see this issue.

@okorshenko
Copy link
Contributor

HI @gpcrocker
Thank you for your contribution. We are closing this PR due to inactivity. Please, feel free reopen this PR once ready. Thank you

@okorshenko okorshenko closed this Jul 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants