Switch away from django-multiselectfield.

Review Request #13561 — Created Feb. 20, 2024 and submitted

Information

Review Board
release-7.x

Reviewers

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.

  • Ran unit tests.
  • Ran the new evolution.
  • Tested the WebHookTarget admin form and verified that everything
    worked as expected.
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
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 …

chipx86chipx86

Include a "Version Added" here.

maubinmaubin

Can you add , optional while you're here.

maubinmaubin

Can you add , optional while you're here.

maubinmaubin

Missing the description.

chipx86chipx86
david
maubin
  1. 
      
  2. Show all issues

    Include a "Version Added" here.

  3. reviewboard/notifications/managers.py (Diff revision 2)
     
     
    Show all issues

    Can you add , optional while you're here.

  4. reviewboard/notifications/managers.py (Diff revision 2)
     
     
    Show all issues

    Can you add , optional while you're here.

  5. 
      
david
chipx86
  1. 
      
  2. 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(...)
    
    1. 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.

  3. reviewboard/notifications/managers.py (Diff revision 3)
     
     
     
     
    Show all issues

    Missing the description.

  4. 
      
david
maubin
  1. Ship It!
  2. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-7.x (8c7eb33)