Basket middleware active in Django admin

[jayvdb]

When loading Django admin, the basket middleware failed (below; worthy of a separate less critical issue). It should detect Django admin and not do anything.

  File "/home/jayvdb/.cache/pypoetry/virtualenvs/django-backend-tUiU8616-py3.8/src/django-oscar/src/oscar/apps/basket/middleware.py", line 41, in load_full_basket
    self.apply_offers_to_basket(request, basket)
  File "/home/jayvdb/.cache/pypoetry/virtualenvs/django-backend-tUiU8616-py3.8/src/django-oscar/src/oscar/apps/basket/middleware.py", line 207, in apply_offers_to_basket
    Applicator().apply(basket, request.user, request)
  File "/home/jayvdb/.cache/pypoetry/virtualenvs/django-backend-tUiU8616-py3.8/src/django-oscar/src/oscar/apps/offer/applicator.py", line 24, in apply
    self.apply_offers(basket, offers)
  File "/home/jayvdb/.cache/pypoetry/virtualenvs/django-backend-tUiU8616-py3.8/src/django-oscar/src/oscar/apps/offer/applicator.py", line 34, in apply_offers
    result = offer.apply_benefit(basket)
  File "/home/jayvdb/.cache/pypoetry/virtualenvs/django-backend-tUiU8616-py3.8/src/django-oscar/src/oscar/apps/offer/abstract_models.py", line 293, in apply_benefit
    if not self.is_condition_satisfied(basket):
  File "/home/jayvdb/.cache/pypoetry/virtualenvs/django-backend-tUiU8616-py3.8/src/django-oscar/src/oscar/apps/offer/abstract_models.py", line 281, in is_condition_satisfied
    return self.condition.proxy().is_satisfied(self, basket)
  File "/home/jayvdb/.cache/pypoetry/virtualenvs/django-backend-tUiU8616-py3.8/src/django-oscar/src/oscar/apps/offer/abstract_models.py", line 48, in proxy
    klass = load_proxy(self.proxy_class)
  File "/home/jayvdb/.cache/pypoetry/virtualenvs/django-backend-tUiU8616-py3.8/src/django-oscar/src/oscar/apps/offer/utils.py", line 29, in load_proxy
    raise exceptions.ImproperlyConfigured(
django.core.exceptions.ImproperlyConfigured: Error importing module oscarbluelight.offer.conditions: No module named 'oscarbluelight'

[specialunderwear]

Why?

[jayvdb]

What reason should the Django admin ever be showing info about the current basket of the logged in user? That is not its purpose.

If you insist that basket middleware should be active under the Django admin, it should at least never ever raise an exception which breaks the Django admin. I needed to delete a record in order to fix the problem, and this middleware problem prevents me from doing that.

[solarissmoke]

@jayvdb I think there's a legitimate argument for disabling the middleware for the django admin, which we can give some thought to.

However, the exception you have posted doesn't appear to originate from Oscar core. oscarbluelight is not a core Oscar package, and there is nothing in this code base that would attempt to import it. So the exception seems to be an issue with oscarbluelight, or with your project setup, rather than with Oscar's middleware? I'm not sure that it's Oscar's job to anticipate and catch random exceptions that happen in project-level code.

[jayvdb]

As I said already, I know the issue about oscarbluelight is separate. I did not ask for help with that, and dont expect Oscar to do voodoo. I'll provide a more specific issue about that later when I've better understood it, and thought about what Oscar might do better in that situation, if anything.

This issue is about enhancing Oscar to fix a problem clearly in Oscar's domain. Middleware and context processors need to be written conservatively, not throwing exceptions on every single page, and most certainly never breaking the Django admin front page.

[solarissmoke]

Looking at this more closely it seems the underlying issue is that there is a reference to a proxy benefit/condition class that no longer exists. Oscar could probably handle that more gracefully - i.e., log the error and return no benefit/condition instead of throwing an exception. Can you confirm that that is what happened?

As regards your other comments, I would request that you try to be a little more measured in the tone of your statements. Statements like "As I said already", "if you insist that...", "don't expect Oscar to do voodoo..." and "most certainly never breaking the Django admin front page" come across as somewhat aggressive/impatient, and are not going to encourage discussion. Everyone that contributes to this project does so in their spare time, and is more likely to engage if the tone is respectable.

In this particular case I would note that Oscar explicitly declares that it does not officially support the Django admin - so it doesn't actually provide any guarantee that the admin will be functional. I am not saying that that is ideal (especially since some things cannot be done from the dashboard) - but it's where we are - and we can discuss how to improve things without making blanket statements that leave no room for debate.

[jayvdb]

Can you confirm that that is what happened?

That is about a different issue. Why can't you let me raise it separately? There is more than enough to chew on here wrt to Django admin, and the Django admin aspect is very important to me. The spark which caused this issue is very low priority, and it is only one spark -- there are many others which could have caused this problem with Django admin, so the spark is irrelevant here except as context to show the types of causes of this problem.

... come across as somewhat aggressive/impatient, and are not going to encourage discussion. Everyone that contributes to this project does so in their spare time, and is more likely to engage if the tone is respectable.

You should look at the tone of the replies I got here to a legitimate issue. And this is not the first time either, not to me, and I see it regularly in other issues by other people. From the issues I have read, it seems common for the main people responding to issues here to be antagonistic, dismissive and baiting. It may be their "spare time" - it is also typically the spare time of the people who are raising the issues, and their experience and input should be valued as much, if not more, than the maintainers, because it is users experiences that help a OSS project grow, and happy users turn into new members of the support team if cultivated properly, reducing the stress on the maintainers/core devs. That could be me, even helping with bug fixes, but so far the responses from the maintainers makes me pause about even using Oscar again for another project, which is a shame because it is fairly well designed, but it is awkward listing issues in the docs my manager sees because I am hoping he doesnt read them for fear it will alarm him about its long term support viability.

In this particular case I would note that Oscar explicitly declares that it does not officially support the Django admin - so it doesn't actually provide any guarantee that the admin will be functional.

Explicitly? Would you mind pointing that out; I havent seen that.

So what parts of Django does Oscar consider it appropriate to interoperate with Django core features?

If Oscar does not consider it appropriate to interoperate with certain Django features then it should include a disclaimer about that very prominently, and sadly I have missed it. Some apps do that - they very clearly frame Django as part of their app - a low level component, rather than their app sitting on top of Django or being designed for Django. They expect the implementer to install Django only following their process, no more, no less. That is a legitimate approach. I didnt get that feeling from the Oscar docs, excepting that it was clear that Oscar was (sanely) allowing only a limited support matrix of versions for dependencies, which is a fairly typical approach for larger apps and were the target market is a more professional segment.

Side issue: Whether 'admin' is 'core' or 'contrib' Django is debatable these days - it can be disabled, but it is heavily baked into the docs as a prominent and important component.

[solarissmoke]

Why can't you let me raise it separately?

I'm not preventing you raising it separately - please do that.

The point I was trying to make (and perhaps where we disagree) is that I am not convinced Oscar's middleware should be responsible for catching all possible exceptions just in case it results in a 500 (and therefore that the underlying issue is what needs to be addressed).

You should look at the tone of the replies I got here to a legitimate issue. And this is not the first time either, not to me, and I see it regularly in other issues by other people.

I agree that the tone here hasn't always been great - and I'm sorry you've experienced that. We're try to improve that for everyone, and call it out when it needs to be. That is not, however, reasonable justification to use the same tone yourself.

Explicitly? Would you mind pointing that out; I haven't seen that.

See here and here. This could probably be stated more visibly somewhere as you say.

So what parts of Django does Oscar consider it appropriate to interoperate with Django core features?

I don't think Oscar has a clear definition on this, which is part of the problem. The admin is specifically excluded because the dashboard is intended to be a replacement for it.

[jayvdb]

https://django-oscar.readthedocs.io/en/latest/internals/getting_started.html#urls

You can also include the Django admin for debugging purposes. But please note that Oscar makes no attempts at having that be a workable interface; admin integration exists to ease the life of developers.

https://django-oscar.readthedocs.io/en/latest/internals/contributing/bugs-and-features.html#reporting-bugs-and-requesting-features

Be aware that Windows and the Django admin interface are unsupported; any tickets regarding that will get closed.

Oscar team appreciates that developers need Django admin (and I bet they use it themselves), but refuse to accept problem reports (from 'other' people) about it not working. ergo, Oscar is unfriendly to outside developers...?

https://github.com/django-oscar/django-oscar/blob/master/.github/CONTRIBUTING.md sounds so much more welcoming.

I find the anti-developer tone alarming. Oscar needs more active developers. It has a solid core, but so many parts of the ecosystem are bitrotten (support 0.x or 1.x) or not implemented yet. And Oscar isnt an app that can be used without a developer getting their fingers dirty, unless the ecommerce site is only selling free stuff. Really dirty at times. And that is fine. But it needs developers. And developers have developer needs.

I think it would be better to frame Django admin as a valid developer and sys admin tool, like many others (debug toolbar, etc), and that issues related to interoperability with non-production tools will be labelled/treated as low priority unless it is blocking many active developers. Then developers can expect to get no assistance in fixing them (I dont), but they can raise them and they can expect their interoperability PRs will be welcomed and appreciated, but may not be reviewed & merged quickly as they are low-priority. After all, devs should be able to carry around their own patches, so they don't need quick merges. But they are not going to write an issue or a patch if it will be discarded.

Can Oscar team see that would be more welcoming?

Does it need further conditions in order to not hurt their productivity / abuse their spare time ?

[solarissmoke]

I find the anti-developer tone alarming.

Agreed. The wording here is not welcoming and needs to change. I've only been involved with Oscar for the last year or so, so can't speak to why it was written the way it was - but let's improve it now.

The decision not to support the Django admin is definitely one that needs to be reviewed, and you're right that it's not really possible to administer an Oscar site without relying on the admin for some things.

Oscar needs more active developers.

Absolutely.

Can Oscar team see that would be more welcoming?

Agreed (I speak for myself here).

I think there are three separate issues coming out of this

1. Support for django admin (maybe best discussed on a new issue).
2. Basket middleware handling of exceptions and whether it should run for admin views (this ticket).
3. Handling of missing proxy classes for offers (worth a new issue after confirming that this was the spark for this ticket).

[specialunderwear]

Dear @jayvdb thank you for reporting this issue. Let me rephrase my initial question.

Because the reported error does not immediately seem related to the proposed changes, we need some more explanation.

Why is disabling the basket middleware for the django admin a proper solution for this kind of error? This is not at all obvious to me sorry. That a middleware is totally useless in a certain context, does not immediately entail that code should written to disable it. Maybe you just discovered an interesting error that has a solution which solves an edge case.

In any case there was no explanation in your issue on why the basket middleware should be disabled for the admin, so I asked for more information. No need to get all defensive, just add the information requested so it can be discussed!

[jayvdb]

@specialunderwear , thank you for taking the time to explain why my initial issue didnt seem obvious to you. That is much easier to reply to than a mere Why?. I assumed that the target audience of my issue understood principles of Django middleware, and also understood that Oscar basket middleware breaking an unrelated part of a Django site, especially Django Admin, is highly unexpected behaviour for middleware, which should be coded defensively, and they should almost never raise exceptions unless DEBUG is enabled. Each middleware should only help were it can, and only take decisive action if it is confident that it needs to. Otherwise the middleware should defer, so that the remainder of the hooks can have their turn. These defensive programming principles are exhibited throughout the middleware in Django contrib, and usually in externally maintained installable apps.

I had assumed Oscar sees itself as a cooperating part of a broader Django ecosystem, but maybe I was wrong and it sees itself as an implementable e-commerce software that Django is merely a component of. In the former case, standard principles of Django middleware should apply, and best efforts should be taken to avoid raising exceptions. In the latter case, Oscar can do whatever it feels is best in light of the set of other middleware it supports.

My mentioning of Django Admin obviously hit a nerve or two, but the middleware can also break Oscar Dashboard. The backtrace is the same - the implications of exceptions in middleware are wide.

But anyway, the above discussion with @solarissmoke suggests that Django Admin might be reconsidered as a useful component for Oscar implementations. How far Oscar should 'support' Django-Admin is a word-smithing exercise, and but IMO it would be an important community improvement if issues mentioning Django Admin were not dismissed immediately. Do you agree @specialunderwear ?

Why is disabling the basket middleware for the django admin a proper solution for this kind of error?

Disabling the basket middleware is IMO the proper solution for every kind of error on the Django admin, and Oscar Dashboard for that matter, etc, where the user basket is not relevant. The middleware should add what it can, including adding errors to the context which are then available for the oscar apps to use appropriately. Or middleware should raise exceptions only if it knows it has understood the request and knows it needs to be short-circuited, preventing the request/response from reaching any other of the middleware that is active on the site.

You can disagree with that, and Oscar can even have an official position about that to avoid endless discussions about the pros/cons of it, which goes back to the question of how Oscar sees 'itself'.

[solarissmoke]

Having given this some thought, and my suggestion is that we allow short-circuiting the middleware for certain path prefixes (see sample implementation here).

Projects can then exclude whatever they want to - including the Django admin and possibly also the dashboard (plus anything else - it's common to have a CMS for other content etc). This would actually result in a significant performance improvement for the dashboard.

[sasha0]

Yeah, or similar how csrf_exempt implemented in Django - https://github.com/django/django/blob/f283ffaa84ef0a558eb466b8fc3fae7e6fbb547c/django/middleware/csrf.py#L211-L212.

The implementation would include:

• Decorator which would add property to the view class/function - is_basket_disabled = True.
• Allow to manually set the above property for the view class.
• Skip further basket middleware execution if callback.is_basket_disabled is True.

WTYD @solarissmoke ?

[solarissmoke]

That's an interesting approach, and works better than path-based filtering I think! Would need to find a way to cleanly apply it to all dashboard views.

[samitnuk]

Probably it is better to use is_basket_enabled (instead of is_basket_disabled) and "add basket" only where needed (instead of "remove basket" from where we do not need it - e.g. in Dashboard and Django Admin).