WIP Issues/3388/basket middleware

[sasha0]

[codecov[bot]]

Codecov Report

Merging #3518 into master will increase coverage by 0.02%. The diff coverage is 89.83%.

@@            Coverage Diff             @@
##           master    #3518      +/-   ##
==========================================
+ Coverage   85.41%   85.44%   +0.02%     
==========================================
  Files         288      288              
  Lines       15596    15627      +31     
==========================================
+ Hits        13322    13352      +30     
- Misses       2274     2275       +1     

Impacted Files	Coverage Δ
src/oscar/apps/basket/middleware.py	83.01% <85.29%> (+1.01%)	:arrow_up:
src/oscar/views/decorators.py	96.22% <94.11%> (-1.00%)	:arrow_down:
src/oscar/core/application.py	91.22% <100.00%> (+1.43%)	:arrow_up:

[samitnuk]

@solarissmoke @sasha0

I have slightly updated this PR. Please review when you will have a chance. Thanks!

[samitnuk]

Also we need to think about the case when basket is disabled within the view, but called in the templates. Easiest approach I have in mind is to throw exception/message that basket is disabled. Why do you think guys @samitnuk @solarissmoke?

Yea, good idea, but I am not sure how can we check that the basket is called in a template.

From the docs:

Many templates, including some of Django’s, rely upon the silence of the template system when a nonexistent variable is encountered.

[solarissmoke]

Also we need to think about the case when basket is disabled within the view, but called in the templates. Easiest approach I have in mind is to throw exception/message that basket is disabled.

I don't think we should raise an exception for the reasons mentioned by @samitnuk - it should be up to projects to ensure they only disable the basket on views that really don't use it at all.

[specialunderwear]

I'm a little late to the party, I'm sorry, but I really don't like this approach.

The issue this PR would solve (#3388 ) show in the stacktrace that load_full_basket is being called which somehow loads custom proxy models that breaks due to a cyclic import. Since load_full_basket is wrapped in a SimpleLazyObject means that the request.basket was actually accessed by that users code even if it was an admin url! Disabling the basket would not solve the problem this user is having, because it would simpy cause an AttributeError! (request has no attribute basket).

I think that basketmiddleware is just fine, since it is lazy and doesn't really do anything util somebody actually accessed the basket. If this turns out not to be true, and the basket middleware DOES actually load the basket even if it wasn't accessed by user code, we should fix that instead, to make it truly lazy.