Don't use $rate as an object (it is an array) #30052

flancer64 · 2020-09-15T11:41:19Z

Description (*)

$rate is an array and it does not contain id item. We can use rate's code item as part of id for HTML nodes.

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

`$rate` is an array and it does not contain `id` item. We can use rate's `code` item as part of `id` for HTML nodes.

m2-assistant · 2020-09-15T11:41:22Z

Hi @flancer64. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

Stepa4man · 2020-09-15T19:21:28Z

@magento run all tests

Stepa4man

Hi @flancer64
Thanks for your contribution

Could you, please take a look at my comments?

Stepa4man · 2020-09-15T19:23:57Z

app/code/Magento/Sales/view/adminhtml/templates/order/totals/tax.phtml

+                <?php
+                    $rateCode = strtolower($rate['code']);
+                    $rateId = str_replace(' ', '-', $rateCode);
+                ?>


Please, don't hold logic in the template. Can you put this to view Model?

Stepa4man · 2020-09-15T19:25:38Z

app/code/Magento/Sales/view/adminhtml/templates/order/totals/tax.phtml

+                    $rateCode = strtolower($rate['code']);
+                    $rateId = str_replace(' ', '-', $rateCode);
+                ?>
+                <tr id="rate-<?= /* @noEscape */ $rateId ?>"


Why should we keep $rateId as the name of the variable?

flancer64 · 2020-09-16T05:49:50Z

Hello, @Stepa4man .

Sorry, I cannot "move logic from template to view Model". I don't know what a right logic should be here. I've just fixed the bug. Someone wrote this:

$rate->getId()

for array $rate. It interrupts templates rendering.

I'm sorry again, but I have no answer to question "Why should we keep $rateId as the name of the variable?" :( You shouldn't. It just my own choice.

Please, feel free to reject this pull request if it doesn't match your rules.

engcom-Golf · 2020-09-23T13:47:59Z

Hi, @flancer64, are you able to continue with this pull request?
As a unique value, you can use tax_id field and if it is still necessary you can move logic to view model and use in this template.
Instructions can be found by the following link View models

Also could you please cover your code changes with an MFTF test? You can use the following Magento Functional Testing Framework Guide to get acquainted with it.
If you have any additional questions or appointments feel free to ask, I will try to help,

Thank you!

flancer64 · 2020-09-23T15:50:17Z

@engcom-Golf , I cannot use tax_id. There was no such field under debugger in the structure. I don't know why this field was omitted on rate data selection from DB but it was:

I agree that the better way is to use tax_id field here instead of code but we need to fix code that selects data from DB first. I'm sorry, but It's too deep for me, I don't have so much free time to check side effects. I've created this "dirty fix" and it's pretty enough for me and my projects.

flancer64 · 2020-09-23T16:02:14Z

Maybe we can use position field or even 'foreach' loop index here.

engcom-Golf · 2020-09-28T14:18:30Z

@flancer64, please look at \Magento\Tax\Model\ResourceModel\Calculation::getCalculationProcess method. There should be an ability to add rate id to the output array.

flancer64 · 2020-09-29T04:36:49Z

Thanks, @engcom-Golf . This is the way.

This SQL is composed to select rates data in \Magento\Sales\Block\Adminhtml\Order\Totals\Tax::getFullTaxInfo:

SELECT
	`main_table` .*
FROM
	`sales_order_tax` AS `main_table`
WHERE
	(main_table.order_id = 1234)
ORDER BY
	`process` ASC

so, we can use sales_order_tax.tax_id as identifier for tax items.

engcom-Golf · 2020-09-29T07:09:13Z

@magento run all tests

engcom-Golf · 2020-09-29T10:33:56Z

Hi, @flancer64, thank you for your changes, but as we can see from failed tests, this approach is not acceptable. I have made some investigations and found that rates are taken not only from the database. So tax_id can be not present in rates array. Suppose we can use 'for each' loop index, as you have proposed before.

flancer64 · 2020-09-29T16:24:13Z

Suppose we can use 'for each' loop index, as you have proposed before.

@engcom-Golf , done.

ihor-sviziev · 2020-10-28T15:11:28Z

Seems like this PR fixes #30094.

We also have another PR #30516 that fixes this issue, so probably the only one PR will be accepted.

The main reason why this issue appeared - it wasn't covered with automated tests.

engcom-Delta

@flancer64 sorry for delay
Due to Magento Definition of Done all code must be covered by tests.

Could you add automation test that cover your changes?

flancer64 · 2020-11-05T07:20:23Z

@engcom-Delta , I'm sorry, I couldn't.

