Summary

Switch away from django-multiselectfield.

Review Request #13561 — Created Feb. 20, 2024 and submitted March 5, 2024, 8:19 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

The django-multiselectfield package hasn't been maintained in a few

years, and it doesn't support Django 4.x+. There's a third-party fork

which has had some updates, but no releases that we can consume.
django-multiselectfield itself isn't very big, and of what it offers, we

don't actually use very much. A new field has been added to Djblets to

handle the list serialization/deserialization for comma-separated

values. This change switches us over to that.
The major difference here is that we have to instantiate the form

field ourselves. While the old MultiSelectField had its own validation

for the database side, we can rely on the form's MultipleChoiceField

validation.
This also manually sets a max_length on the field. The MultiSelectField

would compute the max_length based on the provided choices, but that

meant that any change to the choices would need an evolution.

Testing Done


Ran unit tests.
Ran the new evolution.
Tested the WebHookTarget admin form and verified that everything

  worked as expected.

Commits

Summary	ID
Switch away from django-multiselectfield. The django-multiselectfield package hasn't been maintained in a few years, and it doesn't support Django 4.x+. There's a third-party fork which has had some updates, but no releases that we can consume. django-multiselectfield itself isn't very big, and of what it offers, we don't actually use very much. A new field has been added to Djblets to handle the list serialization/deserialization for comma-separated values. This change switches us over to that. The major difference here is that we have to instantiate the form field ourselves. While the old MultiSelectField had its own validation for the database side, we can rely on the form's MultipleChoiceField validation. This also manually sets a max_length on the field. The MultiSelectField would compute the max_length based on the provided choices, but that meant that any change to the choices would need an evolution. Testing Done: - Ran unit tests. - Ran the new evolution. - Tested the WebHookTarget admin form and verified that everything worked as expected.	fec20fcde4a23e94e5024985824d5f36b314951b

Summary

Switch away from django-multiselectfield.

The django-multiselectfield package hasn't been maintained in a few years, and it doesn't support Django 4.x+. There's a third-party fork which has had some updates, but no releases that we can consume. django-multiselectfield itself isn't very big, and of what it offers, we don't actually use very much. A new field has been added to Djblets to handle the list serialization/deserialization for comma-separated values. This change switches us over to that. The major difference here is that we have to instantiate the form field ourselves. While the old MultiSelectField had its own validation for the database side, we can rely on the form's MultipleChoiceField validation. This also manually sets a max_length on the field. The MultiSelectField would compute the max_length based on the provided choices, but that meant that any change to the choices would need an evolution. Testing Done: - Ran unit tests. - Ran the new evolution. - Tested the WebHookTarget admin form and verified that everything worked as expected.

fec20fcde4a23e94e5024985824d5f36b314951b

Issues

Description	From	Last Updated
Change looks good, and I'm glad to get rid of this. The only thing that makes me a tad uncomfortable …	chipx86	Feb. 27, 2024, 8:42 a.m.
Include a "Version Added" here.	maubin	Feb. 26, 2024, 9:08 a.m.
Can you add , optional while you're here.	maubin	Feb. 26, 2024, 9:09 a.m.
Can you add , optional while you're here.	maubin	Feb. 26, 2024, 9:09 a.m.
Missing the description.	chipx86	Feb. 27, 2024, 8:26 a.m.

flake8 passed.

JSHint passed.

Change Summary:

Mark form field as not required.

