Summary

Add a form field for configuring conditions.

Review Request #8279 — Created July 11, 2016 and submitted July 18, 2016, 2:28 a.m.

Information

Owner

chipx86

Repository

Djblets

Branch

release-0.10.x

Bugs

Depends On

Commit

67b270a...

Reviewers

Groups

djblets

People

Description

This introduces a new Django form field that allows users to view and
configure conditions. This is presented as a JavaScript-backed widget
that shows each condition as a row containing drop-downs for the choice
and operator, and a field for the value. Below these are a button for
adding new condition rows.

Changing a choice will change the list of operators available, and
reset the value for the new type of field. Changing just the operator
will retain the value unless the new or old operators had a custom value
field.

Conditions can be deleted by clicking a "-" button on the left of the
row.

The widget is designed to handle form validation and to be resilient to
missing choices/operators (which could happen if working with conditions
that were based on choices/operators from extensions). Errors specific
to a condition (value validation issues, for instance) will show up in
the row for that condition, and will be resolved once corrected and the
form re-submitted. Missing condition choices/operators will result in a
disabled condition row, which must be deleted before the form can be
submitted again.

Testing Done

Tested loading/editing existing conditions, adding new ones, and deleting
them.

Tested custom value fields on both choices and operators.

Tested choices/operators that don't need a value.

Tested that the value was preserved when switching operators within a choice,
unless the old/new operator had a custom value field.

Tested validation errors for value fields.

Tested usage of extension-provided custom choices/operators.

Tested missing choice/operators, their presentation, and inability to save
until the conditions were removed.

Unit tests pass.

Files

Issues

