[WIP] Modifying djblets.forms.widgets.ListEditWidget to handle any type of widget and form field

Review Request #11858 — Created Oct. 20, 2021 and updated

maubin
Djblets
master
djblets

Previously, ListEditWidget expects values to be strings with a valid
separator, thus it can only be used to edit a list of strings.
The corresponding Django template and Backbone view are hardcoded to
display string values with text inputs.

This change modifies ListEditWidget so that it can modify a list of
any type of values, not just strings. This change includes the creation
of a ListEditField which has the ListEditWidget as its default widget.
This field splits a string into a list of strings, reproducing the
behaviour of the old ListEditWidget. We also introduce a
ListEditDictionaryField, which takes a dictionary and splits it into a
list of (key, value) tuples.

TODO:
- Modify tests for ListEditWidget, its template and backbone view

Added tests for ListEditField and ListEditDictionaryField.

Summary Author
initial modifications to list edit widget
Michelle
modified html classes and names
Michelle
create listeditfield
Michelle
create listeditdictionaryfield
Michelle
add tests for listEditField
Michelle
add listEditDictionary tests and some typo fixes
Michelle
fixed flake8 issues
Michelle
add get_context and value_from_datadict methods
Michelle
working on javascript logic for dynamically adding and removing values in listeditwidget
Michelle
kinda working but need to fix getting proper index for data-list-index and names
Michelle
done logic for adding and removing values in listeditwidget
Michelle
address issues from review request
Michelle
handle old behavior of rejoining values into a string
Michelle
applied review request feedback
Michelle
Description From Last Updated

Typo in summary: dbjlets -> djblets

daviddavid

Please wrap the description/testing done to 70 columns.

daviddavid

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

Will change {{forloop.counter0}} to 0

maubinmaubin

This trailing comma will break older browsers, since it's outputted directly to the browser, and is not in a .es6.js ...

chipx86chipx86

Django's MultiWidget also takes in a child widget (well, a list of them). It accepts either a class or an ...

chipx86chipx86

So here's a big thing I missed in my prior review (and this is all from the original design, which ...

chipx86chipx86

This could be one statement.

chipx86chipx86

No need for parens around name. In fact, (name) is equivalent to name. If we instead had (name,), then it'd ...

chipx86chipx86

I suspect we won't want to use attrs itself here. Those are the attributes for the ListEditWidget, which will probably ...

chipx86chipx86

We reference self.value_widget many times. Each time we do that, we have to go through the attribute accessor paths in ...

chipx86chipx86

Missing a trailing comma.

chipx86chipx86

