Summary:

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

Review Request #12346 — Created June 9, 2022 and submitted June 13, 2022, 2:39 p.m.

Information
Owner:	maubin
Repository:	Djblets
Branch:	release-3.x
Bugs:
Depends On:
Blocks:	12347
Reviewers
Groups:	djblets
People:

Description

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.

Testing Done


Ran Unit tests for ListEditWidget, ListEditField and

ListEditDictionaryField.
Ran all Javascript Unit tests.

Commits

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

Screenshots

Issues

Description	From	Last Updated
Can you add unit tests covering these edge cases that were just fixed?	chipx86	June 9, 2022, 2:20 p.m.
You can avoid the isinstance check in this case by using elif.	chipx86	June 9, 2022, 2:01 p.m.
Same note here.	chipx86	June 9, 2022, 2:01 p.m.
We have special classes for deprecation warnings. They also wrap the warnings module. You'll want to import RemovedInDjblets40Warning from djblets.deprecations …	chipx86	June 9, 2022, 2:15 p.m.
Missing a trailing period.	chipx86	June 9, 2022, 2:15 p.m.
How about we cast to a boolean when pulling it out above, like: use_legacy_behavior = (data[...] == 'True') And actually, …	chipx86	June 9, 2022, 2:16 p.m.
Same note about deprecations here.	chipx86	June 9, 2022, 2:16 p.m.
Can you update this to specifically say "Djblets 4.0"?	chipx86	June 9, 2022, 9:18 p.m.
Can you update this to specifically say "Djblets 4.0"?	chipx86	June 9, 2022, 9:18 p.m.

flake8 passed.

JSHint passed.

Show all issues

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

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

Show all issues

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

djblets/forms/fields.py (Diff revision 1)
Show all issues
```
Same note here.
```

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

djblets/forms/widgets.py (Diff revision 1)
Show all issues
```
Missing a trailing period.
```

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.

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

Show all issues

Same note about deprecations here.

Change Summary:


Removed an unused import
Added unit tests for null values in ListEditField and ListEditDictionaryField
Added unit tests for when the ListEditWidget is passed values without being rendered
Changed deprecation warnings to use the RemovedInDjblets40Warning class
Switched to using elif to avoid some checks
Avoided Pythonyms in use_legacy_behavior

Commits:

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

Diff:

Revision 2 (+176 -38)

Show changes

	djblets/forms/fields.py
	djblets/forms/widgets.py
	djblets/forms/tests/test_list_edit_dictionary_field.py
	djblets/forms/tests/test_list_edit_field.py
	djblets/forms/tests/test_list_edit_widget.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

Show all issues

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

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

Show all issues

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

Change Summary:

Included "Djblets" in the deprecation warning.

Commits:

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

Diff:

Revision 3 (+193 -188)

Show changes

	djblets/datagrid/grids.py
	djblets/datagrid/tests.py
	djblets/forms/fields.py
	djblets/forms/widgets.py
	djblets/forms/tests/test_list_edit_dictionary_field.py
	djblets/forms/tests/test_list_edit_field.py
	djblets/forms/tests/test_list_edit_widget.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

rebased with release-3.x

Commits:

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

Diff:

Revision 4 (+176 -38)

Show changes

	djblets/forms/fields.py
	djblets/forms/widgets.py
	djblets/forms/tests/test_list_edit_dictionary_field.py
	djblets/forms/tests/test_list_edit_field.py
	djblets/forms/tests/test_list_edit_widget.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.x (4555e46)