#323 Enable migrating branches in bulk

[arthanson]

Fixes: #323

Allows bulk selecting branches with "Migrate Selected" button:

[arthanson]

@claude /review

[claude]

Claude finished @arthanson's task in 1m 45s —— View job

---

Code Review: #323 Enable migrating branches in bulk

• Gather context and read changed files
• Analyze code quality and correctness
• Check test coverage
• Provide review feedback

---

Overview

This PR adds a "Migrate Selected" bulk action to the branch list view, allowing users to apply pending database migrations to multiple branches at once. The feature follows the existing bulk-action pattern (bulk edit, delete, import). The PR also bundles a simplification of the migration executor logic (_fake_for_branch removal) which is a separate, significant change.

---

Issues

1. Missing return_url in confirmation form ⚠️

netbox_branching/templates/netbox_branching/branch_bulk_migrate.html:50-52

The confirmation form only renders form.hidden_fields(), which outputs only the pk hidden inputs. The return_url context variable is used for the Cancel button but is not included as a hidden form input. When the user submits _confirm, GetReturnURLMixin.get_return_url() won't find return_url in request.POST and will fall back to the default URL.

In practice the fallback is the branch list, so this may appear to work, but it's fragile — if someone is on a filtered or paginated URL it won't be preserved. Fix: add <input type="hidden" name="return_url" value="{{ return_url }}"> to the confirmation form.

Fix this →

---

2. Inconsistent filtering between confirmation display and submission ⚠️

views.py:533-536 vs views.py:558-561

The confirmation page shows all branches with PENDING_MIGRATIONS status. But on actual submission, the additional check branch.can_migrate is applied. A branch that appears in the confirmation list could be silently skipped on submit if can_migrate returns False (e.g. a validator fails). The skipped count message doesn't explain why branches were skipped.

Either:

• Apply the same can_migrate filter at the confirmation stage so users see exactly what will run, or
• Show the reason in the skipped warning (e.g. include branch names)

---

3. Bundled behavioral regression risk: _fake_for_branch removal

netbox_branching/models/branches.py (large deletion)

The PR removes the _fake_for_branch logic and the per-migration executor.apply_migration() loop in favor of simple executor.migrate(targets, plan). This was added to fix GitHub issue #423 (RunSQL migrations inadvertently acting on the main schema via search_path). This is a significant behavioral change that:

• Is not mentioned in the PR description
• Removes 35+ lines of carefully considered migration safety code
• Could reintroduce issue Branches do not migrate to 4.5.2 #423

This change should be documented with reasoning for why #423 is no longer a concern, or split into a separate PR with clear justification.

---

Minor Observations

Redundant status check at submission (non-blocking)

views.py:535: branch.status == BranchStatusChoices.PENDING_MIGRATIONS and branch.can_migrate — since can_migrate runs validators that already gate on status, the explicit status check is somewhat redundant (though harmless as a defensive measure).

Test: test_permission_required could be more specific

tests/test_views.py:132: self.assertNotEqual(response.status_code, 200) — this passes for a redirect (302) as well as a 403. It would be clearer to assert self.assertEqual(response.status_code, 403) or follow the redirect and check for a login redirect.

test_confirm_enqueues_jobs_and_redirects mock target

tests/test_views.py:106: The mock patches netbox_branching.views.MigrateBranchJob.enqueue. This is correct since the views module imports MigrateBranchJob. Good practice.

No test for GET request behavior

There's no test verifying that a GET request to branch_bulk_migrate redirects properly (only POST paths are tested).

---

Summary

The bulk migrate feature itself is well-structured, follows existing patterns, and includes reasonable test coverage. Two actionable fixes are recommended before merge: the missing return_url in the template form (which would cause subtle redirect bugs), and documenting/reconsidering the _fake_for_branch removal. The inconsistent can_migrate filtering between confirmation and execution is a UX issue worth addressing.
| Branch: 323-migrate-bulk

[arthanson]

re claude review, 1 and 2 addressed. 3 is a hallucination? it is looking at another PR.