Technically, this makes this a breaking change, which requires a Djblets major version bump (we follow semantic versioning in Djblets, ...

chipx86chipx86

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W391 blank line at end of file

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

E231 missing whitespace after ','

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

It's possible that when you are creating ListEditView here, it is not defined, That's why the error Cannot read properties ...

akim.ruslanovakim.ruslanov

You're actually storing this as _sep in the __init__ method. We don't need to have any documentation for purely internal/private ...

daviddavid

This can be dedented 4 spaces.

daviddavid

Dedent 4 spaces.

daviddavid

This should just be indented 4 spaces, not 7. Even better would be to reformat the docstring so the first ...

daviddavid

Because this init method doesn't do anything but call the superclass, we can just get rid of it.

daviddavid

Dedent 4 spaces.

daviddavid

Dedent 4 spaces.

daviddavid

This should have a "Raises" section with django.forms.ValidationError

daviddavid

This new line can go away (plus you added some trailing whitespace).

daviddavid

This new line can go away (plus you added some trailing whitespace).

daviddavid

Please add a module docstring for this.

daviddavid

Please add a module docstring for this.

daviddavid

We generally avoid the python ternary syntax because it's not particularly readable. Let's just make this an if/else.

daviddavid

Add a blank line between these two. For properties, we can also skip "Returns" and just put in: Type: django.forms.widgets.Media ...

daviddavid

We do sometimes say "list or unicode" to clarify here.

daviddavid

Trailing whitespace on this line.

daviddavid

Trailing whitespace on this line.

daviddavid

Col: 87 Missing semicolon.

reviewbotreviewbot

Trailing whitespace on this line.

daviddavid

Trailing whitespace on this line.

daviddavid

Trailing whitespace on this line.

daviddavid

Trailing whitespace on this line.

daviddavid

We don't use var in .es6 files. That said, if you change the .each() to use a fat-arrow function, you ...

daviddavid

Col: 62 Missing semicolon.

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

maubin
maubin
  1. 
      
  2. Will change {{forloop.counter0}} to 0

  3. 
      
chipx86
  1. 
      
  2. This trailing comma will break older browsers, since it's outputted directly to the browser, and is not in a .es6.js file (which would compile suitably for the browsers we need to support).

    1. Given that this is future-looking work that won't ship until RB5.0, I don't think we need to worry about IE11 compat

  3. djblets/forms/widgets.py (Diff revision 2)
     
     

    Django's MultiWidget also takes in a child widget (well, a list of them). It accepts either a class or an instance in the constructor, and if it's a class, it'll instantiate it.

    I think that's the right pattern, and the right default. Taking in a class just stores a reference to the class in the list of default values for the function.

    Making the default an instance means that:

    1. We instantiate the TextInput and keep this around forever (using memory that may not be needed)
    2. We actually share that default instance across all ListEditWidget instances by default, which could lead to errors with state being clobbered.

    Let's make the default a reference to the class, and then in the constructor we can instantiate it if it's a class or store directly if the caller provided an instance.

  4. djblets/forms/widgets.py (Diff revision 2)
     
     
     
     
     
     

    This could be one statement.

  5. djblets/forms/widgets.py (Diff revision 2)
     
     

    No need for parens around name. In fact, (name) is equivalent to name. If we instead had (name,), then it'd be a tuple with one value (name) in it.

    A tuple is often a correct thing to pass in using %-based formatting, but isn't necessary in this particular case.

    Caveat: If the value being passed in was iterable (a list or tuple, for example), we would need to wrap it as (value,), or the %-formatting would be looking inside of it for values to stuff into the %s and such.

  6. djblets/forms/widgets.py (Diff revision 2)
     
     

    I suspect we won't want to use attrs itself here. Those are the attributes for the ListEditWidget, which will probably differ.

    A base set of attrs are generally provided upon widget instantiation (or overridden in the widget itself), and then built upon when it's time to render. So starting out, we'd probably want:

    widget_attrs = value_widget.attrs
    

    Which we could pass in to render().

    If we wanted to add our own attributes to each value (say, giving them each a unique ID in the DOM — something worth considering in the future — consider the following an example only), we'd do something like:

    common_widget_attrs = value_widget.attrs
    
    for i, val in enumerate(value_list):
        widget_attrs = {}
    
        if 'id' in attrs:
            widget_attrs['id'] = '%s_%s' % (self.id_for_label(attrs['id']), i)
    
        render(
            ...,
            attrs=value_widget.build_attrs(common_widget_attrs, widget_attrs))
    

    So, taking those base attributes, and then modifying them on each loop so it's suitable for that row.

    For reference:

    • enumerate wraps an iterable, yielding an index and the value for each iteration.
    • id_for_label returns a suitable ID for the widget.
    • build_attrs takes in common attributes and specific attributes and returns a new dictionary combining the two.
    1. Actually, I just looked at the code above a bit, and we are already building attrs as the per-child-widget attrs. So let's keep doing that. (I'm not sure that's strictly correct, but it's current behavior). So much of the above can be ignored.

  7. djblets/forms/widgets.py (Diff revision 2)
     
     

    We reference self.value_widget many times. Each time we do that, we have to go through the attribute accessor paths in the class. Not particularly slow, but not worth doing. Let's pull out value_widget into a local variable before the loop above, so accesses are fast.

  8. djblets/forms/widgets.py (Diff revision 2)
     
     

    Missing a trailing comma.

  9. Technically, this makes this a breaking change, which requires a Djblets major version bump (we follow semantic versioning in Djblets, at least nowadays).

    This is probably fine, since we're targeting Djblets 3.0 for this work, but we need to document this with the following before the Args section:

    Version Changed:
        3.0:
        * Removed the `inputAttrs` option.
        * Added the `renderedDefaultRow` option.
    

    (Also, renderedDefaultRow will need to be documented.)

  10. 
      
chipx86
  1. 
      
  2. djblets/forms/widgets.py (Diff revision 2)
     
     
     
     

    So here's a big thing I missed in my prior review (and this is all from the original design, which makes modernizing this tricky).

    This code is still assuming that we're dealing strictly with individual strings (and that we can just split them). That isn't safe, as the widget may be returning an integer, a list, a dictionary, or even string-encoded content that may get split wrong.

    However, we don't want to break existing consumers.

    The problem comes down to the original design. This assumed it was only ever going to work with strings, and that the data being fed in was a string that had a valid separator.

    It should have been designed to work in conjunction with a matching field that prepared the value before it got to render(), turning whatever the content would be into a list of values that pertained to the widgets.

    Basically, it should be the job of the field to prepare data and the job of a widget to render it. The widget shouldn't have to be parsing anything.

    So I think we can work our way toward this design by doing the following:

    1. In render(), check if value is a list. If so, we're good. This is modern. If not, convert it to a list through this existing logic we have here. (Update the code to reuse value, instead of value_list, to make it clear that this is what we expect of it.)
    2. Add a comment saying that we plan to deprecate this behavior. In a future release, we'll enable deprecation warnings for this code path.
    3. Introduce a djblets.forms.fields.ListEditField. This will be the new field intended to work with ListEditWidget. Its job will be to take in a string and convert it to values, and to associate ListEditWidget as the default widget. Subclasses (such as your field for the Pygments work) can do that conversion however is needed. By default, it should split in the same way that the widget otherwise would.

    This effectively gives us a migration path to that ideal world in which the widget just renders, and the field handles the data processing. It would then be really easy to subclass these and get the data and widget setup in the right form.

    That may sound like a lot, but the field work isn't going to be too bad. Not documented as well as it should be, but I can guide you through any of it.

  3. 
      
