Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug fix: AbstractProduct.primary_images() should check against... #4419

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Naggafin
Copy link

@Naggafin Naggafin commented Jan 6, 2025

In oscar.apps.catalogue.abstract_models.AbstractProduct, we have a method as follows:

def primary_image(self):
        """
        Returns the primary image for a product. Usually used when one can
        only display one product image, e.g. in a list of products.
        """
        images = self.get_all_images()
        ordering = self.images.model.Meta.ordering
        if not ordering or ordering[0] != "display_order":
            # Only apply order_by() if a custom model doesn't use default
            # ordering. Applying order_by() busts the prefetch cache of
            # the ProductManager
            images = images.order_by("display_order")
        try:
            return images[0]
        except IndexError:
            # We return a dict with fields that mirror the key properties of
            # the ProductImage class so this missing image can be used
            # interchangeably in templates.  Strategy pattern ftw!
            missing_image = self.get_missing_image()
            return {"original": missing_image.name, "caption": "", "is_missing": True}

This looks benign and without issue, however, we do have a problem. It is on this line:

ordering = self.images.model.Meta.ordering

The problem here is if one overrides the Meta class on their own ProductImage model class, without specifying an ordering attribute, this results in an AttributeError. I personally encountered this myself. Code snippet below:

class ProductImage(auto_prefetch.Model, AbstractProductImage):
	product = auto_prefetch.ForeignKey(
		"catalogue.Product",
		on_delete=models.CASCADE,
		related_name="images",
		verbose_name=_("Product"),
	)

	class Meta(auto_prefetch.Model.Meta, AbstractProductImage.Meta):
		pass

Due to the way that Django handles manifesting the Meta implementation, the correct way to refer to the real Meta class (which is actually an Options class after being processed) is by referencing _meta, not Meta directly. This small changes fixes this issue, and code like mine now works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant