[Up port] Fixed the issue #19347. #19393

tiagosampaio · 2018-11-24T14:13:03Z

After removing the Estimate Shipping Costs and Tax from cart using layout the totals were not displayed.

Description (*)

Please check all the description in issue #19347.

Fixed Issues (if relevant)

Removing "Estimate shipping costs and tax" from cart removes cart totals #19347: Removing "Estimate shipping costs and tax" from cart removes cart totals

Manual testing scenarios (*)

The testing scenarios are described in the issue page.

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 on Travis CI are green)

magento-engcom-team · 2018-11-24T14:13:09Z

Hi @tiagosampaio. 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-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

sivaschenko · 2018-11-26T09:09:40Z

Original pull request #19351

app/code/Magento/Checkout/Model/Cart/ConfigProvider.php

app/code/Magento/Checkout/view/frontend/templates/cart/totals.phtml

tiagosampaio · 2018-12-09T15:46:05Z

I'm still working on this.

tiagosampaio · 2018-12-28T20:54:05Z

Hey guys, please I need you to review this PR.

sivaschenko

Hi @tiagosampaio thanks for your updates! There is one question I have in the code review. Also, can you please fix the failing static tests on travis.

app/code/Magento/Checkout/Block/Cart/Totals.php

magento-engcom-team · 2019-01-17T08:05:05Z

Hi @sivaschenko, thank you for the review.
ENGCOM-3890 has been created to process this Pull Request

orlangur

@tiagosampaio please squash changes into a single commit so that we have perfectly clean history 😉

m2-assistant · 2019-04-14T16:45:20Z

Hi @tiagosampaio, 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.

magento-engcom-team · 2019-04-15T09:18:40Z

Hi @orlangur, thank you for the review.
ENGCOM-4760 has been created to process this Pull Request

sumeetmobiwebtech · 2019-04-17T09:44:52Z

is anyone there how to solve this in step by step ?

VasylShvorak · 2019-04-17T13:13:15Z

✔️ QA passed

sivaschenko

Hi @tiagosampaio can you please adjust the pull request according to my review comment.

sivaschenko · 2019-04-29T09:51:58Z

app/code/Magento/Checkout/Block/Cart/Totals.php

Unfortunately, we couldn't approve adding a new public function to this API class.