maubin
Review request changed

Change Summary:

Create ListEditField and ListEditDictionaryField and their tests

Description:

~  

Previously, ListEditWidget expects values to be strings rendered with a TextInput widget.

~   Initial modifications to list edit widget so that values can be rendered by any widget

  ~

Previously, ListEditWidget expects values to be strings with a valid separator, thus it can only be used to edit a list of strings. The corresponding Django template and Backbone view are hardcoded to display string values with text inputs.

  ~
  +

This change modifies ListEditWidget so that it can modify a list of any type of values, not just strings. This change also includes the creation of a ListEditField which has the ListEditWidget as its default widget. This field splits a string into a list of strings, reproducing the behaviour of the old ListEditWidget. We also introduce a ListEditDictionaryField, which takes a dictionary and splits it into a list of (key, value) tuples.

  +
  +

TODO:

  + - ModifyListEditWidget and its template and backbone view (and tests for each of those)

Testing Done:

~  

Testing on dev server, "Add Item" button saves the entire form instead of just adding another entry to the list widget (not sure why)

  ~

Added tests for ListEditField and ListEditDictionaryField, ran successfully

Commits:

Summary Author
-
initial modifications to list edit widget
Michelle
-
modified html classes and names
Michelle
+
initial modifications to list edit widget
Michelle
+
modified html classes and names
Michelle
+
create listeditfield
Michelle
+
create listeditdictionaryfield
Michelle
+
add tests for listEditField
Michelle
+
add listEditDictionary tests and some typo fixes
Michelle

Diff:

