Quote is loaded without totals

[thomas-kl1]

Description (*)

For exemple, when we load the quote on the checkout cart page, some fields are missing on the items because the totals have not been collected. For exemple the original price is missing. It results in weird scenarios (for exempleif you want to print the diff between the final price and the original one on a quote item, an original price is 0).

Manual testing scenarios (*)

1. Load quote from session
2. use getOriginalPrice method on quote items

Questions or comments

Not sure why originally the check was forced to checking "false" when in almost every case the Magento core checkout for true (which is set when the totals are collected). Also the collectTotals method is already proof as it already checks for the flag.

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)
• [ ] README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• [ ] All automated tests passed successfully (all builds are green)

Resolved issues:

1. [x] resolves magento/magento2#39205: Quote is loaded without totals

[m2-assistant[bot]]

Hi @thomas-kl1. Thank you for your contribution! Here are some useful tips on how you can test your changes using Magento test environment. :exclamation: 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:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here :information_source: Run only required test builds during development. Run all test builds before sending your pull request for review.

---

For more details, review the Code Contributions documentation. Join Magento Community Engineering Slack and ask your questions in #github channel.

[thomas-kl1]

@magento run all tests

[engcom-Hotel]

@magento create issue

[engcom-Hotel]

@magento run all tests

[engcom-Dash]

@magento run all tests

[thomas-kl1]

@magento run all tests

[thomas-kl1]

Functional test failures doesn't seems to be related to this changes. However there's still some integration tests failures. I'll take a look on it.

[thomas-kl1]

Integration test is weird because it tests for getTriggerRecollect instead of getTotalsCollectedFlag. As I understand it, getTotalsCollectedFlag is an internal flag of the quote object that store into the object state if the totals have been collected or not (or should be). However, because the method is public anyone can play with it. When looking the Magento codebase, we can see many usage of this flag in order to force the collect of totals (a lot actually).

However, we only see a single usage of setTriggerRecollect in Magento codebase (if we exclude tests) and it is executed in the following method:

\Magento\Quote\Model\Quote::_afterLoad

    protected function _afterLoad()
    {
        // collect totals and save me, if required
        if (1 == $this->getTriggerRecollect()) {
            $this->collectTotals()
                ->setTriggerRecollect(0)
                ->save();
        }
        return parent::_afterLoad();
    }

We found more occurrence if we look for update on the field trigger_recollect. What I understand it is mainly used to trigger a recollect externally (for exemple cart rule update). Also this is a persisted field on the quote entity, while 'totals_collected_flag' is just an un persisted flag as its names intend to.

Basically it seems that in some scenario in the codebase (with B2B and EE packages too) the flag or trigger_recollect are used together in order to keep consistency. However the flag does not have any effect if the collectTotals method is not called behind, while trigger_recollect if persisted, will trigger a collectTotals and save after the quote entity is loaded.

What does it mean?

It means that trigger_recollect is used to tell the quote entity if its totals should be collected. Ex: a shipping method has been updated or a carte rule. The flag is only intended to be used at runtime a tell the quote object if the totals have been collected or not, regarless its trigger_recollect field. It means that somewhere in Magento, the quote should have been updated to trigger the recollect but hasn't been so we need as a workaround to collect totals when pulling the quote from the session. As we have seen earlier, the trigger_recollect trigger the collectTotals method on the quote entity load. So if the trigger_recollect is set to 1, when we fetch the quote from the sessions, its totals is collected before our code change, so the flag is equals to true and won't trigger a new recollect. If trigger_recollect is set to 0 and we fetch the quote from the session, then our fallback mechanism will recollect the totals anyway (best until better solution).

Now if we look at the following integration test: \Magento\Quote\Model\QuoteInfiniteLoopTest::testLoadQuoteSuccessfully It should be updated to only be tested with disabled observer, because having an observer that loads the quote from the session after the collect totals call must throw an infinite loop.

[thomas-kl1]

@magento run all tests

[engcom-Hotel]

Hi @thomas-kl1,

Thank you for the detailed explanation of the difference between getTotalsCollectedFlag and getTriggerRecollect mechanisms. Your analysis of their distinct purposes and interactions in the codebase is spot on.

Regarding your suggestion about the \Magento\Quote\Model\QuoteInfiniteLoopTest::testLoadQuoteSuccessfully test - I agree that testing this with enabled observers could indeed create an infinite loop scenario when an observer loads the quote from session after the collect totals call.

It seems you already started updating the PR to modify this test case as you suggested, but still we can see failures in the integration tests.

Specifically, ensuring it's only tested with disabled observers would prevent the infinite loop issue while still validating the core functionality.

Once this change is implemented, we'll be able to move forward with the final review of this PR. Thank you for your thorough contribution to improving Magento's quote handling logic.

Thanks

[thomas-kl1]

Hello @engcom-Hotel

I'm dedicated to work on this, it may take some time to sort things out and check all the tests. I'll notify you as soon as the PR is ready for review.

Regards,

[engcom-Hotel]

Hello @thomas-kl1,

Gentle reminder to look into this PR.

Thanks

[thomas-kl1]

Hello @engcom-Hotel

I didn't forget! It's a rush period, I'll work on this PR, promise 😊

[engcom-Hotel]

Hello @thomas-kl1,

We can understand that you are occupied in some other tasks, hence we are moving this PR On hold and mark it as draft.

Thanks

[thomas-kl1]

@magento run all tests

[engcom-Hotel]

Since some tests are still failing, hence moving this PR On Hold again.

Thanks

[thomas-kl1]

I'm working on it today

[thomas-kl1]

@magento run all tests

[thomas-kl1]

@engcom-Hotel Any idea on this static test failure? https://public-results-storage-prod.magento-testing-service.engineering/reports/magento/magento2/pull/39201/35d6fbb49cff0f164bb4675c82002ce4/Statics/console-error-logs.html

I don't see any weird symbol, and the class is correctly resolved at runtime. Should we exclude it from the analysis?

[engcom-Hotel]

Hello @thomas-kl1,

I tried to run this integration test on my local, it is running successfully by resolving the close runtime. I think we can suppress this error to resolve it.

Thanks

[thomas-kl1]

@magento run all tests

[thomas-kl1]

Hello @thomas-kl1,

I tried to run this integration test on my local, it is running successfully by resolving the close runtime. I think we can suppress this error to resolve it.

Thanks

Actually the directory was missing in the scan directory of the phpstan neon config, it's fixed now

[thomas-kl1]

@engcom-Hotel I'm having a last issue to resolve with the integration test and it should be good.

[engcom-Hotel]

@magento run all tests

[engcom-Hotel]

As @thomas-kl1 is still actively working on this pull request, I am moving it back to ‘On hold’ status for the time being.

[thomas-kl1]

@magento run all tests

[engcom-Hotel]

Hello @thomas-kl1,

Gentle reminder for this PR.

Thanks

[engcom-Hotel]

Hello @thomas-kl1,

Please pick this PR to fix the failed tests.

Thanks

[engcom-Hotel]

Hello @thomas-kl1,

Have you got a chance to look into this PR?

Thanks

[engcom-Hotel]

Hello @thomas-kl1,

We have noticed that this PR has not been updated for a while. Therefore, we are closing it for now.

Please let us know when you are ready to work on it again, and we will be happy to reopen the PR.

Thank you for your valuable contribution!