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	ID
Allow ListEditWidget to properly handle data when the widget is not rendered	5de9d3be7776ebacaa611031482d4c19fa406b8a

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.

The issue has been resolved. Show all issues

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

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

The issue has been resolved. Show all issues

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

djblets/forms/fields.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same note here.
```

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

The issue has been resolved. 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)
The issue has been resolved. Show all issues
```
Missing a trailing period.
```

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

The issue has been resolved. 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)
The issue has been resolved. 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	ID
	Allow ListEditWidget to properly handle data when the widget is not rendered	050b4f12e792b2074ae4fb6457c7fbcd9ece839c
	Allow ListEditWidget to properly handle data when the widget is not rendered	8d14fd4636c0f98a558c3caed782771cae6a9e2d

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)
The issue has been resolved. Show all issues
```
Can you update this to specifically say "Djblets 4.0"?
```
djblets/forms/widgets.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you update this to specifically say "Djblets 4.0"?
```

Change Summary:

Included "Djblets" in the deprecation warning.

Commits:

	Summary	ID
	Allow ListEditWidget to properly handle data when the widget is not rendered	8d14fd4636c0f98a558c3caed782771cae6a9e2d
	Allow ListEditWidget to properly handle data when the widget is not rendered	0ef07d1c5106df8f0cd8a7b9e0646e6e018a4f45

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	ID
	Allow ListEditWidget to properly handle data when the widget is not rendered	0ef07d1c5106df8f0cd8a7b9e0646e6e018a4f45
	Allow ListEditWidget to properly handle data when the widget is not rendered	5de9d3be7776ebacaa611031482d4c19fa406b8a

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)