Revision 3 (+539 -63)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
maubin
maubin
akim.ruslanov
  1. 
      
  2. djblets/forms/templates/djblets_forms/list_edit_widget.html (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    It's possible that when you are creating ListEditView here, it is not defined, That's why the error Cannot read properties of undefined (reading 'ListEditView') happens? I would try to see if I can do something like console.log(Djblets.Forms) just to see if the class is properly imported/accessible within this HTML. You might need to add extra code to specify where to get the ListEditView from

  3. 
      
mxwang
  1. 
      
  2. Are you calling/importing the Form correctly? Adding the correct classes (ex. multipart/form) when rendering it in the HTML? Does new items in the listeditview require a different form of parsing than just parsing the text?

    I would also console.log() it out to see if the result is what you're expecting, and check the types of the results too.

  3. 
      
maubin
Review request changed

Change Summary:

Done all the logic for ListEditWidget, need to modify tests

Description:

   

Previously, ListEditWidget expects values to be strings with a valid separator, thus it can only be used to edit a list of strings. The corresponding Django template and Backbone view are hardcoded to display string values with text inputs.

   
~  

This change modifies ListEditWidget so that it can modify a list of any type of values, not just strings. This change also includes the creation of a ListEditField which has the ListEditWidget as its default widget. This field splits a string into a list of strings, reproducing the behaviour of the old ListEditWidget. We also introduce a ListEditDictionaryField, which takes a dictionary and splits it into a list of (key, value) tuples.

  ~

This change modifies ListEditWidget so that it can modify a list of any type of values, not just strings. This change includes the creation of a ListEditField which has the ListEditWidget as its default widget. This field splits a string into a list of strings, reproducing the behaviour of the old ListEditWidget. We also introduce a ListEditDictionaryField, which takes a dictionary and splits it into a list of (key, value) tuples.

   
   

TODO:

~   - ModifyListEditWidget and its template and backbone view (and tests for each of those)

  ~ - Modify tests for ListEditWidget, its template and backbone view

Commits:

Summary Author
-
initial modifications to list edit widget
Michelle
-
modified html classes and names
Michelle
-
create listeditfield
Michelle
-
create listeditdictionaryfield
Michelle
-
add tests for listEditField
Michelle
-
add listEditDictionary tests and some typo fixes
Michelle
-
fixed flake8 issues
Michelle
-
add get_context and value_from_datadict methods
Michelle
-
working on javascript logic for dynamically adding and removing values in listeditwidget
Michelle
+
initial modifications to list edit widget
Michelle
+
modified html classes and names
Michelle
+
create listeditfield
Michelle
+
create listeditdictionaryfield
Michelle
+
add tests for listEditField
Michelle
+
add listEditDictionary tests and some typo fixes
Michelle
+
fixed flake8 issues
Michelle
+
add get_context and value_from_datadict methods
Michelle
+
working on javascript logic for dynamically adding and removing values in listeditwidget
Michelle
+
kinda working but need to fix getting proper index for data-list-index and names
Michelle
+
done logic for adding and removing values in listeditwidget
Michelle
+
address issues from review request
Michelle

Diff:

Revision 7 (+974 -326)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

david
  1. I know this is still marked WIP, but it's looking really solid so I did a pretty thorough review.

  2. Typo in summary: dbjlets -> djblets

  3. Please wrap the description/testing done to 70 columns.

  4. djblets/forms/fields.py (Diff revision 7)
     
     
     

    You're actually storing this as _sep in the __init__ method. We don't need to have any documentation for purely internal/private attributes, unless they're particularly confusing or complicated.

  5. djblets/forms/fields.py (Diff revision 7)
     
     

    This can be dedented 4 spaces.

  6. djblets/forms/fields.py (Diff revision 7)
     
     

    Dedent 4 spaces.

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

    This should just be indented 4 spaces, not 7. Even better would be to reformat the docstring so the first line fits on one line and move any details into a second paragraph.

  8. djblets/forms/fields.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     

    Because this init method doesn't do anything but call the superclass, we can just get rid of it.

  9. djblets/forms/fields.py (Diff revision 7)
     
     

    Dedent 4 spaces.

  10. djblets/forms/fields.py (Diff revision 7)
     
     

    Dedent 4 spaces.

  11. djblets/forms/fields.py (Diff revision 7)
     
     

    This should have a "Raises" section with django.forms.ValidationError

  12. This new line can go away (plus you added some trailing whitespace).

  13. This new line can go away (plus you added some trailing whitespace).

  14. Please add a module docstring for this.

    1. What should the docstring say? I looked at the other tests in djblets.forms.tests but didn't see any files that had a module docstring

    2. It can probably be the same as the class docstring.

  15. Please add a module docstring for this.

  16. djblets/forms/widgets.py (Diff revision 7)
     
     
     

    We generally avoid the python ternary syntax because it's not particularly readable. Let's just make this an if/else.

  17. djblets/forms/widgets.py (Diff revision 7)
     
     
     

    Add a blank line between these two.

    For properties, we can also skip "Returns" and just put in:

    Type:
        django.forms.widgets.Media
    

    because callers can use it as if it were just a plain attribute and not a method.

    1. there's another media() method in this file for the ConditionsWidget that has "Returns". Should we change this too?

    2. Sure.

  18. djblets/forms/widgets.py (Diff revision 7)
     
     

    We do sometimes say "list or unicode" to clarify here.

  19. Trailing whitespace on this line.

  20. Trailing whitespace on this line.

  21. Trailing whitespace on this line.

  22. Trailing whitespace on this line.

  23. Trailing whitespace on this line.

  24. Trailing whitespace on this line.

  25. We don't use var in .es6 files. That said, if you change the .each() to use a fat-arrow function, you can just use this directly instead of aliasing it to self.

  26. 
      
maubin
maubin
Review request changed

Summary:

-[WIP] Modifying dbjlets.forms.widgets.ListEditWidget to handle any type of widget and form field
+[WIP] Modifying djblets.forms.widgets.ListEditWidget to handle any type of widget and form field

Description:

~  

Previously, ListEditWidget expects values to be strings with a valid separator, thus it can only be used to edit a list of strings. The corresponding Django template and Backbone view are hardcoded to display string values with text inputs.

  ~

Previously, ListEditWidget expects values to be strings with a valid

  + separator, thus it can only be used to edit a list of strings.
  + The corresponding Django template and Backbone view are hardcoded to
  + display string values with text inputs.

   
~  

This change modifies ListEditWidget so that it can modify a list of any type of values, not just strings. This change includes the creation of a ListEditField which has the ListEditWidget as its default widget. This field splits a string into a list of strings, reproducing the behaviour of the old ListEditWidget. We also introduce a ListEditDictionaryField, which takes a dictionary and splits it into a list of (key, value) tuples.

  ~

This change modifies ListEditWidget so that it can modify a list of

  + any type of values, not just strings. This change includes the creation
  + of a ListEditField which has the ListEditWidget as its default widget.
  + This field splits a string into a list of strings, reproducing the
  + behaviour of the old ListEditWidget. We also introduce a
  + ListEditDictionaryField, which takes a dictionary and splits it into a
  + list of (key, value) tuples.

   
   

TODO:

    - Modify tests for ListEditWidget, its template and backbone view

Testing Done:

~  

Added tests for ListEditField and ListEditDictionaryField, ran successfully

  ~

Added tests for ListEditField and ListEditDictionaryField.

Loading...