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

Allow to create attribute values from product form #1973

Merged

Conversation

maarcingebala
Copy link
Member

@maarcingebala maarcingebala commented Mar 23, 2018

This PR is follow-up to #1964. Just as with variant attributes, it introduces the ability to create new attribute values from the product form.

Pull Request Checklist

  1. Privileged views and APIs are guarded by proper permission checks.
  2. All visible strings are translated with proper context.
  3. All data-formatting is locale-aware (dates, numbers, and so on).
  4. Database queries are optimized and the number of queries is constant.
  5. The changes are tested.
  6. The code is documented (docstrings, project documentation).
  7. Python code quality checks pass: pycodestyle, pydocstyle, pylint.
  8. JavaScript code quality checks pass: eslint.


# if the passed attribute value is a string,
# create the attribute value.
if not isinstance(value, AttributeChoiceValue):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block is the same as in VariantAttributeForm, maybe we can make a function from it?

@maarcingebala
Copy link
Member Author

@akjanik You were right about duplicated code, so I've extracted a form mixin that handles dynamic attributes fields in the same way in both product and variant form. I think the code looks a bit better now.

Also, I've taken the opportunity and dropped get_attribute and set_attribute functions from models, that were used mostly in tests. There is definitely too many confusing utility functions related to attributes, that we should eventually organize.

@codecov
Copy link

codecov bot commented Mar 26, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@f88d44f). Click here to learn what that means.
The diff coverage is 72.91%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1973   +/-   ##
=========================================
  Coverage          ?   84.95%           
=========================================
  Files             ?      161           
  Lines             ?     6892           
  Branches          ?      699           
=========================================
  Hits              ?     5855           
  Misses            ?      849           
  Partials          ?      188
Impacted Files Coverage Δ
saleor/product/models.py 94.71% <ø> (ø)
saleor/data_feeds/google_merchant.py 72.32% <0%> (ø)
saleor/dashboard/product/views.py 69.31% <0%> (ø)
saleor/dashboard/product/forms.py 86.53% <87.5%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f88d44f...7fe6045. Read the comment docs.


def iter_attribute_fields(self):
for attr in self.available_attributes:
yield self[attr.get_formfield_name()]
Copy link
Contributor

@mad-anne mad-anne Mar 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be rather self.fields[attr.get_formfield_name()]?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can access fields in this way as well. It was here before so I didn't change it.

@maarcingebala maarcingebala merged commit a7e5129 into saleor:master Mar 27, 2018
@maarcingebala maarcingebala deleted the save-product-attributes-values branch March 27, 2018 10:00
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.

None yet

4 participants