[FIX] Wizard Attributes #104
Conversation
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 Report
@@ 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
Continue to review full report at Codecov.
|
|
@PCatinean Total coverage increased, despite lower hit on diff |
|
@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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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): | |||
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 \ |
There was a problem hiding this comment.
In what case would a required field not show up the list of options that we need to do it implicitly?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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))) |
There was a problem hiding this comment.
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']) | ||
| ) |
There was a problem hiding this comment.
If the code that updates the support fields is in the write method do we still need this?
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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?
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:
not be
be validated if the domain is empty.
passed from the front end.
and required values, ands and ors and inherited, as the
driver is now purely the list of valid ids.
with inheritance possible to drive these virtual columns.