Skip to content

Commit

Permalink
Optimize handling of attributes (#334)
Browse files Browse the repository at this point in the history
* Optimize attribute values (#321)

* Implemented faster attributes save

* renamed for clarity

* Renamed warning

* Enable shortcut when saving child products. (#322)

* Make sure to only return attributevalues that are on the actual product to avoid transferring (#331)

attributesvalues from parent to child.

* Improve multi db support (#335)

Use the same database as the passed-in manager

* The image and file fields where nolonger working. (#337)

* The image and file fields where nolonger working.

This PR restores that functionality.

* nergens voor

* fixes tests

* black

* Fixes cyclic import error

* Fixes handling of null product image attributes

When an image product attribute is `null`, the product API would throw the
following exception:

```
ValueError: The 'value_image' attribute has no file associated with it.

  […]

  File "rest_framework/serializers.py", line 522, in to_representation
    ret[field.field_name] = field.to_representation(attribute)
  File "rest_framework/serializers.py", line 686, in to_representation
    return [
  File "rest_framework/serializers.py", line 687, in <listcomp>
    self.child.to_representation(item) for item in iterable
  File "rest_framework/serializers.py", line 522, in to_representation
    ret[field.field_name] = field.to_representation(attribute)
  File "oscarapi/serializers/fields.py", line 227, in to_representation
    return value.value.url
  File "django/db/models/fields/files.py", line 66, in url
    self._require_file()
  File "django/db/models/fields/files.py", line 41, in _require_file
    raise ValueError(
```

This patch fixes this by checking if the image field has a value before trying
to generate a URL.

* Add category bulk admin api

* Add tests to prove how it works

* wops

* Added Jenkinsfile

* hehe

* set to next release version

* Setup local CI (#348)

* Improve category upsert and add test (#352)

* Doe some updating and add test

* refrhes_from_db only path

* Add transaction that rollsback on error

* update versions

---------

Co-authored-by: Craig Weber <[email protected]>
Co-authored-by: Viggo de Vries <[email protected]>
  • Loading branch information
3 people authored Sep 19, 2024
1 parent f24ef46 commit 965c782
Show file tree
Hide file tree
Showing 16 changed files with 1,323 additions and 90 deletions.
10 changes: 5 additions & 5 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,19 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Checkout the repository
uses: actions/checkout@v2
uses: actions/checkout@v4
with:
path: ./src
- name: Setup Python 3.11
uses: actions/setup-python@v2
uses: actions/setup-python@v5
with:
python-version: '3.11'
- name: Install all dependencies
run: make install
- name: Run all linting
run: make lint
- name: Upload src dir as artefact
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v4
with:
name: src
path: ./src
Expand All @@ -38,12 +38,12 @@ jobs:
oscar-version: ["3.2"]
steps:
- name: Download src dir
uses: actions/download-artifact@v2
uses: actions/download-artifact@v4
with:
name: src
path: ./src
- name: Setup Python 3.x
uses: actions/setup-python@v2
uses: actions/setup-python@v5
with:
python-version: ${{ matrix.python-version }}
- name: Install all dependencies
Expand Down
70 changes: 70 additions & 0 deletions Jenkinsfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
#!/usr/bin/env groovy

pipeline {
agent { label 'GEITENPETRA' }
options { disableConcurrentBuilds() }

stages {
stage('Build') {
steps {
withPythonEnv('System-CPython-3.10') {
withEnv(['PIP_INDEX_URL=https://pypi.uwkm.nl/voxyan/oscar/+simple/']) {
pysh "make install"
}
}
}
}
stage('Lint') {
steps {
withPythonEnv('System-CPython-3.10') {
pysh "make lint"
}
}
}
stage('Test') {
steps {
withPythonEnv('System-CPython-3.10') {
pysh "make coverage"
}
}
post {
always {
junit allowEmptyResults: true, testResults: '**/nosetests.xml'
}
success {
step([
$class: 'CoberturaPublisher',
coberturaReportFile: '**/coverage.xml',
])
}
}
}
}
post {
always {
echo 'This will always run'
}
success {
echo 'This will run only if successful'
withPythonEnv('System-CPython-3.10') {
echo 'This will run only if successful'
pysh "version --plugin=wheel -B${env.BUILD_NUMBER} --skip-build"
sh "which git"
sh "git push --tags"
}
}
failure {
emailext subject: "JENKINS-NOTIFICATION: ${currentBuild.currentResult}: Job '${env.JOB_NAME} #${env.BUILD_NUMBER}'",
body: '${SCRIPT, template="groovy-text.template"}',
recipientProviders: [culprits(), brokenBuildSuspects(), brokenTestsSuspects()]

}
unstable {
echo 'This will run only if the run was marked as unstable'
}
changed {
echo 'This will run only if the state of the Pipeline has changed'
echo 'For example, if the Pipeline was previously failing but is now successful'
}
}
}
10 changes: 3 additions & 7 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ clean:
rm -Rf build/

install:
pip install -e .[dev]
pip install -e .[dev] --upgrade --upgrade-strategy=eager --pre

sandbox: install
python sandbox/manage.py migrate
Expand Down Expand Up @@ -43,15 +43,11 @@ publish_release_testpypi: build_release
publish_release: build_release
twine upload dist/*

lint.installed:
pip install -e .[lint]
touch $@

lint: lint.installed
lint:
black --check --exclude "migrations/*" oscarapi/
pylint setup.py oscarapi/

black: lint.installed
black:
black --exclude "/migrations/" oscarapi/

uwsgi:
Expand Down
5 changes: 4 additions & 1 deletion oscarapi/serializers/admin/product.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,15 @@ def update(self, instance, validated_data):
if (
self.partial
): # we need to clean up all the attributes with wrong product class
attribute_codes = product_class.attributes.values_list(
"code", flat=True
)
for attribute_value in instance.attribute_values.exclude(
attribute__product_class=product_class
):
code = attribute_value.attribute.code
if (
code in pclass_option_codes
code in attribute_codes
): # if the attribute exist also on the new product class, update the attribute
attribute_value.attribute = product_class.attributes.get(
code=code
Expand Down
110 changes: 46 additions & 64 deletions oscarapi/serializers/fields.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# pylint: disable=W0212, W0201, W0632
import logging
import operator
import warnings

from os.path import basename, join
from urllib.parse import urlsplit, parse_qs
Expand All @@ -15,8 +16,10 @@
from rest_framework.fields import get_attribute

from oscar.core.loading import get_model, get_class
from oscarapi.utils.deprecations import RemovedInFutureRelease

from oscarapi import settings
from oscarapi.utils.attributes import AttributeFieldBase, attribute_details
from oscarapi.utils.loading import get_api_class
from oscarapi.utils.exists import bound_unique_together_get_or_create
from .exceptions import FieldError
Expand All @@ -27,7 +30,6 @@
create_from_breadcrumbs = get_class("catalogue.categories", "create_from_breadcrumbs")
entity_internal_value = get_api_class("serializers.hooks", "entity_internal_value")
RetrieveFileMixin = get_api_class(settings.FILE_DOWNLOADER_MODULE, "RetrieveFileMixin")
attribute_details = operator.itemgetter("code", "value")


class TaxIncludedDecimalField(serializers.DecimalField):
Expand Down Expand Up @@ -93,7 +95,7 @@ def use_pk_only_optimization(self):
return False


class AttributeValueField(serializers.Field):
class AttributeValueField(AttributeFieldBase, serializers.Field):
"""
This field is used to handle the value of the ProductAttributeValue model
Expand All @@ -103,80 +105,56 @@ class AttributeValueField(serializers.Field):
"""

def __init__(self, **kwargs):
warnings.warn(
"AttributeValueField is deprecated and will be removed in a future version of oscarapi",
RemovedInFutureRelease,
stacklevel=2,
)
# this field always needs the full object
kwargs["source"] = "*"
kwargs["error_messages"] = {
"no_such_option": _("{code}: Option {value} does not exist."),
"invalid": _("Wrong type, {error}."),
"attribute_validation_error": _(
"Error assigning `{value}` to {code}, {error}."
),
"attribute_required": _("Attribute {code} is required."),
"attribute_missing": _(
"No attribute exist with code={code}, "
"please define it in the product_class first."
),
"child_without_parent": _(
"Can not find attribute if product_class is empty and "
"parent is empty as well, child without parent?"
),
}
super(AttributeValueField, self).__init__(**kwargs)

def get_value(self, dictionary):
# return all the data because this field uses everything
return dictionary

def to_product_attribute(self, data):
if "product" in data:
# we need the attribute to determine the type of the value
return ProductAttribute.objects.get(
code=data["code"], product_class__products__id=data["product"]
)
elif "product_class" in data and data["product_class"] is not None:
return ProductAttribute.objects.get(
code=data["code"], product_class__slug=data.get("product_class")
)
elif "parent" in data:
return ProductAttribute.objects.get(
code=data["code"], product_class__products__id=data["parent"]
)

def to_attribute_type_value(self, attribute, code, value):
internal_value = super().to_attribute_type_value(attribute, code, value)
if attribute.type in [
attribute.IMAGE,
attribute.FILE,
]:
image_field = ImageUrlField()
image_field._context = self.context
internal_value = image_field.to_internal_value(value)

return internal_value

def to_internal_value(self, data): # noqa
assert "product" in data or "product_class" in data or "parent" in data

try:
code, value = attribute_details(data)
internal_value = value

if "product" in data:
# we need the attribute to determine the type of the value
attribute = ProductAttribute.objects.get(
code=code, product_class__products__id=data["product"]
)
elif "product_class" in data and data["product_class"] is not None:
attribute = ProductAttribute.objects.get(
code=code, product_class__slug=data.get("product_class")
)
elif "parent" in data:
attribute = ProductAttribute.objects.get(
code=code, product_class__products__id=data["parent"]
)
attribute = self.to_product_attribute(data)

if attribute.required and value is None:
self.fail("attribute_required", code=code)

# some of these attribute types need special processing, or their
# validation will fail
if attribute.type == attribute.OPTION:
internal_value = attribute.option_group.options.get(option=value)
elif attribute.type == attribute.MULTI_OPTION:
if attribute.required and not value:
self.fail("attribute_required", code=code)
internal_value = attribute.option_group.options.filter(option__in=value)
if len(value) != internal_value.count():
non_existing = set(value) - set(
internal_value.values_list("option", flat=True)
)
non_existing_as_error = ",".join(sorted(non_existing))
self.fail("no_such_option", value=non_existing_as_error, code=code)
elif attribute.type == attribute.DATE:
date_field = serializers.DateField()
internal_value = date_field.to_internal_value(value)
elif attribute.type == attribute.DATETIME:
date_field = serializers.DateTimeField()
internal_value = date_field.to_internal_value(value)
elif attribute.type == attribute.ENTITY:
internal_value = entity_internal_value(attribute, value)
elif attribute.type in [attribute.IMAGE, attribute.FILE]:
image_field = ImageUrlField()
image_field._context = self.context
internal_value = image_field.to_internal_value(value)
internal_value = self.to_attribute_type_value(attribute, code, value)

# the rest of the attribute types don't need special processing
try:
Expand Down Expand Up @@ -221,10 +199,14 @@ def to_representation(self, value):
return value.value.option
elif obj_type == value.attribute.MULTI_OPTION:
return value.value.values_list("option", flat=True)
elif obj_type == value.attribute.FILE:
return value.value.url
elif obj_type == value.attribute.IMAGE:
return value.value.url
elif obj_type in [value.attribute.FILE, value.attribute.IMAGE]:
if not value.value:
return None
url = value.value.url
request = self.context.get("request", None)
if request is not None:
url = request.build_absolute_uri(url)
return url
elif obj_type == value.attribute.ENTITY:
if hasattr(value.value, "json"):
return value.value.json()
Expand Down
Loading

0 comments on commit 965c782

Please sign in to comment.