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. Show all issues

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

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

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

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

    Same note here.

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

    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)
     
     
    Show all issues

    Missing a trailing period.

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

    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)
     
     
     
     
     
     
    Show all issues

    Same note about deprecations here.

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

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

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

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

  4. 
      
maubin
maubin
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.x (4555e46)