Description	From	Last Updated
These aren't quite all aligned vertically. Have fun!	brennie	July 11, 2016, 6:17 p.m.
It doesn't actually matter but I think it would be conceptually nicer to do this after choices has been validated.	david	July 11, 2016, 4:48 p.m.
The first comma in here is unnecessary.	david	July 11, 2016, 4:48 p.m.
serialized as a dict ?	brennie	July 11, 2016, 4:50 p.m.
Should we assert isinstance(data, dict) after this?	brennie	July 11, 2016, 4:50 p.m.
Module docstring?	david	July 11, 2016, 4:49 p.m.
list comprehension redefines 'choice' from line 220	reviewbot	July 11, 2016, 3:19 p.m.
This has a bunch of selectors that are nested deeper than they need to be. Everything that starts with conditions- …	david	July 11, 2016, 5:51 p.m.
Doc comment?	david	July 11, 2016, 4:59 p.m.
Do you really want this to run on every change to every property or should it be this.on('change:choice', ...?	david	July 11, 2016, 4:58 p.m.
Mind moving these down closer to where they're used?	david	July 11, 2016, 5:21 p.m.
Same with this?	david	July 11, 2016, 5:21 p.m.
Parens aren't needed when there's just one argument.	david	July 11, 2016, 6:13 p.m.
Parens aren't needed when there's just one argument.	david	July 11, 2016, 6:13 p.m.
Parens aren't needed when there's just one argument.	david	July 11, 2016, 6:13 p.m.
Parens around this expression aren't needed.	david	July 11, 2016, 5:39 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	July 11, 2016, 3:18 p.m.
I don't see this file listed in the diff.	david	July 11, 2016, 5:39 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	July 11, 2016, 3:18 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	July 11, 2016, 6:18 p.m.

Tool: PEP8 Style Checker
Processed Files:
    djblets/forms/widgets.py
    djblets/forms/tests/test_conditions_field.py
    djblets/staticbundles.py
    djblets/forms/fields.py
    djblets/forms/tests/test_conditions_widget.py

Ignored Files:
    djblets/static/djblets/js/forms/views/tests/conditionValueFormFieldViewTests.es6.js
    djblets/static/djblets/js/forms/models/conditionValueField.es6.js
    djblets/static/djblets/js/forms/models/tests/conditionModelTests.es6.js
    djblets/static/djblets/js/forms/models/tests/conditionOperatorModelTests.es6.js
    djblets/static/djblets/js/forms/base.js
    djblets/static/djblets/js/forms/models/conditionSetModel.es6.js
    djblets/static/djblets/js/forms/models/conditionModel.es6.js
    djblets/static/djblets/js/forms/views/tests/conditionSetViewTests.es6.js
    djblets/static/djblets/js/forms/views/baseConditionValueFieldView.es6.js
    djblets/static/djblets/js/forms/models/conditionOperatorModel.es6.js
    djblets/static/djblets/js/forms/models/conditionChoiceModel.es6.js
    djblets/forms/templates/djblets_forms/conditions_widget.html
    djblets/static/djblets/js/forms/models/tests/conditionChoiceModelTests.es6.js
    djblets/static/djblets/js/forms/views/conditionValueFormFieldView.es6.js
    djblets/static/djblets/js/forms/views/conditionSetView.es6.js
    djblets/static/djblets/js/forms/models/tests/conditionSetModelTests.es6.js
    djblets/static/djblets/css/forms/conditions.less
    djblets/static/djblets/js/jquery.gravy.util.js



Tool: Pyflakes
Processed Files:
    djblets/forms/widgets.py
    djblets/forms/tests/test_conditions_field.py
    djblets/staticbundles.py
    djblets/forms/fields.py
    djblets/forms/tests/test_conditions_widget.py

Ignored Files:
    djblets/static/djblets/js/forms/views/tests/conditionValueFormFieldViewTests.es6.js
    djblets/static/djblets/js/forms/models/conditionValueField.es6.js
    djblets/static/djblets/js/forms/models/tests/conditionModelTests.es6.js
    djblets/static/djblets/js/forms/models/tests/conditionOperatorModelTests.es6.js
    djblets/static/djblets/js/forms/base.js
    djblets/static/djblets/js/forms/models/conditionSetModel.es6.js
    djblets/static/djblets/js/forms/models/conditionModel.es6.js
    djblets/static/djblets/js/forms/views/tests/conditionSetViewTests.es6.js
    djblets/static/djblets/js/forms/views/baseConditionValueFieldView.es6.js
    djblets/static/djblets/js/forms/models/conditionOperatorModel.es6.js
    djblets/static/djblets/js/forms/models/conditionChoiceModel.es6.js
    djblets/forms/templates/djblets_forms/conditions_widget.html
    djblets/static/djblets/js/forms/models/tests/conditionChoiceModelTests.es6.js
    djblets/static/djblets/js/forms/views/conditionValueFormFieldView.es6.js
    djblets/static/djblets/js/forms/views/conditionSetView.es6.js
    djblets/static/djblets/js/forms/models/tests/conditionSetModelTests.es6.js
    djblets/static/djblets/css/forms/conditions.less
    djblets/static/djblets/js/jquery.gravy.util.js

djblets/forms/widgets.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 list comprehension redefines 'choice' from line 220
```
djblets/staticbundles.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
djblets/staticbundles.py (Diff revision 1)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```

djblets/forms/fields.py (Diff revision 1)

The issue has been resolved. Show all issues

It doesn't actually matter but I think it would be conceptually nicer to do this after choices has been validated.

djblets/forms/fields.py (Diff revision 1)
The issue has been resolved. Show all issues
```
The first comma in here is unnecessary.
```
djblets/forms/widgets.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Module docstring?
```

djblets/static/djblets/css/forms/conditions.less (Diff revision 1)

The issue has been dropped. Show all issues

This has a bunch of selectors that are nested deeper than they need to be. Everything that starts with conditions- can be pulled out to the top level with no loss of specificity.

chipx86 July 11, 2016, 5:51 p.m.

Many of these are actually needed in order to achieve more specific precedence than Django's CSS rules for the admin UI (particularly for lists, inputs, links), so I'm going to keep it as-is.

djblets/static/djblets/js/forms/models/conditionChoiceModel.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Doc comment?
```

djblets/static/djblets/js/forms/models/conditionModel.es6.js (Diff revision 1)

The issue has been dropped. Show all issues

Do you really want this to run on every change to every property or should it be this.on('change:choice', ...?

chipx86 July 11, 2016, 5:39 p.m.

I sadly really want this... It's not how I want to do it, but I haven't found a more elegant way.

Basically, in response to a choice change, I want to set some other state on the model: operator and value. However, these need to happen after the choice change is triggered, in order for callers to be able to update things in a sane order (choice, operator, value) without having to jump through hoops.

By connecting in initialize, this event handler gets registered first, which means it'll update the operator/value before any other event handlers (from the UI) manage to get notified. The event handler can't be registered last, and it can't trigger the event itself and bail out of the previous loop.

I also want this to be done within the same event loop, and not through a defer. The defer makes it harder to test, and I want the state to all be in place before the caller (doing the set('choice', ...)) gets control again.

Given limited options, I found that the only place I can really tie in would be in change. I don't love it, and I'd rather have an alternative, but short of rewriting how eventing works or crafting a new series of custom (redundant) events, I don't really have a good alternative.

david July 11, 2016, 5:52 p.m.

In that case, your comment should be clearer. It currently says "When the choice changes..."

chipx86 July 11, 2016, 6:11 p.m.

The first thing we check in the change handler is for whether the choice has changed, and only then do we handle any logic. It's basically equivalent to using change:choice (more or less).

djblets/static/djblets/js/forms/views/conditionSetView.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

Mind moving these down closer to where they're used?

chipx86 July 11, 2016, 5:39 p.m.

Going to group fieldName, rowNum, and $rowOptions together, since they're all equally common between the other sections.

djblets/static/djblets/js/forms/views/conditionSetView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Same with this?
```

djblets/static/djblets/js/forms/views/conditionSetView.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

Parens aren't needed when there's just one argument.

chipx86 July 11, 2016, 5:39 p.m.

I'd think I'd prefer keeping it for consistency with multi-args and (). It helps distinguish it at a faster glance than things like variable assignments or comparisons, and it's a (trivial) bit easier to add additional args (in cases like signal handlers). It also just feels easier to quickly scan:

const foo = (bar) => blah();


than:

const foo = bar => blah();


The latter looks too much like a chained variable assignment.
Some Googling shows that there are many opinions on both sides of this. One linter has settled on by default requiring (bar), for consistency (after a lot of debate), while others don't really impose a rule yet.
Thoughts?

david July 11, 2016, 5:52 p.m.

I agree that during assignment (your example) it's nice to have the parens to make scanning easier. When it's an argument to a method, I think that:

foo.each(bar => blah(bar));


is a lot more readable than:

foo.each((bar) => blah(bar));

chipx86 July 11, 2016, 6:12 p.m.

I'll change it then, but it's pretty inconsistent.

david July 12, 2016, 10:22 a.m.

We have a similar "inconsistency" with regards to truth tests. So at least it's consistently inconsistent?

x = (y === z);

x(y === z);

djblets/static/djblets/js/forms/views/conditionSetView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Parens aren't needed when there's just one argument.
```
djblets/static/djblets/js/forms/views/conditionSetView.es6.js (Diff revision 1)
The issue has been resolved. Show all issues
```
Parens aren't needed when there's just one argument.
```

djblets/static/djblets/js/forms/views/conditionSetView.es6.js (Diff revision 1)

The issue has been resolved. Show all issues

Parens around this expression aren't needed.

chipx86 July 11, 2016, 5:39 p.m.

Usually have parens due to === checks and such in there, for readability. I'll remove it for this case.

djblets/staticbundles.py (Diff revision 1)

The issue has been resolved. Show all issues

I don't see this file listed in the diff.

chipx86 July 11, 2016, 5:39 p.m.

Ended up not needing tests for this (it's very bare-bones). Removing frmo staticbundles.

Validation (Revision 1)
The issue has been resolved. Show all issues
```
These aren't quite all aligned vertically. Have fun!
```
djblets/forms/fields.py (Diff revision 1)
The issue has been resolved. Show all issues
```
serialized as a dict ?
```
djblets/forms/fields.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should we assert isinstance(data, dict) after this?
```

Change Summary:


Removed a reference to a non-existent (and unneeded) unit test file.
Cleaned up some docs and comments.
Added missing docstrings for functions and file headers.
Sanity-checked data being passed to a ConditionsField, raising an error if the end result isn't a dict.
Fixed vertical alignment of mode radio buttons.
Fixed a bad lookup in the JavaScript for conditions with an invalid choice ID, and added a unit test.
Removed parens for single-arg fat arrow functions when inside a function call.
Other misc code cleanups.

Commit:

-	c8c45c79425218b91302cb5cb391206c992aaf4b
+	67b270a9e806a8c5606bcddafed59c70c288bdee

Diff:

Revision 2 (+3443)

Show changes

	djblets/staticbundles.py
	djblets/forms/fields.py
	djblets/forms/widgets.py
	djblets/forms/templates/djblets_forms/conditions_widget.html
	djblets/forms/tests/test_conditions_field.py
	djblets/forms/tests/test_conditions_widget.py
	djblets/static/djblets/css/forms/conditions.less
	djblets/static/djblets/js/jquery.gravy.util.js
	15 more

Added Files:

Tool: Pyflakes
Processed Files:
    djblets/forms/widgets.py
    djblets/forms/tests/test_conditions_field.py
    djblets/staticbundles.py
    djblets/forms/fields.py
    djblets/forms/tests/test_conditions_widget.py

Ignored Files:
    djblets/static/djblets/js/forms/views/tests/conditionValueFormFieldViewTests.es6.js
    djblets/static/djblets/js/forms/models/conditionValueField.es6.js
    djblets/static/djblets/js/forms/models/tests/conditionModelTests.es6.js
    djblets/static/djblets/js/forms/models/tests/conditionOperatorModelTests.es6.js
    djblets/static/djblets/js/forms/base.js
    djblets/static/djblets/js/forms/models/conditionSetModel.es6.js
    djblets/static/djblets/js/forms/models/conditionModel.es6.js
    djblets/static/djblets/js/forms/views/tests/conditionSetViewTests.es6.js
    djblets/static/djblets/js/forms/views/baseConditionValueFieldView.es6.js
    djblets/static/djblets/js/forms/models/conditionOperatorModel.es6.js
    djblets/static/djblets/js/forms/models/conditionChoiceModel.es6.js
    djblets/forms/templates/djblets_forms/conditions_widget.html
    djblets/static/djblets/js/forms/models/tests/conditionChoiceModelTests.es6.js
    djblets/static/djblets/js/forms/views/conditionValueFormFieldView.es6.js
    djblets/static/djblets/js/forms/views/conditionSetView.es6.js
    djblets/static/djblets/js/forms/models/tests/conditionSetModelTests.es6.js
    djblets/static/djblets/css/forms/conditions.less
    djblets/static/djblets/js/jquery.gravy.util.js



Tool: PEP8 Style Checker
Processed Files:
    djblets/forms/widgets.py
    djblets/forms/tests/test_conditions_field.py
    djblets/staticbundles.py
    djblets/forms/fields.py
    djblets/forms/tests/test_conditions_widget.py

Ignored Files:
    djblets/static/djblets/js/forms/views/tests/conditionValueFormFieldViewTests.es6.js
    djblets/static/djblets/js/forms/models/conditionValueField.es6.js
    djblets/static/djblets/js/forms/models/tests/conditionModelTests.es6.js
    djblets/static/djblets/js/forms/models/tests/conditionOperatorModelTests.es6.js
    djblets/static/djblets/js/forms/base.js
    djblets/static/djblets/js/forms/models/conditionSetModel.es6.js
    djblets/static/djblets/js/forms/models/conditionModel.es6.js
    djblets/static/djblets/js/forms/views/tests/conditionSetViewTests.es6.js
    djblets/static/djblets/js/forms/views/baseConditionValueFieldView.es6.js
    djblets/static/djblets/js/forms/models/conditionOperatorModel.es6.js
    djblets/static/djblets/js/forms/models/conditionChoiceModel.es6.js
    djblets/forms/templates/djblets_forms/conditions_widget.html
    djblets/static/djblets/js/forms/models/tests/conditionChoiceModelTests.es6.js
    djblets/static/djblets/js/forms/views/conditionValueFormFieldView.es6.js
    djblets/static/djblets/js/forms/views/conditionSetView.es6.js
    djblets/static/djblets/js/forms/models/tests/conditionSetModelTests.es6.js
    djblets/static/djblets/css/forms/conditions.less
    djblets/static/djblets/js/jquery.gravy.util.js

djblets/staticbundles.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.10.x (39bc6d2)