Allow ListEditWidget to properly handle data when the widget is not rendered

Review Request #12346 — Created June 9, 2022 and submitted

Information

Djblets
release-3.x

Reviewers

When retrieving the value of a field from a submitted form, ListEditWidget
relies on certain special keys to be present in the form's data in order to
retrieve the value for the field. These keys are properly set when the widget is
rendered, but are not set in scenarios where the widget is not rendered (such as
in an API call posting to a form that uses this widget, or in test cases where a
form object is built without being rendered). This change allows
ListEditWidget to properly retrieve the value of a field even when the special
keys are missing.

This change also adds a deprecation warning for when the legacy behavior is used
(passing a string to the widget instead of a list). This change also prevents
the ListEdit fields from raising a ValueError when being passed null values.

  • Ran Unit tests for ListEditWidget, ListEditField and
    ListEditDictionaryField.
  • Ran all Javascript Unit tests.
Summary ID
Allow ListEditWidget to properly handle data when the widget is not rendered
5de9d3be7776ebacaa611031482d4c19fa406b8a
Description From Last Updated

Can you add unit tests covering these edge cases that were just fixed?

chipx86chipx86

You can avoid the isinstance check in this case by using elif.

chipx86chipx86

Same note here.

chipx86chipx86

We have special classes for deprecation warnings. They also wrap the warnings module. You'll want to import RemovedInDjblets40Warning from djblets.deprecations …

chipx86chipx86

Missing a trailing period.

chipx86chipx86

How about we cast to a boolean when pulling it out above, like: use_legacy_behavior = (data[...] == 'True') And actually, …

chipx86chipx86

Same note about deprecations here.

chipx86chipx86

Can you update this to specifically say "Djblets 4.0"?

chipx86chipx86

Can you update this to specifically say "Djblets 4.0"?

chipx86chipx86
chipx86
  1. 
      
  2. Can you add unit tests covering these edge cases that were just fixed?

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

    You can avoid the isinstance check in this case by using elif.

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

    Same note here.

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

    We have special classes for deprecation warnings. They also wrap the warnings module.

    You'll want to import RemovedInDjblets40Warning from djblets.deprecations and then call .warn on it with the string.

    Also, since this is Djblets, it shouldn't know or care about Review Board's deprecation schedule. This should specifically reference "Djblets 4.0".

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

    Missing a trailing period.

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

    How about we cast to a boolean when pulling it out above, like:

    use_legacy_behavior = (data[...] == 'True')
    

    And actually, to avoid the Pythonisms in the value, I think when we set the value for the form in the first place in render(), we should explicitly set 'true' or 'false', and then we can check against 'true' here.

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

    Same note about deprecations here.

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

    Can you update this to specifically say "Djblets 4.0"?

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

    Can you update this to specifically say "Djblets 4.0"?

  4. 
      
maubin
maubin
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-3.x (4555e46)
Loading...