ihor-sviziev · 2020-11-05T07:37:02Z

Hi @flancer64,

I just analyzed another solution that we have - seems like array index solution potentially might cause ID duplication, as in some other place on the page might be same id rate-1, or rate-0. I believe using random string is better and this approach is used in #30516.

I'm closing this PR as we just have better solution for the same issue in #30516.

Thank you so much for your contribution!

m2-assistant · 2020-11-05T07:37:03Z

Hi @flancer64, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Don't use $rate as an object (not an array)

c04d95a

`$rate` is an array and it does not contain `id` item. We can use rate's `code` item as part of `id` for HTML nodes.

ghost added the Progress: pending review label Sep 15, 2020

magento-engcom-team added Area: Frontend Component: Sales Release Line: 2.4 labels Sep 15, 2020

Stepa4man self-assigned this Sep 15, 2020

Stepa4man requested changes Sep 15, 2020

View reviewed changes

ghost added Progress: needs update and removed Progress: pending review labels Sep 15, 2020

Stepa4man added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Sep 15, 2020

sidolov added Priority: P3 May be fixed according to the position in the backlog. Risk: low Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Sep 16, 2020

flancer64 mentioned this pull request Sep 16, 2020

Prevent "Overwrite the existing configuration for db-ssl-verify?" question #26818

Closed

1 task

engcom-Golf self-assigned this Sep 23, 2020

Add tax_id attribute selection in Calculation resource model

b72deb5

Use loop index as tax id

a943c33

engcom-Delta self-assigned this Oct 22, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Oct 23, 2020

Morgy93 mentioned this pull request Oct 28, 2020

fix #30094 function call on array #30516

Closed

6 tasks

engcom-Delta requested changes Nov 5, 2020

View reviewed changes

m2-community-project bot added Progress: needs update and removed Progress: testing in progress labels Nov 5, 2020

engcom-Delta added Progress: testing in progress and removed Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Progress: needs update labels Nov 5, 2020

m2-community-project bot added Progress: needs update and removed Progress: testing in progress labels Nov 5, 2020

engcom-Delta added Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests Progress: testing in progress and removed Progress: needs update labels Nov 5, 2020

m2-community-project bot added Progress: needs update and removed Progress: testing in progress labels Nov 5, 2020

ihor-sviziev closed this Nov 5, 2020

m2-community-project bot removed the Progress: needs update label Nov 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't use $rate as an object (it is an array) #30052

Don't use $rate as an object (it is an array) #30052

flancer64 commented Sep 15, 2020

m2-assistant bot commented Sep 15, 2020

Stepa4man commented Sep 15, 2020

Stepa4man left a comment

Stepa4man Sep 15, 2020

Stepa4man Sep 15, 2020

flancer64 commented Sep 16, 2020

engcom-Golf commented Sep 23, 2020

flancer64 commented Sep 23, 2020

flancer64 commented Sep 23, 2020

engcom-Golf commented Sep 28, 2020

flancer64 commented Sep 29, 2020

engcom-Golf commented Sep 29, 2020

engcom-Golf commented Sep 29, 2020

flancer64 commented Sep 29, 2020

ihor-sviziev commented Oct 28, 2020 •

edited

Loading

engcom-Delta left a comment

flancer64 commented Nov 5, 2020

ihor-sviziev commented Nov 5, 2020

m2-assistant bot commented Nov 5, 2020

Don't use $rate as an object (it is an array) #30052

Don't use $rate as an object (it is an array) #30052

Conversation

flancer64 commented Sep 15, 2020

Description (*)

Contribution checklist (*)

m2-assistant bot commented Sep 15, 2020

Stepa4man commented Sep 15, 2020

Stepa4man left a comment

Choose a reason for hiding this comment

Stepa4man Sep 15, 2020

Choose a reason for hiding this comment

Stepa4man Sep 15, 2020

Choose a reason for hiding this comment

flancer64 commented Sep 16, 2020

engcom-Golf commented Sep 23, 2020

flancer64 commented Sep 23, 2020

flancer64 commented Sep 23, 2020

engcom-Golf commented Sep 28, 2020

flancer64 commented Sep 29, 2020

engcom-Golf commented Sep 29, 2020

engcom-Golf commented Sep 29, 2020

flancer64 commented Sep 29, 2020

ihor-sviziev commented Oct 28, 2020 • edited Loading

engcom-Delta left a comment

Choose a reason for hiding this comment

flancer64 commented Nov 5, 2020

ihor-sviziev commented Nov 5, 2020

m2-assistant bot commented Nov 5, 2020

ihor-sviziev commented Oct 28, 2020 •

edited

Loading