Commits:

	Summary	ID
	Switch away from django-multiselectfield. The django-multiselectfield package hasn't been maintained in a few years, and it doesn't support Django 4.x+. There's a third-party fork which has had some updates, but no releases that we can consume. django-multiselectfield itself isn't very big, and of what it offers, we don't actually use very much. A new field has been added to Djblets to handle the list serialization/deserialization for comma-separated values. This change switches us over to that. The major differences here are that we have to instantiate the form field ourselves, and the new Djblets field doesn't actually have validation that the entries in the list are part of the choices. The validation isn't important to us because the checkbox widget that Django provides does the right thing for the typical case, and our notifications backend doesn't actually care if there's unknown items in the events list. This also manually sets a max_length on the field. The MultiSelectField would compute the max_length based on the provided choices, but that meant that any change to the choices would need an evolution. Testing Done: - Ran unit tests. - Ran the new evolution. - Tested the WebHookTarget admin form and verified that everything worked as expected.	09e4dec04d7538fb02fbd38d6c4c91cf155a6000
	Switch away from django-multiselectfield. The django-multiselectfield package hasn't been maintained in a few years, and it doesn't support Django 4.x+. There's a third-party fork which has had some updates, but no releases that we can consume. django-multiselectfield itself isn't very big, and of what it offers, we don't actually use very much. A new field has been added to Djblets to handle the list serialization/deserialization for comma-separated values. This change switches us over to that. The major differences here are that we have to instantiate the form field ourselves, and the new Djblets field doesn't actually have validation that the entries in the list are part of the choices. The validation isn't important to us because the checkbox widget that Django provides does the right thing for the typical case, and our notifications backend doesn't actually care if there's unknown items in the events list. This also manually sets a max_length on the field. The MultiSelectField would compute the max_length based on the provided choices, but that meant that any change to the choices would need an evolution. Testing Done: - Ran unit tests. - Ran the new evolution. - Tested the WebHookTarget admin form and verified that everything worked as expected.	96bbd6bcd5cbb0d18cce85ace3133c9cd4725ff4

Diff:

Revision 2 (+140 -26)

Show changes

	reviewboard/dependencies.py
	reviewboard/notifications/forms.py
	reviewboard/notifications/managers.py
	reviewboard/notifications/models.py
	reviewboard/notifications/evolutions/__init__.py
	reviewboard/notifications/evolutions/webhooktarget_events_charfield.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/notifications/evolutions/webhooktarget_events_charfield.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Include a "Version Added" here.
```
reviewboard/notifications/managers.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you add , optional while you're here.
```
reviewboard/notifications/managers.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you add , optional while you're here.
```

Commits:

	Summary	ID
	Switch away from django-multiselectfield. The django-multiselectfield package hasn't been maintained in a few years, and it doesn't support Django 4.x+. There's a third-party fork which has had some updates, but no releases that we can consume. django-multiselectfield itself isn't very big, and of what it offers, we don't actually use very much. A new field has been added to Djblets to handle the list serialization/deserialization for comma-separated values. This change switches us over to that. The major differences here are that we have to instantiate the form field ourselves, and the new Djblets field doesn't actually have validation that the entries in the list are part of the choices. The validation isn't important to us because the checkbox widget that Django provides does the right thing for the typical case, and our notifications backend doesn't actually care if there's unknown items in the events list. This also manually sets a max_length on the field. The MultiSelectField would compute the max_length based on the provided choices, but that meant that any change to the choices would need an evolution. Testing Done: - Ran unit tests. - Ran the new evolution. - Tested the WebHookTarget admin form and verified that everything worked as expected.	96bbd6bcd5cbb0d18cce85ace3133c9cd4725ff4
	Switch away from django-multiselectfield. The django-multiselectfield package hasn't been maintained in a few years, and it doesn't support Django 4.x+. There's a third-party fork which has had some updates, but no releases that we can consume. django-multiselectfield itself isn't very big, and of what it offers, we don't actually use very much. A new field has been added to Djblets to handle the list serialization/deserialization for comma-separated values. This change switches us over to that. The major differences here are that we have to instantiate the form field ourselves, and the new Djblets field doesn't actually have validation that the entries in the list are part of the choices. The validation isn't important to us because the checkbox widget that Django provides does the right thing for the typical case, and our notifications backend doesn't actually care if there's unknown items in the events list. This also manually sets a max_length on the field. The MultiSelectField would compute the max_length based on the provided choices, but that meant that any change to the choices would need an evolution. Testing Done: - Ran unit tests. - Ran the new evolution. - Tested the WebHookTarget admin form and verified that everything worked as expected.	0a92d9d7bf0b59e438f845e80d8535de84ac77c4

Branch:

-	master
+	release-7.x

Diff:

Revision 3 (+152 -30)

Show changes

	reviewboard/dependencies.py
	reviewboard/notifications/forms.py
	reviewboard/notifications/managers.py
	reviewboard/notifications/models.py
	reviewboard/notifications/evolutions/__init__.py
	reviewboard/notifications/evolutions/webhooktarget_events_charfield.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Change looks good, and I'm glad to get rid of this.
The only thing that makes me a tad uncomfortable is the removal of any validated input for events. I think the case where it'd be more likely to be hit is someone trying to use the API to create events, and screwing up. They wouldn't know, because we don't validate, and that'd be a regression from what we have today.
Could we attach a small validator function to this field that just does like:

if set(value.split(',')) - set(EVENT_CHOICES):
    raise ValidationError(...)

david Feb. 27, 2024, 8:46 a.m.

It turns out we actually already get validation from MultipleChoiceField on the form (which is also used from the API resource). I'll add another unit test.

reviewboard/notifications/managers.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Missing the description.
```

