Skip to content
This repository has been archived by the owner on Apr 28, 2022. It is now read-only.

[ENH] 10.0 raise warnings #84

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

Conversation

richard-willdooit
Copy link
Contributor

I did a proper re-base to remove all the "merge" commits.

Richard deMeester added 2 commits May 5, 2017 11:45
@codecov-io
Copy link

codecov-io commented May 9, 2017

Codecov Report

Merging #84 into 10.0 will decrease coverage by 0.11%.
The diff coverage is 61.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             10.0      #84      +/-   ##
==========================================
- Coverage   61.11%   60.99%   -0.12%     
==========================================
  Files          14       14              
  Lines        1368     1382      +14     
==========================================
+ Hits          836      843       +7     
- Misses        532      539       +7
Impacted Files Coverage Δ
product_configurator/models/product_config.py 76.43% <100%> (+0.12%) ⬆️
product_configurator/models/product.py 72.63% <58.82%> (-1.18%) ⬇️

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 89750da...0bb2c03. Read the comment docs.

@@ -436,24 +438,36 @@ def values_available(self, attr_val_ids, sel_val_ids):
avail = self.validate_domains_against_sels(domains, sel_val_ids)
if avail:
avail_val_ids.append(attr_val_id)

elif do_raise:
if len(config_lines) == 1 and config_lines[0].rule_description:
Copy link
Contributor

@PCatinean PCatinean May 31, 2017

Choose a reason for hiding this comment

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

This has pretty limited applicability, maybe we can find a better way to raise all the errors one by one as they are fixed or drop it I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our use customer's use case is:

They configures the product on a single view, with around 15 options. Some of the rules and dependencies were too complicated to reflect in readonly's on the view (the other PR will fix a lot of that). Then they clicked submit and they just got a "configuration is invalid - please check all the steps" error, which did not help them focus on the problem.

The real benefit of this approach is where a configurable product has been defined badly. A value on step 3 may make a value on step 1 invalid! This is a bad setup. But, the rendering of the page cannot foresee this (and nor should it have to). If they save the configuration after step 3 in this scenario, it just says "please check all your steps", but with a focussed error message, they will see ("Value X should not be chosen when the product is Blue or Red")....

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

Successfully merging this pull request may close these issues.

3 participants