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. resolves [Issue] Quote is loaded without totals #39205: Quote is loaded without totals

[m2-assistant]

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

Hello @thomas-kl1,

Thanks for the contribution!

But it seems the automated tests are failing due to this change. Hence please look into it.

Thanks

[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,