Skip to content

Fixes #21763: Replace M2M selection field with separate add/remove fields#21778

Merged
bctiemann merged 6 commits intomainfrom
21763-m2m-form-fields
Mar 30, 2026
Merged

Fixes #21763: Replace M2M selection field with separate add/remove fields#21778
bctiemann merged 6 commits intomainfrom
21763-m2m-form-fields

Conversation

@jeremystretch
Copy link
Copy Markdown
Member

@jeremystretch jeremystretch commented Mar 27, 2026

Fixes: #21763

  • Introduce M2MAddRemoveFields for conditional rendering M2M form fields as separate add/remove fields when an object has many (100+) assignments
  • Employ M2MAddRemoveFields for ASN relations in the site and provider edit forms
  • Update NetBoxModelForm to support the add/remove logic
  • Add tests

@jeremystretch
Copy link
Copy Markdown
Member Author

@claude /review Check that this PR provides a solution for bug #21763 and satisfies all of the "todo" criteria listed in the P description

@claude
Copy link
Copy Markdown

claude bot commented Mar 30, 2026

Claude finished @jeremystretch's task in 1m 41s —— View job


PR Review: Fixes #21763 - Replace M2M selection field with separate add/remove fields

Tasks:

  • Gather context and understand the request
  • Read changed files and analyze implementation
  • Check TODO criteria satisfaction
  • Post review

Overview

This PR addresses issue #21763 by introducing a new M2MAddRemoveFields rendering primitive that switches M2M form fields between two modes based on the number of existing assignments. When an object has ≥100 assigned M2M items (the THRESHOLD), separate "Add" and "Remove" fields are shown instead of a single potentially massive multi-select — avoiding browser crashes/slowdowns.


TODO Criteria Verification

All three criteria from the PR description are satisfied:

✅ Limit "add" field to objects not already assigned
In both ProviderForm.__init__() and SiteForm.__init__(), the add_asns field filters out already-assigned items using NetBox's negation filter syntax:

  • circuits/forms/model_forms.py:61add_query_param('provider_id__n', self.instance.pk)
  • dcim/forms/model_forms.py:197add_query_param('site_id__n', self.instance.pk)

Symmetrically, remove_asns is restricted to only currently-assigned items via the positive filter (provider_id=pk / site_id=pk).

✅ Render a single selection field if <100 objects assigned
M2MAddRemoveFields.THRESHOLD = 100 controls the cutoff. New objects (no pk) and existing objects with fewer than 100 assignments always use the simple mode. This matches the spec.

✅ Display total number of objects currently assigned
The count is surfaced via help_text on the remove field:

  • circuits/forms/model_forms.py:64_("{count} ASNs currently assigned").format(count=count)
  • dcim/forms/model_forms.py:200 — same pattern

Code Quality Notes

Architecture is clean and extensible
The M2MAddRemoveFields class in rendering.py is well-documented with a clear contract. The template tag handler in form_helpers.py:83-96 correctly dispatches between simple and add/remove rendering modes.

_post_clean scope expansion is sound
The original code only iterated _meta.local_many_to_many. The new version also handles ManyToManyRel (reverse M2M relationships), which is needed because asns on Site/Provider appears to be a reverse relation. The condition guards (if name in self.cleaned_data / elif f'add_{name}' in ...) ensure fields not present in the form are skipped. No regression risk.

_save_m2m override is correct
Since asns was removed from Meta.fields in both forms, Django's default _save_m2m() would skip it. The override correctly saves M2M fields not listed in Meta.fields without double-saving those that are listed (if not meta_fields or field_name not in meta_fields).

Minor observations (non-blocking):

  1. Duplicated __init__ logic: ProviderForm and SiteForm have near-identical __init__ implementations. A reusable mixin (e.g., M2MAddRemoveMixin) could eliminate duplication for future forms adopting this pattern — but given there are only two instances currently, this is fine to defer.

  2. Count display placement: The count is in help_text on the remove field, which places it below the remove widget. This is functional but somewhat indirect — users looking for the total might expect it near the top. Low-priority UX consideration.

  3. No tests included: Per CLAUDE.md, PRs must include tests for new functionality. There are no test changes in this PR. Coverage for _post_clean add/remove logic and the threshold switching behavior would be valuable, though this may be addressed in follow-up work.


Verdict

The PR correctly implements all three TODO criteria and the approach is architecturally sound. The one notable gap is the absence of tests for the new logic.

@jeremystretch jeremystretch marked this pull request as ready for review March 30, 2026 14:28
@jeremystretch jeremystretch requested review from a team and bctiemann and removed request for a team March 30, 2026 14:28
@bctiemann bctiemann merged commit e54ed87 into main Mar 30, 2026
11 checks passed
@pheus pheus deleted the 21763-m2m-form-fields branch March 30, 2026 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Site edit page fails to load with a very large number of ASNs assigned

2 participants