Add a form field for configuring conditions.

Review Request #8279 — Created July 11, 2016 and submitted

Information

Djblets
release-0.10.x
67b270a...

Reviewers

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.

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.


Description From Last Updated

These aren't quite all aligned vertically. Have fun!

brenniebrennie

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

daviddavid

The first comma in here is unnecessary.

daviddavid

serialized as a dict ?

brenniebrennie

Should we assert isinstance(data, dict) after this?

brenniebrennie

Module docstring?

daviddavid

list comprehension redefines 'choice' from line 220

reviewbotreviewbot

This has a bunch of selectors that are nested deeper than they need to be. Everything that starts with conditions- …

daviddavid

Doc comment?

daviddavid

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

daviddavid

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

daviddavid

Same with this?

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

Parens around this expression aren't needed.

daviddavid

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

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

daviddavid

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (83 > 79 characters)

reviewbotreviewbot
reviewbot
  1. 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
    
    
  2. djblets/forms/widgets.py (Diff revision 1)
     
     
    Show all issues
     list comprehension redefines 'choice' from line 220
    
  3. djblets/staticbundles.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (81 > 79 characters)
    
  4. djblets/staticbundles.py (Diff revision 1)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  5. 
      
david
  1. 
      
  2. djblets/forms/fields.py (Diff revision 1)
     
     
    Show all issues

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

  3. djblets/forms/fields.py (Diff revision 1)
     
     
    Show all issues

    The first comma in here is unnecessary.

  4. djblets/forms/widgets.py (Diff revision 1)
     
     
    Show all issues

    Module docstring?

  5. djblets/static/djblets/css/forms/conditions.less (Diff revision 1)
     
     
     
     
     
    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.

    1. 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.

  6. Show all issues

    Doc comment?

  7. Show all issues

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

    1. 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.

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

    3. 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).

  8. Show all issues

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

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

  9. Show all issues

    Same with this?

  10. Show all issues

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

    1. 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?

    2. 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));
      
    3. I'll change it then, but it's pretty inconsistent.

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

      x = (y === z);
      
      x(y === z);
      
  11. Show all issues

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

  12. Show all issues

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

  13. Show all issues

    Parens around this expression aren't needed.

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

  14. djblets/staticbundles.py (Diff revision 1)
     
     
    Show all issues

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

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

  15. 
      
brennie
  1. 
      
  2. Show all issues

    These aren't quite all aligned vertically. Have fun!

  3. djblets/forms/fields.py (Diff revision 1)
     
     
    Show all issues

    serialized as a dict ?

  4. djblets/forms/fields.py (Diff revision 1)
     
     
     
    Show all issues

    Should we assert isinstance(data, dict) after this?

  5. 
      
chipx86
reviewbot
  1. 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
    
    
  2. djblets/staticbundles.py (Diff revision 2)
     
     
    Show all issues
    Col: 80
     E501 line too long (83 > 79 characters)
    
  3. 
      
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-0.10.x (39bc6d2)