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

[FIX] Wizard Attributes #104

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

Conversation

richard-willdooit
Copy link
Contributor

If accepted, this would replace the PRs #88 and #97

Wizard attributes (readonly, required, invisible) now controlled
by 3 virtual fields each, allowing far greater control. This fixes
a number of issues:

  • Attributes with conditional domains might be required, or might
    not be
  • Attributes with conditional domains marked as required can now
    be validated if the domain is empty.
  • Readonly values can be better detected when the values are not
    passed from the front end.
  • Super complex domains can now be used to drive the readonly
    and required values, ands and ors and inherited, as the
    driver is now purely the list of valid ids.
  • Easier modification of behaviour in custom circumstances
    with inheritance possible to drive these virtual columns.

Richard deMeester added 3 commits May 29, 2017 17:33
Wizard attributes (readonly, required, invisible) now controlled
by 3 virtual fields each, allowing far greater control.  This fixes
a number of issues:

* Attributes with conditional domains might be required, or might
    not be
* Attributes with conditional domains marked as required can now
    be validated if the domain is empty.
* Readonly values can be better detected when the values are not
    passed from the front end.
* Super complex domains can now be used to drive the readonly
    and required values, ands and ors and inherited, as the
    driver is now purely the list of valid ids.
* Easier modification of behaviour in custom circumstances
    with inheritance possible to drive these virtual columns.
@codecov-io
Copy link

codecov-io commented May 29, 2017

Codecov Report

Merging #104 into 10.0 will increase coverage by 5.23%.
The diff coverage is 57.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##             10.0     #104      +/-   ##
==========================================
+ Coverage   61.62%   66.85%   +5.23%     
==========================================
  Files          14       14              
  Lines        1368     1412      +44     
==========================================
+ Hits          843      944     +101     
+ Misses        525      468      -57
Impacted Files Coverage Δ
...configurator_wizard/wizard/product_configurator.py 52.94% <56.77%> (+19.69%) ⬆️
product_configurator/models/product.py 74.23% <66.66%> (+0.42%) ⬆️
product_configurator/models/product_config.py 76.84% <0%> (+0.52%) ⬆️

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 64010f5...e2a67d2. Read the comment docs.

@richard-willdooit
Copy link
Contributor Author

@PCatinean Total coverage increased, despite lower hit on diff

@PCatinean
Copy link
Contributor

@richard-willdooit that's quite a hefty PR, I'm 80% through it with the review. Seems really good, I like the approach and have a similar perspective.

I'll try my best to finish the review as soon as possible

# the "required" status.
if set(line.value_ids.ids) & set(avail_val_ids):
# TODO: Verify custom value type to be correct
return False
Copy link
Contributor

Choose a reason for hiding this comment

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

I do agree that there might be a case where a user might want "required if options available" rather than flat required.

On one hand you can have this flexibility on the other hand you are not guaranteed to have a required value and if you mess up your configuration restrictions the configuration will silently pass.

Maybe there's a way we can differentiate between two cases or also provide an error message when a line is required and has certain configurations which do not provide any options for the required line.

What do you 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.

@PCatinean My initial feeling was similar, and I was going to make a 2 way flag. But I fell back to this - if there are no possible selections based on other domains, then it just seemed logical that it can't be invalid - the definition must be...

If you want it changed to two types of required, I think we should do it as another PR after this one is merged.

@@ -109,7 +113,20 @@ def get_onchange_domains(self, values, cfg_val_ids):
continue
return domains

def get_form_vals(self, dynamic_fields, domains):
def get_support_fields(self, k, available_val_ids, tmpl_attribute):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please provide a docstring?

Also attribute_line would be a better name instead of tmpl_attribute.

continue
attribute_id = int(k.replace(self.field_prefix, ''))
tmpl_attribute = self.product_tmpl_id.attribute_line_ids.filtered(
lambda a: a.attribute_id.id == attribute_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

rename var to attribute_line

vals = {}
attribute_id = int(k.replace(self.field_prefix, ''))
vals[self.ro_field_prefix + str(attribute_id)] = \
len(available_val_ids) == 0
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe using a var key for self.ro_field_prefix + str(attribute_id) to make it cleaner would be nicer than using \

dynamic_fields.update({k: None})
vals[k] = None
v = None
if not v:
if tmpl_attribute.required and not tmpl_attribute.multi and \
Copy link
Contributor

Choose a reason for hiding this comment

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

In what case would a required field not show up the list of options that we need to do it implicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, I am allowing for the fact that an attribute may have been invalid and cleared. If, after this, there is only one value available, and it is a required attribute, then auto select that only value.

cfg_step = wiz.get_cfg_step()
vals = wiz.get_form_vals(dynamic_field_vals, domains, cfg_step)
if vals:
wiz.write(vals)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure exactly why the dynamic attribute fields (and only them also) are being written to when the view is rendered.

Shouldn't this happen in the write method better?

From what I understand it's used to update the values of the support_fields but it should be do-able through write, and it's cleaner imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This structure actually came about from setting defaults, and when this is merged and I rebase that one, it may make more sense. The vals returned may have defaults.

return xml_view

@api.multi
def get_cfg_step(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a docstring, in my version I have something like: """Attempt to return product.config.step.line object that has the id
of the wizard state stored as string"""

attrs['readonly'].append(
(dependee_field, 'not in', list(val_ids)))
attrs['required'].append(
(dependee_field, 'in', list(val_ids)))
Copy link
Contributor

Choose a reason for hiding this comment

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

I love how this big chunk just disappeared, very satisfying :)

if res_data.get('stored_support_vals'):
res_data.update(
ast.literal_eval(res_data['stored_support_vals'])
)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the code that updates the support fields is in the write method do we still need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the code which pulls it back out again. I'm happy to look again...

)
product_img = fields.Binary(
compute='_compute_cfg_image',
readonly=True
)
stored_support_vals = fields.Char()
Copy link
Contributor

Choose a reason for hiding this comment

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

If we need to compute the values of the stored fields on each step and we need them only there in that view, do we need to store them like the other values?

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