Can you please inject cart config as a layout argument to this block and retrieve it in the template using magic getter (see https://github.com/magento/magento2/pull/18443/files for the example of implementation)

Hi @sivaschenko! Thank you for your feedback. I've done the modifications to accomplish tha same result by using the layout.

Fixing test for ShippingTest class. Made some modifications according to requests and implemented the unit tests. Removed duplicated semicolon. Changes: - Removed the @api annotation - Removed the @codeCoverageIgnore annotation - Fixed the PHPDoc annotation for the serializer attribute Removing the javascript from html and separated it to a javascript file. Added the use strict to totals.js file. Fixing the code style for totals.js file. Adding a simple semicolon. #19393: Static test fix. Removing the public method \Magento\Checkout\Block\Cart\Totals::getSeializedCheckoutConfig and implementing it as a magic argument. Removing unnecessary dependencies. Removing the unnecessary dependency annotation.

sivaschenko

Hi @tiagosampaio

Thanks for your updates, please see my review comments

sivaschenko · 2019-05-23T15:49:48Z

app/code/Magento/Checkout/Model/Cart/ConfigProvider.php

+     */
+    public function getCheckoutConfig()
+    {
+        return $this->configProvider->getConfig();


Hi @tiagosampaio can you please add caching for the config data retrieved from the config provider (as getCheckoutConfig can be called several times on the same page and there is no caching implemented in the configProvider)

Hi @sivaschenko.

The change was applied to this PR.

…odel\CompositeConfigProvider.

sivaschenko · 2019-05-29T21:20:31Z

@tiagosampaio thanks! looks good

sivaschenko · 2019-05-29T21:21:09Z

@paliarush can you please approve the SVC failure (added constructor optional parameters)

sivaschenko

Hi @tiagosampaio can you please resolve introduced dependencies following the review comments

sivaschenko · 2019-08-29T16:18:33Z

app/code/Magento/Checkout/Block/Cart/Shipping.php

        array $data = [],
-        \Magento\Framework\Serialize\Serializer\Json $serializer = null
+        \Magento\Framework\Serialize\Serializer\Json $serializer = null,
+        \Magento\Checkout\Model\Cart\ConfigProvider $cartConfig = null


Please inject this class via block argument in layout

Hi @sivaschenko

Based on the discussion in #23733 I think it's not backwards compatible to add block arguments via layout xml?
If custom modules try to instantiate the same block class in some custom xml file, then after upgrading Magento the module might crash as it's missing some arguments which are then undefined but still being used by the phtml file.

Can you double check this?

sivaschenko · 2019-08-29T16:19:26Z

app/code/Magento/Checkout/Block/Cart/Totals.php

        array $layoutProcessors = [],
-        array $data = []
+        array $data = [],
+        \Magento\Framework\Serialize\SerializerInterface $serializer = null


Please inject this dependency via block argument in layout

sivaschenko · 2019-08-29T16:20:15Z

app/code/Magento/Checkout/view/frontend/layout/checkout_cart_index.xml

                            <container name="checkout.cart.totals.container" as="totals" label="Shopping Cart Totals">
                                <block class="Magento\Checkout\Block\Cart\Totals" name="checkout.cart.totals" template="Magento_Checkout::cart/totals.phtml">
                                    <arguments>
+                                        <argument name="cart_config" xsi:type="object">Magento\Checkout\Model\Cart\ConfigProvider</argument>


Please apply the same approach for other blocks' dependencies as well

sidolov · 2019-09-26T15:18:46Z

@tiagosampaio , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

m2-assistant · 2019-09-26T15:18:48Z

Hi @tiagosampaio, 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.

tiagosampaio requested a review from rodrigowebjump November 24, 2018 14:13

magento-engcom-team added Area: Frontend Component: Checkout Release Line: 2.3 labels Nov 24, 2018

rodrigowebjump removed their request for review November 25, 2018 15:49

sivaschenko self-assigned this Nov 26, 2018

sivaschenko requested changes Nov 26, 2018

View reviewed changes

app/code/Magento/Checkout/Model/Cart/ConfigProvider.php Outdated Show resolved Hide resolved

app/code/Magento/Checkout/view/frontend/templates/cart/totals.phtml Outdated Show resolved Hide resolved

sivaschenko requested a review from VladimirZaets November 26, 2018 09:19

sivaschenko added the Progress: needs update label Nov 26, 2018

tiagosampaio removed the Progress: needs update label Dec 28, 2018

sivaschenko requested changes Jan 2, 2019

View reviewed changes

app/code/Magento/Checkout/Block/Cart/Totals.php Outdated Show resolved Hide resolved

sivaschenko approved these changes Jan 17, 2019

View reviewed changes

magento-engcom-team added this to the Release: 2.3.1 milestone Jan 17, 2019

magento-engcom-team added the Progress: accept label Jan 27, 2019

okorshenko removed this from the Release: 2.3.1 milestone Jan 28, 2019

sidolov removed the Progress: accept label Feb 28, 2019

ghost assigned orlangur Apr 4, 2019

orlangur requested changes Apr 4, 2019

View reviewed changes

ghost added the Progress: needs update label Apr 4, 2019

tiagosampaio closed this Apr 14, 2019

tiagosampaio reopened this Apr 14, 2019

ghost unassigned orlangur and sivaschenko Apr 14, 2019

tiagosampaio removed the Progress: needs update label Apr 14, 2019

ghost added Progress: ready for testing and removed Progress: needs update labels Apr 15, 2019

tiagosampaio mentioned this pull request Apr 16, 2019

Removing "Estimate shipping costs and tax" from cart removes cart totals #19347

Open

VasylShvorak self-assigned this Apr 17, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Apr 17, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Apr 17, 2019

tiagosampaio mentioned this pull request Apr 17, 2019

Fixed the issue #19347. #19351

Closed

4 tasks

p-bystritsky self-assigned this Apr 18, 2019

ghost added Progress: accept and removed Progress: extended testing labels Apr 19, 2019

sivaschenko requested changes Apr 29, 2019

View reviewed changes

ghost assigned sivaschenko Apr 29, 2019

ghost added Progress: needs update and removed Progress: accept labels Apr 29, 2019

sivaschenko requested changes May 23, 2019

View reviewed changes

Creating local cache for config provided by class \Magento\Checkout\M…

475b464

…odel\CompositeConfigProvider.

engcom-Foxtrot assigned engcom-Foxtrot and unassigned p-bystritsky Jun 18, 2019

sivaschenko requested changes Aug 29, 2019

View reviewed changes

sidolov closed this Sep 26, 2019

[Up port] Fixed the issue #19347. #19393

[Up port] Fixed the issue #19347. #19393

Uh oh!

Conversation

tiagosampaio commented Nov 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

Uh oh!

magento-engcom-team commented Nov 24, 2018

Uh oh!

sivaschenko commented Nov 26, 2018

Uh oh!

Uh oh!

Uh oh!

tiagosampaio commented Dec 9, 2018

Uh oh!

tiagosampaio commented Dec 28, 2018

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

magento-engcom-team commented Jan 17, 2019

Uh oh!

orlangur left a comment

Choose a reason for hiding this comment

Uh oh!

m2-assistant bot commented Apr 14, 2019

Uh oh!

magento-engcom-team commented Apr 15, 2019

Uh oh!

sumeetmobiwebtech commented Apr 17, 2019

Uh oh!

VasylShvorak commented Apr 17, 2019

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sivaschenko commented May 29, 2019

Uh oh!

sivaschenko commented May 29, 2019

Uh oh!

sivaschenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sidolov commented Sep 26, 2019

Uh oh!

m2-assistant bot commented Sep 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

tiagosampaio commented Nov 24, 2018 •

edited

Loading