Load previously lost decorator code and don't delete orders #5905
Conversation
When we imported and merged Spree's controller modules with our decorators, Rails started using Spree's original code again. This was first included in v3.2.0 and deployed on 28 July 2020.
We used to delete old cart orders so that they wouldn't re-appear after a successful checkout of another order. Keeping them ensures that we don't remove an order that is still used by another device. It also makes sure that we keep references of failed payments.
We changed some of Spree's logic and want to use that. And once we remove the spree_core gem, we need to load those files before using them.
I was looking for library files that may be used but are not loaded. I would then add the missing `require` statements. But I found that this module isn't used any more. Usage removed in: 310d1b3
Also removing related unused classes and their specs.
We didn't actually change any logic in our version of the Spree environment file but if we do that in the future, we want to be sure that it takes effect. Our file was ignored and not loaded before.
There was a problem hiding this comment.
Brilliant detective work @mkllnk thanks a lot!
Why draft? Looks good for testing.
|
Note: on all investigations I have done related to failed payments I only found missing entries on spree_payments. spree_orders were always there. So:
I'd say option 1 is more likely and this is a different problem from the problem we have tried to fix in #5806 for example. |
|
I think this could also have an impact on #5862 |
|
Thanks for your early review. I'll continue my work today. I should have mentioned on Friday that I left it as draft because I didn't verify all my claims. I didn't go through the testing steps myself, they are just guesses from what I saw in the code. At the weekend, I also remembered that some controllers did load the helper file (CartController and Spree::OrdersController). So the deletion of old carts could have happened when logging in on the cart page, maybe? I'll try to replicate. |
|
ok 👍 |
|
Hey @mkllnk I think now I remember in delivery train meeting you suggesting I pick this one up so you can work on mobile? |
Every page load creates a cart order if none is present. So when a user logs in, they always have an order stored in their session. And therefore, we never got to recover an old order. We could have fixed the code to restore old orders. But as far as I can tell, order recovery hasn't been working for years and I couldn't find any issue requesting this feature. If we wanted to implement order recovery, it should probably be designed more carefully and included in the `current_order` method.
|
I needed to verify the test instructions and found that one part of it was invalid. I added another two commits, addressing your feedback, @luisramos0, and removing the dead code. There is more I can clean up but I will do that in a separate PR. |
|
@luisramos0 I added two more commits in a separate PR: #5927 If you think it's better to include them here and we can still make it for the release, that would be awesome but I didn't want to put anything in the way of this PR getting into the release. |
This reverts commit 355c5f5. This code is necessary to preserver cart contents across logins on different browser sessions.
filipefurtad0
added
pr-staged-es
pr-staged-uk
|
Hi @mkllnk , Thanks for this investigation and the detailed testing-steps! Since master branch appears to be broken (as discussed here), I have staged the build used for release testing (v3.2.3), before testing this PR. The images below show session 1 on (the left) and session 2 (on the right side). I followed your procedure: i) As guest user, went to a shop and added 5 items to the cart. At this point, both guest sessions have 5 items each. Proceeded: iii) Logged in with the same user in both sessions Here it became clear, that the condition The second session, now contains 10 items, the items from session 1 plus its own:
iv) At this point, attempting to checkout on session 1 generates the error:
So,
v) checking out with session 2 (which contains 10 items) is possible. After staging the PR, following steps i) to iii), leads to the same result as before:
It's possible to check what's in the cart for both sessions - session 2 (right) contains the items from session 1 as well. Also, and as before, it's not possible to check out from session 1; This is only possible for session 2. So, I could not verify the test cases and I am not sure the PR introduces a benefit. Thoughts? (adding feedback label) Another issue found, before and after the PR, and therefore unrelated: After step iii), if instead of attempting to checkout one attempts to:
At this point, the user has items in the cart, but cannot checkout. He cannot visit /cart page - gets redirected to the homepage
Logging out and back in does not solve the issue, so basically, this account becomes blocked, in terms of customer function. This will need an issue, as it seems like a point of no return, for the user. Respective bugsnag error: |
filipefurtad0
added
feedback-needed
and removed
pr-staged-fr
|
Hmmm, the interesting thing is that all this is before you stage this PR! This PR does remove the logic of merging orders so my question is: what happens to this test case after the PR. I think it will be better! Can you please try? |
|
"The two carts should be persisted and independent." this is not correct without the last commit I reverted. |
|
Hey @luisramos0, I ran the tests before and after the PR, and got the same results... Also the issue found applies to before/after (I double checked this). It's quite a lot of text, sorry - it's easy to miss... I've edited it for readability. I also tried the test case you proposed:
So basically, I could not reproduce the scenario "lose carts", when shopping in different sessions. |
|
ok, we need to move this forward today to get into the release. What do you think Filipe? |
|
ok, I talked with Filipe about this on slack. This is my presentation of the situation:
Talking with Filipe we concluded the best approach here is to merge this PR and include an extra round of verifications related to this in the upcoming release testing process. |
|
Fully agree @luisramos0 , |
What? Why?
While investigating #5897 I found some code that could accidentally delete an order. But then I found that the code wasn't loaded and could not have caused a disappearing order.
I continued to investigate the issue of unloaded code because it could be responsible of many random issues. The deeper I dug, the clearer I could see that the unloaded code was mostly insignificant. Nonetheless, I addressed the loading issue and removed the code that could delete orders.
The non-loading was mostly no issue because our code didn't change Spree's logic. The only exception are the changes from 20f610f which were lost since v3 and now brought back.
What should we test?
Parallel browsing
Release notes
When shopping with multiple devices, no carts are lost.
Changelog Category: Changed
How is this related to the Spree upgrade?
During the last Spree update we lost some of our customisations. They were simply not loaded. Now we are loading those customisations again, enabling us to customise more and getting rid of Spree.