Description:

		The django-multiselectfield package hasn't been maintained in a few
		years, and it doesn't support Django 4.x+. There's a third-party fork
		which has had some updates, but no releases that we can consume.

		django-multiselectfield itself isn't very big, and of what it offers, we
		don't actually use very much. A new field has been added to Djblets to
		handle the list serialization/deserialization for comma-separated
		values. This change switches us over to that.

~		The major differences here are that we have to instantiate the form
~		field ourselves, and the new Djblets field doesn't actually have
~		validation that the entries in the list are part of the choices. The
~		validation isn't important to us because the checkbox widget that Django
	~	The major difference here is that we have to instantiate the form
	~	field ourselves. While the old MultiSelectField had its own validation
	~	for the database side, we can rely on the form's MultipleChoiceField
	~	validation.
-		provides does the right thing for the typical case, and our
-		notifications backend doesn't actually care if there's unknown items in
-		the events list.

		This also manually sets a max_length on the field. The MultiSelectField
		would compute the max_length based on the provided choices, but that
		meant that any change to the choices would need an evolution.

Commits:

	Summary	ID
	Switch away from django-multiselectfield. The django-multiselectfield package hasn't been maintained in a few years, and it doesn't support Django 4.x+. There's a third-party fork which has had some updates, but no releases that we can consume. django-multiselectfield itself isn't very big, and of what it offers, we don't actually use very much. A new field has been added to Djblets to handle the list serialization/deserialization for comma-separated values. This change switches us over to that. The major differences here are that we have to instantiate the form field ourselves, and the new Djblets field doesn't actually have validation that the entries in the list are part of the choices. The validation isn't important to us because the checkbox widget that Django provides does the right thing for the typical case, and our notifications backend doesn't actually care if there's unknown items in the events list. This also manually sets a max_length on the field. The MultiSelectField would compute the max_length based on the provided choices, but that meant that any change to the choices would need an evolution. Testing Done: - Ran unit tests. - Ran the new evolution. - Tested the WebHookTarget admin form and verified that everything worked as expected.	0a92d9d7bf0b59e438f845e80d8535de84ac77c4
	Switch away from django-multiselectfield. The django-multiselectfield package hasn't been maintained in a few years, and it doesn't support Django 4.x+. There's a third-party fork which has had some updates, but no releases that we can consume. django-multiselectfield itself isn't very big, and of what it offers, we don't actually use very much. A new field has been added to Djblets to handle the list serialization/deserialization for comma-separated values. This change switches us over to that. The major difference here is that we have to instantiate the form field ourselves. While the old MultiSelectField had its own validation for the database side, we can rely on the form's MultipleChoiceField validation. This also manually sets a max_length on the field. The MultiSelectField would compute the max_length based on the provided choices, but that meant that any change to the choices would need an evolution. Testing Done: - Ran unit tests. - Ran the new evolution. - Tested the WebHookTarget admin form and verified that everything worked as expected.	fec20fcde4a23e94e5024985824d5f36b314951b

Diff:

Revision 4 (+246 -66)

Show changes

	reviewboard/dependencies.py
	reviewboard/notifications/forms.py
	reviewboard/notifications/managers.py
	reviewboard/notifications/models.py
	reviewboard/notifications/evolutions/__init__.py
	reviewboard/notifications/evolutions/webhooktarget_events_charfield.py
	reviewboard/notifications/tests/test_webhook_target_form.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (8c7eb33)