diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index a786486..f0fa06b 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -90,7 +90,11 @@ jobs: - name: Install netbox-branching and the plugin run: | pip install 'netboxlabs-netbox-branching~=1.1.3' - pip install ./netbox-change-control + # Editable, so the tests run from the checkout. Several of them read the pages in + # docs/ and compare them against the code, and a plain install copies the package + # into site-packages without the documentation beside it, where those tests can + # only fail. The release workflow builds and checks the real distribution. + pip install -e ./netbox-change-control - name: Configure NetBox run: | diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bc79c6..4529d52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,11 +2,54 @@ ## Unreleased +## 0.3.0 - 2026-09-01 + +### Added + +- **Return to draft.** A submitted change request can be pulled back out of review, by its author or anybody holding `change_changerequest`, from the page or `POST /change-requests/{id}/return-to-draft/`. The reviews are kept, and resubmitting picks up where it left off. Allowed from Approved too, so an author who spots a problem after approval need not race the merge; it only ever closes the merge gate. +- **Abandon** and **Reopen**, each behind its own permission (`abandon_changerequest`, `reopen_changerequest`), on the page and as `POST /change-requests/{id}/abandon/` and `/reopen/`. With **Submit for review** and **Return to draft** these are the four transitions a person makes; everything else is derived. `submit` is a REST action too, so an integration can drive the whole lifecycle now that `status` is read-only. [The lifecycle diagram](docs/change-requests.md#the-lifecycle) shows the rest. +- **Draft is a state the author holds** rather than one the evaluation computes. A review submitted against a draft moves nothing, and a request pulled back stays pulled back; without this, **Return to draft** would be undone by the next signal. Submitting is what leaves draft, matches the policies and notifies the reviewers. +- **The branch page shows its change request**: status, reference, what is outstanding and who can clear it, with a link. A branch with no change request is told it cannot merge until one is opened, with a button to open it, which is the case netbox-branching's merge form could only refuse. +- `branch_page_placement` decides where that panel sits: `right_page` for a card in the right-hand column, which is the default, `alerts` for a band across the top, both for both, `[]` for neither. Both render the same wording, so they cannot drift apart. +- **Global search** over change requests, policies, rules, reviews, checks and comments; the plugin registered no index at all before. A request is found by its reference first, so a ticket number finds the change it spawned, and one whose branch has been deleted is still found by the branch's name. +- A change comment can be **edited and deleted from the interface**. There was no view for either, so a typo in a review comment was permanent. Editing is restricted to the author, as a review is. + +### Fixed + +- **The policies governing a change request follow the branch.** They were matched at submission and never again, so an author could open a request on a low-risk branch, collect the light approval that attracted, then add the real change to the same branch and merge it under that approval. They are re-matched whenever the branch touches something new, and the merge gate re-matches for itself. +- **A change request's status can no longer be set by hand.** It is derived, but it was writable on the bulk edit form and over the REST API, and Completed is terminal, so one bulk edit blocked a branch from merging for good. It is now read-only on the API and gone from the form. +- **Submit for review** requires `change_changerequest`. It checked nothing, so any signed-in user could push somebody else's draft into review, attaching its policies and announcing the event. +- **The REST API no longer lets a caller forge the author of a change comment.** `author` was writable, so any token holding `add_changecomment` could fake a colleague's sign-off in the discussion a reviewer reads before approving. It is now always the caller, matching `reviewer` on a review. +- **Automatic merge no longer queues two jobs for one branch.** Becoming mergeable can be reached by two routes in one write, and the second arrival still saw Approved because the first had only queued. The second job then failed with "not ready to merge" on a change that had gone through. Auto-merge refuses to queue while a merge for that branch is pending, scheduled or running. +- **Check results reach the changelog.** They were written with a queryset update, which fires no `post_save`, so a required check going from failed to passed left no entry. A re-run finding the same answer still writes nothing. +- Every built-in check reports **skipped** on a change request whose branch is gone. `threads-resolved` failed instead, because threads outlive the branch, so a change nobody can merge was marked blocked for ever. +- A reply to a reply is flattened into its thread on every path. The flattening ran in `clean()`, which the REST serializer discards, so a reply posted over the API was stored as a grandchild and rendered nowhere at all. +- A change comment can no longer name a change in another request's branch, which was invisible on the tab it belonged to while blocking a request it did not describe, and one naming a change that no longer exists reports a validation error rather than a server error. +- Renaming a branch updates the name stored on its change request, which is what the branch filter and the search read, so a renamed branch could not be found by the name shown on its own page. +- List pages no longer offer **Import**, and reviews and merge checks no longer offer **Add** or **Edit Selected**. The plugin routes none of those and NetBox rendered them with `None` as their target, so clicking one gave a 404. +- Four badges used `text-bg-grey`, which NetBox does not define, so they rendered with no background. A test now checks every badge colour against the set NetBox ships. +- The review form opens on the reviewer's standing decision. It reset it to **Approve**, so a reviewer amending a **Request changes** was silently offered an approval. +- A review's own page says whether it has gone stale, and the Changes tab formats timestamps the way the rest of the interface does. +- The second line of the reconciled-conflicts and change-window alerts sits under the sentence it explains rather than beside it. An alert lays its direct children out in a row, so the detail was rendered as a column of its own. + ### Changed -- The **Conflicts** column on the change request list now matches netbox-branching: a red octagon when there are conflicts, a dash when there are none. -- The README now warns that the plugin is below 1.0 and that models, settings and the REST API can still change. -- Improved the docs. +- **The change request list no longer gets slower as it gets longer.** **Conflicts** and **Ready to merge** were computed per row, about five hundred queries for a page of fifty. Both now read a cached field refreshed on the events that move them, the split the plugin already makes for `status`: a cache for display and filtering, never for a decision. The merge gate and the change request page still recompute in full. Both columns are now sortable and filterable, and **Reviews** no longer costs a query per row. +- **One user action refreshes a change request once**, not once per policy. A request governed by three policies recomputed its status and ran every check four times for an identical answer. Nothing is deferred past the commit. +- **A rule names its groups rather than expanding them into members.** A group of fifteen printed fifteen usernames on every rule it satisfied, burying the approval counts that are the point of the panel, and went stale as people joined and left. An empty group is still called out by name, because such a rule can never be satisfied. +- All four lifecycle actions sit on the control bar beside Edit and Delete. Submit and Abandon sat in the Applied policies card, where they read as something to do with the policies; that card now carries only policies. +- **Reopening an abandoned request returns it to Draft** rather than straight to review: its reviews may be stale and its policies may have moved while it was set aside. +- The **Conflicts** column matches netbox-branching: a red octagon when there are conflicts, a dash when there are none. +- Packaging: the build no longer pulls `setuptools-scm`, which it never read because the version is written by hand, and the package declares `Development Status :: 4 - Beta`, which is what the README says in prose. +- CI installs the plugin editable, so the tests that compare a documentation page against the code can find that page. A plain install copies the package into `site-packages` with no `docs/` beside it, and those tests could only error there. +- [Permissions](docs/permissions.md) is a complete reference, with a test comparing it against the models, and there is an [administration guide](docs/admin-guide.md). The README warns that the plugin is below 1.0. `docs/api.md`, `docs/design.md` and `docs/checks.md` are corrected: a wrong policy filter, a wrong value for requesting changes, a permission that does not exist, and two of the moments checks run. + +### Removed + +- **The policy binding table defines no permissions.** Django creates four for every model and no view reads any of them, so granting `delete_changerequestpolicy` to detach a policy by hand only got the binding back at the next re-match. Change the policy's scope instead. +- `ChangeRequestPolicy.matched` and `created`, and `Policy.applies_to_all_object_types`, none of which anything read. `matched` was always true, so the filter reading it was a no-op and the **Auto** badge it drove appeared on every policy. +- The `lock_matched_policies` setting, documented but read nowhere, so turning it off changed nothing. Nothing can attach or detach a policy by hand, which is what it claimed to control. +- `ReviewBulkEditForm`, used by no view, and the `INTERVAL_*` re-exports in `jobs.py`, referenced by nothing. ## 0.2.0 - 2026-08-27 diff --git a/README.md b/README.md index c821710..f6bbc75 100644 --- a/README.md +++ b/README.md @@ -72,7 +72,8 @@ Full documentation is in [`docs/`](docs/index.md). | [Merging, windows and auto-merge](docs/merging.md) | Change windows and automatic merging. | | [Protecting main](docs/protect-main.md) | Requiring a branch, optionally for part of NetBox only. | | [Automatic behaviours](docs/automation.md) | Stale reviews, reevaluation, notifications. | -| [Permissions](docs/permissions.md) | What each role needs. | +| [Administration guide](docs/admin-guide.md) | Roles, the permission matrix, building policies, troubleshooting. | +| [Permissions](docs/permissions.md) | The short reference for every permission name. | | [REST API](docs/api.md) | Every endpoint. | | [Extending this plugin](docs/extending.md) | How another plugin adds content and checks. | | [Design](docs/design.md) | Why it is built this way. | @@ -83,7 +84,7 @@ Full documentation is in [`docs/`](docs/index.md). |---|---| | Policies containing rules with a minimum approval count | Done | | Rules naming reviewer groups and individual reviewers | Done | -| Policies attached automatically, scope-matched from the branch contents and locked against the author | Done | +| Policies attached automatically, scope-matched from the branch contents, following it as it changes, and not selectable by the author | Done | | Policy conditions, narrowing a policy on the values of the changed objects | Done | | Change requests with status and priority | Done | | Reviews with approve, request changes, and comment | Done | diff --git a/docs/admin-guide.md b/docs/admin-guide.md new file mode 100644 index 0000000..5df118d --- /dev/null +++ b/docs/admin-guide.md @@ -0,0 +1,304 @@ +# Administration guide + +How to set this plugin up for a team: who gets which permissions, how to build the policies that govern your changes, and how to check the whole thing works before you rely on it. + +If you only want the list of permission names, [Permissions](permissions.md) is the short reference. This page is the guide. + +## Prerequisites + +Before granting anything: + +1. [netbox-branching](https://github.com/netboxlabs/netbox-branching) is installed and working, and your team already knows how to create a branch and make changes inside it. +2. This plugin is installed, `exempt_models` is set, and the migrations have run. See [Installation and configuration](installation.md). +3. You are comfortable with NetBox's own permission system. This plugin adds no permission machinery of its own; it uses NetBox object permissions exactly as every other model does. + +## Permissions + +### How permissions work here + +This plugin does not invent a permission model. It declares the standard `view`, `add`, `change` and `delete` actions on each of its models, plus four custom actions, and NetBox enforces them. + +That has one consequence worth stating plainly, because it decides how you should grant them: + +> [!IMPORTANT] +> A NetBox object permission applies to **every object of that type**. Granting `netbox_change_control.delete_review` lets that user delete **any** review, not only their own. This is standard NetBox behaviour, not a quirk of this plugin. If you want a narrower grant, add a **constraint** to the permission. See [narrowing a permission with a constraint](#narrowing-a-permission-with-a-constraint). + +Deleting somebody's review changes the outcome of the gate: removing a **Request changes** review removes the rejection, and the request moves off Rejected. So `delete_review` is a privileged grant. Treat it the way you would treat the ability to close a ticket on someone else's behalf. + +> [!NOTE] +> Editing a review is different, and deliberately so. A reviewer may edit only their own review whatever permissions they hold; a superuser may edit any. A review is one person's statement about one change, so reassigning or rewriting somebody else's would forge their position. Only the delete action follows the plain NetBox model. + +### Permission matrix + +Three roles cover most deployments. Build them as NetBox groups and assign object permissions to the group rather than to individual users. + +- **Contributor** opens branches and change requests, and comments on changes. Cannot approve. +- **Reviewer** everything a contributor can do, plus submitting reviews. +- **Administrator** manages policies and checks, and holds the two exemptions. + +#### Branch permissions + +These belong to netbox-branching, not to this plugin, but a change request is worthless without them. + +| Permission | Contributor | Reviewer | Administrator | +|---|:---:|:---:|:---:| +| `netbox_branching.view_branch` | yes | yes | yes | +| `netbox_branching.add_branch` | yes | yes | yes | +| `netbox_branching.change_branch` | yes | yes | yes | +| `netbox_branching.sync_branch` | yes | yes | yes | +| `netbox_branching.merge_branch` | no | no | yes | +| `netbox_branching.view_changediff` | yes | yes | yes | + +> [!NOTE] +> NetBox reads a permission name as `._`, splitting on the **last** underscore. So branching's `merge` action on the `Branch` model is written `netbox_branching.merge_branch`, and its `sync` action is `netbox_branching.sync_branch`. The action you tick in the permission form is `merge`, not `merge_branch`. + +`view_changediff` is what gates the **Changes** tab. Without it a reviewer cannot see what they are being asked to approve. + +#### Change request permissions + +| Permission | Contributor | Reviewer | Administrator | +|---|:---:|:---:|:---:| +| `netbox_change_control.view_changerequest` | yes | yes | yes | +| `netbox_change_control.add_changerequest` | yes | yes | yes | +| `netbox_change_control.change_changerequest` | yes | yes | yes | +| `netbox_change_control.delete_changerequest` | no | no | yes | +| `netbox_change_control.override_window_changerequest` | no | no | optional | +| `netbox_change_control.abandon_changerequest` | no | no | yes | +| `netbox_change_control.reopen_changerequest` | no | no | yes | + +Deleting a change request destroys the record of who approved what. Keep it with the administrators. + +`status` is not editable, by anybody. It is derived from the policy evaluation, so it is read-only on the REST API and absent from the bulk edit form. **Abandon** and **Reopen** are the two transitions a person makes by hand, and each has its own permission and its own button on the change request page. Grant them to whoever is allowed to call off a change. Contributors do not need them to give up on their own work; they can delete a draft they own, or ask an administrator. + +#### Review permissions + +| Permission | Contributor | Reviewer | Administrator | +|---|:---:|:---:|:---:| +| `netbox_change_control.view_review` | yes | yes | yes | +| `netbox_change_control.add_review` | no | yes | yes | +| `netbox_change_control.change_review` | no | yes | yes | +| `netbox_change_control.delete_review` | no | no | yes | + +`add_review` is the one that separates a reviewer from a contributor. A user without it sees the reason in place of the review form rather than a form that fails on submit. + +`change_review` only ever lets a person edit their **own** review, whoever holds it. + +`delete_review` is model-wide. See the warning above, and consider a constraint. + +#### Comment permissions + +| Permission | Contributor | Reviewer | Administrator | +|---|:---:|:---:|:---:| +| `netbox_change_control.view_changecomment` | yes | yes | yes | +| `netbox_change_control.add_changecomment` | yes | yes | yes | +| `netbox_change_control.change_changecomment` | yes | yes | yes | +| `netbox_change_control.delete_changecomment` | no | no | yes | + +`change_changecomment` is what allows resolving and reopening a thread. If you use the `threads-resolved` check, everybody who might need to unblock a merge needs it. + +#### Check permissions + +| Permission | Contributor | Reviewer | Administrator | +|---|:---:|:---:|:---:| +| `netbox_change_control.view_mergecheck` | yes | yes | yes | +| `netbox_change_control.add_mergecheck` | no | no | yes | +| `netbox_change_control.change_mergecheck` | no | optional | yes | +| `netbox_change_control.delete_mergecheck` | no | no | yes | + +`change_mergecheck` does two things: it shows the **Re-run checks** button, and it is what a CI token needs to report a result over the REST API. Give the token its own user and its own permission, constrained if you can, rather than reusing a person's. + +> [!TIP] +> A reporting token cannot weaken the gate even with this permission. `required` is read-only on the API, and the gate reads requiredness from the configuration and the policies rather than from the stored row. + +#### Policy permissions + +| Permission | Contributor | Reviewer | Administrator | +|---|:---:|:---:|:---:| +| `netbox_change_control.view_policy` | yes | yes | yes | +| `netbox_change_control.view_policyrule` | yes | yes | yes | +| `netbox_change_control.add_policy` | no | no | yes | +| `netbox_change_control.change_policy` | no | no | yes | +| `netbox_change_control.delete_policy` | no | no | yes | +| `netbox_change_control.add_policyrule` | no | no | yes | +| `netbox_change_control.change_policyrule` | no | no | yes | +| `netbox_change_control.delete_policyrule` | no | no | yes | +| `netbox_change_control.bypass_policy` | no | no | optional | + +Give everybody `view_policy` and `view_policyrule`. A reviewer who cannot read the policy cannot tell why they were asked, and the approval panel names rules the reader may not be able to open. + +Editing a policy changes who must approve every open change request bound to it. That is the whole gate, so keep it with the administrators. + +#### The two exemptions + +Both are custom actions, and both are optional. + +| Permission | Grants | Grant it to | +|---|---|---| +| `netbox_change_control.bypass_policy` | Write outside a branch while `protect_main` is enabled. | Automation accounts, and the people who run an incident. | +| `netbox_change_control.override_window_changerequest` | Merge a change request outside its change window. | Whoever is allowed to break a change freeze. | + +To grant them, go to **Administration > Permissions > Add**, and enter the action in the *additional actions* field rather than ticking view, add, change or delete: + +| Exemption | Object type | Action to enter | +|---|---|---| +| Write directly to main | Change Control > Policy | `bypass` | +| Merge outside the change window | Change Control > Change Request | `override_window` | + +> [!IMPORTANT] +> The trailing part of a custom permission name has to be a real model name, because NetBox splits on the last underscore. That is why the bypass lives on `Policy` and the window override on `ChangeRequest`, rather than on names that would read better. + +> [!NOTE] +> Superusers hold every permission, so they are exempt from `protect_main` and from every change window without being granted anything. + +### Setting up groups + +1. Go to **Administration > Groups** and create `Change Contributors`, `Change Reviewers` and `Change Administrators`. +2. Go to **Administration > Permissions > Add**. Give the permission a name, tick the actions, choose the object types, and assign it to a group. +3. Group one permission per area rather than making one giant permission. `Change control: reviews`, `Change control: policies` and so on are far easier to audit later. +4. Add users to groups. Do not assign object permissions directly to users except for service accounts. + +The reviewer groups you name in a **policy rule** are the same NetBox groups. A user must be in the group **and** hold `add_review` for their approval to count: the group decides eligibility, the permission decides whether they can act at all. + +### Narrowing a permission with a constraint + +This is a NetBox feature, not something this plugin adds. Every object permission can carry a **constraint**, a JSON queryset filter applied to the objects it covers, and the token `$user` in one resolves to the signed-in user. NetBox uses it itself: bookmarks and notifications are constrained with `{"user": "$user"}` in its own settings. + +What is specific to this plugin is which field name to use for which model. The table below names the real field on each, so the filter matches rather than silently matching nothing. + +To let reviewers delete their own reviews and nobody else's, create a permission with the `delete` action on **Change Control > Review** and this constraint: + +```json +{"reviewer": "$user"} +``` + +`$user` resolves to the signed-in user. The same pattern works elsewhere: + +| Goal | Object type | Constraint | +|---|---|---| +| Delete only my own reviews | Review | `{"reviewer": "$user"}` | +| Delete only my own comments | Change Comment | `{"author": "$user"}` | +| Manage only my own change requests | Change Request | `{"requester": "$user"}` | +| Report results for one check only | Merge Check | `{"name": "ci-pipeline"}` | + +> [!TIP] +> The last one is the right shape for a CI token. It can report the check it owns and touch nothing else. + +## Configuring policies + +### Creating a policy + +**Change Control > Policies > Add**. + +| Field | What to put in it | +|---|---| +| Name | What it governs, in your team's language. `Circuit changes`, not `Policy 3`. | +| Enabled | Leave ticked. A disabled policy is never attached to anything. | +| Weight | Display and evaluation order. Lower is listed first. Leave at 1000 unless you care. | +| Object types | The types this policy governs. **Leave empty to match every branch**, which is how you build a baseline. | +| Conditions | Optional. Narrows on the values of the changed objects. See [Policy conditions](policy-conditions.md). | +| Condition state | Which side of the change the conditions read. The default reads both. | +| Registered checks | Opt-in checks to require wherever this policy applies. | +| Reported checks | Names your own systems report over the REST API. | + +Policies attach on their own, from the object types the branch touches, and they keep following the branch as it changes. The author cannot pick them and cannot remove them. See [policies attach automatically](policies.md#policies-attach-automatically). + +### Configuring policy rules + +A policy with no rules asks for nothing, so add at least one. **Change Control > Policy Rules > Add**. + +| Field | What to put in it | +|---|---| +| Policy | The policy this rule belongs to. | +| Name | What the requirement is. `Two engineers`, `One lead`. This name is shown to reviewers. | +| Minimum reviews | How many eligible people must approve. Zero means none; the checks become the only gate. | +| Reviewer groups | Members of any listed group may satisfy it. | +| Reviewers | Individually named users may satisfy it. | + +A user satisfies a rule if they are in **any** of its groups **or** are named on it. A policy is satisfied only when **every** one of its rules is. + +An approval counts only toward the rules the approver is eligible for. A lead approving does not advance a rule that asks for engineers, which is the property a plain approval counter gets wrong. + +### The pre-merge gate + +Once the plugin is installed, a branch cannot merge unless it carries an approved change request whose policies are still satisfied, every required check passes, and the change window is open. The button is hidden rather than failing after the click, because the gate is registered as a netbox-branching pre-action validator. + +Set `enforce_merge_gate` to `False` only to troubleshoot. It turns off the reason the plugin exists. + +## Policy configuration examples + +**Single approval, everywhere.** The baseline every deployment should start with. + +| Field | Value | +|---|---| +| Object types | *(empty)* | +| Registered checks | `has-changes`, `no-conflicts`, `not-stale` | +| Rule | `One engineer`, minimum reviews 1, group `Change Engineers` | + +**Two-person rule.** For anything that can take a site off the air. + +| Field | Value | +|---|---| +| Object types | `dcim.device`, `dcim.interface` | +| Rule | `Two engineers`, minimum reviews 2, group `Change Engineers` | + +**Senior approval on top.** Two rules on one policy, so both must be met. + +| Field | Value | +|---|---| +| Object types | `circuits.circuit`, `circuits.circuittermination` | +| Rule 1 | `One engineer`, minimum reviews 1, group `Change Engineers` | +| Rule 2 | `One lead`, minimum reviews 1, group `Change Leads` | + +**Live objects only.** The same policy, narrowed so a planned circuit stays a one-person change. + +| Field | Value | +|---|---| +| Object types | `circuits.circuit` | +| Conditions | `{"attr": "status", "value": "active"}` | +| Condition state | Either side of the change | + +**No human at all.** For scripted, low-risk work that must still pass the machine gate. + +| Field | Value | +|---|---| +| Object types | `ipam.prefix`, `ipam.ipaddress` | +| Registered checks | `has-changes`, `no-conflicts`, `not-stale` | +| Rule | `No approval required`, minimum reviews 0 | + +> [!WARNING] +> A zero rule removes the human requirement **of its own policy only**. If your baseline policy also matches the branch and asks for an engineer, the change still waits for that engineer. Scope the automatic policy so it is the only one matching, or narrow the baseline with conditions. + +## Testing the workflow + +Do this once, on a test instance, before you rely on any of it. + +1. Create the three groups and their permissions. +2. Create the baseline policy with one rule asking for one engineer. +3. Create a second policy scoped to `dcim.device`, asking for one lead. +4. Sign in as a contributor. Create a branch and change a prefix inside it. +5. Open a change request against that branch and submit it for review. +6. Confirm the **Applied policies** card lists the baseline only, and the checks have run. +7. Sign in as a reviewer and approve. Confirm the status becomes **Approved** and the merge button appears for whoever holds `merge_branch`. +8. Back as the contributor, edit a **device** inside the same branch. +9. Confirm the request drops back to **Needs review**, and that the device policy has now attached and is asking for a lead. This is the check that the gate follows the branch rather than freezing at submission. +10. Approve as a lead, then merge. Confirm the request becomes **Completed**. + +Steps 8 and 9 are the ones worth repeating after any upgrade. + +## Troubleshooting + +**The merge button is greyed out and the reason says the request is not approved.** The people gate and the machine gate are separate. Read the **Approval status** card: a rule showing 0/1 names who may satisfy it. + +**A reviewer's approval did not count.** Three usual causes. They are not in a group the rule names. They approved, then the branch changed and their review went stale, which is shown with a **Stale** badge. Or they approved a different rule's requirement and the one still short asks for somebody else. + +**The approval panel says no policy rules apply.** No enabled policy matched the branch, or every matching policy has no rules. A request with no rules is never satisfied, on purpose: an unpoliced merge is what the plugin exists to prevent. Add a baseline policy with no object types. + +**Checks sit on pending forever.** A name that is not a registered check is treated as reported from outside, and waits for something to report it. Either something must PATCH a result, or the name is a typo in the policy's **Reported checks** field. + +**A conflict is reported that you believe you already resolved.** netbox-branching never advances a diff's baseline, so a field main touched before your last sync stays flagged. This plugin distinguishes the two and says so on the request. See [Conflicts with main](conflicts.md). + +**A user cannot see the Changes tab.** They are missing `netbox_branching.view_changediff`. + +**`protect_main` is blocking a script.** Writes with no request context are allowed, so a management command or a background job is not affected. An interactive write needs `netbox_change_control.bypass_policy`. + +**A change with a window never merges automatically.** The sweep runs every `auto_merge_interval` minutes and can step over a shorter window. The request warns about this itself. See [the sweep interval](merging.md#the-sweep-interval). diff --git a/docs/api.md b/docs/api.md index 769f2f4..43653ee 100644 --- a/docs/api.md +++ b/docs/api.md @@ -48,7 +48,7 @@ Find the policies which already require a given check: ```bash curl -s -H "Authorization: Token $TOKEN" \ - "$NETBOX/api/plugins/change-control/policies/?check=cab-approval" + "$NETBOX/api/plugins/change-control/policies/?required_checks=cab-approval" ``` See [checks which do not apply everywhere](checks.md#which-checks-apply-and-where) for what each kind means. @@ -77,7 +77,7 @@ The fields worth acting on: | Field | Meaning | |---|---| -| `status` | `draft`, `needs-review`, `approved`, `rejected` or `completed`. The people gate only. | +| `status` | `draft`, `needs-review`, `approved`, `rejected`, `completed` or `abandoned`. The people gate only, and **read-only**: it is derived from the policy evaluation. | | `approved` | Whether the policies are satisfied. | | `ready_to_merge` | Whether it can actually merge now. Approved is not the same thing: a check or the change window can still block. | | `merge_blocked_reason` | Why not, in words, when `ready_to_merge` is false. | @@ -86,7 +86,14 @@ The fields worth acting on: | `branch` / `branch_name` | The branch, and its name kept after the branch is deleted. | | `branch_deleted` | Whether the branch is gone and the record is history only. | -`ready_to_merge` is computed at read time, from the policies, the checks, the change window and the branch itself, so there is no filter for it. Narrow on what is stored and read the field from the results: +`ready_to_merge` and `has_conflicts` are computed at read time on this endpoint, from the policies, the checks, the change window and the branch itself, so neither is what the list filters on. Two cached counterparts are, and they are what the change request list displays: + +| Filter | Matches | +|---|---| +| `gates_cleared` | The policies and every required check were satisfied at the last refresh. Excludes the change window. | +| `has_conflicts` | The branch conflicted with main at the last refresh. | + +Use those to narrow, and read the live fields from the results: ```bash curl -s -H "Authorization: Token $TOKEN" \ @@ -110,10 +117,55 @@ curl -X POST "$NETBOX/api/plugins/change-control/reviews/" \ Note the absent field. `reviewer` is read-only and always the caller, so a token cannot post an approval attributed to a colleague. Submitting one for somebody else is not an error; it is simply recorded as yours. -`decision` takes `approve`, `request-changes` or `comment`. Requesting changes needs a comment. A user cannot review their own change request, and a second review by the same user is refused: edit the existing one instead. +`decision` takes `approve`, `reject` or `comment`. `reject` is what the interface labels **Request changes**. Requesting changes needs a comment. A user cannot review their own change request, and a second review by the same user is refused: edit the existing one instead. Reviews and comments accept Markdown, rendered through NetBox's sanitising filter. +## Commenting on one change + +```bash +curl -X POST "$NETBOX/api/plugins/change-control/change-comments/" \ + -H "Authorization: Token $TOKEN" -H "Content-Type: application/json" \ + -d '{"change_request": 42, "change_diff": 907, "text": "Is this the right rack?"}' +``` + +`author` is read-only and always the caller, for the same reason `reviewer` is on a review: a comment is part of the record a reviewer reads before approving, so a token must not be able to post one under a colleague's name. Naming somebody else is not an error; the comment is simply recorded as yours. + +`change_diff` has to be a change in the request's own branch. Crossing them is refused with a 400, because such a comment would be invisible on the tab it belongs to and counted as an open thread on a request it does not describe. + +Reply within a thread by naming its root comment as `parent`. A reply must sit on the same change as its parent, and replies are one level deep: a reply to a reply joins the same thread. + +## Moving a change request through its lifecycle + +`status` is read-only, so every transition a person makes is an action rather than a field. See [the lifecycle diagram](change-requests.md#the-lifecycle) for how they fit together. + +```bash +curl -X POST "$NETBOX/api/plugins/change-control/change-requests/42/submit/" \ + -H "Authorization: Token $TOKEN" + +curl -X POST "$NETBOX/api/plugins/change-control/change-requests/42/return-to-draft/" \ + -H "Authorization: Token $TOKEN" + +curl -X POST "$NETBOX/api/plugins/change-control/change-requests/42/abandon/" \ + -H "Authorization: Token $TOKEN" + +curl -X POST "$NETBOX/api/plugins/change-control/change-requests/42/reopen/" \ + -H "Authorization: Token $TOKEN" +``` + +| Action | From | Permission | +|---|---|---| +| `submit` | Draft | `change_changerequest` | +| `return-to-draft` | Needs review, Approved, Rejected | `change_changerequest` | +| `abandon` | any open state | `abandon_changerequest` | +| `reopen` | Abandoned | `reopen_changerequest` | + +None needs `add_changerequest`: giving up on a change is not creating one. + +Each returns the updated change request. A transition that does not apply, such as abandoning a completed request or submitting one that is already under review, returns **409** with the reason in `detail`. + +Submitting matches the policies and notifies the outstanding reviewers, exactly as the button does. + ## Events, the other direction To be told when something happens rather than polling, use [event rules](event-rules.md). diff --git a/docs/automation.md b/docs/automation.md index be4c3f5..421eeee 100644 --- a/docs/automation.md +++ b/docs/automation.md @@ -3,6 +3,7 @@ - **Stale reviews.** Each review records the timestamp of the newest change in its branch at the moment it was submitted. A review is stale when the branch has moved on since. Stale approvals and stale rejections are excluded from the evaluation, so an approval can never cover work the reviewer did not see. Stale reviews carry a badge. - **Approval invalidation.** This falls out of staleness. When a branch is synced, reverted, or edited, its existing approvals go stale, the rules stop being satisfied, and the status returns to Needs review on its own. - **Policy reevaluation.** Signal receivers watch policies, rules, the rule's group and user lists, and user group membership. Any change re-evaluates every open change request bound to the affected policy. Raising a rule's minimum, or removing a reviewer from a group, revokes an approval that depended on it. +- **Policy rematching.** Which policies govern a request is decided from the object types in its branch, and a branch keeps moving after its request is opened. When an edit inside the branch brings in an object type no attached policy covers, the policies are matched again and the new one attaches. The merge gate matches again for itself, so the decision never depends on that receiver having fired. See [policies attach automatically](policies.md#policies-attach-automatically). - **Completion on merge.** A `post_merge` receiver sets the status to completed. Terminal statuses are never reopened. - **Check refresh.** Checks re-run when the branch content moves and when a comment thread is opened, resolved or removed. diff --git a/docs/change-requests.md b/docs/change-requests.md index 6011e41..7c1bb67 100644 --- a/docs/change-requests.md +++ b/docs/change-requests.md @@ -16,7 +16,90 @@ One change request per branch. | Window opens / closes | An optional [change window](merging.md#change-windows). | | Merge automatically | Merge without further human action once every gate passes. See [automatic merging](merging.md#automatic-merging). | -Status is derived from the policy evaluation and is refreshed automatically. Completed and Abandoned are terminal and are never reopened. +Status is derived from the policy evaluation and is refreshed automatically, so it is not a field you set. It is read-only on the REST API and absent from the bulk edit form, because Completed is terminal: the merge gate refuses a completed request and nothing reopens one, so setting it by hand blocked its branch from merging for good. + +The two transitions a person makes by hand have an action each, and a permission each. + +| Action | Where | Permission | Allowed from | +|---|---|---|---| +| **Abandon** | The change request page, or `POST /change-requests/{id}/abandon/` | `netbox_change_control.abandon_changerequest` | Draft, Needs review, Approved, Rejected | +| **Reopen** | The change request page, or `POST /change-requests/{id}/reopen/` | `netbox_change_control.reopen_changerequest` | Abandoned only | + +Reopening returns the request to Draft and then recomputes, rather than restoring the status it held before. Its reviews may have gone stale and its policies may have changed while it was set aside, so the honest answer has to be worked out again. + +Completed is never reopened. It records a merge that actually happened, and taking it back up would invite a second merge of a branch already in main. + +## The lifecycle + +```mermaid +stateDiagram-v2 + direction TB + + [*] --> Draft: opened + + Draft --> NeedsReview: Submit for review + NeedsReview --> Draft: Return to draft + Approved --> Draft: Return to draft + Rejected --> Draft: Return to draft + + NeedsReview --> Approved: every rule satisfied + NeedsReview --> Rejected: changes requested + Approved --> NeedsReview: approvals went stale + Approved --> Rejected: changes requested + Rejected --> NeedsReview: rejection withdrawn or stale + Rejected --> Approved: rejection cleared, rules satisfied + + Approved --> Completed: branch merged + + Draft --> Abandoned: Abandon + NeedsReview --> Abandoned: Abandon + Approved --> Abandoned: Abandon + Rejected --> Abandoned: Abandon + Abandoned --> Draft: Reopen + + Completed --> [*] + + classDef manual fill:#e8f0fe,stroke:#3b6fd4,color:#1a3a6b + classDef derived fill:#e9f7ec,stroke:#2f9e44,color:#14532d + classDef terminal fill:#f1f3f5,stroke:#868e96,color:#343a40 + + class Draft manual + class NeedsReview derived + class Approved derived + class Rejected derived + class Completed terminal + class Abandoned terminal +``` + +Blue is a state a person puts the request into. Green is derived: the plugin computes it from the reviews and moves the request there on its own. Grey is terminal. + +### Every transition + +| From | To | How it happens | Who or what does it | Permission | +|---|---|---|---|---| +| *(none)* | Draft | A change request is opened against a branch | Manual | `add_changerequest` | +| Draft | Needs review | **Submit for review**, which also matches the policies | Manual | `change_changerequest` | +| Needs review | Approved | Every rule of every attached policy has its approvals, and nobody has requested changes | Automatic, once the **required human action** of approving has happened | `add_review` to approve | +| Needs review | Rejected | A reviewer requests changes | Automatic, on the **required human action** | `add_review` | +| Approved | Needs review | The branch moved, so the approvals went stale; or a policy, rule or group membership changed | Automatic | none | +| Approved | Rejected | A reviewer requests changes after approval | Automatic | `add_review` | +| Rejected | Needs review | The rejection was withdrawn, or the branch moved and it went stale | Automatic | none | +| Rejected | Approved | The rejection cleared and the rules are satisfied | Automatic | none | +| Needs review, Approved, Rejected | Draft | **Return to draft**, to take the change back off the table | Manual | `change_changerequest` | +| Approved | Completed | The branch merged | Automatic, on the merge | `merge_branch` to merge | +| Any open state | Abandoned | **Abandon** | Manual | `abandon_changerequest` | +| Abandoned | Draft | **Reopen** | Manual | `reopen_changerequest` | + +### What the states mean + +**Draft** is the author's. The plugin does not move a request out of it, and a review submitted against a draft changes nothing. This is what makes **Return to draft** worth having: a request pulled back stays pulled back while the author works, instead of being pushed straight back into review by the next signal. + +**Needs review**, **Approved** and **Rejected** are derived. They are a cached view of the policy evaluation, recomputed whenever anything that could change the answer happens, which is why they are not editable and why the arrows between them carry no permission: nobody sets them, they follow from the reviews. + +**Completed** and **Abandoned** are terminal. Completed records a merge that happened and is never reopened, because the branch is already in main. Abandoned can be reopened, which returns the request to Draft rather than to whatever it held before: its reviews may have gone stale and its policies may have changed while it was set aside, so the author submits it again and the evaluation works out the honest answer. + +> [!NOTE] +> Only **Approved** opens the merge gate, and even then the checks and the change window are separate gates on top of it. See [approved is not the same as mergeable](#approved-is-not-the-same-as-mergeable). ## Approved is not the same as mergeable @@ -27,10 +110,25 @@ The change request page shows both. The status badge reads `Approved`, and besid > **Approved** ยท **Blocked** > Approved by reviewers, but not yet mergeable. Required checks are not passing: Comment threads resolved (failed). -The change request list carries a **Ready to merge** column, and the REST API exposes `ready_to_merge` and `merge_blocked_reason` alongside `approved`. All of them delegate to netbox-branching, so they account for every gate, including validators registered by other plugins. +The REST API exposes `ready_to_merge` and `merge_blocked_reason` alongside `approved`, and this page's own badge reads the same source. All of them delegate to netbox-branching, so they account for every gate, including validators registered by other plugins. + +The change request **list** answers the same question from a cache, because computing it per row cost about eleven queries and a page holds fifty. The cache is refreshed whenever anything that could change the answer happens: a review, a policy, a check, a branch sync, a new change in the branch. The change window is the exception, since a window opens because the clock moved rather than because anything happened, so it is evaluated as the row is rendered from fields already loaded. + +Two consequences worth knowing. The list does not account for merge validators registered by other plugins, which this page does. And the list is what you sort and filter on, which the live version could never support. Where the two could disagree, this page is authoritative, and the merge gate itself never reads the cache at all. + +## Finding a change request + +Change requests appear in NetBox's global search, and the **reference** is weighted above everything else, so typing a ticket number finds the change it spawned. The title, description, comments and the branch name are searched too. + +The branch **name** is what is indexed, not the branch itself, so a change request whose branch has been deleted is still found by the name that branch had. That is the case where search is the only way left to reach it. + +Policies, rules, reviews, pre-merge checks and the per-object comments are all searchable as well. A comment keeps the name of the object it was about, so searching for a device finds the discussion about it long after the branch is gone. + +> [!NOTE] +> Search reads an index NetBox maintains as objects are written. Objects that already existed when this plugin was upgraded are indexed by running `./manage.py reindex netbox_change_control` once. ## The record outlives the branch -A change request is the record of who approved what, so it survives deletion of its branch. The branch link is cleared, the branch **name** is kept, and the title, description, reviews, comment threads, applied policies and check results all remain. Each comment also keeps the name of the object it was about, so the discussion still makes sense once the diff is gone. +A change request is the record of who approved what, so it survives deletion of its branch. The branch link is cleared, the branch **name** is kept and follows the branch through any rename, and the title, description, reviews, comment threads, applied policies and check results all remain. Each comment also keeps the name of the object it was about, so the discussion still makes sense once the diff is gone. Such a request shows its branch name with a **Deleted** badge and is a historical record only: it cannot be merged, its diff is gone, and its checks report as skipped rather than failing. The REST API exposes `branch_name` and `branch_deleted` alongside `branch`. diff --git a/docs/checks.md b/docs/checks.md index 88c337c..371aca7 100644 --- a/docs/checks.md +++ b/docs/checks.md @@ -30,12 +30,18 @@ A result reflects the last run, so knowing when that happens matters: | The change request is created | So a new request never shows checks stuck on `pending`. | | It is submitted for review | The reviewer needs a current answer. | | It reaches **Approved** | This is the moment a merge becomes possible, so the results must be current. A branch edit invalidates the reviews but not the stored check results; without this refresh a request could be edited to introduce a conflict, re-approved, and merged against a stale `no-conflicts` result. | +| A policy attaches or detaches | Which checks apply follows the policies, so the set can change without the branch changing. | | The branch is synced or reverted | Its content changed. | +| A branch diff changes | A conflict can appear with no event on the change request at all, because somebody edited the same object in main. | | A comment thread is opened, resolved or removed | `threads-resolved` reads them. | | Somebody presses **Re-run checks** | On demand. | Externally reported checks are never overwritten by a run: only the system that reports them knows the answer. +Several of those moments arrive together. Attaching policies is one event per policy, so submitting a request would otherwise run every check once per policy for the same answer; the run is collapsed to one per change request instead. It still happens before the page you submitted from comes back, so a result is never a refresh behind. + +A result that moves is recorded in the object's changelog, so the machine half of a merge decision is auditable alongside the human half. A re-run that finds the same answer writes nothing, which keeps the log to real transitions; `completed` therefore marks when the result last changed, not when a check last ran. + ## Built-in checks Four ship, available to any policy. Together they catch the merges that damage data rather than merely lack approval. None of them applies until a policy names it. diff --git a/docs/design.md b/docs/design.md index e93b88c..0015ac0 100644 --- a/docs/design.md +++ b/docs/design.md @@ -1,6 +1,6 @@ # Design: NetBox Change Control -Status: approved, v0.1.0 implemented. Date: 2026-08-25. +Status: approved and implemented. First written 2026-08-25, kept current as the plugin changes. ## Problem @@ -46,7 +46,7 @@ The fix is `exempt_models: ['netbox_change_control.*']` in the branching config. ### `protect_main` is off by default -Enabling it by default would break an existing install the moment the plugin is added. It is opt-in. It is implemented as `pre_save` and `pre_delete` receivers which refuse a write when `active_branch` is unset, the model supports branching, and the user lacks `netbox_change_control.bypass_change_control`. Writes with no request context (migrations, scripts, jobs) are allowed, since they are not interactive edits. +Enabling it by default would break an existing install the moment the plugin is added. It is opt-in. It is implemented as `pre_save` and `pre_delete` receivers which refuse a write when `active_branch` is unset, the model supports branching, and the user lacks `netbox_change_control.bypass_policy`. Writes with no request context (migrations, scripts, jobs) are allowed, since they are not interactive edits. ## Data model @@ -57,8 +57,10 @@ Enabling it by default would break an existing install the moment the plugin is | `ChangeRequest` | `PrimaryModel` | One per branch (`OneToOneField`). Status, priority, requester. | | `ChangeRequestPolicy` | `Model` | Through table. Records `matched` and `matched_object_types`. | | `Review` | `NetBoxModel` | One decision per reviewer per request, enforced by constraint. | +| `MergeCheck` | `NetBoxModel` | One pre-merge check result per request, unique on its name. | +| `ChangeComment` | `NetBoxModel` | A comment on one changed object, or a reply within that thread. | -`Review` was first a `ChangeLoggedModel`, which has no `tags`. `NetBoxModelFilterSet` filters on `tags__slug`, so it failed at import. All four public models are now `NetBoxModel` or `PrimaryModel`, which keeps the filtersets, serializers and tables uniform. +`Review` was first a `ChangeLoggedModel`, which has no `tags`. `NetBoxModelFilterSet` filters on `tags__slug`, so it failed at import. Every public model is now `NetBoxModel` or `PrimaryModel`, which keeps the filtersets, serializers and tables uniform. ## Module layout @@ -79,7 +81,7 @@ The shared fixtures live in `tests/base.py`, so the shape of a change request is Layer two is the seed command, `seed_change_control`. It creates the reviewer groups and users, the object permissions those users need, five graded policies, a branch with a real change, and an open change request. `--adopt` opens a change request for branches that already exist. -The seed commands live in `dev/` as a separate development-only plugin, and are not part of the published package. A production install therefore has no command that can invent policies, users or permissions. +The seed commands live in a development-only plugin which is neither published nor committed, so a production install has no command that can invent policies, users or permissions, and a reader of this repository will not find one. The seed's branch edit must run inside `netbox.context_managers.event_tracking` with a synthetic request. NetBox writes `ObjectChange` records only within a request context; without it the branch records no changes, `ChangeDiff` stays empty, and every scoped policy silently fails to match. @@ -91,12 +93,38 @@ The seed's branch edit must run inside `netbox.context_managers.event_tracking` The merge gate does not depend on this cache; it re-evaluates. The cache is for display and filtering only. +### Repeated work is collapsed, not deferred + +Refreshing a change request is correct on every event that could change its answer, and doing +it per event is what keeps the cached status honest without any caller having to remember. +The cost is that one user action is often many events: attaching policies is one signal each. + +`netbox_change_control/batching.py` collapses those bursts. A caller that knows it is about to +cause one wraps it in `batched()`, and each affected change request is refreshed once when the +block ends. + +It deliberately does not use `transaction.on_commit`. Deferring past the commit would mean a +request's checks are not yet run when the view that submitted it renders the next page, and it +would make every test that asserts on a check result depend on `captureOnCommitCallbacks`. +Outside a block the refresh is immediate, so nothing changes for the callers which are not +part of a burst. + ### Staleness is derived from a snapshot, not a flag A review stores `branch_change_time`, the timestamp of the newest change in its branch when the review was submitted. A review is stale when the branch's newest change is later than that snapshot. This is preferable to a `stale` boolean maintained by signals, because a flag can drift while a derived value cannot. It also means approval invalidation needs no separate mechanism: stale approvals are simply excluded from the evaluation, the rules stop being satisfied, and `refresh_status` returns the request to Needs review. +### `protect_main` listens to every write + +`protect_main_on_save` and `protect_main_on_delete` are registered without a sender, so they +run for every model write anywhere in NetBox. That is deliberate rather than an oversight: the +set of protected models is not knowable at import time, because it depends on which models +branching supports and on `protect_main_scope`, which is configuration. + +The cost is bounded by ordering the guard cheaply. The first thing each receiver does is read +`protect_main`, and with it off, which is the default, that is the whole cost of the call. + ### `protect_main_scope` The commercial product's `protect_main` is all-or-nothing. In practice a team often wants branch discipline on one risky area, such as circuits, without forcing every IPAM edit through review. diff --git a/docs/extending.md b/docs/extending.md index b5b2185..14973f1 100644 --- a/docs/extending.md +++ b/docs/extending.md @@ -2,6 +2,14 @@ Another plugin can add content to this plugin's pages, and checks to its merge gate. Nothing special is required of you. +## What this plugin injects elsewhere + +The traffic runs both ways. This plugin uses the same hooks to put its own content on +netbox-branching's branch page: the change request governing that branch, its status, what is +still outstanding and a link to it, or an offer to open one when there is none. It uses the +`alerts` and `right_page` hooks, and `branch_page_placement` decides which of them renders, so +a deployment can compare the two or turn both off. See `netbox_change_control/template_content.py`. + ## Injecting content into a page Every model here supports NetBox's standard `PluginTemplateExtension` hooks. This is a NetBox feature, and the [NetBox documentation](https://netboxlabs.com/docs/netbox/en/stable/plugins/development/views/#extra-template-content) is the reference for it. diff --git a/docs/img/capture-blocked.png b/docs/img/capture-blocked.png new file mode 100644 index 0000000..192e624 Binary files /dev/null and b/docs/img/capture-blocked.png differ diff --git a/docs/img/capture-ready.png b/docs/img/capture-ready.png new file mode 100644 index 0000000..e98f4e5 Binary files /dev/null and b/docs/img/capture-ready.png differ diff --git a/docs/img/capture-unmanaged.png b/docs/img/capture-unmanaged.png new file mode 100644 index 0000000..5cf20a2 Binary files /dev/null and b/docs/img/capture-unmanaged.png differ diff --git a/docs/index.md b/docs/index.md index 0cdf9c2..b2a547b 100644 --- a/docs/index.md +++ b/docs/index.md @@ -9,7 +9,8 @@ New here? Read [Installation and configuration](installation.md), then [Policies | Page | Covers | |---|---| | [Installation and configuration](installation.md) | Requirements, installing the plugin, and every configuration setting. | -| [Permissions](permissions.md) | The permissions each role needs, and how to grant the two exemptions. | +| [Administration guide](admin-guide.md) | Setting up roles and groups, the permission matrix, building policies, and troubleshooting. | +| [Permissions](permissions.md) | The short reference: every permission name and what it grants. | ## Day to day diff --git a/docs/installation.md b/docs/installation.md index 33a329e..88f4c73 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -32,6 +32,12 @@ Then run the migrations: ./manage.py migrate netbox_change_control ``` +If you are adding this plugin to a NetBox that already holds data, build the search index once so existing objects are findable: + +```bash +./manage.py reindex netbox_change_control +``` + > [!IMPORTANT] > The `exempt_models` entry is not optional. Without it, a change request created while a branch is active is written into that branch's schema and is invisible from main, taking the record of who approved what with it. @@ -44,8 +50,8 @@ Every setting is optional. The defaults are safe to install on an existing NetBo | `protect_main` | `False` | Block writes to branching-supported models outside a branch. | | `protect_main_scope` | `[]` | Limit `protect_main` to specific models. Empty protects every branching-supported model. | | `enforce_merge_gate` | `True` | Refuse to merge a branch without an approved change request. | -| `lock_matched_policies` | `True` | Automatically matched policies cannot be detached by the author. | | `notify_reviewers` | `True` | Raise NetBox notifications on status transitions. | +| `branch_page_placement` | `['right_page']` | Where a branch page shows its change request. `right_page` puts it in a card in the right-hand column, `alerts` puts it across the top of the page, both shows both, and `[]` shows neither. | | `enable_builtin_checks` | `True` | Which built-in checks are available to policies. `True` for all, `False` for none, or a list of names. A check applies only where a policy names it. | | `required_external_checks` | `[]` | Checks reported through the REST API. Each blocks the merge until reported. | | `enable_auto_merge` | `True` | Allow requests that opt in to merge themselves once every gate passes. When `False`, the periodic job is not registered at all. | diff --git a/docs/merging.md b/docs/merging.md index 3acd70e..19da5db 100644 --- a/docs/merging.md +++ b/docs/merging.md @@ -4,6 +4,20 @@ Once a change request reaches **Approved** and every required check passes, a ** ![Merge button](img/merge-button.png) +The branch page under **Branching > Branches** carries the same information from the other side, in a card in the right-hand column: the change request governing it, its status, which rules are still short and who may satisfy them, with a link to it. + +![The change request shown on its own branch page](img/capture-blocked.png) + +A branch nobody has opened a change request against is told so in the same place, with a button to open one. That is the case netbox-branching's merge form can only refuse. + +![A branch with no change request](img/capture-unmanaged.png) + +Once every gate is satisfied the same card turns green and says so, which is the signal to use the Merge button on it. + +Where that panel sits is configuration. `branch_page_placement` takes `right_page` for the card the screenshots here show, which is the default; `alerts` for a band across the top of the page instead; both to show both, which is how you compare them; or `[]` to leave branching's page as it was. See [Configuration](installation.md#configuration). + +![A branch whose change request is satisfied](img/capture-ready.png) + While the merge is blocked the button is greyed out and states the reason. If you may approve but not merge, the panel says so rather than hiding the button. The branch page under **Branching > Branches** carries the same button, since the merge itself belongs to the branching plugin. After a successful merge the change request is marked **Completed**, and it cannot be reopened. @@ -29,7 +43,7 @@ Either bound may be left empty. A start alone means "not before"; an end alone m The window is a **third, independent gate**, checked alongside the policies and the checks. When it blocks, the merge button states when the window opens or when it closed. -Users holding `netbox_change_control.override_window_changerequest` may merge outside the window, which is what an incident needs. See [granting the exemptions](permissions.md#granting-the-exemptions). +Users holding `netbox_change_control.override_window_changerequest` may merge outside the window, which is what an incident needs. See [granting the exemptions](permissions.md#granting-the-custom-actions). > [!IMPORTANT] > The window fails closed when there is no request context. `protect_main` deliberately exempts scripts and background jobs, because those are not interactive edits. A change window is the opposite: a script merging at the wrong hour is exactly what it exists to stop. @@ -45,13 +59,15 @@ It is triggered two ways, because a request can become mergeable either by somet So a change approved at midday with a window opening at 21:00 does merge at 21:00, with no further human action. Nothing has to happen at 21:00 except the clock reaching it. +Only ever one job per branch. A single write can reach the automatic merge by more than one route, and the request is still Approved at the second arrival because the merge has only been queued and not yet run. A queued, scheduled or running merge for the branch stops another being added. + The merge is **enqueued as a background job**, the same path the branching plugin's own merge button takes. It is never run inline: auto-merge is reached from a signal, so merging directly would run a whole branch merge inside the web request that submitted the final review. If the branch changes during the wait, the approvals go stale, the status returns to Needs review, and the evening merge does not happen. Approval is only valid for the branch state it was given against. ## The sweep interval -The sweep is a NetBox **system job**, `Change control: automatic merges`, run by the worker. It is not an event rule. Each run is one indexed query, and it merges nothing unless a request is genuinely ready. +The sweep is a NetBox **system job**, `Change control: automatic merges`, run by the worker. It is not an event rule. One indexed query finds the candidates, which are the approved requests that opted in; each candidate then costs a full re-evaluation of its gates. With no opted-in request waiting, a run is a single query and nothing else. The interval bounds how late a window can fire, and costs one Job record per run: diff --git a/docs/permissions.md b/docs/permissions.md index 376243d..83ac22c 100644 --- a/docs/permissions.md +++ b/docs/permissions.md @@ -1,32 +1,128 @@ # Permissions -The plugin defines the standard NetBox object permissions for each of its models, plus one extra. +Every permission this plugin defines, and what each one grants. Nothing here is abbreviated: if a permission is not on this page, the plugin does not define it. -| Permission | Grants | +For roles, groups and how to assemble them, read the [Administration guide](admin-guide.md). + +> [!IMPORTANT] +> A NetBox object permission applies to **every object of that type** unless you add a constraint. `delete_review` therefore lets a user delete anybody's review, which changes the outcome of the gate: removing a **Request changes** review removes the rejection. Grant it sparingly, or constrain it with `{"reviewer": "$user"}`. See [narrowing a permission with a constraint](admin-guide.md#narrowing-a-permission-with-a-constraint). + +> [!NOTE] +> NetBox reads a permission name as `._`, splitting on the **last** underscore. The **Action** column below is what you tick, or type into *additional actions*, on the permission form. The full name is what code and the REST API use. + +## Change requests + +| Permission | Action | Grants | +|---|---|---| +| `netbox_change_control.view_changerequest` | `view` | See change requests, with their reviews, checks and applied policies. | +| `netbox_change_control.add_changerequest` | `add` | Open a change request against a branch. | +| `netbox_change_control.change_changerequest` | `change` | Edit the title, description, reference, priority, change window and auto-merge flag, and move the request between **Draft** and **Needs review** with **Submit for review** and **Return to draft**. | +| `netbox_change_control.delete_changerequest` | `delete` | Delete a change request, destroying the record of who approved what. | +| `netbox_change_control.abandon_changerequest` | `abandon` | Give up on an open change request, through the button or `POST /change-requests/{id}/abandon/`. | +| `netbox_change_control.reopen_changerequest` | `reopen` | Take an abandoned change request back up, through the button or `POST /change-requests/{id}/reopen/`. | +| `netbox_change_control.override_window_changerequest` | `override_window` | Merge a change request outside its change window. | + +`status` is not editable by anybody. It is read-only on the REST API and absent from the bulk edit form. Four actions move it, and [the lifecycle diagram](change-requests.md#the-lifecycle) shows how they fit together: + +| Action | Permission | |---|---| -| `netbox_change_control.view_changerequest` and friends | The usual view, add, change and delete on each model. | -| `netbox_change_control.add_review` | Submit a review. | -| `netbox_change_control.change_mergecheck` | Re-run checks. | -| `netbox_change_control.add_changecomment` | Comment on a specific change. | -| `netbox_change_control.change_changecomment` | Resolve and reopen threads. | -| `netbox_change_control.bypass_policy` | Write outside a branch while `protect_main` is enabled. | -| `netbox_change_control.override_window_changerequest` | Merge a change request outside its change window. | +| Submit for review | `change_changerequest` | +| Return to draft | `change_changerequest` | +| Abandon | `abandon_changerequest` | +| Reopen | `reopen_changerequest` | + +Everything else is derived: **Needs review**, **Approved** and **Rejected** follow from the reviews, and nobody sets them. + +## Reviews + +| Permission | Action | Grants | +|---|---|---| +| `netbox_change_control.view_review` | `view` | See who has reviewed a change request and what they said. | +| `netbox_change_control.add_review` | `add` | Submit a review: approve, request changes, or comment. This is what separates a reviewer from everybody else. | +| `netbox_change_control.change_review` | `change` | Edit a review. Only ever your **own**, whoever holds this. A superuser may edit any. | +| `netbox_change_control.delete_review` | `delete` | Delete **any** review, unless constrained. See the warning above. | + +Holding `add_review` is not enough on its own to advance a policy rule. The rule names groups and users, and only somebody it names can satisfy it. The permission decides whether you may act; the rule decides whether your approval counts. + +## Change comments + +The per-object discussion on the **Changes** tab. + +| Permission | Action | Grants | +|---|---|---| +| `netbox_change_control.view_changecomment` | `view` | Read the comment threads on a change request. | +| `netbox_change_control.add_changecomment` | `add` | Comment on one changed object, and reply within a thread. | +| `netbox_change_control.change_changecomment` | `change` | Resolve and reopen a thread. Anybody who might have to clear the `threads-resolved` check needs this. | +| `netbox_change_control.delete_changecomment` | `delete` | Delete a comment. Also available from the Changes tab. | + +## Merge checks -## Granting the exemptions +| Permission | Action | Grants | +|---|---|---| +| `netbox_change_control.view_mergecheck` | `view` | See check results on a change request and in the Merge Checks list. | +| `netbox_change_control.add_mergecheck` | `add` | Create a check row by hand. The plugin creates them from the policies, so this is rarely wanted. | +| `netbox_change_control.change_mergecheck` | `change` | Press **Re-run checks**, and report a result over the REST API. This is what a CI token needs. | +| `netbox_change_control.delete_mergecheck` | `delete` | Delete a check row. Deleting one does not open the gate: a required check with no result counts as not run, and blocks. | -The two exemptions are **custom actions on an object type**. Grant them under **Administration > Permissions**: +> [!TIP] +> A reporting token cannot weaken the gate even with `change_mergecheck`. `required` is read-only on the REST API, and the gate reads requiredness from the configuration and the policies rather than from the stored row. -| Exemption | Object type | Action to enter | +## Policies + +| Permission | Action | Grants | |---|---|---| -| Write directly to main | Change Control > Policy | `bypass` | -| Merge outside the change window | Change Control > Change Request | `override_window` | +| `netbox_change_control.view_policy` | `view` | Read a policy: its scope, conditions and required checks. Give this to everybody, or a reviewer cannot tell why they were asked. | +| `netbox_change_control.add_policy` | `add` | Create a policy. | +| `netbox_change_control.change_policy` | `change` | Edit a policy, which changes who must approve every open change request bound to it. | +| `netbox_change_control.delete_policy` | `delete` | Delete a policy. Refused while a change request still references it. | +| `netbox_change_control.bypass_policy` | `bypass` | Write outside a branch while `protect_main` is enabled. | -Enter the action in the permission's *additional actions* field, not the view/add/change/delete checkboxes. +## Policy rules + +| Permission | Action | Grants | +|---|---|---| +| `netbox_change_control.view_policyrule` | `view` | Read a rule: how many approvals it needs, and who may give them. | +| `netbox_change_control.add_policyrule` | `add` | Add a rule to a policy. | +| `netbox_change_control.change_policyrule` | `change` | Edit a rule, including its minimum and its reviewer groups. | +| `netbox_change_control.delete_policyrule` | `delete` | Remove a rule from a policy. | + +## Policy bindings + +The table recording which policies govern which change request **defines no permissions at all**. The plugin maintains it, no page exposes it, and the four Django creates for every model are switched off. > [!NOTE] -> Superusers hold every permission, so they are always exempt from both. +> There is therefore nothing to grant in order to detach a policy from a change request. Which policies govern a change is decided from the objects its branch touches, and they are re-matched as the branch moves, so a binding removed by hand comes back. Change the policy's scope instead. + +## Permissions from netbox-branching + +These belong to [netbox-branching](https://github.com/netboxlabs/netbox-branching) rather than to this plugin, but a change request is useless without them, and the split between the two catches people out. + +| Permission | Action | Grants | +|---|---|---| +| `netbox_branching.view_branch` | `view` | See branches. | +| `netbox_branching.add_branch` | `add` | Create a branch. | +| `netbox_branching.change_branch` | `change` | Rename a branch and edit its description. | +| `netbox_branching.delete_branch` | `delete` | Delete a branch. The change request survives it, keeping the branch name. | +| `netbox_branching.merge_branch` | `merge` | Merge a branch. This is what shows the **Merge branch** button once every gate is satisfied. | +| `netbox_branching.sync_branch` | `sync` | Pull main's changes into a branch. | +| `netbox_branching.revert_branch` | `revert` | Revert a merged branch. | +| `netbox_branching.archive_branch` | `archive` | Archive a merged branch. | +| `netbox_branching.migrate_branch` | `migrate` | Apply outstanding migrations to a branch. | +| `netbox_branching.view_changediff` | `view` | See the branch diff. **This is what gates the Changes tab**: without it, a reviewer cannot see what they are being asked to approve. | + +## Granting the custom actions + +Four actions are not the usual view, add, change and delete. Grant them under **Administration > Permissions > Add**, typing the action into the *additional actions* field rather than ticking a box: + +| To allow | Object type | Action to enter | +|---|---|---| +| Writing directly to main under `protect_main` | Change Control > Policy | `bypass` | +| Merging outside the change window | Change Control > Change Request | `override_window` | +| Abandoning a change request | Change Control > Change Request | `abandon` | +| Reopening an abandoned change request | Change Control > Change Request | `reopen` | > [!IMPORTANT] -> NetBox resolves a permission name as `._`, splitting on the **last** underscore. A custom permission whose trailing component is not a real model name can never be granted by an object permission, and the exemption silently ends up superuser-only. This is why the bypass lives on `Policy` and the window override on `ChangeRequest`. +> The trailing part of a custom permission name has to be a real model name, because NetBox splits on the last underscore. That is why the bypass lives on `Policy`: `bypass_change_control` would resolve to a model called `control`, which does not exist, so the permission could never be granted through an object permission at all and the exemption would silently be superuser-only. -A reviewer typically needs view and add on change requests, reviews and comments, plus view on policies, merge checks and branches. +> [!NOTE] +> Superusers hold every permission, so they are exempt from `protect_main` and from every change window without being granted anything. diff --git a/docs/policies.md b/docs/policies.md index f7fd6d4..fa6e875 100644 --- a/docs/policies.md +++ b/docs/policies.md @@ -28,6 +28,8 @@ A **rule** is one approval requirement inside a policy. A user is eligible if they are in **any** listed group **or** are named directly. A policy is satisfied when **every** one of its rules is satisfied. +The change request shows a rule the way you wrote it, naming its groups rather than listing everybody currently in them. A rule pointing at a group with no members is called out by name, because it can never be satisfied. + An approval counts only toward the rules the reviewer is eligible for. A lead approving does not advance a rule that requires engineers. This is the property a naive approval counter gets wrong. > [!NOTE] @@ -62,4 +64,8 @@ The change request page shows the rule as **No approval required**, and the rule When a change request is opened, the plugin reads `ChangeDiff` for its branch, collects the object types actually touched, and attaches every matching enabled policy. Those bindings are locked against the author. +They also **follow the branch**. A branch is not fixed once its request is submitted: an author can keep editing inside it, and an edit can bring in an object type no attached policy covers. Whenever that happens the policies are matched again, so the new policy attaches and asks for whatever it asks for. The merge gate matches again for itself as well, so the decision never rests on a signal having fired. + +That matters because the alternative is a way round the gate. Open a request on a branch touching only low-risk objects, collect the one approval that attracts, then add the real change to the same branch. The approvals go stale and the status returns to Needs review, but a policy that never attached asks for nothing, so the same reviewer could approve a second time and the work would merge unseen by anybody with the authority to judge it. + This is deliberately stricter than the commercial product, where the author picks the policies. Letting the author choose makes the gate advisory: someone who wants a fast merge picks the weakest policy. diff --git a/docs/policy-conditions.md b/docs/policy-conditions.md index 2bf19a1..d7a4699 100644 --- a/docs/policy-conditions.md +++ b/docs/policy-conditions.md @@ -50,7 +50,7 @@ Three things decide whether the condition matches. > A condition naming a field the object does not have simply does not match; it does not error. That means a typo in `attr` silently produces a policy that never applies. > [!NOTE] -> Conditions cost a scan of the branch diff per condition-bearing policy. Policies scoped only by object type cost nothing extra. +> Conditions cost a scan of the branch diff per condition-bearing policy, evaluated in Python, one condition set per changed object. A policy that matches stops at the first object that satisfies it; one that does not match reads them all. On a branch touching thousands of objects with several conditional policies, this is the slowest thing the plugin does. Policies scoped only by object type cost nothing extra. ## Which side a condition reads diff --git a/docs/protect-main.md b/docs/protect-main.md index af28eeb..85d95f0 100644 --- a/docs/protect-main.md +++ b/docs/protect-main.md @@ -14,7 +14,7 @@ A refused write keeps the user on the form they were filling in, with the reason A delete shows the same message as an error toast, and the REST API returns 400 with it as `detail`. The refusal is raised as NetBox's `AbortRequest`, which is the mechanism a signal receiver is given for exactly this. Raising `PermissionDenied` instead produced a bare **Access Denied** page with the explanation discarded, which reads like a misconfigured permission rather than a deliberate policy. -Users holding `netbox_change_control.bypass_policy` are exempt; see [granting the exemptions](permissions.md#granting-the-exemptions). Writes with no request context, such as migrations, scripts and background jobs, are allowed: they are not interactive edits. +Users holding `netbox_change_control.bypass_policy` are exempt; see [granting the exemptions](permissions.md#granting-the-custom-actions). Writes with no request context, such as migrations, scripts and background jobs, are allowed: they are not interactive edits. > [!NOTE] > Superusers hold every permission, so a superuser is never blocked. diff --git a/netbox_change_control/__init__.py b/netbox_change_control/__init__.py index 4744853..6ce82aa 100644 --- a/netbox_change_control/__init__.py +++ b/netbox_change_control/__init__.py @@ -1,6 +1,6 @@ from netbox.plugins import PluginConfig, get_plugin_config -__version__ = '0.2.0' +__version__ = '0.3.0' class ChangeControlConfig(PluginConfig): @@ -25,12 +25,14 @@ class ChangeControlConfig(PluginConfig): # Refuse to merge a branch which has no approved change request. This is the core # guarantee of the plugin; disable it only to troubleshoot. 'enforce_merge_gate': True, - # Attach every matching policy to a change request and forbid its author from - # detaching them. Turning this off makes policies advisory. - 'lock_matched_policies': True, # Send a NetBox notification to the outstanding reviewers when a change request # needs review, and to the requester when it is approved or rejected. 'notify_reviewers': True, + # Where a branch page shows its change request. 'right_page' puts it in a card in + # the right-hand column, beside branching's own cards; 'alerts' puts it in a band + # across the top of the page, above them. Naming both shows both, and an empty list + # shows neither. + 'branch_page_placement': ['right_page'], # Which built-in pre-merge checks to make available. True offers all of them, False # none, and a list a subset. Registering only makes a check selectable: a policy # decides where it applies, by naming it in its Checks field. Valid names: diff --git a/netbox_change_control/api/serializers.py b/netbox_change_control/api/serializers.py index c32d79c..2679fb1 100644 --- a/netbox_change_control/api/serializers.py +++ b/netbox_change_control/api/serializers.py @@ -75,6 +75,10 @@ class ChangeRequestSerializer(NetBoxModelSerializer): branch_name = serializers.CharField(read_only=True) branch_deleted = serializers.BooleanField(read_only=True) ready_to_merge = serializers.BooleanField(source='is_ready_to_merge', read_only=True) + # Derived from the policy evaluation, so it is reported and never set. A writable status + # let a caller mark a request Completed, which is terminal and blocks its branch from ever + # merging. Use the abandon and reopen actions for the two transitions a person makes. + status = serializers.CharField(read_only=True) merge_blocked_reason = serializers.CharField(read_only=True) has_conflicts = serializers.BooleanField(read_only=True) @@ -148,7 +152,9 @@ def validate(self, attrs): super(), since NetBox's ValidatedModelSerializer builds a model instance from these attributes and calls full_clean() on it, which would reject the missing reviewer. """ - if self.instance is None: + # `not self.nested` matters: a nested serializer's to_internal_value returns a model + # instance rather than a dict, and assigning into it raises TypeError. + if self.instance is None and not self.nested: request = self.context.get('request') if request is not None: attrs['reviewer'] = request.user @@ -195,7 +201,35 @@ class Meta: class ChangeCommentSerializer(NetBoxModelSerializer): + """ + A comment is a personal statement, so the API records it as the caller and never as + somebody else. + + `author` is read-only and defaults to the requesting user, for the same reason `reviewer` + is on ReviewSerializer. Leaving it writable let any token holding `add_changecomment` post + a comment attributed to a colleague, which is enough to fake a sign-off in the discussion + a reviewer reads before approving. + """ + url = serializers.HyperlinkedIdentityField(view_name='plugins-api:netbox_change_control-api:changecomment-detail') + author = serializers.PrimaryKeyRelatedField(read_only=True) + + def validate(self, attrs): + """ + Attribute a new comment to the caller. + + A read-only field carrying a default is not enough, because DRF leaves a read-only + field out of validated_data entirely. The assignment also has to happen before + super(), since NetBox's ValidatedModelSerializer builds a model instance from these + attributes and calls full_clean() on it, which would reject the missing author. + """ + # `not self.nested` matters: a nested serializer's to_internal_value returns a model + # instance rather than a dict, and assigning into it raises TypeError. + if self.instance is None and not self.nested: + request = self.context.get('request') + if request is not None: + attrs['author'] = request.user + return super().validate(attrs) class Meta: model = ChangeComment diff --git a/netbox_change_control/api/views.py b/netbox_change_control/api/views.py index 6fcc265..1d10cb4 100644 --- a/netbox_change_control/api/views.py +++ b/netbox_change_control/api/views.py @@ -1,4 +1,12 @@ +from typing import ClassVar + +from django.shortcuts import get_object_or_404 +from netbox.api.authentication import TokenPermissions from netbox.api.viewsets import NetBoxModelViewSet +from rest_framework import status +from rest_framework.decorators import action +from rest_framework.exceptions import PermissionDenied +from rest_framework.response import Response from netbox_change_control import filtersets from netbox_change_control.models import ( @@ -9,6 +17,7 @@ PolicyRule, Review, ) +from netbox_change_control.permissions import ABANDON_PERMISSION, CHANGE_PERMISSION, REOPEN_PERMISSION from .serializers import ( ChangeCommentSerializer, @@ -41,11 +50,110 @@ class PolicyRuleViewSet(NetBoxModelViewSet): filterset_class = filtersets.PolicyRuleFilterSet +class ObjectActionPermissions(TokenPermissions): + """ + Permissions for an action performed *on* an object rather than one that creates one. + + NetBox maps every POST to `add_`, which is right for creating an object and wrong + here: abandoning a change request is not adding one, and requiring `add_changerequest` to + give up on somebody else's change is both surprising and too broad. + + The blanket requirement is dropped, and each action states the permission it wants and + restricts its own queryset to the objects the caller holds that permission on. Everything + else is inherited, including the check that a write token is not read-only. + """ + + perms_map: ClassVar = {**TokenPermissions.perms_map, 'POST': []} + + class ChangeRequestViewSet(NetBoxModelViewSet): queryset = ChangeRequest.objects.prefetch_related('policies', 'tags') serializer_class = ChangeRequestSerializer filterset_class = filtersets.ChangeRequestFilterSet + def _restricted(self, request, action_name, pk): + """ + Fetch the object, honouring any constraint on the caller's permission for this action. + + `self.get_object()` cannot be used: BaseViewSet.initial() has already narrowed + `self.queryset` to what the caller may *add*, which is the wrong question for these. + + A caller holding the permission but constrained away from this object gets a 404, + which is the NetBox convention for an object-level miss. Not holding it at all is a + 403, raised by the action before it gets here. + """ + return get_object_or_404(ChangeRequest.objects.restrict(request.user, action_name), pk=pk) + + # `status` is read-only on the serializer because it is derived from the policy + # evaluation. These are the two transitions a person makes by hand, each behind its own + # permission, so an integration can still give up on a change or take one back up without + # the field being writable and Completed being one typo away. + + @action(detail=True, methods=['post'], permission_classes=[ObjectActionPermissions]) + def submit(self, request, pk=None): + if not request.user.has_perm(CHANGE_PERMISSION): + raise PermissionDenied('You do not have permission to change change requests.') + + change_request = self._restricted(request, 'change', pk) + if not change_request.submit(): + return Response( + {'detail': 'Only a draft can be submitted for review.'}, + status=status.HTTP_409_CONFLICT, + ) + + from netbox_change_control import events + + events.emit(change_request, events.CHANGE_REQUEST_SUBMITTED) + change_request.refresh_from_db() + return Response(self.get_serializer(change_request).data) + + @action(detail=True, methods=['post'], url_path='return-to-draft') + def return_to_draft(self, request, pk=None): + if not request.user.has_perm(CHANGE_PERMISSION): + raise PermissionDenied('You do not have permission to change change requests.') + + change_request = self._restricted(request, 'change', pk) + if not change_request.return_to_draft(): + return Response( + { + 'detail': ( + f'A {change_request.get_status_display().lower()} change request cannot be returned to draft.' + ) + }, + status=status.HTTP_409_CONFLICT, + ) + + return Response(self.get_serializer(change_request).data) + + @action(detail=True, methods=['post'], permission_classes=[ObjectActionPermissions]) + def abandon(self, request, pk=None): + if not request.user.has_perm(ABANDON_PERMISSION): + raise PermissionDenied('You do not have permission to abandon change requests.') + + change_request = self._restricted(request, 'abandon', pk) + if not change_request.abandon(): + return Response( + {'detail': f'A {change_request.get_status_display().lower()} change request cannot be abandoned.'}, + status=status.HTTP_409_CONFLICT, + ) + + return Response(self.get_serializer(change_request).data) + + @action(detail=True, methods=['post'], permission_classes=[ObjectActionPermissions]) + def reopen(self, request, pk=None): + if not request.user.has_perm(REOPEN_PERMISSION): + raise PermissionDenied('You do not have permission to reopen change requests.') + + change_request = self._restricted(request, 'reopen', pk) + if not change_request.reopen(): + return Response( + {'detail': 'Only an abandoned change request can be reopened.'}, + status=status.HTTP_409_CONFLICT, + ) + + change_request.refresh_from_db() + return Response(self.get_serializer(change_request).data) + class ReviewViewSet(NetBoxModelViewSet): queryset = Review.objects.select_related('reviewer', 'change_request') diff --git a/netbox_change_control/automerge.py b/netbox_change_control/automerge.py index 9dc7562..dd0fd24 100644 --- a/netbox_change_control/automerge.py +++ b/netbox_change_control/automerge.py @@ -17,6 +17,7 @@ import logging from dataclasses import dataclass +from core.choices import JobStatusChoices from netbox.plugins import get_plugin_config from netbox_change_control.choices import ChangeRequestStatusChoices @@ -105,10 +106,6 @@ def try_auto_merge(change_request): return False branch = change_request.branch - indicator = branch.can_merge - if not indicator.permitted: - logger.debug('Auto-merge skipped for %s: %s', change_request, indicator.message) - return False # Enqueue rather than merge inline. try_auto_merge is reached from a signal, so a direct # call would run a whole branch merge inside the web request that submitted the final @@ -116,6 +113,26 @@ def try_auto_merge(change_request): # path the branching plugin's own merge button takes. from netbox_branching.jobs import MergeBranchJob + # One merge per branch, however many times this is reached. + # + # Nothing about becoming mergeable happens once. A single write can arrive here by more + # than one route: refreshing the status runs the checks, which try to merge, and a caller + # that then runs the checks itself tries again. The status is still Approved at the second + # call, because the merge has only been queued and not yet run, so the second call used to + # queue a duplicate. The first job merged and the second then failed with "not ready to + # merge", which reads as a broken merge on a change that in fact went through. + # + # Guarding on the queue rather than on the call sites is what makes this hold for routes + # nobody has thought of yet. This is the same test NetBox's own enqueue_once applies. + if already := MergeBranchJob.get_jobs(branch).filter(status__in=JobStatusChoices.ENQUEUED_STATE_CHOICES).first(): + logger.debug('Auto-merge for %s already queued as job %s', change_request, already.pk) + return False + + indicator = branch.can_merge + if not indicator.permitted: + logger.debug('Auto-merge skipped for %s: %s', change_request, indicator.message) + return False + logger.info('Enqueuing auto-merge for %s', change_request) MergeBranchJob.enqueue( instance=branch, diff --git a/netbox_change_control/batching.py b/netbox_change_control/batching.py new file mode 100644 index 0000000..7fc2f15 --- /dev/null +++ b/netbox_change_control/batching.py @@ -0,0 +1,102 @@ +""" +Collapsing repeated work on one change request. + +Refreshing a change request means recomputing its status and running its checks. Both are +correct to do on every event that could change the answer, and doing them per event is how the +plugin stays consistent without anybody remembering to call anything. + +The cost is that one user action is often many events. Submitting a request attaches every +matching policy, and each binding is its own signal, so a request governed by three policies +refreshed three times and ran every check three times before the view had even returned. The +answer was identical each time. + +This is the fix, and it is deliberately not `transaction.on_commit`. Deferring past the commit +would mean a request's checks are not yet run when the view that submitted it renders the next +page, and it would leave every test that asserts on a check result depending on +`captureOnCommitCallbacks`. Instead the work is collapsed within an explicit block: a caller +that knows it is about to cause a burst of events wraps it, and the refresh runs once per +change request when the block ends. + +Outside such a block `schedule_refresh` runs immediately, so nothing changes for the many +callers which are not part of a burst. +""" + +import logging +from contextlib import contextmanager +from contextvars import ContextVar + +__all__ = ( + 'batched', + 'schedule_refresh', +) + +logger = logging.getLogger('netbox.plugins.netbox_change_control.batching') + +# The change request ids waiting to be refreshed, or None when not inside a block. +# +# None and an empty set mean different things here: None is "refresh as you go", an empty set +# is "a block is open and nothing has asked yet". The two must not be conflated, or a block +# that happens to collect nothing would start running work immediately. +_pending = ContextVar('netbox_change_control.pending_refreshes', default=None) + + +@contextmanager +def batched(): + """ + Collapse the refreshes caused inside this block into one per change request. + + Nesting is safe: only the outermost block flushes, so a caller can wrap an operation + without knowing whether its own caller already did. + + Nothing is flushed if the block raises. The work would be computed against a state that is + about to roll back, and the exception is the caller's to handle rather than something to + bury under a burst of check runs. + """ + if _pending.get() is not None: + # Already inside a block. The outermost one owns the flush. + yield + return + + token = _pending.set(set()) + try: + yield + except Exception: + _pending.reset(token) + raise + else: + pending = _pending.get() + _pending.reset(token) + for change_request_id in sorted(pending): + _refresh(change_request_id) + + +def schedule_refresh(change_request): + """ + Refresh this change request's status and checks, now or at the end of the current block. + """ + if (pending := _pending.get()) is None: + _refresh(change_request.pk) + else: + pending.add(change_request.pk) + + +def _refresh(change_request_id): + """ + Recompute one change request's status, then run its checks. + + Status first, because `run_checks` may auto-merge and should decide against a current + status. `refresh_status` is told not to run the checks itself on the way to Approved, + since they are run here unconditionally a moment later; letting it would put the whole + check suite through twice for the one transition that matters most. + """ + from netbox_change_control.checks import run_checks + from netbox_change_control.models import ChangeRequest + from netbox_change_control.policy import refresh_status + + change_request = ChangeRequest.objects.filter(pk=change_request_id).first() + if change_request is None: + return + + refresh_status(change_request, run_checks_on_approval=False) + change_request.refresh_from_db() + run_checks(change_request) diff --git a/netbox_change_control/checks.py b/netbox_change_control/checks.py index 9c7ef6d..07a4787 100644 --- a/netbox_change_control/checks.py +++ b/netbox_change_control/checks.py @@ -195,6 +195,8 @@ def run_checks(change_request): sync_checks(change_request) applicable = expected_checks(change_request) + rows = {row.name: row for row in MergeCheck.objects.filter(change_request=change_request)} + for check in _registry.values(): # A policy-scoped check that no attached policy asked for has no row and must not run. if check.name not in applicable: @@ -205,17 +207,36 @@ def run_checks(change_request): logger.exception('Merge check %s raised', check.name) result = CheckResult(MergeCheckStatusChoices.ERROR, f'{type(e).__name__}: {e}'[:500]) - MergeCheck.objects.filter(change_request=change_request, name=check.name).update( - status=result.status, - summary=result.summary[:500], - details_url=result.details_url, - completed=timezone.now(), - ) + row = rows.get(check.name) + if row is None: + continue + + summary = result.summary[:500] + if (row.status, row.summary, row.details_url) == (result.status, summary, result.details_url): + # Nothing moved, so nothing is written. A re-run that finds the same answer is not + # a change, and recording one would fill the changelog with noise and bury the + # transitions that matter. + continue - # An in-process check turning green can be the last gate. The writes above use .update(), - # which fires no post_save, so the MergeCheck receiver never sees them; without this an - # auto-merge would wait for the periodic job instead of going immediately. + row.status = result.status + row.summary = summary + row.details_url = result.details_url + row.completed = timezone.now() + # save(), not queryset.update(). update() writes straight to the database and fires no + # post_save, so NetBox never recorded a change: a required check going from failed to + # passed, which is what opens the gate, left no entry in the changelog at all. For a + # plugin whose job is the record of who allowed what, "the pipeline went green at + # 14:02" is exactly the fact worth keeping. + row.save(update_fields=['status', 'summary', 'details_url', 'completed']) + + # An in-process check turning green can be the last gate a request was waiting on. The + # saves above reach the MergeCheck receiver, but only when a result actually moved, so this + # also covers the run where everything was already passing. from netbox_change_control.automerge import try_auto_merge + from netbox_change_control.policy import refresh_cached_state + + # A check result moves whether the request is ready, which the list reads from a cache. + refresh_cached_state(change_request) try_auto_merge(change_request) @@ -278,6 +299,12 @@ def check_threads_resolved(change_request): An open thread is an unanswered concern about a specific object. Merging past one loses the discussion, since the branch diff disappears once merged. """ + if change_request.branch_deleted: + # Nothing to merge, so nothing to hold up. Its siblings already skipped here, and this + # one failing instead made the documented promise that a branchless request skips its + # checks true of three checks out of four. + return CheckResult.skipped('The branch no longer exists.') + open_threads = change_request.change_comments.filter(parent__isnull=True, resolved=False) count = open_threads.count() if not count: diff --git a/netbox_change_control/filtersets.py b/netbox_change_control/filtersets.py index a021e21..bc43b11 100644 --- a/netbox_change_control/filtersets.py +++ b/netbox_change_control/filtersets.py @@ -249,6 +249,14 @@ class ChangeRequestFilterSet(NetBoxModelFilterSet): method='filter_has_window', label=_('Has a change window'), ) + has_conflicts = django_filters.BooleanFilter( + field_name='cached_conflicted', + label=_('Conflicts with main'), + ) + gates_cleared = django_filters.BooleanFilter( + field_name='cached_gates_cleared', + label=_('Policies and checks satisfied'), + ) class Meta: model = ChangeRequest diff --git a/netbox_change_control/forms.py b/netbox_change_control/forms.py index 370d398..8936667 100644 --- a/netbox_change_control/forms.py +++ b/netbox_change_control/forms.py @@ -16,12 +16,13 @@ MergeCheckStatusChoices, ReviewDecisionChoices, ) -from netbox_change_control.models import ChangeRequest, MergeCheck, Policy, PolicyRule, Review +from netbox_change_control.models import ChangeComment, ChangeRequest, MergeCheck, Policy, PolicyRule, Review # NetBox renders this next to any Markdown-capable field. MARKDOWN_HELP = _(' Markdown syntax is supported') __all__ = ( + 'ChangeCommentForm', 'ChangeRequestBulkEditForm', 'ChangeRequestFilterForm', 'ChangeRequestForm', @@ -33,7 +34,6 @@ 'PolicyRuleBulkEditForm', 'PolicyRuleFilterForm', 'PolicyRuleForm', - 'ReviewBulkEditForm', 'ReviewEditForm', 'ReviewFilterForm', 'ReviewForm', @@ -229,6 +229,29 @@ class Meta: fields = ('decision', 'comment', 'tags') +class ChangeCommentForm(NetBoxModelForm): + """ + Edit a comment. + + Only the text. `change_request`, `change_diff`, `parent` and `author` are all absent: a + comment is one person's remark about one changed object, so moving it or reattributing it + would rewrite a record somebody else is relying on. It is the same reasoning that keeps + those fields off ReviewEditForm. + """ + + text = forms.CharField( + widget=MarkdownWidget(attrs={'rows': 6}), + label=_('Comment'), + help_text=MARKDOWN_HELP, + ) + + fieldsets = (FieldSet('text', name=_('Comment')),) + + class Meta: + model = ChangeComment + fields = ('text', 'tags') + + class MergeCheckForm(NetBoxModelForm): change_request = DynamicModelChoiceField( queryset=ChangeRequest.objects.all(), @@ -312,7 +335,8 @@ class ChangeRequestFilterForm(NetBoxModelFilterSetForm): FieldSet('ref', 'status', 'priority', 'requester_id', name=_('Request')), FieldSet('branch_id', 'branch', 'branch_deleted', name=_('Branch')), FieldSet('policy_id', 'reviewer_id', 'has_reviews', name=_('Review')), - FieldSet('check_status', 'has_open_threads', name=_('Checks')), + FieldSet('check_status', 'has_open_threads', 'gates_cleared', name=_('Checks')), + FieldSet('has_conflicts', name=_('Conflicts')), FieldSet('has_window', 'auto_merge', name=_('Scheduling')), ) @@ -330,6 +354,14 @@ class ChangeRequestFilterForm(NetBoxModelFilterSetForm): has_reviews = forms.NullBooleanField(required=False, widget=forms.Select(choices=BOOLEAN_WITH_BLANK_CHOICES)) check_status = forms.MultipleChoiceField(choices=MergeCheckStatusChoices, required=False, label=_('Check status')) has_open_threads = forms.NullBooleanField(required=False, widget=forms.Select(choices=BOOLEAN_WITH_BLANK_CHOICES)) + has_conflicts = forms.NullBooleanField( + required=False, widget=forms.Select(choices=BOOLEAN_WITH_BLANK_CHOICES), label=_('Conflicts with main') + ) + gates_cleared = forms.NullBooleanField( + required=False, + widget=forms.Select(choices=BOOLEAN_WITH_BLANK_CHOICES), + label=_('Policies and checks satisfied'), + ) has_window = forms.NullBooleanField(required=False, widget=forms.Select(choices=BOOLEAN_WITH_BLANK_CHOICES)) auto_merge = forms.NullBooleanField(required=False, widget=forms.Select(choices=BOOLEAN_WITH_BLANK_CHOICES)) @@ -381,9 +413,18 @@ class PolicyRuleBulkEditForm(NetBoxModelBulkEditForm): class ChangeRequestBulkEditForm(NetBoxModelBulkEditForm): + """ + `status` is deliberately absent. + + It is a cached view of the policy evaluation, not something to type in. Offering it here + let anybody holding change_changerequest set a request to Completed, which is terminal: + the merge gate refuses a completed request and nothing reopens one, so the branch was + blocked for good. Abandoning and reopening are separate permissioned actions on the + change request itself. + """ + model = ChangeRequest ref = forms.CharField(max_length=100, required=False, label=_('Reference')) - status = forms.ChoiceField(choices=ChangeRequestStatusChoices, required=False) priority = forms.ChoiceField(choices=ChangeRequestPriorityChoices, required=False) description = forms.CharField(max_length=200, required=False) scheduled_start = forms.DateTimeField(required=False, widget=DateTimePicker()) @@ -391,8 +432,3 @@ class ChangeRequestBulkEditForm(NetBoxModelBulkEditForm): auto_merge = forms.NullBooleanField(required=False) nullable_fields = ('ref', 'description', 'scheduled_start', 'scheduled_end') - - -class ReviewBulkEditForm(NetBoxModelBulkEditForm): - model = Review - decision = forms.ChoiceField(choices=ReviewDecisionChoices, required=False) diff --git a/netbox_change_control/jobs.py b/netbox_change_control/jobs.py index c4e072d..3e87bca 100644 --- a/netbox_change_control/jobs.py +++ b/netbox_change_control/jobs.py @@ -6,7 +6,6 @@ start-up. Changing the setting therefore takes effect on the next worker restart. """ -from core.choices import JobIntervalChoices from django.core.exceptions import ImproperlyConfigured from netbox.jobs import JobRunner, system_job from netbox.plugins import get_plugin_config @@ -50,10 +49,3 @@ def run(self, *args, **kwargs): merged = run_due_auto_merges() return f'Merged {merged} change request(s).' - - -# Named intervals, re-exported so a configuration file can use them by name rather than by -# a bare number of minutes. -INTERVAL_MINUTELY = JobIntervalChoices.INTERVAL_MINUTELY -INTERVAL_HOURLY = JobIntervalChoices.INTERVAL_HOURLY -INTERVAL_DAILY = JobIntervalChoices.INTERVAL_DAILY diff --git a/netbox_change_control/migrations/0004_alter_changerequest_options.py b/netbox_change_control/migrations/0004_alter_changerequest_options.py new file mode 100644 index 0000000..9f261a0 --- /dev/null +++ b/netbox_change_control/migrations/0004_alter_changerequest_options.py @@ -0,0 +1,17 @@ +# Generated by Django 6.0.8 on 2026-08-31 13:30 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('netbox_change_control', '0003_changerequest_ref'), + ] + + operations = [ + migrations.AlterModelOptions( + name='changerequest', + options={'ordering': ('-created',), 'permissions': (('override_window_changerequest', 'Can merge a change request outside its change window'), ('abandon_changerequest', 'Can abandon a change request'), ('reopen_changerequest', 'Can reopen an abandoned change request'))}, + ), + ] diff --git a/netbox_change_control/migrations/0005_changerequest_cached_conflicted_and_more.py b/netbox_change_control/migrations/0005_changerequest_cached_conflicted_and_more.py new file mode 100644 index 0000000..e3de861 --- /dev/null +++ b/netbox_change_control/migrations/0005_changerequest_cached_conflicted_and_more.py @@ -0,0 +1,68 @@ +# Generated by Django 6.0.8 on 2026-08-31 14:11 + +from django.db import migrations, models + + +def populate_cached_state(apps, schema_editor): + """ + Fill the new columns for change requests that already exist. + + Without this an upgraded install shows every open request as conflict-free until something + happens to refresh it, which is worse than showing nothing: the column would be + confidently wrong. + + The **live** Branch model is used deliberately. A migration is normally handed historical + models, but those carry fields only and no methods, and working out whether a conflict is + real needs `Branch.get_unsynced_changes()`. Reading it from the historical model silently + did nothing at all, which is the failure this comment exists to prevent repeating. + + Only the conflict half is filled. It reads ChangeDiff and the branch's unsynced changes, + both in the main schema, so it is safe here. Readiness needs the policy evaluation, which + reaches into each branch's own schema; a migration is the wrong place to go looking for + those, so it is left at the default and corrects itself the first time anything touches + the request. + """ + ChangeRequest = apps.get_model('netbox_change_control', 'ChangeRequest') + + try: + from netbox_branching.models import Branch + from netbox_change_control.conflicts import conflicting_diffs + except Exception: + return + + branches = {b.pk: b for b in Branch.objects.all()} + conflicted = [] + for pk, branch_id in ChangeRequest.objects.filter(branch__isnull=False).values_list('pk', 'branch_id'): + branch = branches.get(branch_id) + if branch is None: + continue + try: + if conflicting_diffs(branch): + conflicted.append(pk) + except Exception: + # A branch whose schema has gone, or any other surprise, must not stop an upgrade. + continue + + if conflicted: + ChangeRequest.objects.filter(pk__in=conflicted).update(cached_conflicted=True) + + +class Migration(migrations.Migration): + + dependencies = [ + ('netbox_change_control', '0004_alter_changerequest_options'), + ] + + operations = [ + migrations.AddField( + model_name='changerequest', + name='cached_conflicted', + field=models.BooleanField(default=False, editable=False), + ), + migrations.AddField( + model_name='changerequest', + name='cached_gates_cleared', + field=models.BooleanField(default=False, editable=False), + ), + migrations.RunPython(populate_cached_state, migrations.RunPython.noop), + ] diff --git a/netbox_change_control/migrations/0006_remove_changerequestpolicy_created_and_more.py b/netbox_change_control/migrations/0006_remove_changerequestpolicy_created_and_more.py new file mode 100644 index 0000000..678efc0 --- /dev/null +++ b/netbox_change_control/migrations/0006_remove_changerequestpolicy_created_and_more.py @@ -0,0 +1,21 @@ +# Generated by Django 6.0.8 on 2026-08-31 19:39 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('netbox_change_control', '0005_changerequest_cached_conflicted_and_more'), + ] + + operations = [ + migrations.RemoveField( + model_name='changerequestpolicy', + name='created', + ), + migrations.RemoveField( + model_name='changerequestpolicy', + name='matched', + ), + ] diff --git a/netbox_change_control/migrations/0007_alter_changerequestpolicy_options.py b/netbox_change_control/migrations/0007_alter_changerequestpolicy_options.py new file mode 100644 index 0000000..1c27e5d --- /dev/null +++ b/netbox_change_control/migrations/0007_alter_changerequestpolicy_options.py @@ -0,0 +1,41 @@ +from django.db import migrations + +CODENAMES = ( + 'add_changerequestpolicy', + 'change_changerequestpolicy', + 'delete_changerequestpolicy', + 'view_changerequestpolicy', +) + + +def drop_stale_permissions(apps, schema_editor): + """ + Django creates the four permissions on the first migration and never removes them again, + so an existing install keeps rows the plugin no longer defines. Removing them here stops + the permission form offering an action nothing reads. + """ + ContentType = apps.get_model('contenttypes', 'ContentType') + Permission = apps.get_model('auth', 'Permission') + + content_type = ContentType.objects.filter(app_label='netbox_change_control', model='changerequestpolicy').first() + if content_type: + Permission.objects.filter(content_type=content_type, codename__in=CODENAMES).delete() + + +class Migration(migrations.Migration): + + dependencies = [ + ('auth', '0001_initial'), + ('contenttypes', '0002_remove_content_type_name'), + ('netbox_change_control', '0006_remove_changerequestpolicy_created_and_more'), + ] + + operations = [ + migrations.AlterModelOptions( + name='changerequestpolicy', + options={'default_permissions': (), 'ordering': ('policy__weight', 'policy__name')}, + ), + # Reversing this needs the model definition back as well, and Django recreates the + # permissions itself once it is, so there is nothing to undo here. + migrations.RunPython(drop_stale_permissions, migrations.RunPython.noop), + ] diff --git a/netbox_change_control/models/comments.py b/netbox_change_control/models/comments.py index 6840ac7..cca1374 100644 --- a/netbox_change_control/models/comments.py +++ b/netbox_change_control/models/comments.py @@ -72,6 +72,16 @@ def __str__(self): return f'{self.author} on {self.change_label or self.change_diff_id}' def save(self, *args, **kwargs): + # One level of nesting only, enforced here rather than only in clean(). + # + # clean() reassigns self.parent, but NetBox's ValidatedModelSerializer runs full_clean() + # on a throw-away copy and keeps only the original attributes, so the flattening was + # discarded on every REST write. A reply to a reply was then stored as a grandchild, + # and the Changes tab builds its threads from roots alone, so the comment rendered + # nowhere at all. + if self.parent_id and self.parent.parent_id: + self.parent = self.parent.parent + # Resolution belongs to a thread, not to an individual comment. Leaving the flag # settable on a reply produced rows that looked unresolved while their thread was # closed, which is what made the tab badge disagree with the page. @@ -95,6 +105,23 @@ def clean(self): if self.change_diff_id is None and self.pk is None: raise ValidationError({'change_diff': _('A comment must name the change it refers to.')}) + # The change must be one of this request's own. The Changes tab looks the diff up + # scoped to the branch, so the interface could not cross them, but the REST API took + # both as plain ids and would happily file a comment on one request against another + # request's diff. It would then be invisible on the tab it belongs to and counted as + # an open thread on a request it does not describe. + if self.change_diff_id and self.change_request_id: + # Fetch rather than dereference. A stale id raises ChangeDiff.DoesNotExist, which + # is not a ValidationError and escapes as a server error; a branch deleted + # concurrently is the realistic way to reach that. + from netbox_branching.models import ChangeDiff + + branch_id = ChangeDiff.objects.filter(pk=self.change_diff_id).values_list('branch_id', flat=True).first() + if branch_id is None: + raise ValidationError({'change_diff': _('That change no longer exists.')}) + if branch_id != self.change_request.branch_id: + raise ValidationError({'change_diff': _('That change belongs to a different branch.')}) + if self.parent_id: if self.parent_id == self.pk: raise ValidationError({'parent': _('A comment cannot reply to itself.')}) diff --git a/netbox_change_control/models/policies.py b/netbox_change_control/models/policies.py index 8d127fb..0a30a74 100644 --- a/netbox_change_control/models/policies.py +++ b/netbox_change_control/models/policies.py @@ -1,3 +1,5 @@ +from dataclasses import dataclass + from django.contrib.postgres.fields import ArrayField from django.core.exceptions import ValidationError from django.db import models @@ -10,9 +12,29 @@ __all__ = ( 'Policy', 'PolicyRule', + 'ReviewerSummary', ) +@dataclass(frozen=True) +class ReviewerSummary: + """ + Who may satisfy a rule, described the way the rule is written. + + A rule names groups and individual users. Rendering the people it currently resolves to + instead meant a group of fifteen printed fifteen usernames, on every rule it satisfied, + on two different pages, which buried the counts that are the point of the panel. It also + went stale in a way the policy never does: the list changed as people joined and left, + while the rule itself had not moved. + """ + + group_names: tuple + user_names: tuple + #: Whether anybody at all is eligible. A rule naming only an empty group can never be + #: satisfied, and saying so is the one thing the expanded list did better. + anybody: bool + + class Policy(PrimaryModel): """ A named set of approval rules, plus the scope which decides when those rules apply. @@ -96,10 +118,6 @@ def clean(self): except (InvalidCondition, ValueError) as e: raise ValidationError({'conditions': str(e)}) from e - @property - def applies_to_all_object_types(self): - return not self.object_types.exists() - class PolicyRule(NetBoxModel): """ @@ -165,6 +183,24 @@ def is_eligible(self, user): return True return self.groups.filter(pk__in=user.groups.values_list('pk', flat=True)).exists() + @property + def reviewer_summary(self): + """ + Describe who may satisfy this rule, for display. + + The groups and users are prefetched wherever a rule table is rendered, so this + normally costs nothing. The eligibility test is skipped entirely when the rule names + somebody directly, since that answer is already known. + """ + group_names = tuple(sorted(group.name for group in self.groups.all())) + user_names = tuple(sorted(user.username for user in self.users.all())) + + return ReviewerSummary( + group_names=group_names, + user_names=user_names, + anybody=bool(user_names) or (bool(group_names) and self.eligible_users().exists()), + ) + def eligible_users(self): """ Return a queryset of every user who may satisfy this rule. diff --git a/netbox_change_control/models/requests.py b/netbox_change_control/models/requests.py index 6f0faf7..c457441 100644 --- a/netbox_change_control/models/requests.py +++ b/netbox_change_control/models/requests.py @@ -87,6 +87,24 @@ class ChangeRequest(PrimaryModel): 'Merge as soon as the change is approved, every required check passes, and the change window is open.' ), ) + # Two cached views of the branch, kept so the change request list can show them without + # asking the database per row. + # + # This is the same split the plugin already makes for `status`: a cache for display and + # filtering, never for a decision. The merge gate recomputes, and so does the change + # request page. Nothing that permits a merge reads these. + cached_conflicted = models.BooleanField( + verbose_name=_('conflicts with main'), + default=False, + editable=False, + help_text=_('Whether the branch genuinely conflicts with main, as of the last refresh.'), + ) + cached_gates_cleared = models.BooleanField( + verbose_name=_('gates cleared'), + default=False, + editable=False, + help_text=_('Whether the policies and the required checks were satisfied at the last refresh.'), + ) policies = models.ManyToManyField( to='netbox_change_control.Policy', through='netbox_change_control.ChangeRequestPolicy', @@ -97,7 +115,14 @@ class ChangeRequest(PrimaryModel): class Meta: ordering = ('-created',) - permissions = (('override_window_changerequest', 'Can merge a change request outside its change window'),) + permissions = ( + ('override_window_changerequest', 'Can merge a change request outside its change window'), + # Status is derived from the policy evaluation, so it is not an editable field. + # The two transitions a person legitimately makes by hand get an action each, + # rather than leaving the field writable and hoping nobody sets it to completed. + ('abandon_changerequest', 'Can abandon a change request'), + ('reopen_changerequest', 'Can reopen an abandoned change request'), + ) verbose_name = _('change request') verbose_name_plural = _('change requests') @@ -216,7 +241,14 @@ def conflicts(self): @property def has_conflicts(self): - return bool(self.conflicts) + """ + The cached answer, so a list of change requests costs no queries for this column. + + Reading it live means one query for the diffs and one against the branch for what main + has done since the last sync, per row. Use `conflicts` where the actual objects are + wanted, which is a single change request's own page. + """ + return self.cached_conflicted @property def reconciled_conflicts(self): @@ -242,9 +274,28 @@ def merge_indicator(self): @property def is_ready_to_merge(self): + """ + The authoritative answer, recomputed now. Used by the change request page and the API. + """ indicator = self.merge_indicator return bool(indicator and indicator.permitted) + @property + def cached_ready_to_merge(self): + """ + The cheap answer, for a list of change requests. No queries at all. + + `cached_gates_cleared` covers the policies and the required checks, which only change + when something happens and so can be cached on that event. The change window is the + one gate that turns on the clock alone, with no event to hang a refresh on, so it is + evaluated here from two fields already loaded on the row. + + It does not account for merge validators registered by other plugins, which + `is_ready_to_merge` does. The change request page is the authoritative answer, and the + merge gate itself never reads this. + """ + return self.cached_gates_cleared and self.window_is_open + @property def merge_blocked_reason(self): """ @@ -261,6 +312,130 @@ def merge_blocked_reason(self): def is_open(self): return self.status in ChangeRequestStatusChoices.OPEN + @property + def can_be_submitted(self): + """ + A draft is the only thing there is to submit. + """ + return self.status == ChangeRequestStatusChoices.DRAFT + + @property + def can_return_to_draft(self): + """ + Anything open and already submitted can be pulled back. + + Including an approved one: an author who spots a problem after approval should be able + to take the change off the table rather than race the merge, and pulling it back is + the reversible way to do that. Abandoning is the one-way door. + """ + return self.is_open and self.status != ChangeRequestStatusChoices.DRAFT + + def submit(self): + """ + Put a draft into review, matching its policies. Returns True if the status moved. + + The status is set here rather than left to `refresh_status`, because that treats draft + as the author's to hold and will not move a request out of it on its own. + + That also means the announcement has to be made here. `refresh_status` tells the + reviewers only when it moves a request itself, and by the time it runs the status is + already Needs review, so it sees no transition and says nothing. Submitting without + telling anybody is the one outcome that makes the whole thing pointless. + """ + if not self.can_be_submitted: + return False + + from netbox_change_control.batching import batched, schedule_refresh + from netbox_change_control.policy import sync_policies + + self.status = ChangeRequestStatusChoices.NEEDS_REVIEW + self.save(update_fields=['status']) + + with batched(): + sync_policies(self) + schedule_refresh(self) + + # The refresh may have carried it further, to Approved on a policy needing no human or + # to Rejected on a standing rejection, and it announces those itself. Only the case it + # cannot see is left here. + self.refresh_from_db() + if self.status == ChangeRequestStatusChoices.NEEDS_REVIEW: + from netbox_change_control import events + from netbox_change_control.notifications import notify_status_change + + notify_status_change(self, ChangeRequestStatusChoices.NEEDS_REVIEW) + events.emit(self, events.CHANGE_REQUEST_REVIEW_REQUESTED) + + return True + + def return_to_draft(self): + """ + Pull a submitted request back out of review. Returns True if the status moved. + + The reviews are kept. They may already be stale, and if they are not they still stand: + a reviewer's position on the change does not stop being their position because the + author wants to work on it some more. What stops is the merge, because a draft is not + approved and the gate reads the status. + """ + if not self.can_return_to_draft: + return False + + self.status = ChangeRequestStatusChoices.DRAFT + self.save(update_fields=['status']) + + from netbox_change_control.policy import refresh_cached_state + + refresh_cached_state(self) + return True + + @property + def can_be_abandoned(self): + """ + An open request can be given up on. A finished one cannot: abandoning a merged change + would claim it never happened. + """ + return self.status in ChangeRequestStatusChoices.OPEN + + @property + def can_be_reopened(self): + """ + Only an abandoned request comes back. + + Completed is the record of a merge that actually happened, so reopening it would + invite a second merge of a branch that is already in main. + """ + return self.status == ChangeRequestStatusChoices.ABANDONED + + def abandon(self): + """ + Give up on this change request. Returns True if the status moved. + """ + if not self.can_be_abandoned: + return False + self.status = ChangeRequestStatusChoices.ABANDONED + self.save(update_fields=['status']) + return True + + def reopen(self): + """ + Bring an abandoned request back, then let the evaluation decide where it lands. + + It returns to Draft rather than to whatever it held before, because the reviews it + carried may since have gone stale and the policies may since have changed. Refreshing + computes the honest answer instead of restoring a remembered one. + """ + if not self.can_be_reopened: + return False + + from netbox_change_control.policy import refresh_status, sync_policies + + self.status = ChangeRequestStatusChoices.DRAFT + self.save(update_fields=['status']) + if not self.branch_deleted: + sync_policies(self) + refresh_status(self) + return True + def evaluate(self): """ Return a PolicyEvaluation describing which rules are satisfied. @@ -274,8 +449,12 @@ class ChangeRequestPolicy(models.Model): """ Through table binding a policy to a change request. - `matched` records whether the plugin attached the policy automatically from its scope. - Automatically matched policies cannot be removed by the request author. + `matched_object_types` records which object types in the branch caused the policy to + attach, which is what the change request page shows beside each policy. + + There is no "attached by hand" state. Which policies govern a change is decided from the + objects its branch touches, and nothing else can attach one, which is the whole point: + an author who could choose would choose the weakest. """ change_request = models.ForeignKey( @@ -288,19 +467,12 @@ class ChangeRequestPolicy(models.Model): on_delete=models.PROTECT, related_name='policy_bindings', ) - matched = models.BooleanField( - verbose_name=_('automatically matched'), - default=True, - ) matched_object_types = models.JSONField( verbose_name=_('matched object types'), default=list, blank=True, help_text=_('The object types in the branch which caused this policy to attach.'), ) - created = models.DateTimeField( - auto_now_add=True, - ) class Meta: ordering = ('policy__weight', 'policy__name') @@ -310,6 +482,11 @@ class Meta: name='%(app_label)s_%(class)s_unique_request_policy', ), ) + # Django creates four permissions for every model. No view of this plugin reads any of + # these, and granting one was a trap: an administrator reaching for + # delete_changerequestpolicy to detach a policy by hand gets it back at the next + # re-match. Restore them if this table ever grows a view. + default_permissions = () verbose_name = _('change request policy') verbose_name_plural = _('change request policies') diff --git a/netbox_change_control/permissions.py b/netbox_change_control/permissions.py index 4a4d595..60e737c 100644 --- a/netbox_change_control/permissions.py +++ b/netbox_change_control/permissions.py @@ -8,8 +8,11 @@ from netbox.context import current_request __all__ = ( + 'ABANDON_PERMISSION', 'BYPASS_PERMISSION', + 'CHANGE_PERMISSION', 'OVERRIDE_WINDOW_PERMISSION', + 'REOPEN_PERMISSION', 'current_user_has_perm', ) @@ -20,6 +23,16 @@ BYPASS_PERMISSION = 'netbox_change_control.bypass_policy' OVERRIDE_WINDOW_PERMISSION = 'netbox_change_control.override_window_changerequest' +# Status is derived from the policy evaluation, so it is not an editable field. These two are +# the transitions a person makes by hand, and each is granted separately: giving up on a change +# and taking one back up are different decisions from editing its title. +# Submitting and withdrawing are two halves of one control the author holds over whether the +# change is under review, so they share the ordinary change permission rather than inventing +# two more. Abandoning is different: it is a one-way door, and it has its own. +CHANGE_PERMISSION = 'netbox_change_control.change_changerequest' +ABANDON_PERMISSION = 'netbox_change_control.abandon_changerequest' +REOPEN_PERMISSION = 'netbox_change_control.reopen_changerequest' + def current_user_has_perm(permission, *, without_request): """ diff --git a/netbox_change_control/policy.py b/netbox_change_control/policy.py index fe12b66..aa61978 100644 --- a/netbox_change_control/policy.py +++ b/netbox_change_control/policy.py @@ -22,7 +22,9 @@ 'evaluate_change_request', 'get_touched_object_types', 'match_policies', + 'refresh_cached_state', 'refresh_status', + 'scope_may_have_drifted', 'sync_policies', ) @@ -203,11 +205,44 @@ def match_policies(branch): return results -def sync_policies(change_request): +def scope_may_have_drifted(change_request): + """ + Cheap test for whether a full re-match could attach a policy which is not attached yet. + + `match_policies` is not cheap: it reads every enabled policy, prefetches its object types, + and scans the branch diff once per policy carrying conditions. The merge gate is read + whenever a merge button is rendered, including once per row of the change request list, so + it must not pay that on every read. + + A policy can only newly match if it is enabled, is not attached already, and is either + unscoped or scoped to an object type the branch actually touches. That is two indexed + queries, and it is the whole candidate set, so a False here is a guarantee rather than a + guess. Conditions can only narrow a policy further, never widen it, so a conditional + policy which this misses could not have matched anyway. + + This deliberately says nothing about policies which should be *detached*. An attached + policy that no longer matches asks for approvals the change no longer needs, which is + tighter than the truth rather than looser, so the gate does not need to force that. """ - Attach every matching policy to the change request and drop stale automatic bindings. + from django.db.models import Q + + if change_request.branch_deleted: + return False + + touched = get_touched_object_types(change_request.branch) + attached = ChangeRequestPolicy.objects.filter(change_request=change_request).values_list('policy_id', flat=True) + + return ( + Policy.objects.filter(enabled=True) + .exclude(pk__in=attached) + .filter(Q(object_types__in=touched) | Q(object_types__isnull=True)) + .exists() + ) - Bindings the author added by hand (matched=False) are left alone. + +def sync_policies(change_request): + """ + Attach every matching policy to the change request, and drop the ones that no longer match. """ if change_request.branch_deleted: # The diff is gone, so there is nothing left to match against. Existing bindings are @@ -229,11 +264,10 @@ def sync_policies(change_request): ChangeRequestPolicy.objects.create( change_request=change_request, policy_id=policy_id, - matched=True, matched_object_types=names, ) - stale = [binding.pk for policy_id, binding in existing.items() if binding.matched and policy_id not in matches] + stale = [binding.pk for policy_id, binding in existing.items() if policy_id not in matches] if stale: ChangeRequestPolicy.objects.filter(pk__in=stale).delete() @@ -283,6 +317,52 @@ def evaluate_change_request(change_request): return evaluation +def refresh_cached_state(change_request): + """ + Recompute the two cached columns on a change request. Returns True if either moved. + + The change request list shows whether a branch conflicts with main and whether it is ready + to merge. Read live, those cost about eleven queries per row: the conflict test asks the + database twice, and the readiness test runs the whole merge gate. Fifty rows is five + hundred queries for two columns. + + Both are cached instead, and refreshed on the events that can change them, which is what + the callers of this function are. It is the same split the plugin already makes for + `status`: a cache for display and filtering, never for a decision. + + `cached_gates_cleared` deliberately excludes the change window. A window opens because the + clock moved, not because anything happened, so there is no event on which to refresh it; + `cached_ready_to_merge` combines this flag with the window at read time, from fields the + row already carries. + + It also computes the plugin's own gates directly rather than through `Branch.can_merge`. + Going through the gate would re-enter policy matching, which writes, which lands back + here. The cost is that a merge validator registered by another plugin is not reflected in + the column; the change request page and the gate itself both still recompute in full. + """ + from netbox_change_control.conflicts import conflicting_diffs + from netbox_change_control.validators import blocking_checks + + if change_request.branch_deleted: + conflicted = False + gates_cleared = False + else: + conflicted = bool(conflicting_diffs(change_request.branch)) + gates_cleared = ( + change_request.status == ChangeRequestStatusChoices.APPROVED + and evaluate_change_request(change_request).satisfied + and not blocking_checks(change_request) + ) + + if (change_request.cached_conflicted, change_request.cached_gates_cleared) == (conflicted, gates_cleared): + return False + + change_request.cached_conflicted = conflicted + change_request.cached_gates_cleared = gates_cleared + change_request.save(update_fields=['cached_conflicted', 'cached_gates_cleared']) + return True + + def _emit_lifecycle_event(status, change_request): """ Put the matching lifecycle event through NetBox's event pipeline, so an event rule can @@ -299,7 +379,7 @@ def _emit_lifecycle_event(status, change_request): events.emit(change_request, event_type) -def refresh_status(change_request): +def refresh_status(change_request, run_checks_on_approval=True): """ Recompute a change request's status from its current policy evaluation. @@ -307,11 +387,26 @@ def refresh_status(change_request): can change the outcome: a review added, edited or removed, or a policy attached or detached. Signal receivers call this so no caller can forget. - Terminal statuses are never reopened. + Two statuses are the author's to hold rather than the evaluation's to compute, and are + left alone here. + + Terminal statuses are never reopened. Draft means "not submitted", so a request the author + has pulled back must stay pulled back even while reviews arrive and policies move around + it; without this, the first signal after a withdrawal would push it straight back into + review. Submitting is what leaves draft, and it says so explicitly. + + `run_checks_on_approval` exists for the one caller which runs the checks itself immediately + afterwards. Reaching Approved normally has to refresh them here, but a caller that is about + to do it anyway would otherwise put the whole suite through twice. """ if change_request.status in ChangeRequestStatusChoices.TERMINAL: return change_request.status + if change_request.status == ChangeRequestStatusChoices.DRAFT: + # The cached columns still have to follow the branch: a draft can gain a conflict. + refresh_cached_state(change_request) + return change_request.status + evaluation = evaluate_change_request(change_request) if evaluation.rejections: status = ChangeRequestStatusChoices.REJECTED @@ -331,7 +426,7 @@ def refresh_status(change_request): notify_status_change(change_request, status, evaluation) _emit_lifecycle_event(status, change_request) - if status == ChangeRequestStatusChoices.APPROVED: + if status == ChangeRequestStatusChoices.APPROVED and run_checks_on_approval: # Reaching Approved is the moment a merge becomes possible, so refresh the checks # here. A branch edit invalidates the reviews but not the stored check results, so # without this a request could be edited to introduce a conflict, re-approved, and @@ -342,5 +437,11 @@ def refresh_status(change_request): from netbox_change_control.checks import run_checks run_checks(change_request) + # run_checks refreshes the cached columns itself, and recomputing them here would + # repeat the whole evaluation for the same answer. + return status + + # Last, so it reads the settled status. + refresh_cached_state(change_request) return status diff --git a/netbox_change_control/search.py b/netbox_change_control/search.py new file mode 100644 index 0000000..417062b --- /dev/null +++ b/netbox_change_control/search.py @@ -0,0 +1,114 @@ +""" +Global search. + +NetBox auto-imports a plugin's `search` module, and a model with no index registered here +simply never appears in the search box, however well it is filtered on its own list page. + +That was the state of every model in this plugin, which mattered most for `ChangeRequest.ref`: +the field exists so a change can be found by the ticket that spawned it, and the one place +somebody would type a ticket number is the search box. + +Weights follow NetBox's own convention: lower sorts first, so the most identifying field on +each model carries the smallest number. 100 is a name or an identifier, 500 a description, +5000 free prose. +""" + +from netbox.search import SearchIndex, register_search + +from netbox_change_control.models import ( + ChangeComment, + ChangeRequest, + MergeCheck, + Policy, + PolicyRule, + Review, +) + +__all__ = ( + 'ChangeCommentIndex', + 'ChangeRequestIndex', + 'MergeCheckIndex', + 'PolicyIndex', + 'PolicyRuleIndex', + 'ReviewIndex', +) + + +@register_search +class ChangeRequestIndex(SearchIndex): + """ + `ref` is weighted above the title deliberately. It is an exact external identifier, so + somebody typing `CHG0012345` means that one request and nothing else. + + `branch_name` is indexed rather than the branch itself, because it is the copy that + survives the branch being deleted, and a change request outliving its branch is exactly + when search is the only way left to find it. + """ + + model = ChangeRequest + fields = ( + ('ref', 100), + ('title', 150), + ('branch_name', 500), + ('description', 500), + ('comments', 5000), + ) + display_attrs = ('ref', 'status', 'priority', 'requester', 'branch_name', 'description') + + +@register_search +class PolicyIndex(SearchIndex): + model = Policy + fields = ( + ('name', 100), + ('description', 500), + ('comments', 5000), + ) + display_attrs = ('description',) + + +@register_search +class PolicyRuleIndex(SearchIndex): + model = PolicyRule + fields = (('name', 100),) + display_attrs = ('policy', 'min_reviews') + + +@register_search +class ReviewIndex(SearchIndex): + """ + A review is found by what the reviewer wrote. There is nothing else on it to search: the + decision is a choice field and the reviewer is a relation, both of which the list page + filters far better than free text would. + """ + + model = Review + fields = (('comment', 1000),) + display_attrs = ('change_request', 'reviewer', 'decision') + + +@register_search +class MergeCheckIndex(SearchIndex): + model = MergeCheck + fields = ( + ('name', 100), + ('label', 150), + ('summary', 1000), + ) + display_attrs = ('change_request', 'status', 'summary') + + +@register_search +class ChangeCommentIndex(SearchIndex): + """ + `change_label` is the name of the object the comment was about, kept on the comment so the + discussion still makes sense once the branch and its diff are gone. Indexing it is what + lets somebody find the conversation about a device months later. + """ + + model = ChangeComment + fields = ( + ('change_label', 500), + ('text', 1000), + ) + display_attrs = ('change_request', 'author', 'change_label') diff --git a/netbox_change_control/signal_receivers.py b/netbox_change_control/signal_receivers.py index 7a8a9a6..23c270b 100644 --- a/netbox_change_control/signal_receivers.py +++ b/netbox_change_control/signal_receivers.py @@ -12,12 +12,13 @@ from django.dispatch import receiver from netbox.plugins import get_plugin_config from netbox_branching.contextvars import active_branch -from netbox_branching.models import ChangeDiff +from netbox_branching.models import Branch, ChangeDiff from netbox_branching.signals import post_merge, post_revert, post_sync from users.models import Group, User from utilities.exceptions import AbortRequest from netbox_change_control.automerge import try_auto_merge +from netbox_change_control.batching import batched, schedule_refresh from netbox_change_control.checks import run_checks from netbox_change_control.choices import ChangeRequestStatusChoices, MergeCheckStatusChoices from netbox_change_control.models import ( @@ -30,16 +31,16 @@ Review, ) from netbox_change_control.permissions import BYPASS_PERMISSION, current_user_has_perm -from netbox_change_control.policy import refresh_status, sync_policies +from netbox_change_control.policy import refresh_cached_state, refresh_status, sync_policies __all__ = ( - 'auto_merge_on_check_result', 'complete_on_merge', 'emit_on_review_submitted', 'invalidate_approval_on_branch_change', 'mark_change_request_deleting', 'protect_main_on_delete', 'protect_main_on_save', + 'react_to_check_result', 'refresh_on_branch_change', 'refresh_on_group_membership_change', 'refresh_on_policy_binding_change', @@ -49,6 +50,7 @@ 'rerun_checks_on_comment_change', 'rerun_checks_on_diff_change', 'run_checks_for_new_request', + 'track_branch_name', 'unmark_change_request_deleting', ) @@ -202,9 +204,10 @@ def refresh_on_policy_binding_change(sender, instance, **kwargs): if change_request is None: return - # Status first: run_checks may auto-merge, and it should decide against a current status. - refresh_status(change_request) - run_checks(change_request) + # Scheduled rather than run here. Attaching a policy is one signal per policy, so a + # request governed by three of them refreshed three times and ran every check three times + # for the same answer. Inside a batched() block this collapses to one. + schedule_refresh(change_request) def _refresh(change_request_id): @@ -238,6 +241,10 @@ def complete_on_merge(sender, branch, **kwargs): change_request.status = ChangeRequestStatusChoices.COMPLETED change_request.save(update_fields=['status']) + # Completion never passes through refresh_status, so the cached columns the list shows + # would otherwise keep reporting a merged request as ready to merge. + refresh_cached_state(change_request) + # refresh_status emits the other transitions, but completion is set here and never passes # through it, so this is the only place the event can come from. from netbox_change_control import events @@ -245,6 +252,20 @@ def complete_on_merge(sender, branch, **kwargs): events.emit(change_request, events.CHANGE_REQUEST_COMPLETED) +@receiver(post_save, sender=Branch) +def track_branch_name(sender, instance, **kwargs): + """ + Keep the denormalised branch name in step with the branch. + + The name is stored on the change request so the record stays readable once the branch is + deleted, and it was written only when the change request itself was saved. A rename left + it behind: the page read the live branch and showed the new name, while the branch filter + and the global `q` search read the stored copy and still matched the old one. Searching for + a branch by the name on screen found nothing. + """ + ChangeRequest.objects.filter(branch=instance).exclude(branch_name=instance.name).update(branch_name=instance.name) + + @receiver([post_sync, post_revert]) def refresh_on_branch_change(sender, branch, **kwargs): """ @@ -257,9 +278,11 @@ def refresh_on_branch_change(sender, branch, **kwargs): change_request = ChangeRequest.objects.filter(branch=branch).first() if change_request is None: return - sync_policies(change_request) - run_checks(change_request) - refresh_status(change_request) + + # sync_policies can attach or detach several policies, each its own signal. + with batched(): + sync_policies(change_request) + schedule_refresh(change_request) # @@ -276,8 +299,11 @@ def _refresh_for_policies(policy_ids): policy_bindings__policy_id__in=policy_ids, status__in=ChangeRequestStatusChoices.OPEN, ).distinct() - for change_request in requests: - refresh_status(change_request) + # One rule edit can reach the same request through more than one policy, so the batch + # deduplicates as well as collapsing. + with batched(): + for change_request in requests: + schedule_refresh(change_request) @receiver([post_save, post_delete], sender=Policy) @@ -370,16 +396,90 @@ def rerun_checks_on_comment_change(sender, instance, **kwargs): @receiver(post_save, sender=MergeCheck) -def auto_merge_on_check_result(sender, instance, **kwargs): +def react_to_check_result(sender, instance, **kwargs): """ - A passing check can be the last gate a request was waiting on. + A check result moves whether the request is ready, and can be the last gate it was + waiting on. + + Both halves matter, and they are not the same condition. The cached readiness the change + request list shows has to follow a result in either direction: a pipeline reporting + success over the REST API is the common way the last gate clears, and it reaches this + plugin as a plain save with no other signal behind it. Auto-merge only cares about a + result that passes. """ - if not instance.is_passing or _is_being_deleted(instance.change_request_id): + if _is_being_deleted(instance.change_request_id): return change_request = ChangeRequest.objects.filter(pk=instance.change_request_id).first() if change_request is None: return - try_auto_merge(change_request) + + refresh_cached_state(change_request) + + if instance.is_passing: + try_auto_merge(change_request) + + +def _scope_may_have_changed(diff): + """ + Cheap test for whether one new ChangeDiff can change which policies match. + + A policy scoped by object type cannot change its answer for the second object of a type + the branch already held, so the common case of a bulk edit costs one indexed query per + object rather than a full re-match. + + A policy carrying conditions reads the changed objects themselves, so for those any new + object can change the answer and the re-match has to run. + """ + first_of_its_type = ( + not ChangeDiff.objects.filter(branch_id=diff.branch_id, object_type_id=diff.object_type_id) + .exclude(pk=diff.pk) + .exists() + ) + if first_of_its_type: + return True + return Policy.objects.filter(enabled=True).exclude(conditions__isnull=True).exists() + + +@receiver(post_save, sender=ChangeDiff) +def resync_policies_on_new_diff(sender, instance, created, **kwargs): + """ + Re-match the policies when the branch starts touching something new. + + Which policies govern a change request is decided from the object types in its branch. + That question used to be asked twice only: when the author pressed Submit for review, and + when branching synced or reverted the branch. An ordinary edit inside a branch writes an + ObjectChange and a ChangeDiff, and neither re-asked it, so the governing set stayed frozen + against the branch as it looked at submission. + + That was a way round the gate. An author could open a request on a branch touching only + low-risk objects, collect the light approval that attracted, then add the real change to + the same branch. The approvals went stale and the status returned to Needs review, but the + policy governing the new object type never attached, so the same reviewer could approve a + second time and merge work nobody with the authority to judge it had seen. + + A ChangeDiff is created once per changed object, which makes it the cheapest signal + meaning "this branch now holds something it did not before". Reacting to ObjectChange + instead would fire on every save of every object for the same answer. + """ + if not created: + return + + change_request = ChangeRequest.objects.filter( + branch_id=instance.branch_id, + status__in=ChangeRequestStatusChoices.OPEN, + ).first() + if change_request is None or _is_being_deleted(change_request.pk): + return + + if not _scope_may_have_changed(instance): + return + + # A newly attached policy brings rules with it, so the request may no longer be satisfied. + # sync_policies writes bindings, whose own receiver schedules the refresh; scheduling it + # here too covers the case where the matched set turned out to be unchanged. + with batched(): + sync_policies(change_request) + schedule_refresh(change_request) @receiver(post_save, sender=ChangeDiff) @@ -402,15 +502,29 @@ def rerun_checks_on_diff_change(sender, instance, **kwargs): if change_request is None or _is_being_deleted(change_request.pk): return - stored = change_request.checks.filter(name='no-conflicts').first() - if stored is None: + # A clean diff on a request nothing has ever flagged cannot change the answer. Another + # diff turning conflicted arrives as its own save, so it is caught there. This matters: + # a bulk edit inside a branch writes one ChangeDiff per object, and without this guard + # every one of them pays for the conflict test below. + if not instance.conflicts and not change_request.cached_conflicted: return - # Use the same real-versus-reconciled test the check applies, or this would keep - # re-running checks over a flag the check deliberately ignores. + # Use the same real-versus-reconciled test the check applies, or this would react to a + # flag the check deliberately ignores. Computed once and used for both jobs below. from netbox_change_control.conflicts import conflicting_diffs conflicted = bool(conflicting_diffs(change_request.branch)) + + # The change request list reads a cached conflict flag, and it has to follow the diff + # whether or not any policy asked for the no-conflicts check. Only that check creates the + # row consulted below, so a request governed by a policy which does not require it had no + # path back to the cache at all. + if conflicted != change_request.cached_conflicted: + ChangeRequest.objects.filter(pk=change_request.pk).update(cached_conflicted=conflicted) + + stored = change_request.checks.filter(name='no-conflicts').first() + if stored is None: + return if conflicted == (stored.status != MergeCheckStatusChoices.SUCCESS): # The stored result already agrees with reality. return diff --git a/netbox_change_control/tables.py b/netbox_change_control/tables.py index 58d8e56..357bdd5 100644 --- a/netbox_change_control/tables.py +++ b/netbox_change_control/tables.py @@ -20,7 +20,7 @@ class ConflictsColumn(tables.TemplateColumn): """ template_code = """ - {% if record.has_conflicts %} + {% if record.cached_conflicted %} {% else %} {{ ''|placeholder }} @@ -87,15 +87,22 @@ class ChangeRequestTable(NetBoxTable): priority = columns.ChoiceFieldColumn() requester = tables.Column(linkify=True) policies = columns.ManyToManyColumn(verbose_name=_('Policies')) - review_count = tables.Column(accessor='reviews__count', verbose_name=_('Reviews')) + # No accessor: the name matches the annotation ChangeRequestListView adds, and declaring + # `reviews__count` instead resolved the related manager and called .count() per row, + # paying for a query the annotation had already done in bulk. + review_count = tables.Column(verbose_name=_('Reviews')) auto_merge = columns.BooleanColumn(verbose_name=_('Auto merge')) - is_ready_to_merge = columns.BooleanColumn( + # Both read a cached field rather than recomputing per row. Live, they cost about eleven + # queries each row, which is five hundred for a default page of fifty. Sortable as a + # result, which the live versions could never be. + cached_ready_to_merge = columns.BooleanColumn( verbose_name=_('Ready to merge'), - orderable=False, + accessor='cached_ready_to_merge', + order_by='cached_gates_cleared', ) - has_conflicts = ConflictsColumn( + cached_conflicted = ConflictsColumn( verbose_name=_('Conflicts'), - orderable=False, + order_by='cached_conflicted', ) tags = columns.TagColumn(url_name='plugins:netbox_change_control:changerequest_list') @@ -117,7 +124,8 @@ class Meta(NetBoxTable.Meta): 'scheduled_start', 'scheduled_end', 'auto_merge', - 'is_ready_to_merge', + 'cached_ready_to_merge', + 'cached_conflicted', 'created', 'tags', ) @@ -127,8 +135,8 @@ class Meta(NetBoxTable.Meta): 'description', 'branch', 'status', - 'has_conflicts', - 'is_ready_to_merge', + 'cached_conflicted', + 'cached_ready_to_merge', 'priority', 'requester', 'policies', diff --git a/netbox_change_control/template_content.py b/netbox_change_control/template_content.py new file mode 100644 index 0000000..ed379eb --- /dev/null +++ b/netbox_change_control/template_content.py @@ -0,0 +1,121 @@ +""" +Content injected into netbox-branching's own pages. + +A branch and its change request are two halves of one job, and until now they only pointed one +way: the change request page carries the merge button, while the branch page said nothing at +all. Somebody who had just finished working in a branch had to leave it, find another menu, +and search for the request by name. Somebody whose merge was refused was told the reason on +branching's merge form, in plain text, with nothing to click. + +This closes the loop. Both hooks are read-only and cheap; neither changes how branching +behaves. + +Where it appears is configuration, because the two placements suit different pages and neither +is obviously right until you have looked at both. `branch_page_placement` names them: + + 'right_page' a card in the right-hand column, beside the branch's own cards + 'alerts' a band across the top of the page, above them + +The card is the default, because it reads as part of the page rather than as an interruption. +Naming both shows both, which is how you compare them. An empty list shows neither. +""" + +import logging + +from netbox.plugins import PluginTemplateExtension, get_plugin_config + +__all__ = ( + 'ALERTS', + 'PLACEMENTS', + 'RIGHT_PAGE', + 'BranchChangeRequest', + 'configured_placements', + 'template_extensions', +) + +logger = logging.getLogger('netbox.plugins.netbox_change_control') + +ALERTS = 'alerts' +RIGHT_PAGE = 'right_page' +PLACEMENTS = (ALERTS, RIGHT_PAGE) + + +def configured_placements(): + """ + The placements the deployment asked for, dropping any name which is not one. + + An unrecognised name is logged and skipped rather than raising, the same way an unknown + built-in check is, so a typo in configuration does not stop NetBox booting. + """ + configured = get_plugin_config('netbox_change_control', 'branch_page_placement') or () + if isinstance(configured, str): + configured = (configured,) + + placements = [] + for name in configured: + if name not in PLACEMENTS: + logger.warning( + "Unknown placement '%s' in branch_page_placement. Valid names: %s", + name, + ', '.join(PLACEMENTS), + ) + continue + placements.append(name) + + return placements + + +class BranchChangeRequest(PluginTemplateExtension): + """ + Show a branch its change request, or offer to open one. + """ + + models = ('netbox_branching.branch',) + + def alerts(self): + return self.render_placement(ALERTS) + + def right_page(self): + return self.render_placement(RIGHT_PAGE) + + def render_placement(self, placement): + """ + Render the panel for one placement, or nothing if it is not configured. + + The two placements render the same content in a different frame, so they share their + wording and differ only in which template wraps it. + """ + if placement not in configured_placements(): + return '' + + from netbox_change_control.models import ChangeRequest + + branch = self.context.get('object') + if branch is None: + return '' + + suffix = '_card' if placement == RIGHT_PAGE else '' + change_request = ChangeRequest.objects.filter(branch=branch).select_related('requester').first() + + if change_request is None: + # The gate refuses a branch with no change request, so saying so here, next to a + # button that fixes it, is the difference between a dead end and a next step. + return self.render( + f'netbox_change_control/inc/branch_no_request{suffix}.html', + extra_context={'branch': branch}, + ) + + # Evaluated rather than read from the cached column: this is one object on one page, + # which is exactly where the authoritative answer belongs, and it is what the merge + # will actually use. + return self.render( + f'netbox_change_control/inc/branch_change_request{suffix}.html', + extra_context={ + 'change_request': change_request, + 'evaluation': change_request.evaluate(), + 'blocked_reason': change_request.merge_blocked_reason, + }, + ) + + +template_extensions = (BranchChangeRequest,) diff --git a/netbox_change_control/templates/netbox_change_control/changerequest.html b/netbox_change_control/templates/netbox_change_control/changerequest.html index 66892b8..4417618 100644 --- a/netbox_change_control/templates/netbox_change_control/changerequest.html +++ b/netbox_change_control/templates/netbox_change_control/changerequest.html @@ -3,6 +3,41 @@ {% load plugins %} {% load form_helpers %} +{% block extra_controls %} + {% if can_submit %} +
+ {% csrf_token %} + +
+ {% endif %} + {% if can_return_to_draft %} +
+ {% csrf_token %} + +
+ {% endif %} + {% if can_abandon %} +
+ {% csrf_token %} + +
+ {% endif %} + {% if can_reopen %} +
+ {% csrf_token %} + +
+ {% endif %} +{% endblock extra_controls %} + {% block content %} {% if conflicts %}
@@ -32,11 +67,15 @@ {% if reconciled_conflicts %}
-
- - {% blocktrans count counter=reconciled_conflicts|length %}The branching plugin flags {{ counter }} object as conflicting, but a sync has already reconciled it.{% plural %}The branching plugin flags {{ counter }} objects as conflicting, but a sync has already reconciled them.{% endblocktrans %} -
- {% trans "Main has not changed these objects since this branch last synced, so merging cannot discard anything. The branch page will still ask you to acknowledge them." %} + {# An alert lays its direct children out in a row, so the second line has to sit inside #} + {# the text block. As a sibling of the sentence it reads beside it, not under it. #} +
+ +
+ {% blocktrans count counter=reconciled_conflicts|length %}The branching plugin flags {{ counter }} object as conflicting, but a sync has already reconciled it.{% plural %}The branching plugin flags {{ counter }} objects as conflicting, but a sync has already reconciled them.{% endblocktrans %} +
+ {% trans "Main has not changed these objects since this branch last synced, so merging cannot discard anything. The branch page will still ask you to acknowledge them." %} +
@@ -45,11 +84,13 @@ {% if window_warning %}
-
- - {% trans "This change may never merge automatically." %} -
- {% blocktrans with window=window_warning.window_minutes interval=window_warning.interval_minutes %}The change window is {{ window }} minutes long, but automatic merges are only attempted every {{ interval }} minutes, so the window can pass unnoticed. Widen the window, or lower auto_merge_interval.{% endblocktrans %} +
+ +
+ {% trans "This change may never merge automatically." %} +
+ {% blocktrans with window=window_warning.window_minutes interval=window_warning.interval_minutes %}The change window is {{ window }} minutes long, but automatic merges are only attempted every {{ interval }} minutes, so the window can pass unnoticed. Widen the window, or lower auto_merge_interval.{% endblocktrans %} +
@@ -127,8 +168,7 @@

{{ rule.rule.policy }} / {{ rule.rule.name }}
{% if rule.required %} - {% trans "May approve:" %} - {% for user in rule.rule.eligible_users %}{{ user.username }}{% if not forloop.last %}, {% endif %}{% empty %}{% trans "nobody" %}{% endfor %} + {% include 'netbox_change_control/inc/rule_reviewers.html' with rule=rule.rule %} {% else %} {% trans "The pre-merge checks are the only gate." %} {% endif %} @@ -162,7 +202,7 @@

{{ check.display_label }} - {% if check.required %}{% trans "Required" %}{% endif %} + {% if check.required %}{% trans "Required" %}{% endif %} {% if check.summary %}
{{ check.summary }}
{% endif %} @@ -222,22 +262,13 @@

{% trans "Applied policies" %}

{{ binding.policy|linkify }} - {% if binding.matched %}{% trans "Auto" %}{% endif %} - {% for name in binding.matched_object_types %}{{ name }} {% endfor %} + {% for name in binding.matched_object_types %}{{ name }} {% endfor %} {% empty %} - {% trans "No policies attached." %} + {% trans "No policies attached yet. Submit the request for review to match them." %} {% endfor %} - {% if object.status == 'draft' %} - - {% endif %}
{% plugin_left_page object %}

diff --git a/netbox_change_control/templates/netbox_change_control/changerequest_changes.html b/netbox_change_control/templates/netbox_change_control/changerequest_changes.html index 3d1e51f..5dff48d 100644 --- a/netbox_change_control/templates/netbox_change_control/changerequest_changes.html +++ b/netbox_change_control/templates/netbox_change_control/changerequest_changes.html @@ -53,8 +53,9 @@

{{ thread.comment.author }} - {{ thread.comment.created }} + {{ thread.comment.created|isodatetime:"minutes" }} {% if thread.comment.resolved %}{% trans "Resolved" %}{% endif %} + {% include 'netbox_change_control/inc/comment_actions.html' with comment=thread.comment %}
{% if can_resolve %}
@@ -69,7 +70,11 @@

{% for reply in thread.replies %}
-
{{ reply.author }} {{ reply.created }}
+
+ {{ reply.author }} + {{ reply.created|isodatetime:"minutes" }} + {% include 'netbox_change_control/inc/comment_actions.html' with comment=reply %} +
{{ reply.text|markdown }}
{% endfor %} @@ -123,16 +128,16 @@

{% trans "Discussion from the deleted branch" %}

{{ thread.comment.author }} - {{ thread.comment.created }} + {{ thread.comment.created|isodatetime:"minutes" }} {% if thread.comment.change_label %} - {{ thread.comment.change_label }} + {{ thread.comment.change_label }} {% endif %} {% if thread.comment.resolved %}{% trans "Resolved" %}{% endif %}
{{ thread.comment.text|markdown }}
{% for reply in thread.replies %}
-
{{ reply.author }} {{ reply.created }}
+
{{ reply.author }} {{ reply.created|isodatetime:"minutes" }}
{{ reply.text|markdown }}
{% endfor %} diff --git a/netbox_change_control/templates/netbox_change_control/inc/branch_change_request.html b/netbox_change_control/templates/netbox_change_control/inc/branch_change_request.html new file mode 100644 index 0000000..aa5d725 --- /dev/null +++ b/netbox_change_control/templates/netbox_change_control/inc/branch_change_request.html @@ -0,0 +1,20 @@ +{% load i18n %} +{% comment %} +The change request governing this branch, as an alert across the top of the branch page. + +Read against the merge form beside it: branching states the refusal, this states what would +clear it and links to the place it can be cleared. +{% endcomment %} +
+
+ + {% trans "Change request" %} + {% badge change_request.get_status_display bg_color=change_request.get_status_color %} + {% if change_request.ref %}{{ change_request.ref }}{% endif %} + + {% include 'netbox_change_control/inc/branch_change_request_body.html' %} +
+ + {% trans "Open change request" %} + +
diff --git a/netbox_change_control/templates/netbox_change_control/inc/branch_change_request_body.html b/netbox_change_control/templates/netbox_change_control/inc/branch_change_request_body.html new file mode 100644 index 0000000..8e3f429 --- /dev/null +++ b/netbox_change_control/templates/netbox_change_control/inc/branch_change_request_body.html @@ -0,0 +1,23 @@ +{% load i18n %} +{% comment %} +What the branch page says about its change request, without the box around it. The alert and +the card both show this; only the frame differs, so the wording cannot drift between them. +{% endcomment %} +
{{ change_request.title }}
+ +{% if change_request.is_ready_to_merge %} +
+ + {% trans "Every gate is satisfied. This branch can be merged." %} +
+{% else %} +
{{ blocked_reason }}
+ {% for rule in evaluation.rules %} + {% if not rule.satisfied %} +
+ {{ rule.rule.policy }} / {{ rule.rule.name }}: {{ rule.count }} / {{ rule.required }} + · {% include 'netbox_change_control/inc/rule_reviewers.html' with rule=rule.rule %} +
+ {% endif %} + {% endfor %} +{% endif %} diff --git a/netbox_change_control/templates/netbox_change_control/inc/branch_change_request_card.html b/netbox_change_control/templates/netbox_change_control/inc/branch_change_request_card.html new file mode 100644 index 0000000..8a61a78 --- /dev/null +++ b/netbox_change_control/templates/netbox_change_control/inc/branch_change_request_card.html @@ -0,0 +1,21 @@ +{% load i18n %} +{% comment %} +The same content as the alert, as a card in the branch page's right-hand column. + +Written the way branching writes its own cards on that page: an `h5` header, and a coloured +border for the state, as its Created and Deleted cards do. Green means every gate is satisfied, +which is what the alert says with its own background. +{% endcomment %} +
+
+ {% trans "Change request" %} + {% badge change_request.get_status_display bg_color=change_request.get_status_color %} +
+
+ {% if change_request.ref %}{{ change_request.ref }}{% endif %} + {% include 'netbox_change_control/inc/branch_change_request_body.html' %} + + {% trans "Open change request" %} + +
+
diff --git a/netbox_change_control/templates/netbox_change_control/inc/branch_no_request.html b/netbox_change_control/templates/netbox_change_control/inc/branch_no_request.html new file mode 100644 index 0000000..7db591e --- /dev/null +++ b/netbox_change_control/templates/netbox_change_control/inc/branch_no_request.html @@ -0,0 +1,20 @@ +{% load i18n %} +{% comment %} +Shown as an alert across the top of a branch page which has no change request. Only a branch +which is ready is offered one: a branch still provisioning cannot merge for another reason. +{% endcomment %} +{% if branch.ready %} +
+
+ + {% trans "No change request" %} + {% include 'netbox_change_control/inc/branch_no_request_body.html' %} +
+ {% if perms.netbox_change_control.add_changerequest %} + + {% trans "Open a change request" %} + + {% endif %} +
+{% endif %} diff --git a/netbox_change_control/templates/netbox_change_control/inc/branch_no_request_body.html b/netbox_change_control/templates/netbox_change_control/inc/branch_no_request_body.html new file mode 100644 index 0000000..7d1e2f6 --- /dev/null +++ b/netbox_change_control/templates/netbox_change_control/inc/branch_no_request_body.html @@ -0,0 +1,8 @@ +{% load i18n %} +{% comment %} +A branch with no change request cannot merge. Saying so here, beside the button that fixes it, +turns a refusal on the merge form into a next step. +{% endcomment %} +
+ {% trans "This branch cannot be merged until a change request is opened against it and approved." %} +
diff --git a/netbox_change_control/templates/netbox_change_control/inc/branch_no_request_card.html b/netbox_change_control/templates/netbox_change_control/inc/branch_no_request_card.html new file mode 100644 index 0000000..5e527a3 --- /dev/null +++ b/netbox_change_control/templates/netbox_change_control/inc/branch_no_request_card.html @@ -0,0 +1,19 @@ +{% load i18n %} +{% comment %} +The same message as the alert, as a card in the right-hand column, coloured the way branching +colours a card that wants something from you. +{% endcomment %} +{% if branch.ready %} +
+
{% trans "No change request" %}
+
+ {% include 'netbox_change_control/inc/branch_no_request_body.html' %} + {% if perms.netbox_change_control.add_changerequest %} + + {% trans "Open a change request" %} + + {% endif %} +
+
+{% endif %} diff --git a/netbox_change_control/templates/netbox_change_control/inc/comment_actions.html b/netbox_change_control/templates/netbox_change_control/inc/comment_actions.html new file mode 100644 index 0000000..9a48ac1 --- /dev/null +++ b/netbox_change_control/templates/netbox_change_control/inc/comment_actions.html @@ -0,0 +1,21 @@ +{% load i18n %} +{% comment %} +Edit and delete links for one comment. + +Kept small and quiet: this is a discussion, not an object list, so the actions sit beside the +timestamp rather than as buttons. Expects `comment`. +{% endcomment %} +{% if comment.author_id == request.user.pk or perms.netbox_change_control.change_changecomment %} + {% if comment.author_id == request.user.pk %} + + + + {% endif %} +{% endif %} +{% if perms.netbox_change_control.delete_changecomment %} + + + +{% endif %} diff --git a/netbox_change_control/templates/netbox_change_control/inc/review_form.html b/netbox_change_control/templates/netbox_change_control/inc/review_form.html index 65e081a..87a6397 100644 --- a/netbox_change_control/templates/netbox_change_control/inc/review_form.html +++ b/netbox_change_control/templates/netbox_change_control/inc/review_form.html @@ -8,10 +8,15 @@ {% csrf_token %}
+ {% comment %} + Preselect the standing decision. Without it a reviewer editing their own review saw + their comment prefilled but the decision reset to Approve, so the form silently offered + to turn a "Request changes" into an approval. + {% endcomment %}
diff --git a/netbox_change_control/templates/netbox_change_control/inc/rule_reviewers.html b/netbox_change_control/templates/netbox_change_control/inc/rule_reviewers.html new file mode 100644 index 0000000..b262141 --- /dev/null +++ b/netbox_change_control/templates/netbox_change_control/inc/rule_reviewers.html @@ -0,0 +1,24 @@ +{% load i18n %} +{% comment %} +Who may satisfy one rule, written the way the rule is. + +Shared by the change request page and the alert on the branch page so the two cannot drift. +Expects `rule` to be a PolicyRule. + +The names are always shown, including when they resolve to nobody: a rule pointing at an empty +group can never be satisfied, and naming the group is the difference between knowing that and +knowing what to fix. +{% endcomment %} +{% with summary=rule.reviewer_summary %} + {% trans "May approve:" %} + {% if summary.group_names or summary.user_names %} + {% for name in summary.group_names %}{{ name }}{% if not forloop.last %}, {% endif %}{% endfor %} + {% if summary.group_names and summary.user_names %}, {% endif %} + {% for name in summary.user_names %}{{ name }}{% if not forloop.last %}, {% endif %}{% endfor %} + {% if not summary.anybody %} + {% trans "(no members, so this rule can never be satisfied)" %} + {% endif %} + {% else %} + {% trans "nobody, so this rule can never be satisfied" %} + {% endif %} +{% endwith %} diff --git a/netbox_change_control/templates/netbox_change_control/policy.html b/netbox_change_control/templates/netbox_change_control/policy.html index af2528a..b9074e4 100644 --- a/netbox_change_control/templates/netbox_change_control/policy.html +++ b/netbox_change_control/templates/netbox_change_control/policy.html @@ -25,7 +25,7 @@

{% trans "Scope" %}

{% trans "Object types" %} {% for ot in object.object_types.all %} - {{ ot.app_label }}.{{ ot.model }} + {{ ot.app_label }}.{{ ot.model }} {% empty %} {% trans "Every object type" %} {% endfor %} diff --git a/netbox_change_control/templates/netbox_change_control/review.html b/netbox_change_control/templates/netbox_change_control/review.html index fbff78d..4ce8e24 100644 --- a/netbox_change_control/templates/netbox_change_control/review.html +++ b/netbox_change_control/templates/netbox_change_control/review.html @@ -10,7 +10,18 @@

{% trans "Review" %}

- + + + +
{% trans "Change request" %}{{ object.change_request|linkify }}
{% trans "Reviewer" %}{{ object.reviewer }}
{% trans "Decision" %}{% badge object.get_decision_display bg_color=object.get_decision_color %}
{% trans "Decision" %} + {% badge object.get_decision_display bg_color=object.get_decision_color %} + {% if object.is_stale %} + {% trans "Stale" %} +
+ {% trans "The branch has changed since this review was submitted, so it no longer counts towards its policy rules. Submit it again to restate it." %} +
+ {% endif %} +
{% trans "Submitted" %}{{ object.created|isodatetime:"minutes" }}
diff --git a/netbox_change_control/tests/base.py b/netbox_change_control/tests/base.py index 545750d..9b8d3a5 100644 --- a/netbox_change_control/tests/base.py +++ b/netbox_change_control/tests/base.py @@ -6,6 +6,8 @@ the count without covering anything new. """ +from pathlib import Path + from django.test import TestCase from netbox_branching.models import Branch from users.models import Group, User @@ -15,14 +17,35 @@ from netbox_change_control.models import ChangeRequest, ChangeRequestPolicy, Policy, PolicyRule, Review __all__ = ( + 'DOCS', 'ChangeControlTestCase', 'add_rule', 'approve', + 'docs_page', 'make_branch', 'make_policy', 'pass_checks', + 'submit', ) +# The documentation sits beside the package, not inside it, so the tests which compare a page +# against the code can only read it from a checkout. Installed with `pip install .` there is no +# docs directory at all, which is why CI installs the plugin editable. +DOCS = Path(__file__).resolve().parent.parent.parent / 'docs' + + +def docs_page(name): + """ + The text of one documentation page, with a usable message when it is not there. + """ + page = DOCS / name + if not page.exists(): + raise AssertionError( + f'{page} is missing. The documentation tests read the pages beside the package, ' + f'so the plugin has to be installed from a checkout (pip install -e).' + ) + return page.read_text() + def make_policy(name='One review', *, groups=(), users=(), min_reviews=1, rule_name='Rule', checks=(), **kwargs): """ @@ -47,6 +70,18 @@ def make_branch(prefix, suffix): return Branch.objects.create(name=f'{prefix}-{suffix}'[:100]) +def submit(change_request): + """ + Put a change request into review, as its author would. + + Draft is the author's to hold, so a review submitted against a draft moves nothing. A test + that is about reviewing has to get past this first. + """ + change_request.submit() + change_request.refresh_from_db() + return change_request + + def approve(change_request, reviewer, comment=''): return Review.objects.create( change_request=change_request, @@ -69,11 +104,18 @@ class ChangeControlTestCase(TestCase): requiring a single review from that group. Each test gets its own branch and a change request with the policy applied. Set - `approved = True` for the review to be given already. + `approved = True` for the review to be given already, or `submitted = False` to leave the + request as a draft. """ branch_prefix = 'cc' approved = False + #: Whether the request has been submitted for review. + #: + #: Draft is the author's to hold, so the evaluation does not move a request out of it and + #: a review submitted against a draft changes nothing. Nearly every test is about what + #: happens after submission, which is also the only state a reviewer ever sees. + submitted = True #: Checks the fixture policy requires. A built-in is registered but never applied until a #: policy names it, so the default mirrors the catch-all policy a real deployment uses to #: get the built-ins everywhere. Set it to () where the registry is cleared. @@ -92,6 +134,8 @@ def setUp(self): self.branch = make_branch(self.branch_prefix, self._testMethodName) self.cr = ChangeRequest.objects.create(branch=self.branch, title='T', requester=self.requester) ChangeRequestPolicy.objects.create(change_request=self.cr, policy=self.policy) + if self.submitted: + self.cr.submit() if self.approved: approve(self.cr, self.reviewer) self.cr.refresh_from_db() diff --git a/netbox_change_control/tests/test_automerge_once.py b/netbox_change_control/tests/test_automerge_once.py new file mode 100644 index 0000000..2c3acc1 --- /dev/null +++ b/netbox_change_control/tests/test_automerge_once.py @@ -0,0 +1,116 @@ +""" +One merge per branch, however many times auto-merge is reached. + +Becoming mergeable is not a single event. One write can arrive at try_auto_merge by more than +one route, and the status is still Approved at the second arrival because the merge has only +been queued, not run. A duplicate job then merges nothing and fails with "not ready to merge", +which reads as a broken merge on a change that in fact went through. + +These tests go through the signals rather than calling try_auto_merge directly, because +calling it directly is exactly what hid the problem. +""" + +import uuid + +from core.choices import JobStatusChoices +from core.models import Job +from django.contrib.contenttypes.models import ContentType +from django.test import TestCase +from netbox_branching.jobs import MergeBranchJob +from netbox_branching.models import Branch +from users.models import Group, User + +from netbox_change_control.automerge import try_auto_merge +from netbox_change_control.choices import ChangeRequestStatusChoices, MergeCheckStatusChoices +from netbox_change_control.models import ChangeRequest, ChangeRequestPolicy, Policy, PolicyRule +from netbox_change_control.tests.base import approve, make_branch + + +def queued_merges(branch): + return MergeBranchJob.get_jobs(branch).filter(status__in=JobStatusChoices.ENQUEUED_STATE_CHOICES) + + +class AutoMergeIsEnqueuedOnceTest(TestCase): + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.group = Group.objects.create(name='Engineers') + cls.reviewer = User.objects.create(username='reviewer') + cls.reviewer.groups.add(cls.group) + + def test_binding_a_zero_rule_policy_queues_one_merge(self): + """ + The path that used to queue two: attaching a policy refreshes the status, which runs + the checks, which try to merge; the caller then runs the checks again. + """ + policy = Policy.objects.create(name='Automatic') + PolicyRule.objects.create(policy=policy, name='None needed', min_reviews=0) + branch = make_branch('once', 'zero') + cr = ChangeRequest.objects.create(branch=branch, title='T', requester=self.requester, auto_merge=True) + ChangeRequestPolicy.objects.create(change_request=cr, policy=policy) + + cr.submit() + + cr.refresh_from_db() + self.assertEqual(cr.status, ChangeRequestStatusChoices.APPROVED) + self.assertEqual(queued_merges(branch).count(), 1) + + def test_a_second_attempt_while_one_is_queued_does_nothing(self): + policy = Policy.objects.create(name='One review') + PolicyRule.objects.create(policy=policy, name='One engineer', min_reviews=1).groups.set([self.group]) + branch = make_branch('once', 'second') + cr = ChangeRequest.objects.create(branch=branch, title='T', requester=self.requester, auto_merge=True) + ChangeRequestPolicy.objects.create(change_request=cr, policy=policy) + cr.submit() + approve(cr, self.reviewer) + cr.refresh_from_db() + cr.checks.update(status=MergeCheckStatusChoices.SUCCESS) + + self.assertEqual(queued_merges(branch).count(), 1) + + cr.refresh_from_db() + self.assertFalse(try_auto_merge(cr)) + self.assertEqual(queued_merges(branch).count(), 1) + + def test_a_finished_job_does_not_block_a_later_merge(self): + """ + The guard must read the queue, not the history. A branch whose earlier merge job + completed, errored or failed must still be able to queue a new one. + """ + policy = Policy.objects.create(name='Automatic') + PolicyRule.objects.create(policy=policy, name='None needed', min_reviews=0) + branch = make_branch('once', 'finished') + cr = ChangeRequest.objects.create(branch=branch, title='T', requester=self.requester, auto_merge=True) + ChangeRequestPolicy.objects.create(change_request=cr, policy=policy) + cr.submit() + + job = queued_merges(branch).get() + job.status = JobStatusChoices.STATUS_ERRORED + job.save() + + self.assertEqual(queued_merges(branch).count(), 0) + cr.refresh_from_db() + self.assertTrue(try_auto_merge(cr)) + self.assertEqual(queued_merges(branch).count(), 1) + + def test_a_queued_merge_for_another_branch_is_not_confused_with_this_one(self): + other = make_branch('once', 'other') + Job.objects.create( + name=MergeBranchJob.name, + object_type=ContentType.objects.get_for_model(Branch), + object_id=other.pk, + status=JobStatusChoices.STATUS_PENDING, + user=self.requester, + # core.Job identifies a run by a UUID the queue assigns; nothing creates a Job by + # hand in normal use, so it has no default. + job_id=uuid.uuid4(), + ) + + policy = Policy.objects.create(name='Automatic') + PolicyRule.objects.create(policy=policy, name='None needed', min_reviews=0) + branch = make_branch('once', 'mine') + cr = ChangeRequest.objects.create(branch=branch, title='T', requester=self.requester, auto_merge=True) + ChangeRequestPolicy.objects.create(change_request=cr, policy=policy) + cr.submit() + + self.assertEqual(queued_merges(branch).count(), 1) diff --git a/netbox_change_control/tests/test_batching.py b/netbox_change_control/tests/test_batching.py new file mode 100644 index 0000000..36d6980 --- /dev/null +++ b/netbox_change_control/tests/test_batching.py @@ -0,0 +1,178 @@ +""" +Collapsing the repeated refresh of one change request. + +Refreshing means recomputing the status and running the checks, and doing it on every event +that could change the answer is how the plugin stays consistent. The cost is that one user +action is often many events: submitting a request attaches every matching policy, and each +binding is its own signal, so a request governed by three policies refreshed three times and +ran every check three times for an identical answer. +""" + +from unittest.mock import patch + +from django.db import connection +from django.test import TestCase +from django.test.utils import CaptureQueriesContext +from users.models import Group, ObjectPermission, User + +from netbox_change_control.batching import batched, schedule_refresh +from netbox_change_control.checks import BUILTIN_CHECKS +from netbox_change_control.choices import ChangeRequestStatusChoices +from netbox_change_control.models import ChangeRequest, Policy, PolicyRule +from netbox_change_control.tests.base import make_branch + + +def count_check_runs(): + """ + Count real calls to run_checks, wherever they are reached from. + """ + import netbox_change_control.checks as checks_mod + + calls = [] + original = checks_mod.run_checks + + def counted(change_request): + calls.append(change_request.pk) + return original(change_request) + + return calls, patch.object(checks_mod, 'run_checks', counted) + + +class SubmitCollapsesTheBurstTest(TestCase): + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.group = Group.objects.create(name='Engineers') + for i in range(3): + policy = Policy.objects.create(name=f'Policy {i}', checks=list(BUILTIN_CHECKS)) + PolicyRule.objects.create(policy=policy, name='One engineer', min_reviews=1).groups.set([cls.group]) + + permission = ObjectPermission.objects.create(name='cr', actions=['view', 'change']) + from django.contrib.contenttypes.models import ContentType + + permission.object_types.add(ContentType.objects.get_for_model(ChangeRequest)) + permission.users.add(cls.requester) + + def setUp(self): + self.branch = make_branch('batch', self._testMethodName) + self.cr = ChangeRequest.objects.create(branch=self.branch, title='T', requester=self.requester) + self.client.force_login(self.requester) + + def submit(self): + return self.client.post(f'/plugins/change-control/change-requests/{self.cr.pk}/submit/') + + def test_submitting_runs_the_checks_once_for_three_policies(self): + """ + The measurement this file exists for. Before, three policies meant four runs: one per + binding signal and one more from the view. + """ + calls, counting = count_check_runs() + with counting: + self.submit() + + self.assertEqual(len(calls), 1, f'checks ran {len(calls)} times for one submission') + + def test_submitting_still_leaves_the_request_correct(self): + """ + Collapsing the work must not lose any of it. + """ + self.submit() + + self.cr.refresh_from_db() + self.assertEqual(self.cr.policies.count(), 3) + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.NEEDS_REVIEW) + self.assertEqual(self.cr.checks.count(), len(BUILTIN_CHECKS)) + self.assertTrue(all(c.completed for c in self.cr.checks.all())) + + def test_the_query_count_does_not_grow_with_the_number_of_policies(self): + with CaptureQueriesContext(connection) as three: + self.submit() + + for i in range(3, 8): + policy = Policy.objects.create(name=f'Extra {i}', checks=list(BUILTIN_CHECKS)) + PolicyRule.objects.create(policy=policy, name='One engineer', min_reviews=1).groups.set([self.group]) + other = ChangeRequest.objects.create(branch=make_branch('batch', 'eight'), title='T2', requester=self.requester) + with CaptureQueriesContext(connection) as eight: + self.client.post(f'/plugins/change-control/change-requests/{other.pk}/submit/') + + # Matching five more policies costs a little, but nothing like a further check run + # each. Before batching, each policy added a full refresh and a full check suite. + per_policy = (len(eight.captured_queries) - len(three.captured_queries)) / 5 + self.assertLess( + per_policy, + 6, + f'each additional policy costs ~{per_policy:.1f} queries on submit', + ) + + +class BatchedBlockTest(TestCase): + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.group = Group.objects.create(name='Engineers') + cls.policy = Policy.objects.create(name='P', checks=list(BUILTIN_CHECKS)) + PolicyRule.objects.create(policy=cls.policy, name='r', min_reviews=1).groups.set([cls.group]) + + def setUp(self): + self.cr = ChangeRequest.objects.create( + branch=make_branch('block', self._testMethodName), title='T', requester=self.requester + ) + + def test_outside_a_block_the_refresh_is_immediate(self): + """ + Most callers are not part of a burst, and nothing changes for them. + """ + calls, counting = count_check_runs() + with counting: + schedule_refresh(self.cr) + + self.assertEqual(len(calls), 1) + + def test_inside_a_block_repeated_requests_collapse(self): + calls, counting = count_check_runs() + with counting, batched(): + for _ in range(5): + schedule_refresh(self.cr) + self.assertEqual(len(calls), 0, 'work ran before the block ended') + + self.assertEqual(len(calls), 1) + + def test_two_different_requests_each_get_one_refresh(self): + other = ChangeRequest.objects.create(branch=make_branch('block', 'other'), title='T2', requester=self.requester) + calls, counting = count_check_runs() + with counting, batched(): + schedule_refresh(self.cr) + schedule_refresh(other) + schedule_refresh(self.cr) + + self.assertCountEqual(calls, [self.cr.pk, other.pk]) + + def test_nesting_is_safe_and_only_the_outermost_flushes(self): + calls, counting = count_check_runs() + with counting, batched(): + with batched(): + schedule_refresh(self.cr) + self.assertEqual(len(calls), 0, 'an inner block flushed') + + self.assertEqual(len(calls), 1) + + def test_a_block_that_raises_flushes_nothing(self): + """ + The work would be computed against a state that is about to roll back. + """ + calls, counting = count_check_runs() + with counting: + with self.assertRaises(RuntimeError): + with batched(): + schedule_refresh(self.cr) + raise RuntimeError('boom') + + self.assertEqual(calls, []) + + def test_a_deleted_request_is_skipped_rather_than_raising(self): + pk = self.cr.pk + with batched(): + schedule_refresh(self.cr) + ChangeRequest.objects.filter(pk=pk).delete() + # Reaching here without an exception is the assertion. + self.assertFalse(ChangeRequest.objects.filter(pk=pk).exists()) diff --git a/netbox_change_control/tests/test_branch_deletion.py b/netbox_change_control/tests/test_branch_deletion.py index 8a71f7e..dc74672 100644 --- a/netbox_change_control/tests/test_branch_deletion.py +++ b/netbox_change_control/tests/test_branch_deletion.py @@ -173,3 +173,86 @@ def test_it_is_never_mergeable(self): self.assertIsNone(self.cr.merge_indicator) self.assertFalse(self.cr.is_ready_to_merge) self.assertIn('deleted', self.cr.merge_blocked_reason) + + +class BranchRenameTest(ChangeControlTestCase): + """ + The stored branch name has to follow the branch. + + It exists so the record stays readable once the branch is gone, but it is also what the + branch filter and the global search read. A rename that did not reach it made a change + request unfindable by the name shown on its own page. + """ + + branch_prefix = 'renamed' + + def test_a_rename_reaches_the_change_request(self): + self.branch.name = 'renamed-in-place' + self.branch.save() + + self.cr.refresh_from_db() + self.assertEqual(self.cr.branch_name, 'renamed-in-place') + + def test_the_filter_finds_the_new_name(self): + from netbox_change_control.filtersets import ChangeRequestFilterSet + from netbox_change_control.models import ChangeRequest + + self.branch.name = 'renamed-in-place' + self.branch.save() + + found = ChangeRequestFilterSet({'branch': 'renamed-in-place'}, ChangeRequest.objects.all()).qs + self.assertIn(self.cr, found) + + def test_the_filter_no_longer_finds_the_old_name(self): + from netbox_change_control.filtersets import ChangeRequestFilterSet + from netbox_change_control.models import ChangeRequest + + old = self.branch.name + self.branch.name = 'renamed-in-place' + self.branch.save() + + found = ChangeRequestFilterSet({'branch': old}, ChangeRequest.objects.all()).qs + self.assertNotIn(self.cr, found) + + def test_the_name_still_survives_deletion_after_a_rename(self): + self.branch.name = 'renamed-in-place' + self.branch.save() + self.branch.delete() + + self.cr.refresh_from_db() + self.assertTrue(self.cr.branch_deleted) + self.assertEqual(self.cr.branch_label, 'renamed-in-place') + + +class AllChecksSkipWithoutABranchTest(ChangeControlTestCase): + """ + Every built-in check must skip once the branch is gone, not just most of them. + + The documentation promises a branchless request reports its checks as skipped rather than + failing. `threads-resolved` did not: it counted the comment threads, which survive the + branch on purpose, and failed on any that were still open. The record of a change nobody + can merge any more was then permanently marked as blocked. + """ + + branch_prefix = 'skipall' + + def test_every_builtin_skips(self): + from netbox_change_control.checks import BUILTIN_CHECKS + from netbox_change_control.choices import MergeCheckStatusChoices + from netbox_change_control.models import ChangeComment + + # An open thread, which is what used to make threads-resolved fail. + ChangeComment.objects.create( + change_request=self.cr, author=self.requester, text='unresolved', change_label='something' + ) + + self.branch.delete() + self.cr.refresh_from_db() + + for name, (_label, func) in BUILTIN_CHECKS.items(): + with self.subTest(check=name): + self.assertEqual( + func(self.cr).status, + MergeCheckStatusChoices.SKIPPED, + f'{name} does not skip on a request whose branch is gone', + ) diff --git a/netbox_change_control/tests/test_branch_page.py b/netbox_change_control/tests/test_branch_page.py new file mode 100644 index 0000000..aa189fd --- /dev/null +++ b/netbox_change_control/tests/test_branch_page.py @@ -0,0 +1,237 @@ +""" +What netbox-branching's own branch page says about change control. + +A branch and its change request are two halves of one job, and they only pointed one way. The +change request page carries the merge button; the branch page said nothing, so somebody who +had just finished working in a branch had to leave it and find the request by name in another +menu, and somebody whose merge was refused was told the reason on the merge form as plain text +with nothing to click. + +These pin what the injected content says, because it is the only part of the plugin that +renders inside somebody else's template and would break silently if that template moved. +""" + +from django.conf import settings +from django.test import TestCase, override_settings +from netbox_branching.choices import BranchStatusChoices +from netbox_branching.models import Branch +from users.models import Group, ObjectPermission, User + +from netbox_change_control.models import ChangeRequest, ChangeRequestPolicy, Policy, PolicyRule +from netbox_change_control.tests.base import approve, make_branch + + +def ready_branch(prefix, suffix): + """ + A branch the interface would treat as workable. Tests create branches without provisioning + a schema, which leaves them in `new`, and a branch that is not ready yet has nothing to + say about merging. + """ + branch = make_branch(prefix, suffix) + Branch.objects.filter(pk=branch.pk).update(status=BranchStatusChoices.READY) + branch.refresh_from_db() + return branch + + +# The alert and the card render the same content in a different frame, so a test has to look +# at the frame to tell which one it got. These markers are how each wrapper writes its own +# heading, which the other never does. The icons are no use: the Change Control menu carries +# the same one on every page. +ALERT_MARKER = 'Change request' +CARD_MARKER = 'Change request' +NO_REQUEST_ALERT_MARKER = 'No change request' +NO_REQUEST_CARD_MARKER = '
No change request
' + + +def placement(*names): + """ + Run a test with the branch page placed where it says. + + `override_settings` replaces PLUGINS_CONFIG wholesale, and netbox-branching reads its own + settings out of that same dictionary, so the override has to carry every plugin's + configuration and change one key of it. + """ + config = {plugin: dict(plugin_config) for plugin, plugin_config in settings.PLUGINS_CONFIG.items()} + config['netbox_change_control']['branch_page_placement'] = list(names) + return override_settings(PLUGINS_CONFIG=config) + + +class BranchPageTestCase(TestCase): + """ + A branch page, a policy nobody has satisfied, and an administrator looking at it. + """ + + @classmethod + def setUpTestData(cls): + cls.admin = User.objects.create(username='admin', is_superuser=True) + cls.group = Group.objects.create(name='Leads') + cls.lead = User.objects.create(username='lead') + cls.lead.groups.add(cls.group) + cls.policy = Policy.objects.create(name='Needs a lead') + PolicyRule.objects.create(policy=cls.policy, name='One lead', min_reviews=1).groups.set([cls.group]) + + def setUp(self): + self.client.force_login(self.admin) + + def page(self, branch): + response = self.client.get(branch.get_absolute_url()) + self.assertEqual(response.status_code, 200) + return response.content.decode() + + def with_request(self, suffix, **kwargs): + branch = ready_branch('bpage', suffix) + cr = ChangeRequest.objects.create(branch=branch, requester=self.admin, **kwargs) + ChangeRequestPolicy.objects.create(change_request=cr, policy=self.policy) + cr.submit() + cr.refresh_from_db() + return branch, cr + + +class BranchPageTest(BranchPageTestCase): + def test_the_branch_page_links_to_its_change_request(self): + branch, cr = self.with_request('link', title='Upgrade the access switch', ref='CHG0012345') + html = self.page(branch) + + self.assertIn(cr.get_absolute_url(), html) + self.assertIn('Upgrade the access switch', html) + self.assertIn('CHG0012345', html) + + def test_it_names_what_is_still_outstanding(self): + """ + The reason the merge is refused, and who can clear it. Branching's merge form states + the refusal; only this says what to do about it. + """ + branch, _cr = self.with_request('outstanding', title='Blocked one') + html = self.page(branch) + + self.assertIn('One lead', html) + self.assertIn('0 / 1', html) + self.assertIn('lead', html) + + def test_a_ready_branch_says_so(self): + branch, cr = self.with_request('ready', title='Ready one') + approve(cr, self.lead) + cr.refresh_from_db() + self.assertTrue(cr.is_ready_to_merge) + + html = self.page(branch) + + self.assertIn('Every gate is satisfied', html) + + def test_a_branch_with_no_change_request_is_told_to_open_one(self): + branch = ready_branch('bpage', 'none') + + html = self.page(branch) + + self.assertIn('No change request', html) + self.assertIn(f'/change-requests/add/?branch={branch.pk}', html) + + def test_a_branch_that_is_not_ready_yet_is_left_alone(self): + """ + A branch still provisioning cannot merge for reasons that have nothing to do with + change control, so nagging about a change request there is noise. + """ + branch = make_branch('bpage', 'provisioning') + self.assertFalse(branch.ready) + + html = self.page(branch) + + self.assertNotIn('No change request', html) + + def test_the_offer_to_open_one_is_permission_gated(self): + """ + Somebody who cannot open a change request is told why the branch is blocked, but not + offered a button that would refuse them. + """ + from django.contrib.contenttypes.models import ContentType + + branch = ready_branch('bpage', 'noperm') + viewer = User.objects.create(username='viewer') + permission = ObjectPermission.objects.create(name='see branches', actions=['view']) + permission.object_types.add(ContentType.objects.get_for_model(Branch)) + permission.users.add(viewer) + self.client.force_login(viewer) + + html = self.page(branch) + + self.assertIn('No change request', html) + self.assertNotIn('/change-requests/add/', html) + + +class PlacementTest(BranchPageTestCase): + """ + Where the panel appears is configuration, and the two placements are meant to be compared, + so naming both has to show both rather than one winning. + """ + + def test_the_card_is_what_the_plugin_ships(self): + """ + Read from `default_settings` rather than from the running configuration, which a + deployment is free to change, and this one does to compare the two. + """ + from netbox_change_control import ChangeControlConfig + + self.assertEqual(ChangeControlConfig.default_settings['branch_page_placement'], ['right_page']) + + @placement('alerts') + def test_the_alert_sits_across_the_top(self): + branch, _cr = self.with_request('default') + + html = self.page(branch) + + self.assertIn(ALERT_MARKER, html) + self.assertNotIn(CARD_MARKER, html) + + @placement('right_page') + def test_the_right_hand_column_replaces_the_alert(self): + branch, cr = self.with_request('right') + + html = self.page(branch) + + self.assertIn(CARD_MARKER, html) + self.assertNotIn(ALERT_MARKER, html) + self.assertIn(cr.get_absolute_url(), html) + + @placement('alerts', 'right_page') + def test_naming_both_shows_both(self): + branch, _cr = self.with_request('both') + + html = self.page(branch) + + self.assertIn(ALERT_MARKER, html) + self.assertIn(CARD_MARKER, html) + + @placement() + def test_an_empty_list_leaves_the_branch_page_alone(self): + branch, cr = self.with_request('off') + + html = self.page(branch) + + self.assertNotIn(ALERT_MARKER, html) + self.assertNotIn(CARD_MARKER, html) + self.assertNotIn(cr.get_absolute_url(), html) + + @placement('right_page') + def test_the_offer_to_open_one_follows_the_placement(self): + branch = ready_branch('bpage', 'right-none') + + html = self.page(branch) + + self.assertIn(NO_REQUEST_CARD_MARKER, html) + self.assertNotIn(NO_REQUEST_ALERT_MARKER, html) + self.assertIn(f'/change-requests/add/?branch={branch.pk}', html) + + @placement('left_page') + def test_a_name_that_is_not_a_placement_is_skipped_rather_than_raising(self): + """ + A typo in configuration must not take the branch page down with it, which is how the + built-in check selection behaves as well. + """ + branch, _cr = self.with_request('typo') + + with self.assertLogs('netbox.plugins.netbox_change_control', level='WARNING') as logs: + html = self.page(branch) + + self.assertNotIn(ALERT_MARKER, html) + self.assertNotIn(CARD_MARKER, html) + self.assertIn('left_page', logs.output[0]) diff --git a/netbox_change_control/tests/test_cached_state.py b/netbox_change_control/tests/test_cached_state.py new file mode 100644 index 0000000..9e33cbe --- /dev/null +++ b/netbox_change_control/tests/test_cached_state.py @@ -0,0 +1,305 @@ +""" +The cached columns on the change request list. + +Reading conflicts and readiness live costs about eleven queries per row, which is five hundred +for a default page of fifty. Both are cached and refreshed on the events that change them. + +A cache is only worth having if it agrees with the truth, so most of this file compares the +cached answer against the live one across the states a change request passes through. +""" + +from django.db import connection +from django.test import TestCase +from django.test.utils import CaptureQueriesContext +from users.models import Group, User + +from netbox_change_control.checks import BUILTIN_CHECKS, run_checks +from netbox_change_control.choices import ChangeRequestStatusChoices, MergeCheckStatusChoices +from netbox_change_control.models import ChangeRequest, ChangeRequestPolicy, Policy, PolicyRule +from netbox_change_control.tests.base import approve, make_branch + + +class CachedStateAgreesWithLiveTest(TestCase): + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.group = Group.objects.create(name='Engineers') + cls.reviewer = User.objects.create(username='reviewer') + cls.reviewer.groups.add(cls.group) + cls.policy = Policy.objects.create(name='One review', checks=list(BUILTIN_CHECKS)) + PolicyRule.objects.create(policy=cls.policy, name='One engineer', min_reviews=1).groups.set([cls.group]) + + def setUp(self): + self.branch = make_branch('cached', self._testMethodName) + self.cr = ChangeRequest.objects.create(branch=self.branch, title='T', requester=self.requester) + ChangeRequestPolicy.objects.create(change_request=self.cr, policy=self.policy) + self.cr.refresh_from_db() + + def assert_cache_agrees(self): + """ + The cached column and the authoritative answer must say the same thing. + + They are allowed to differ only where the cache deliberately knows less: it does not + run merge validators registered by other plugins. No other plugin is loaded here. + """ + self.cr.refresh_from_db() + self.assertEqual( + self.cr.cached_ready_to_merge, + self.cr.is_ready_to_merge, + 'the cached readiness disagrees with a live evaluation', + ) + self.assertEqual( + self.cr.cached_conflicted, + bool(self.cr.conflicts), + 'the cached conflict flag disagrees with a live evaluation', + ) + + def test_a_new_request_agrees(self): + self.assert_cache_agrees() + + def test_an_approved_request_agrees(self): + approve(self.cr, self.reviewer) + self.assert_cache_agrees() + + def test_a_request_with_a_failing_check_agrees(self): + approve(self.cr, self.reviewer) + self.cr.refresh_from_db() + self.cr.checks.filter(name='no-conflicts').update(status=MergeCheckStatusChoices.FAILURE) + run_checks(self.cr) + self.assert_cache_agrees() + + def test_a_rejected_request_agrees(self): + from netbox_change_control.choices import ReviewDecisionChoices + from netbox_change_control.models import Review + + Review.objects.create( + change_request=self.cr, + reviewer=self.reviewer, + decision=ReviewDecisionChoices.REJECT, + comment='no', + ) + self.assert_cache_agrees() + + def test_a_request_whose_branch_is_gone_agrees(self): + approve(self.cr, self.reviewer) + self.branch.delete() + self.assert_cache_agrees() + + def test_removing_an_approval_clears_the_cache(self): + approve(self.cr, self.reviewer) + self.cr.refresh_from_db() + was = self.cr.cached_gates_cleared + + self.cr.reviews.all().delete() + self.cr.refresh_from_db() + + self.assertNotEqual(self.cr.status, ChangeRequestStatusChoices.APPROVED) + self.assertFalse(self.cr.cached_gates_cleared) + self.assert_cache_agrees() + self.assertIsNotNone(was) + + +class WindowIsNotCachedTest(TestCase): + """ + A window opens because the clock moved, and nothing happens to hang a refresh on. + + So the window is deliberately left out of the cached flag and evaluated at read time from + the two fields the row already carries. + """ + + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.group = Group.objects.create(name='Engineers') + cls.reviewer = User.objects.create(username='reviewer') + cls.reviewer.groups.add(cls.group) + cls.policy = Policy.objects.create(name='One review') + PolicyRule.objects.create(policy=cls.policy, name='One engineer', min_reviews=1).groups.set([cls.group]) + + def test_a_future_window_is_not_ready_without_any_refresh(self): + from datetime import timedelta + + from django.utils import timezone + + branch = make_branch('window', 'cached') + cr = ChangeRequest.objects.create(branch=branch, title='T', requester=self.requester) + ChangeRequestPolicy.objects.create(change_request=cr, policy=self.policy) + cr.submit() + approve(cr, self.reviewer) + cr.refresh_from_db() + self.assertTrue(cr.cached_gates_cleared) + self.assertTrue(cr.cached_ready_to_merge) + + # Move the window into the future by writing the fields only. Nothing refreshes the + # cache, and the answer still has to change. + ChangeRequest.objects.filter(pk=cr.pk).update(scheduled_start=timezone.now() + timedelta(hours=2)) + cr.refresh_from_db() + + self.assertTrue(cr.cached_gates_cleared) + self.assertFalse(cr.cached_ready_to_merge) + + +class ListCostTest(TestCase): + """ + The point of the cache: the list must not get more expensive as it gets longer. + """ + + def test_the_query_count_does_not_grow_with_the_number_of_rows(self): + user = User.objects.create(username='lister', is_superuser=True) + group = Group.objects.create(name='Engineers') + reviewer = User.objects.create(username='rev') + reviewer.groups.add(group) + policy = Policy.objects.create(name='One review', checks=list(BUILTIN_CHECKS)) + PolicyRule.objects.create(policy=policy, name='One engineer', min_reviews=1).groups.set([group]) + + kept = None + for i in range(10): + cr = ChangeRequest.objects.create(branch=make_branch('cost', f'{i}'), title=f'CR {i}', requester=user) + ChangeRequestPolicy.objects.create(change_request=cr, policy=policy) + cr.submit() + approve(cr, reviewer) + kept = kept or cr + + self.client.force_login(user) + with CaptureQueriesContext(connection) as ten_rows: + self.assertEqual(self.client.get('/plugins/change-control/change-requests/').status_code, 200) + + ChangeRequest.objects.exclude(pk=kept.pk).delete() + with CaptureQueriesContext(connection) as one_row: + self.client.get('/plugins/change-control/change-requests/') + + per_row = (len(ten_rows.captured_queries) - len(one_row.captured_queries)) / 9 + self.assertLessEqual( + per_row, + 0.5, + f'the change request list costs ~{per_row:.1f} queries per row; the columns are not reading the cache', + ) + + +class CacheFollowsTheSignalsTest(TestCase): + """ + The cache has to be refreshed by the events themselves, not by a caller remembering to. + + Every test here drives a real signal path and then compares the cached answer with the + live one. Calling `refresh_cached_state` directly would only prove the function works, + which was never the doubt: the risk in a cache is the event nobody wired up. + """ + + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.group = Group.objects.create(name='Engineers') + cls.reviewer = User.objects.create(username='reviewer') + cls.reviewer.groups.add(cls.group) + + def make_request(self, checks=(), name='P'): + policy = Policy.objects.create(name=f'{name}-{self._testMethodName}'[:100], checks=list(checks)) + PolicyRule.objects.create(policy=policy, name='One engineer', min_reviews=1).groups.set([self.group]) + branch = make_branch('signal', self._testMethodName) + cr = ChangeRequest.objects.create(branch=branch, title='T', requester=self.requester) + ChangeRequestPolicy.objects.create(change_request=cr, policy=policy) + cr.submit() + cr.refresh_from_db() + return cr, branch + + def assert_agrees(self, cr): + cr.refresh_from_db() + self.assertEqual(cr.cached_ready_to_merge, cr.is_ready_to_merge) + self.assertEqual(cr.cached_conflicted, bool(cr.conflicts)) + + def test_an_externally_reported_check_updates_the_cache(self): + """ + A pipeline reporting the last gate reaches this plugin as a plain save on one row, + with no other signal behind it. That is the common way a change becomes ready. + """ + cr, _branch = self.make_request(checks=['ci-pipeline']) + approve(cr, self.reviewer) + cr.refresh_from_db() + self.assertFalse(cr.cached_gates_cleared) + + check = cr.checks.get(name='ci-pipeline') + check.status = MergeCheckStatusChoices.SUCCESS + check.save() + + cr.refresh_from_db() + self.assertTrue(cr.cached_gates_cleared) + self.assert_agrees(cr) + + def test_a_check_going_red_again_updates_the_cache(self): + cr, _branch = self.make_request(checks=['ci-pipeline']) + approve(cr, self.reviewer) + check = cr.checks.get(name='ci-pipeline') + check.status = MergeCheckStatusChoices.SUCCESS + check.save() + cr.refresh_from_db() + self.assertTrue(cr.cached_gates_cleared) + + check.status = MergeCheckStatusChoices.FAILURE + check.save() + + cr.refresh_from_db() + self.assertFalse(cr.cached_gates_cleared) + self.assert_agrees(cr) + + def test_a_merged_request_is_no_longer_shown_as_ready(self): + """ + Completion is set by the post_merge receiver and never passes through refresh_status, + so the list went on offering a merged change as ready to merge. + """ + from netbox_branching.signals import post_merge + + cr, branch = self.make_request() + approve(cr, self.reviewer) + cr.refresh_from_db() + self.assertTrue(cr.cached_ready_to_merge) + + post_merge.send(sender=type(branch), branch=branch, user=self.requester) + + cr.refresh_from_db() + self.assertEqual(cr.status, ChangeRequestStatusChoices.COMPLETED) + self.assertFalse(cr.cached_ready_to_merge) + self.assert_agrees(cr) + + def test_a_conflict_updates_the_cache_without_the_no_conflicts_check(self): + """ + Only the no-conflicts check creates the row the diff receiver used to consult, so a + request governed by a policy that does not require it had no path back to the cache. + """ + from unittest.mock import patch + + from django.contrib.contenttypes.models import ContentType + from netbox_branching.models import ChangeDiff + + cr, branch = self.make_request(checks=()) + self.assertFalse(cr.checks.filter(name='no-conflicts').exists()) + + diff = ChangeDiff.objects.create( + branch=branch, + object_type=ContentType.objects.get_for_model(Policy), + object_id=1, + object_repr='x', + action='update', + ) + ChangeDiff.objects.filter(pk=diff.pk).update(conflicts=['name']) + diff.refresh_from_db() + + class _Unsynced: + def values_list(self, *args, **kwargs): + return [(diff.object_type_id, diff.object_id)] + + with patch.object(type(branch), 'get_unsynced_changes', return_value=_Unsynced()): + diff.save() + + cr.refresh_from_db() + self.assertTrue(cr.cached_conflicted) + self.assert_agrees(cr) + + def test_a_review_updates_the_cache(self): + cr, _branch = self.make_request() + self.assertFalse(cr.cached_gates_cleared) + + approve(cr, self.reviewer) + + cr.refresh_from_db() + self.assertTrue(cr.cached_gates_cleared) + self.assert_agrees(cr) diff --git a/netbox_change_control/tests/test_check_changelog.py b/netbox_change_control/tests/test_check_changelog.py new file mode 100644 index 0000000..deffac2 --- /dev/null +++ b/netbox_change_control/tests/test_check_changelog.py @@ -0,0 +1,105 @@ +""" +Check results belong in the changelog. + +A required check going from failed to passed is what opens the merge gate. Results were +written with a queryset update, which goes straight to the database and fires no post_save, so +NetBox recorded nothing: the plugin whose job is the record of who allowed what kept no record +of the machine half of that decision. +""" + +import uuid + +from core.models import ObjectChange +from django.contrib.contenttypes.models import ContentType +from django.test import TestCase +from netbox.context_managers import event_tracking +from users.models import Group, User + +from netbox_change_control.checks import CheckResult, register_check, run_checks +from netbox_change_control.checks import _registry as check_registry +from netbox_change_control.choices import MergeCheckStatusChoices +from netbox_change_control.models import ChangeRequest, ChangeRequestPolicy, MergeCheck, Policy, PolicyRule +from netbox_change_control.tests.base import make_branch + + +class CheckResultsAreLoggedTest(TestCase): + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.group = Group.objects.create(name='Engineers') + cls.policy = Policy.objects.create(name='Probe policy', checks=['probe']) + PolicyRule.objects.create(policy=cls.policy, name='One engineer', min_reviews=1).groups.set([cls.group]) + + def setUp(self): + from django.test import RequestFactory + + self.outcome = CheckResult.failed('not yet') + register_check('probe', 'Probe', lambda cr: self.outcome, scope='policy') + self.addCleanup(check_registry.pop, 'probe', None) + + self.branch = make_branch('log', self._testMethodName) + self.cr = ChangeRequest.objects.create(branch=self.branch, title='T', requester=self.requester) + ChangeRequestPolicy.objects.create(change_request=self.cr, policy=self.policy) + + self.request = RequestFactory().post('/') + self.request.user = self.requester + # ObjectChange.request_id is not nullable, and NetBox normally gets this from its + # request middleware. + self.request.id = uuid.uuid4() + + def changes_for(self, check): + return ObjectChange.objects.filter( + changed_object_type=ContentType.objects.get_for_model(MergeCheck), + changed_object_id=check.pk, + ) + + def test_a_result_that_moves_is_recorded(self): + check = MergeCheck.objects.get(change_request=self.cr, name='probe') + self.assertEqual(check.status, MergeCheckStatusChoices.FAILURE) + before = self.changes_for(check).count() + + self.outcome = CheckResult.passed('all good') + with event_tracking(self.request): + run_checks(self.cr) + + check.refresh_from_db() + self.assertEqual(check.status, MergeCheckStatusChoices.SUCCESS) + self.assertGreater(self.changes_for(check).count(), before) + + def test_a_re_run_finding_the_same_answer_records_nothing(self): + """ + Checks re-run on many signals. Logging every run would bury the transitions that + matter under identical entries. + """ + check = MergeCheck.objects.get(change_request=self.cr, name='probe') + with event_tracking(self.request): + run_checks(self.cr) + before = self.changes_for(check).count() + + with event_tracking(self.request): + run_checks(self.cr) + + self.assertEqual(self.changes_for(check).count(), before) + + def test_the_stored_result_is_still_correct(self): + self.outcome = CheckResult.passed('all good') + run_checks(self.cr) + + check = MergeCheck.objects.get(change_request=self.cr, name='probe') + self.assertEqual(check.status, MergeCheckStatusChoices.SUCCESS) + self.assertEqual(check.summary, 'all good') + self.assertIsNotNone(check.completed) + + def test_completed_moves_only_when_the_result_moves(self): + run_checks(self.cr) + check = MergeCheck.objects.get(change_request=self.cr, name='probe') + first = check.completed + + run_checks(self.cr) + check.refresh_from_db() + self.assertEqual(check.completed, first) + + self.outcome = CheckResult.passed('all good') + run_checks(self.cr) + check.refresh_from_db() + self.assertGreater(check.completed, first) diff --git a/netbox_change_control/tests/test_checks.py b/netbox_change_control/tests/test_checks.py index 9fb0560..e909a99 100644 --- a/netbox_change_control/tests/test_checks.py +++ b/netbox_change_control/tests/test_checks.py @@ -8,6 +8,7 @@ from netbox_change_control.checks import ( CheckResult, _registry, + check_threads_resolved, register_check, run_checks, sync_checks, @@ -56,14 +57,19 @@ def test_run_checks_records_a_pass(self): def test_a_raising_check_is_recorded_as_errored(self): """ - A broken check must not take the page down with it. + A broken check must not take the page down with it, and must say so in the log. + + The log is captured rather than left to print: the traceback is deliberate, and on a + green CI run it is read as a failure by whoever sees it. """ def boom(cr): raise RuntimeError('exploded') register_check('broken', 'Broken', boom) - run_checks(self.cr) + with self.assertLogs('netbox.plugins.netbox_change_control.checks', level='ERROR') as logs: + run_checks(self.cr) + self.assertIn('Merge check broken raised', logs.output[0]) check = MergeCheck.objects.get(change_request=self.cr, name='broken') self.assertEqual(check.status, MergeCheckStatusChoices.ERROR) self.assertIn('exploded', check.summary) @@ -361,13 +367,11 @@ class ThreadsCheckAfterBranchDeletionTest(CheckTestCase): null. Reaching through that relation recorded the check as errored. """ - def test_the_check_reports_the_stored_label_not_a_crash(self): - + def _thread(self): from core.choices import ObjectChangeActionChoices from django.contrib.contenttypes.models import ContentType from netbox_branching.models import ChangeDiff - from netbox_change_control.checks import check_threads_resolved from netbox_change_control.models import ChangeComment diff = ChangeDiff.objects.create( @@ -377,53 +381,74 @@ def test_the_check_reports_the_stored_label_not_a_crash(self): object_repr='some-object', action=ObjectChangeActionChoices.ACTION_UPDATE, ) - comment = ChangeComment.objects.create( + return ChangeComment.objects.create( change_request=self.cr, change_diff=diff, author=self.reviewer, text='Wait', ) - label = comment.change_label + + def test_an_open_thread_is_named_by_its_stored_label(self): + """ + The original point of this test: the summary comes from `change_label`, not from + reaching through `change_diff`, which is what used to record the check as errored. + """ + comment = self._thread() + + result = check_threads_resolved(self.cr) + + self.assertEqual(result.status, MergeCheckStatusChoices.FAILURE) + self.assertIn(comment.change_label, result.summary) + + def test_it_skips_once_the_branch_is_gone(self): + """ + Every built-in skips on a request whose branch has been deleted, and this one has to + agree with the other three. Such a request can never merge anyway, so failing here + marked a historical record as blocked for ever over a thread nobody can act on. + """ + self._thread() self.branch.delete() self.cr.refresh_from_db() result = check_threads_resolved(self.cr) - self.assertEqual(result.status, MergeCheckStatusChoices.FAILURE) - self.assertIn(label, result.summary) + + self.assertEqual(result.status, MergeCheckStatusChoices.SKIPPED) class AutoMergeOnCheckPassTest(CheckTestCase): """ - run_checks writes results with .update(), which fires no post_save. Auto-merge must still - fire when an in-process check turns green. - """ + An in-process check turning green can be the last gate a request was waiting on. - def test_a_passing_check_triggers_auto_merge(self): - from unittest.mock import patch + These count queued jobs rather than calls to enqueue. A passing check reaches auto-merge + twice, once from the MergeCheck receiver and once from run_checks itself, and what must + hold is that only one merge ends up queued. Mocking enqueue would remove the very + mechanism that deduplicates them and prove nothing. + """ + def queued(self): + from core.choices import JobStatusChoices from netbox_branching.jobs import MergeBranchJob + return MergeBranchJob.get_jobs(self.branch).filter(status__in=JobStatusChoices.ENQUEUED_STATE_CHOICES) + + def test_a_passing_check_queues_exactly_one_merge(self): self.cr.auto_merge = True self.cr.save() register_check('ok', 'OK', lambda cr: CheckResult.passed()) - with patch.object(MergeBranchJob, 'enqueue') as enqueue: - run_checks(self.cr) - enqueue.assert_called_once() - - def test_a_failing_check_does_not_trigger_auto_merge(self): - from unittest.mock import patch + run_checks(self.cr) - from netbox_branching.jobs import MergeBranchJob + self.assertEqual(self.queued().count(), 1) + def test_a_failing_check_does_not_trigger_auto_merge(self): self.cr.auto_merge = True self.cr.save() register_check('bad', 'Bad', lambda cr: CheckResult.failed('no')) - with patch.object(MergeBranchJob, 'enqueue') as enqueue: - run_checks(self.cr) - enqueue.assert_not_called() + run_checks(self.cr) + + self.assertEqual(self.queued().count(), 0) class ChecksRunOnCreationTest(TestCase): diff --git a/netbox_change_control/tests/test_comment_attribution.py b/netbox_change_control/tests/test_comment_attribution.py new file mode 100644 index 0000000..8f018bc --- /dev/null +++ b/netbox_change_control/tests/test_comment_attribution.py @@ -0,0 +1,234 @@ +""" +Who a comment is attributed to, and which change it may refer to. + +A comment on the Changes tab is part of the record a reviewer reads before approving. If a +token can post one under somebody else's name, it can fake a colleague's sign-off in that +discussion; if it can file one against another request's diff, the comment is invisible where +it belongs and counted as an open thread where it does not. + +The review side of this is pinned in test_api.py. This file pins the comment side. +""" + +from django.contrib.contenttypes.models import ContentType +from django.core.exceptions import ValidationError +from netbox_branching.models import ChangeDiff +from rest_framework import status +from users.models import ObjectPermission, User +from utilities.testing import APITestCase + +from netbox_change_control.models import ChangeComment, ChangeRequest, Policy +from netbox_change_control.tests.base import make_branch + + +def make_diff(branch, object_id=1, repr_='thing'): + return ChangeDiff.objects.create( + branch=branch, + action='update', + object_type=ContentType.objects.get_for_model(Policy), + object_id=object_id, + object_repr=repr_, + original={'a': 1}, + modified={'a': 2}, + current={'a': 1}, + ) + + +class ChangeCommentAttributionTest(APITestCase): + def setUp(self): + super().setUp() + self.victim = User.objects.create(username='victim') + self.requester = User.objects.create(username='requester') + self.branch = make_branch('comment-attr', 'x') + self.cr = ChangeRequest.objects.create(branch=self.branch, title='T', requester=self.requester) + self.diff = make_diff(self.branch) + + permission = ObjectPermission.objects.create(name='comment', actions=['view', 'add', 'change']) + permission.object_types.add(ContentType.objects.get_for_model(ChangeComment)) + permission.users.add(self.user) + + def test_the_author_is_the_caller_not_whoever_is_named(self): + response = self.client.post( + '/api/plugins/change-control/change-comments/', + { + 'change_request': self.cr.pk, + 'change_diff': self.diff.pk, + 'author': self.victim.pk, + 'text': 'Looks fine to me', + }, + format='json', + **self.header, + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + comment = ChangeComment.objects.get(pk=response.data['id']) + self.assertEqual(comment.author, self.user) + self.assertNotEqual(comment.author, self.victim) + + def test_a_comment_posted_without_an_author_is_accepted(self): + """ + The field is read-only, so a well behaved client omits it entirely and the caller is + still recorded. + """ + response = self.client.post( + '/api/plugins/change-control/change-comments/', + {'change_request': self.cr.pk, 'change_diff': self.diff.pk, 'text': 'Fine'}, + format='json', + **self.header, + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + self.assertEqual(ChangeComment.objects.get(pk=response.data['id']).author, self.user) + + def test_editing_a_comment_does_not_reassign_its_author(self): + comment = ChangeComment.objects.create( + change_request=self.cr, change_diff=self.diff, author=self.victim, text='original' + ) + response = self.client.patch( + f'/api/plugins/change-control/change-comments/{comment.pk}/', + {'text': 'edited', 'author': self.user.pk}, + format='json', + **self.header, + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + comment.refresh_from_db() + self.assertEqual(comment.author, self.victim) + self.assertEqual(comment.text, 'edited') + + +class ChangeCommentBranchScopeTest(APITestCase): + """ + A comment must refer to a change in its own request's branch. + """ + + def setUp(self): + super().setUp() + self.requester = User.objects.create(username='requester') + + self.branch = make_branch('scope-mine', 'x') + self.cr = ChangeRequest.objects.create(branch=self.branch, title='Mine', requester=self.requester) + self.diff = make_diff(self.branch, object_id=1, repr_='mine') + + self.other_branch = make_branch('scope-other', 'x') + self.other_cr = ChangeRequest.objects.create(branch=self.other_branch, title='Theirs', requester=self.requester) + self.other_diff = make_diff(self.other_branch, object_id=2, repr_='theirs') + + permission = ObjectPermission.objects.create(name='comment', actions=['view', 'add']) + permission.object_types.add(ContentType.objects.get_for_model(ChangeComment)) + permission.users.add(self.user) + + def test_a_comment_on_its_own_branch_is_accepted(self): + response = self.client.post( + '/api/plugins/change-control/change-comments/', + {'change_request': self.cr.pk, 'change_diff': self.diff.pk, 'text': 'Fine'}, + format='json', + **self.header, + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + + def test_a_comment_naming_another_requests_change_is_refused(self): + response = self.client.post( + '/api/plugins/change-control/change-comments/', + {'change_request': self.cr.pk, 'change_diff': self.other_diff.pk, 'text': 'Sneaky'}, + format='json', + **self.header, + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual(ChangeComment.objects.count(), 0) + + def test_the_model_refuses_it_too(self): + comment = ChangeComment(change_request=self.cr, change_diff=self.other_diff, author=self.user, text='Sneaky') + with self.assertRaises(ValidationError): + comment.full_clean() + + +class ChangeCommentThreadDepthTest(APITestCase): + """ + A reply to a reply joins the same thread, on every path. + + The flattening lived in clean(), which reassigns self.parent. NetBox's + ValidatedModelSerializer runs full_clean() on a throw-away copy and keeps only the original + attributes, so the REST path stored a grandchild instead. The Changes tab builds its + threads from roots alone, so such a comment rendered nowhere at all. + """ + + def setUp(self): + super().setUp() + self.requester = User.objects.create(username='requester') + self.branch = make_branch('depth', 'x') + self.cr = ChangeRequest.objects.create(branch=self.branch, title='T', requester=self.requester) + self.diff = make_diff(self.branch) + self.root = ChangeComment.objects.create( + change_request=self.cr, change_diff=self.diff, author=self.user, text='root' + ) + self.reply = ChangeComment.objects.create( + change_request=self.cr, change_diff=self.diff, author=self.user, parent=self.root, text='reply' + ) + + permission = ObjectPermission.objects.create(name='comment', actions=['view', 'add']) + permission.object_types.add(ContentType.objects.get_for_model(ChangeComment)) + permission.users.add(self.user) + + def test_a_reply_to_a_reply_joins_the_same_thread_over_the_api(self): + response = self.client.post( + '/api/plugins/change-control/change-comments/', + { + 'change_request': self.cr.pk, + 'change_diff': self.diff.pk, + 'parent': self.reply.pk, + 'text': 'grandchild', + }, + format='json', + **self.header, + ) + self.assertEqual(response.status_code, status.HTTP_201_CREATED) + created = ChangeComment.objects.get(pk=response.data['id']) + self.assertEqual(created.parent_id, self.root.pk) + + def test_a_reply_to_a_reply_joins_the_same_thread_through_the_orm(self): + created = ChangeComment.objects.create( + change_request=self.cr, + change_diff=self.diff, + author=self.user, + parent=self.reply, + text='grandchild', + ) + self.assertEqual(created.parent_id, self.root.pk) + + +class ChangeCommentStaleDiffTest(APITestCase): + def test_a_change_that_no_longer_exists_is_a_validation_error(self): + """ + Not a server error. A branch deleted while a comment is in flight is the realistic + way to hold an id that has gone. + """ + requester = User.objects.create(username='requester') + branch = make_branch('stale', 'x') + cr = ChangeRequest.objects.create(branch=branch, title='T', requester=requester) + diff = make_diff(branch) + comment = ChangeComment(change_request=cr, change_diff_id=diff.pk, author=self.user, text='x') + diff.delete() + + with self.assertRaises(ValidationError): + comment.full_clean() + + +class ChangeCommentBranchScopeOnUpdateTest(ChangeCommentBranchScopeTest): + """ + The branch rule has to hold on update as well as on create. + """ + + def test_patching_a_comment_onto_another_branch_is_refused(self): + comment = ChangeComment.objects.create( + change_request=self.cr, change_diff=self.diff, author=self.user, text='mine' + ) + permission = ObjectPermission.objects.create(name='comment-change', actions=['change']) + permission.object_types.add(ContentType.objects.get_for_model(ChangeComment)) + permission.users.add(self.user) + + response = self.client.patch( + f'/api/plugins/change-control/change-comments/{comment.pk}/', + {'change_diff': self.other_diff.pk}, + format='json', + **self.header, + ) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + comment.refresh_from_db() + self.assertEqual(comment.change_diff_id, self.diff.pk) diff --git a/netbox_change_control/tests/test_comments.py b/netbox_change_control/tests/test_comments.py index 7c83a12..48bdeaf 100644 --- a/netbox_change_control/tests/test_comments.py +++ b/netbox_change_control/tests/test_comments.py @@ -7,7 +7,7 @@ from django.core.exceptions import ValidationError from django.test import TestCase from netbox_branching.models import ChangeDiff -from users.models import User +from users.models import ObjectPermission, User from netbox_change_control.models import ChangeComment, ChangeRequest, Policy from netbox_change_control.tests.base import ChangeControlTestCase, approve, make_branch @@ -225,6 +225,7 @@ def test_an_unrelated_request_still_refreshes_during_a_delete(self): other_branch = make_branch('del-other', self._testMethodName) other = ChangeRequest.objects.create(branch=other_branch, title='Other', requester=self.requester) ChangeRequestPolicy.objects.create(change_request=other, policy=self.policy) + other.submit() self._comment() self.cr.delete() @@ -232,3 +233,144 @@ def test_an_unrelated_request_still_refreshes_during_a_delete(self): approve(other, self.reviewer) other.refresh_from_db() self.assertEqual(other.status, ChangeRequestStatusChoices.APPROVED) + + +class CommentEditingTest(TestCase): + """ + A comment can be corrected. + + Until now it could not: ChangeComment had no edit or delete view at all, so a typo in a + review comment was permanent unless somebody went to the REST API or the Django admin. + + Editing is restricted to the author, for the same reason a review is: a comment is a + statement attributed to a person, and rewriting somebody else's puts words in their mouth + in the record a reviewer reads before approving. Deleting follows NetBox's plain model. + """ + + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.author = User.objects.create(username='author') + cls.other = User.objects.create(username='other') + + def setUp(self): + from core.choices import ObjectChangeActionChoices + from django.contrib.contenttypes.models import ContentType + from netbox_branching.models import ChangeDiff + + from netbox_change_control.models import Policy + + self.branch = make_branch('editcomment', self._testMethodName) + self.cr = ChangeRequest.objects.create(branch=self.branch, title='T', requester=self.requester) + policy = Policy.objects.create(name=f'P-{self._testMethodName}'[:100]) + self.diff = ChangeDiff.objects.create( + branch=self.branch, + object_type=ContentType.objects.get_for_model(Policy), + object_id=policy.pk, + object_repr='thing', + action=ObjectChangeActionChoices.ACTION_UPDATE, + ) + self.comment = ChangeComment.objects.create( + change_request=self.cr, change_diff=self.diff, author=self.author, text='orignal typo' + ) + self.url = f'/plugins/change-control/change-comments/{self.comment.pk}/edit/' + + def grant(self, user, actions): + from django.contrib.contenttypes.models import ContentType + + permission = ObjectPermission.objects.create(name=f'c-{user.pk}-{actions[0]}', actions=list(actions)) + permission.object_types.add(ContentType.objects.get_for_model(ChangeComment)) + permission.users.add(user) + + def grant_tab_access(self, user): + """ + The Changes tab is gated on seeing the request and the branch diff, neither of which + is what these tests are about. + """ + from django.contrib.contenttypes.models import ContentType + + permission = ObjectPermission.objects.create(name=f'tab-{user.pk}', actions=['view']) + permission.object_types.set( + [ + ContentType.objects.get_for_model(ChangeRequest), + ContentType.objects.get_for_model(ChangeDiff), + ] + ) + permission.users.add(user) + + def changes_tab(self, user): + self.grant_tab_access(user) + self.client.force_login(user) + response = self.client.get(f'{self.cr.get_absolute_url()}changes/') + self.assertEqual(response.status_code, 200) + return response.content.decode() + + def test_the_author_can_correct_their_own_comment(self): + self.grant(self.author, ['view', 'change']) + self.client.force_login(self.author) + + self.client.post(self.url, {'text': 'original, corrected'}) + + self.comment.refresh_from_db() + self.assertEqual(self.comment.text, 'original, corrected') + + def test_somebody_else_cannot_rewrite_it(self): + self.grant(self.other, ['view', 'change']) + self.client.force_login(self.other) + + self.client.post(self.url, {'text': 'words I never wrote'}) + + self.comment.refresh_from_db() + self.assertEqual(self.comment.text, 'orignal typo') + + def test_a_superuser_can(self): + admin = User.objects.create(username='admin-editor', is_superuser=True) + self.client.force_login(admin) + + self.client.post(self.url, {'text': 'moderated'}) + + self.comment.refresh_from_db() + self.assertEqual(self.comment.text, 'moderated') + + def test_the_form_cannot_move_or_reattribute_the_comment(self): + """ + Only the text is editable, so a POST naming another author or another change is + ignored rather than obeyed. + """ + self.grant(self.author, ['view', 'change']) + self.client.force_login(self.author) + + self.client.post( + self.url, + {'text': 'still mine', 'author': self.other.pk, 'change_request': self.cr.pk, 'change_diff': ''}, + ) + + self.comment.refresh_from_db() + self.assertEqual(self.comment.author, self.author) + self.assertEqual(self.comment.change_diff_id, self.diff.pk) + + def test_deleting_needs_the_delete_permission(self): + self.grant(self.other, ['view', 'change']) + self.client.force_login(self.other) + + self.client.post(f'/plugins/change-control/change-comments/{self.comment.pk}/delete/', {'confirm': True}) + + self.assertTrue(ChangeComment.objects.filter(pk=self.comment.pk).exists()) + + def test_deleting_follows_the_plain_netbox_model(self): + self.grant(self.other, ['view', 'delete']) + self.client.force_login(self.other) + + self.client.post(f'/plugins/change-control/change-comments/{self.comment.pk}/delete/', {'confirm': True}) + + self.assertFalse(ChangeComment.objects.filter(pk=self.comment.pk).exists()) + + def test_the_changes_tab_offers_the_author_an_edit_link(self): + self.grant(self.author, ['view', 'change']) + + self.assertIn(f'/change-comments/{self.comment.pk}/edit/', self.changes_tab(self.author)) + + def test_the_changes_tab_offers_no_edit_link_to_anybody_else(self): + self.grant(self.other, ['view', 'change']) + + self.assertNotIn(f'/change-comments/{self.comment.pk}/edit/', self.changes_tab(self.other)) diff --git a/netbox_change_control/tests/test_conflicts.py b/netbox_change_control/tests/test_conflicts.py index 8c6c622..ceb754b 100644 --- a/netbox_change_control/tests/test_conflicts.py +++ b/netbox_change_control/tests/test_conflicts.py @@ -74,17 +74,52 @@ def test_a_clean_branch_reports_no_conflicts(self): self.assertEqual(self.cr.conflicts, []) self.assertFalse(self.cr.has_conflicts) - def test_conflicts_are_read_live(self): + def test_the_conflict_list_is_read_live(self): + """ + `conflicts` returns the actual objects and is always computed now. It is read once per + page, on the change request itself, so the two queries it costs are fine there. + """ # ChangeDiff.save() rewrites object_repr from the live object, so read it back rather # than asserting on the value this test passed in. diff = self._diff(conflicts=['device_type']) with main_moved(self.branch, diff): - self.assertTrue(self.cr.has_conflicts) self.assertEqual( [(d.object_repr, d.conflicts) for d in self.cr.conflicts], [(diff.object_repr, ['device_type'])], ) + def test_the_conflict_flag_is_cached_and_follows_a_refresh(self): + """ + `has_conflicts` reads a cached column, because the change request list shows it once + per row and computing it live cost two queries each time. + + It is refreshed by the same events that re-run the checks, which is what the diff + receiver does when branching recomputes a diff. + """ + from netbox_change_control.policy import refresh_cached_state + + diff = self._diff(conflicts=['device_type']) + with main_moved(self.branch, diff): + refresh_cached_state(self.cr) + + self.cr.refresh_from_db() + self.assertTrue(self.cr.has_conflicts) + self.assertTrue(self.cr.cached_conflicted) + + def test_the_cached_flag_clears_when_the_conflict_goes(self): + from netbox_change_control.policy import refresh_cached_state + + diff = self._diff(conflicts=['device_type']) + with main_moved(self.branch, diff): + refresh_cached_state(self.cr) + self.cr.refresh_from_db() + self.assertTrue(self.cr.has_conflicts) + + with main_moved(self.branch, diff, moved=False): + refresh_cached_state(self.cr) + self.cr.refresh_from_db() + self.assertFalse(self.cr.has_conflicts) + def test_the_check_reports_a_conflict(self): diff = self._diff(conflicts=['device_type']) with main_moved(self.branch, diff): diff --git a/netbox_change_control/tests/test_documented_constraints.py b/netbox_change_control/tests/test_documented_constraints.py new file mode 100644 index 0000000..957dd47 --- /dev/null +++ b/netbox_change_control/tests/test_documented_constraints.py @@ -0,0 +1,143 @@ +""" +The permission constraints the administration guide tells people to use. + +NetBox object permissions carry an optional `constraints` queryset filter, and the `$user` +token in one resolves to the signed-in user. That is a NetBox feature, not something this +plugin adds: the token is `users.constants.CONSTRAINT_TOKEN_USER`, it is substituted by +`ObjectPermissionBackend`, and NetBox core uses it itself for bookmarks and notifications. + +What this plugin does add is the claim that a particular field name works for a particular +model. `{"reviewer": "$user"}` is only useful advice if `Review.reviewer` is really the field +the constraint matches on, so these check each one the guide prints. +""" + +import re + +from django.contrib.contenttypes.models import ContentType +from django.test import TestCase +from netbox_branching.models import ChangeDiff +from users.models import ObjectPermission, User + +from netbox_change_control.choices import MergeCheckStatusChoices, ReviewDecisionChoices +from netbox_change_control.models import ChangeComment, ChangeRequest, MergeCheck, Policy, Review +from netbox_change_control.tests.base import docs_page, make_branch + + +def constrain(user, model, actions, constraints): + permission = ObjectPermission.objects.create( + name=f'{model._meta.model_name}-{user.pk}', actions=list(actions), constraints=constraints + ) + permission.object_types.add(ContentType.objects.get_for_model(model)) + permission.users.add(user) + return permission + + +class DocumentedConstraintsWorkTest(TestCase): + """ + Each row of the guide's constraint table, checked against the real permission backend. + """ + + @classmethod + def setUpTestData(cls): + cls.mine = User.objects.create(username='mine') + cls.theirs = User.objects.create(username='theirs') + cls.requester = User.objects.create(username='requester') + + def setUp(self): + self.branch = make_branch('constraint', self._testMethodName) + self.cr_mine = ChangeRequest.objects.create(branch=self.branch, title='Mine', requester=self.mine) + self.cr_theirs = ChangeRequest.objects.create( + branch=make_branch('constraint2', self._testMethodName), title='Theirs', requester=self.theirs + ) + + def test_a_reviewer_constraint_limits_deletion_to_own_reviews(self): + """ + The guide's headline example, and the reason it is there: `delete_review` is otherwise + model-wide, and deleting somebody's rejection changes the outcome of the gate. + """ + ours = Review.objects.create( + change_request=self.cr_theirs, reviewer=self.mine, decision=ReviewDecisionChoices.APPROVE + ) + not_ours = Review.objects.create( + change_request=self.cr_mine, reviewer=self.theirs, decision=ReviewDecisionChoices.APPROVE + ) + constrain(self.mine, Review, ['view', 'delete'], {'reviewer': '$user'}) + user = User.objects.get(pk=self.mine.pk) + + self.assertTrue(user.has_perm('netbox_change_control.delete_review', ours)) + self.assertFalse(user.has_perm('netbox_change_control.delete_review', not_ours)) + + def test_an_author_constraint_limits_a_comment_to_its_writer(self): + diff = ChangeDiff.objects.create( + branch=self.branch, + object_type=ContentType.objects.get_for_model(Policy), + object_id=1, + object_repr='x', + action='update', + ) + ours = ChangeComment.objects.create( + change_request=self.cr_mine, change_diff=diff, author=self.mine, text='mine' + ) + not_ours = ChangeComment.objects.create( + change_request=self.cr_mine, change_diff=diff, author=self.theirs, text='theirs' + ) + constrain(self.mine, ChangeComment, ['view', 'delete'], {'author': '$user'}) + user = User.objects.get(pk=self.mine.pk) + + self.assertTrue(user.has_perm('netbox_change_control.delete_changecomment', ours)) + self.assertFalse(user.has_perm('netbox_change_control.delete_changecomment', not_ours)) + + def test_a_requester_constraint_limits_a_change_request_to_its_owner(self): + constrain(self.mine, ChangeRequest, ['view', 'change'], {'requester': '$user'}) + user = User.objects.get(pk=self.mine.pk) + + self.assertTrue(user.has_perm('netbox_change_control.change_changerequest', self.cr_mine)) + self.assertFalse(user.has_perm('netbox_change_control.change_changerequest', self.cr_theirs)) + + def test_a_name_constraint_limits_a_ci_token_to_its_own_check(self): + """ + The shape the guide recommends for a reporting token: it can report the check it owns + and touch nothing else. + """ + ours = MergeCheck.objects.create( + change_request=self.cr_mine, name='ci-pipeline', status=MergeCheckStatusChoices.PENDING + ) + not_ours = MergeCheck.objects.create( + change_request=self.cr_mine, name='cab-approval', status=MergeCheckStatusChoices.PENDING + ) + constrain(self.mine, MergeCheck, ['view', 'change'], {'name': 'ci-pipeline'}) + user = User.objects.get(pk=self.mine.pk) + + self.assertTrue(user.has_perm('netbox_change_control.change_mergecheck', ours)) + self.assertFalse(user.has_perm('netbox_change_control.change_mergecheck', not_ours)) + + +class DocumentedConstraintFieldsExistTest(TestCase): + """ + Every field the guide's constraint table names has to be a real field, or the advice is a + filter that silently matches nothing. + """ + + def test_each_constraint_field_resolves(self): + table = re.search( + r'\| Goal \| Object type \| Constraint \|(.*?)\n\n', + docs_page('admin-guide.md'), + re.S, + ) + self.assertIsNotNone(table, 'the constraint table is no longer in the administration guide') + + models = { + 'Review': Review, + 'Change Comment': ChangeComment, + 'Change Request': ChangeRequest, + 'Merge Check': MergeCheck, + } + rows = re.findall(r'\|([^|]+)\|([^|]+)\|\s*`([^`]+)`\s*\|', table.group(1)) + self.assertTrue(rows, 'no constraint rows were parsed') + + for _goal, object_type, constraint in rows: + model = models.get(object_type.strip()) + self.assertIsNotNone(model, f'the guide names an object type this test does not know: {object_type}') + for field in re.findall(r'"(\w+)":', constraint): + with self.subTest(model=model.__name__, field=field): + model._meta.get_field(field) diff --git a/netbox_change_control/tests/test_documented_permissions.py b/netbox_change_control/tests/test_documented_permissions.py new file mode 100644 index 0000000..ea748a9 --- /dev/null +++ b/netbox_change_control/tests/test_documented_permissions.py @@ -0,0 +1,103 @@ +""" +The permissions page must list every permission, and no permission that does not exist. + +It used to say "view_changerequest and friends", which meant a reader had to guess the rest +and an administrator building a group had nothing to work from. Guessing is also how the page +drifts: a permission added later is simply never written down. + +This compares the page against the model definitions, so neither can move without the other. +""" + +import re + +from django.contrib.contenttypes.models import ContentType +from django.test import TestCase + +from netbox_change_control.models import ( + ChangeComment, + ChangeRequest, + ChangeRequestPolicy, + MergeCheck, + Policy, + PolicyRule, + Review, +) +from netbox_change_control.tests.base import docs_page + +MODELS = (ChangeComment, ChangeRequest, ChangeRequestPolicy, MergeCheck, Policy, PolicyRule, Review) + + +def declared_permissions(): + """ + Every permission Django and the plugin define for this plugin's models, read from the + database rather than from a list somebody has to maintain. + """ + from django.contrib.auth.models import Permission + + return { + f'netbox_change_control.{p.codename}' + for p in Permission.objects.filter(content_type__in=[ContentType.objects.get_for_model(m) for m in MODELS]) + } + + +def documented_permissions(page): + return set(re.findall(r'`(netbox_change_control\.[a-z_]+)`', docs_page(page))) + + +class PermissionsPageTest(TestCase): + page = 'permissions.md' + + def test_every_permission_is_documented(self): + missing = sorted(declared_permissions() - documented_permissions(self.page)) + self.assertEqual( + missing, + [], + f'{self.page} does not mention these permissions, which the plugin defines', + ) + + def test_no_documented_permission_is_invented(self): + invented = sorted(documented_permissions(self.page) - declared_permissions()) + self.assertEqual( + invented, + [], + f'{self.page} names permissions which do not exist', + ) + + def test_each_one_is_in_a_table_row_with_its_action_and_meaning(self): + """ + Listing a name in prose is not documenting it. Each has to be a row carrying the + action to enter on the permission form and what it grants. + """ + text = docs_page(self.page) + rows = { + m.group(1) + for m in re.finditer(r'^\| `(netbox_change_control\.[a-z_]+)` \| `[a-z_]+` \| .+ \|$', text, re.M) + } + missing = sorted(declared_permissions() - rows) + self.assertEqual(missing, [], f'{self.page} mentions these but not as a full table row') + + +class AdminGuideTest(TestCase): + """ + The guide carries a role matrix. It does not have to name every permission, but every name + it does carry has to be real. + """ + + def test_no_invented_permission_in_the_admin_guide(self): + invented = sorted(documented_permissions('admin-guide.md') - declared_permissions()) + self.assertEqual(invented, [], 'admin-guide.md names permissions which do not exist') + + +class PolicyBindingPermissionsTest(TestCase): + """ + The through table binding a policy to a change request defines no permissions. + + Django would create four, and all four were dead: nothing reads them, and an administrator + granting one to detach a policy by hand gets the binding back at the next re-match. + """ + + def test_the_binding_table_defines_no_permissions(self): + from django.contrib.auth.models import Permission + + content_type = ContentType.objects.get_for_model(ChangeRequestPolicy) + self.assertEqual(list(Permission.objects.filter(content_type=content_type)), []) diff --git a/netbox_change_control/tests/test_documented_settings.py b/netbox_change_control/tests/test_documented_settings.py new file mode 100644 index 0000000..87daf6d --- /dev/null +++ b/netbox_change_control/tests/test_documented_settings.py @@ -0,0 +1,46 @@ +""" +The configuration table must list every setting, with the default the plugin actually uses. + +A setting added in code and not written down is a setting nobody knows to set, and a default +that has moved since somebody wrote the table is worse than no table: it is read as a promise. +This compares the page against `default_settings`, the same way the permissions page is +compared against the models. +""" + +import re + +from django.test import TestCase + +from netbox_change_control import ChangeControlConfig +from netbox_change_control.tests.base import docs_page + +PAGE = 'installation.md' + +# A row of the configuration table: the setting, its default, and what it means. +ROW = re.compile(r'^\| `([a-z_]+)` \| `(.+?)` \| .+ \|$', re.M) + + +def documented_settings(): + return {name: default for name, default in ROW.findall(docs_page(PAGE))} + + +class DocumentedSettingsTest(TestCase): + def test_every_setting_is_documented(self): + missing = sorted(set(ChangeControlConfig.default_settings) - set(documented_settings())) + self.assertEqual(missing, [], f'{PAGE} does not list these settings, which the plugin defines') + + def test_no_documented_setting_is_invented(self): + invented = sorted(set(documented_settings()) - set(ChangeControlConfig.default_settings)) + self.assertEqual(invented, [], f'{PAGE} lists these settings, which the plugin does not define') + + def test_every_documented_default_is_the_real_one(self): + """ + Written as Python, because that is what a reader copies into PLUGINS_CONFIG. + """ + documented = documented_settings() + wrong = { + name: (documented[name], repr(default)) + for name, default in ChangeControlConfig.default_settings.items() + if name in documented and documented[name] != repr(default) + } + self.assertEqual(wrong, {}, f'{PAGE} gives a default which is not the one in the code') diff --git a/netbox_change_control/tests/test_lifecycle_actions.py b/netbox_change_control/tests/test_lifecycle_actions.py new file mode 100644 index 0000000..6772008 --- /dev/null +++ b/netbox_change_control/tests/test_lifecycle_actions.py @@ -0,0 +1,462 @@ +""" +Abandoning and reopening a change request. + +`status` is a cached view of the policy evaluation, so it is not an editable field. It used to +be, on the bulk edit form and over the REST API, and Completed is terminal: the merge gate +refuses a completed request and nothing reopens one, so setting it by hand blocked a branch +from merging for good with no way back through the interface. + +These are the two transitions a person legitimately makes, each behind its own permission. +""" + +from django.contrib.contenttypes.models import ContentType +from django.test import TestCase +from rest_framework import status as http +from users.models import Group, ObjectPermission, User +from utilities.testing import APITestCase + +from netbox_change_control.choices import ChangeRequestStatusChoices +from netbox_change_control.models import ChangeRequest, ChangeRequestPolicy, Policy, PolicyRule +from netbox_change_control.tests.base import approve, make_branch + + +def grant(user, actions, model=ChangeRequest, name='cr'): + permission = ObjectPermission.objects.create(name=name, actions=actions) + permission.object_types.add(ContentType.objects.get_for_model(model)) + permission.users.add(user) + return permission + + +class AbandonAndReopenModelTest(TestCase): + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.group = Group.objects.create(name='Engineers') + cls.reviewer = User.objects.create(username='reviewer') + cls.reviewer.groups.add(cls.group) + cls.policy = Policy.objects.create(name='One review') + PolicyRule.objects.create(policy=cls.policy, name='One engineer', min_reviews=1).groups.set([cls.group]) + + def make_request(self): + cr = ChangeRequest.objects.create( + branch=make_branch('life', self._testMethodName), title='T', requester=self.requester + ) + ChangeRequestPolicy.objects.create(change_request=cr, policy=self.policy) + cr.submit() + cr.refresh_from_db() + return cr + + def test_an_open_request_can_be_abandoned(self): + cr = self.make_request() + self.assertTrue(cr.abandon()) + cr.refresh_from_db() + self.assertEqual(cr.status, ChangeRequestStatusChoices.ABANDONED) + + def test_a_completed_request_cannot_be_abandoned(self): + cr = self.make_request() + cr.status = ChangeRequestStatusChoices.COMPLETED + cr.save(update_fields=['status']) + self.assertFalse(cr.abandon()) + cr.refresh_from_db() + self.assertEqual(cr.status, ChangeRequestStatusChoices.COMPLETED) + + def test_an_abandoned_request_reopens_as_a_draft(self): + """ + Back into the author's hands, not straight into somebody's review queue. Its reviews + may have gone stale and its policies may have moved while it was set aside, so it is + submitted again and the evaluation works out the honest answer then. + """ + cr = self.make_request() + cr.abandon() + + self.assertTrue(cr.reopen()) + + cr.refresh_from_db() + self.assertEqual(cr.status, ChangeRequestStatusChoices.DRAFT) + + def test_a_completed_request_cannot_be_reopened(self): + """ + Completed records a merge that happened. Reopening it would invite a second merge of a + branch already in main. + """ + cr = self.make_request() + cr.status = ChangeRequestStatusChoices.COMPLETED + cr.save(update_fields=['status']) + self.assertFalse(cr.reopen()) + cr.refresh_from_db() + self.assertEqual(cr.status, ChangeRequestStatusChoices.COMPLETED) + + def test_reopening_does_not_restore_a_previous_approval(self): + """ + A request approved before it was abandoned must not come back approved. + """ + cr = self.make_request() + approve(cr, self.reviewer) + cr.refresh_from_db() + self.assertEqual(cr.status, ChangeRequestStatusChoices.APPROVED) + + cr.abandon() + + self.assertTrue(cr.reopen()) + cr.refresh_from_db() + self.assertEqual(cr.status, ChangeRequestStatusChoices.DRAFT) + + # Submitting again is what re-runs the evaluation, and it is honest about what it finds. + cr.submit() + cr.refresh_from_db() + self.assertEqual(cr.status, ChangeRequestStatusChoices.APPROVED) + + +class AbandonAndReopenViewTest(TestCase): + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.actor = User.objects.create(username='actor') + + def setUp(self): + self.cr = ChangeRequest.objects.create( + branch=make_branch('lifeview', self._testMethodName), title='T', requester=self.requester + ) + self.client.force_login(self.actor) + + def test_abandoning_without_the_permission_is_refused(self): + grant(self.actor, ['view', 'change']) + self.client.post(f'/plugins/change-control/change-requests/{self.cr.pk}/abandon/') + self.cr.refresh_from_db() + self.assertNotEqual(self.cr.status, ChangeRequestStatusChoices.ABANDONED) + + def test_abandoning_with_the_permission_works(self): + grant(self.actor, ['view', 'abandon']) + self.client.post(f'/plugins/change-control/change-requests/{self.cr.pk}/abandon/') + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.ABANDONED) + + def test_reopening_needs_its_own_permission(self): + self.cr.abandon() + grant(self.actor, ['view', 'abandon']) + self.client.post(f'/plugins/change-control/change-requests/{self.cr.pk}/reopen/') + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.ABANDONED) + + def test_reopening_with_the_permission_works(self): + self.cr.abandon() + grant(self.actor, ['view', 'reopen']) + self.client.post(f'/plugins/change-control/change-requests/{self.cr.pk}/reopen/') + self.cr.refresh_from_db() + self.assertNotEqual(self.cr.status, ChangeRequestStatusChoices.ABANDONED) + + +class StatusIsNotWritableTest(APITestCase): + def setUp(self): + super().setUp() + self.requester = User.objects.create(username='requester') + self.cr = ChangeRequest.objects.create(branch=make_branch('apilife', 'x'), title='T', requester=self.requester) + grant(self.user, ['view', 'change']) + + def test_patching_status_to_completed_is_ignored(self): + """ + The bug this file exists for. Completed is terminal and the gate refuses it, so a + writable status was one request away from blocking a branch permanently. + """ + response = self.client.patch( + f'/api/plugins/change-control/change-requests/{self.cr.pk}/', + {'status': ChangeRequestStatusChoices.COMPLETED}, + format='json', + **self.header, + ) + self.assertEqual(response.status_code, http.HTTP_200_OK) + self.cr.refresh_from_db() + self.assertNotEqual(self.cr.status, ChangeRequestStatusChoices.COMPLETED) + + def test_status_is_still_reported(self): + response = self.client.get(f'/api/plugins/change-control/change-requests/{self.cr.pk}/', **self.header) + self.assertEqual(response.data['status'], self.cr.status) + + def test_the_abandon_action_needs_its_permission(self): + response = self.client.post(f'/api/plugins/change-control/change-requests/{self.cr.pk}/abandon/', **self.header) + self.assertEqual(response.status_code, http.HTTP_403_FORBIDDEN) + + def test_the_abandon_action_works_with_it(self): + grant(self.user, ['view', 'abandon'], name='cr-abandon') + response = self.client.post(f'/api/plugins/change-control/change-requests/{self.cr.pk}/abandon/', **self.header) + self.assertEqual(response.status_code, http.HTTP_200_OK) + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.ABANDONED) + + def test_abandoning_a_completed_request_reports_a_conflict(self): + self.cr.status = ChangeRequestStatusChoices.COMPLETED + self.cr.save(update_fields=['status']) + grant(self.user, ['view', 'abandon'], name='cr-abandon') + response = self.client.post(f'/api/plugins/change-control/change-requests/{self.cr.pk}/abandon/', **self.header) + self.assertEqual(response.status_code, http.HTTP_409_CONFLICT) + + +class SubmitForReviewPermissionTest(TestCase): + """ + Submitting is an edit to the request, so it needs the permission to change one. + + It used to need nothing: any signed-in user could push somebody else's draft into review, + attaching its policies and announcing change_request_submitted to every event rule. + """ + + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.outsider = User.objects.create(username='outsider') + cls.group = Group.objects.create(name='Engineers') + cls.policy = Policy.objects.create(name='One review') + PolicyRule.objects.create(policy=cls.policy, name='One engineer', min_reviews=1).groups.set([cls.group]) + + def setUp(self): + self.cr = ChangeRequest.objects.create( + branch=make_branch('submitperm', self._testMethodName), title='T', requester=self.requester + ) + self.url = f'/plugins/change-control/change-requests/{self.cr.pk}/submit/' + + def test_a_user_with_only_view_cannot_submit(self): + grant(self.outsider, ['view'], name='view-only') + self.client.force_login(self.outsider) + self.client.post(self.url) + + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.DRAFT) + self.assertEqual(self.cr.policies.count(), 0) + + def test_a_user_with_change_can_submit(self): + grant(self.requester, ['view', 'change'], name='may-change') + self.client.force_login(self.requester) + self.client.post(self.url) + + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.NEEDS_REVIEW) + self.assertEqual(self.cr.policies.count(), 1) + + +class ReadOnlyTokenTest(APITestCase): + """ + The abandon and reopen actions carry their own permission class, which drops NetBox's + blanket `add_` requirement for POST. Everything else it inherits has to keep + working, and a read-only token being refused is the part worth pinning: dropping an entry + from a perms_map is exactly the kind of change that quietly widens more than intended. + """ + + def setUp(self): + super().setUp() + self.requester = User.objects.create(username='requester') + self.cr = ChangeRequest.objects.create(branch=make_branch('rotoken', 'x'), title='T', requester=self.requester) + grant(self.user, ['view', 'abandon'], name='cr-abandon') + self.url = f'/api/plugins/change-control/change-requests/{self.cr.pk}/abandon/' + + def test_a_write_token_holding_the_permission_may_abandon(self): + response = self.client.post(self.url, **self.header) + self.assertEqual(response.status_code, http.HTTP_200_OK) + + def test_a_read_only_token_may_not(self): + self.token.write_enabled = False + self.token.save() + + response = self.client.post(self.url, **self.header) + + self.assertEqual(response.status_code, http.HTTP_403_FORBIDDEN) + self.cr.refresh_from_db() + self.assertNotEqual(self.cr.status, ChangeRequestStatusChoices.ABANDONED) + + +class ActionButtonPlacementTest(TestCase): + """ + Abandon and reopen act on the whole change request, so they belong with Edit and Delete in + the page's control bar. They were in the footer of the Applied policies card, where they + read as something to do with the policies. + """ + + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.admin = User.objects.create(username='admin-viewer', is_superuser=True) + + def setUp(self): + self.cr = ChangeRequest.objects.create( + branch=make_branch('placement', self._testMethodName), title='T', requester=self.requester + ) + self.client.force_login(self.admin) + + def controls(self): + """ + The buttons in the page's control bar, which is where object actions live. + """ + import re + + html = self.client.get(self.cr.get_absolute_url()).content.decode() + bar = re.search( + r'
(.*?)
', + html, + re.S, + ) + self.assertIsNotNone(bar, 'the control bar was not found; the NetBox template may have changed') + return re.sub(r'\s+', ' ', re.sub(r'<[^>]+>', ' ', bar.group(1))) + + def policies_card(self): + import re + + html = self.client.get(self.cr.get_absolute_url()).content.decode() + card = re.search(r'Applied policies(.*?)Reviews', html, re.S) + return card.group(1) if card else '' + + def test_abandon_is_in_the_control_bar(self): + self.assertIn('Abandon', self.controls()) + + def test_abandon_is_not_in_the_policies_card(self): + self.assertNotIn('Abandon', self.policies_card()) + + def test_reopen_replaces_it_once_abandoned(self): + self.cr.abandon() + + controls = self.controls() + + self.assertIn('Reopen', controls) + self.assertNotIn('Abandon', controls) + + def test_a_completed_request_offers_neither(self): + self.cr.status = ChangeRequestStatusChoices.COMPLETED + self.cr.save(update_fields=['status']) + + controls = self.controls() + + self.assertNotIn('Abandon', controls) + self.assertNotIn('Reopen', controls) + + def test_submit_is_in_the_control_bar_too(self): + """ + All four lifecycle actions live together. Submitting used to sit in the Applied + policies card, where it read as something to do with the policies. + """ + self.assertIn('Submit for review', self.controls()) + self.assertNotIn('Submit for review', self.policies_card()) + + +class DraftIsTheAuthorsTest(TestCase): + """ + Draft is a state a person holds, not one the evaluation computes. + + Without that, **Return to draft** would be a button that does nothing: the next review, + policy edit or branch change would push the request straight back into review. It also + means submitting has to say so explicitly, and has to do its own announcing. + """ + + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.group = Group.objects.create(name='Engineers') + cls.reviewer = User.objects.create(username='reviewer') + cls.reviewer.groups.add(cls.group) + cls.policy = Policy.objects.create(name='One review') + PolicyRule.objects.create(policy=cls.policy, name='One engineer', min_reviews=1).groups.set([cls.group]) + + def setUp(self): + self.cr = ChangeRequest.objects.create( + branch=make_branch('draft', self._testMethodName), title='T', requester=self.requester + ) + + def test_a_review_against_a_draft_moves_nothing(self): + approve(self.cr, self.reviewer) + + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.DRAFT) + + def test_submitting_attaches_the_policies_and_asks_for_review(self): + self.assertTrue(self.cr.submit()) + + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.NEEDS_REVIEW) + self.assertEqual(self.cr.policies.count(), 1) + + def test_submitting_notifies_the_reviewers(self): + """ + refresh_status announces only the transitions it makes itself, and by the time it runs + the status is already Needs review, so it sees nothing to announce. Submitting in + silence is the one outcome that makes the whole thing pointless. + """ + from extras.models import Notification + + self.cr.submit() + + self.assertEqual(Notification.objects.filter(user=self.reviewer).count(), 1) + + def test_a_submitted_request_can_be_pulled_back(self): + self.cr.submit() + + self.assertTrue(self.cr.return_to_draft()) + + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.DRAFT) + + def test_a_request_pulled_back_stays_pulled_back(self): + """ + The property that makes the button worth having. + """ + self.cr.submit() + self.cr.return_to_draft() + + approve(self.cr, self.reviewer) + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.DRAFT) + + self.policy.description = 'edited, which re-evaluates every bound request' + self.policy.save() + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.DRAFT) + + def test_an_approved_request_can_be_pulled_back(self): + """ + An author who spots a problem after approval takes the change off the table rather + than racing the merge. It only ever closes the gate: a draft cannot merge. + """ + self.cr.submit() + approve(self.cr, self.reviewer) + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.APPROVED) + + self.assertTrue(self.cr.return_to_draft()) + + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.DRAFT) + self.assertFalse(self.cr.is_ready_to_merge) + + def test_resubmitting_recovers_the_standing_approval(self): + """ + The reviews are kept, so an author who pulls a change back and changes nothing gets + the same answer when they submit it again. + """ + self.cr.submit() + approve(self.cr, self.reviewer) + self.cr.return_to_draft() + + self.cr.submit() + + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.APPROVED) + + def test_a_draft_cannot_be_returned_to_draft(self): + self.assertFalse(self.cr.return_to_draft()) + + def test_a_completed_request_cannot_be_returned_to_draft(self): + self.cr.submit() + self.cr.status = ChangeRequestStatusChoices.COMPLETED + self.cr.save(update_fields=['status']) + + self.assertFalse(self.cr.return_to_draft()) + + def test_only_a_draft_can_be_submitted(self): + self.cr.submit() + + self.assertFalse(self.cr.submit()) + + def test_the_cached_columns_still_follow_a_draft(self): + """ + A draft's status is frozen; the branch it points at is not. + """ + self.cr.submit() + self.cr.return_to_draft() + + self.cr.refresh_from_db() + self.assertEqual(self.cr.cached_conflicted, bool(self.cr.conflicts)) diff --git a/netbox_change_control/tests/test_list_actions.py b/netbox_change_control/tests/test_list_actions.py new file mode 100644 index 0000000..5bf93e1 --- /dev/null +++ b/netbox_change_control/tests/test_list_actions.py @@ -0,0 +1,100 @@ +""" +Every action button on a list page must lead somewhere. + +NetBox's ObjectListView offers add, import, export, edit, rename and delete by default, and +`ObjectAction.get_url` swallows the NoReverseMatch for any the plugin does not route. The +button still renders, with the literal string "None" as its target, and 404s on click. Every +list view therefore declares the actions it actually has. + +This walks every list page rather than checking a fixed set, so a view added later is covered +without anybody remembering to add it here. +""" + +import re + +from django.test import TestCase +from users.models import User + +from netbox_change_control import views + +# Matches an anchor or a button whose target is the string "None", which is what a missing +# route renders as. +BROKEN = re.compile(r'<(?:a|button)[^>]*(?:href|formaction)="None"[^>]*>(.*?)', re.S) + +LIST_VIEW_CLASSES = ( + views.PolicyListView, + views.PolicyRuleListView, + views.ChangeRequestListView, + views.ReviewListView, + views.MergeCheckListView, +) + +LIST_VIEWS = ( + ('policies', '/plugins/change-control/policies/'), + ('policy rules', '/plugins/change-control/policy-rules/'), + ('change requests', '/plugins/change-control/change-requests/'), + ('reviews', '/plugins/change-control/reviews/'), + ('merge checks', '/plugins/change-control/checks/'), +) + + +class ListActionButtonsTest(TestCase): + @classmethod + def setUpTestData(cls): + # A superuser holds every permission, so every action the view offers is rendered. + # That is the case which exposes a missing route. + cls.user = User.objects.create(username='admin', is_superuser=True) + + def setUp(self): + self.client.force_login(self.user) + + def test_every_list_page_renders(self): + for label, path in LIST_VIEWS: + with self.subTest(page=label): + self.assertEqual(self.client.get(path).status_code, 200) + + def test_no_action_button_targets_nothing(self): + for label, path in LIST_VIEWS: + with self.subTest(page=label): + html = self.client.get(path).content.decode() + broken = sorted({re.sub(r'<[^>]+>', '', m).strip() for m in BROKEN.findall(html)}) + self.assertEqual(broken, [], f'{label} renders buttons with no target: {broken}') + + def test_every_declared_action_that_needs_a_route_has_one(self): + """ + The other direction, so a declared action cannot be the one rendering "None". + + BulkExport is excluded because it needs no route: NetBox renders it as a dropdown of + query-string links against the list view itself, and its template never uses `url`. + """ + from netbox.object_actions import BulkExport + + for view_class in LIST_VIEW_CLASSES: + model = view_class.queryset.model + for action_class in view_class.actions: + if action_class is BulkExport: + continue + with self.subTest(view=view_class.__name__, action=action_class.__name__): + self.assertIsNotNone( + action_class.get_url(model), + f'{view_class.__name__} offers {action_class.__name__} but no route resolves for it', + ) + + def test_the_dropped_actions_are_still_unroutable(self): + """ + Import and bulk rename were dropped because this plugin routes neither. If either is + given a view later, re-add it to `actions` rather than leaving it silently missing; + this test is the reminder. + """ + from netbox.object_actions import BulkImport, BulkRename + + for view_class in LIST_VIEW_CLASSES: + model = view_class.queryset.model + for action_class in (BulkImport, BulkRename): + with self.subTest(view=view_class.__name__, action=action_class.__name__): + self.assertNotIn(action_class, view_class.actions) + self.assertIsNone( + action_class.get_url(model), + f'{model.__name__} now routes {action_class.__name__}; add it back to ' + f'{view_class.__name__}.actions', + ) diff --git a/netbox_change_control/tests/test_notifications.py b/netbox_change_control/tests/test_notifications.py index b01af3d..795f044 100644 --- a/netbox_change_control/tests/test_notifications.py +++ b/netbox_change_control/tests/test_notifications.py @@ -32,6 +32,7 @@ def setUp(self): self.branch = make_branch('notif', self._testMethodName) self.cr = ChangeRequest.objects.create(branch=self.branch, title='T', requester=self.requester) ChangeRequestPolicy.objects.create(change_request=self.cr, policy=self.policy) + self.cr.submit() self.cr.refresh_from_db() def _notifications(self, user): @@ -75,16 +76,18 @@ def test_repeat_notification_updates_rather_than_failing(self): from django.utils import timezone from extras.models import Notification - refresh_status(self.cr) + self.assertEqual(self._notifications(self.eng).count(), 1) Notification.objects.filter(user=self.eng).update(read=timezone.now()) - # Force a transition back into needs-review. - self.cr.status = ChangeRequestStatusChoices.DRAFT - self.cr.save(update_fields=['status']) - refresh_status(self.cr) + # The real round trip: the author pulls the change back, works on it, and submits it + # again. Writing the status by hand no longer does anything, because draft is the + # author's to hold and the evaluation will not move a request out of it. + self.cr.return_to_draft() + self.cr.submit() self.assertEqual(self._notifications(self.eng).count(), 1) self.assertIsNone(self._notifications(self.eng).first().read) + self.assertIsNone(self._notifications(self.eng).first().read) class EventTypeRenderingTest(TestCase): diff --git a/netbox_change_control/tests/test_policy.py b/netbox_change_control/tests/test_policy.py index c16fbac..5ea9507 100644 --- a/netbox_change_control/tests/test_policy.py +++ b/netbox_change_control/tests/test_policy.py @@ -182,6 +182,7 @@ def _request(self): requester=self.requester, ) ChangeRequestPolicy.objects.create(change_request=cr, policy=self.policy) + cr.submit() cr.refresh_from_db() return cr @@ -236,10 +237,10 @@ def test_it_does_not_weaken_another_policy(self): group = Group.objects.create(name='Engineers') engineer = User.objects.create(username='engineer') engineer.groups.add(group) - strict, _rule = make_policy('Strict', groups=[group], rule_name='One engineer') + _strict, _rule = make_policy('Strict', groups=[group], rule_name='One engineer') + # Both policies are unscoped, so submitting matches them both. cr = self._request() - ChangeRequestPolicy.objects.create(change_request=cr, policy=strict) cr.refresh_from_db() self.assertEqual(cr.status, ChangeRequestStatusChoices.NEEDS_REVIEW) diff --git a/netbox_change_control/tests/test_protect_main.py b/netbox_change_control/tests/test_protect_main.py index 0d04aee..2636cac 100644 --- a/netbox_change_control/tests/test_protect_main.py +++ b/netbox_change_control/tests/test_protect_main.py @@ -24,7 +24,6 @@ def _plugin_config(name, setting, default=None): 'protect_main': True, 'protect_main_scope': [], 'enforce_merge_gate': True, - 'lock_matched_policies': True, } return values.get(setting, default) @@ -39,7 +38,6 @@ def _config(name, setting, default=None): 'protect_main': True, 'protect_main_scope': scope, 'enforce_merge_gate': True, - 'lock_matched_policies': True, } return values.get(setting, default) diff --git a/netbox_change_control/tests/test_reviewer_display.py b/netbox_change_control/tests/test_reviewer_display.py new file mode 100644 index 0000000..33de21c --- /dev/null +++ b/netbox_change_control/tests/test_reviewer_display.py @@ -0,0 +1,180 @@ +""" +How a rule says who may satisfy it. + +The panel used to expand the rule into everybody it currently resolved to. A group of fifteen +therefore printed fifteen usernames, on every rule that group satisfied, on two pages, and +buried the approval counts that are the point of the panel. It also went stale in a way the +policy never does: the list moved as people joined and left while the rule had not changed. + +It now shows the rule as written. The one thing the expansion did better, warning that nobody +at all is eligible, is kept. +""" + +from django.test import TestCase +from users.models import Group, User + +from netbox_change_control.models import ChangeRequest, ChangeRequestPolicy, Policy, PolicyRule +from netbox_change_control.tests.base import make_branch + + +class ReviewerSummaryTest(TestCase): + def rule(self, groups=(), users=()): + policy = Policy.objects.create(name=f'P-{self._testMethodName}'[:100]) + rule = PolicyRule.objects.create(policy=policy, name='r', min_reviews=1) + rule.groups.set(groups) + rule.users.set(users) + return rule + + def test_a_group_is_named_not_expanded(self): + group = Group.objects.create(name='Change Engineers') + for name in ('erin', 'frank', 'grace'): + User.objects.create(username=name).groups.add(group) + + summary = self.rule(groups=[group]).reviewer_summary + + self.assertEqual(summary.group_names, ('Change Engineers',)) + self.assertEqual(summary.user_names, ()) + self.assertTrue(summary.anybody) + + def test_several_groups_are_listed_in_order(self): + leads = Group.objects.create(name='Change Leads') + engineers = Group.objects.create(name='Change Engineers') + User.objects.create(username='erin').groups.add(engineers) + + summary = self.rule(groups=[leads, engineers]).reviewer_summary + + self.assertEqual(summary.group_names, ('Change Engineers', 'Change Leads')) + + def test_a_directly_named_user_is_listed(self): + dave = User.objects.create(username='dave') + + summary = self.rule(users=[dave]).reviewer_summary + + self.assertEqual(summary.user_names, ('dave',)) + self.assertTrue(summary.anybody) + + def test_groups_and_named_users_are_both_shown(self): + group = Group.objects.create(name='Change Engineers') + User.objects.create(username='erin').groups.add(group) + dave = User.objects.create(username='dave') + + summary = self.rule(groups=[group], users=[dave]).reviewer_summary + + self.assertEqual(summary.group_names, ('Change Engineers',)) + self.assertEqual(summary.user_names, ('dave',)) + + def test_a_rule_naming_an_empty_group_reports_nobody(self): + """ + The trap the expanded list did catch: a rule pointing at a group with no members can + never be satisfied, and the panel has to say so. + """ + empty = Group.objects.create(name='Nobody Here') + + summary = self.rule(groups=[empty]).reviewer_summary + + self.assertEqual(summary.group_names, ('Nobody Here',)) + self.assertFalse(summary.anybody) + + def test_a_rule_naming_nothing_at_all_reports_nobody(self): + summary = self.rule().reviewer_summary + + self.assertFalse(summary.anybody) + + def test_a_named_user_needs_no_extra_query(self): + """ + The eligibility test is skipped when the rule names somebody directly, because the + answer is already known. + """ + dave = User.objects.create(username='dave') + rule = self.rule(users=[dave]) + rule = PolicyRule.objects.prefetch_related('groups', 'users').get(pk=rule.pk) + + with self.assertNumQueries(0): + self.assertTrue(rule.reviewer_summary.anybody) + + +class ReviewerDisplayInThePagesTest(TestCase): + """ + Both pages render the same shared include, so they cannot drift apart. + """ + + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.group = Group.objects.create(name='Change Engineers') + # A group large enough that expanding it would be the problem this fixed. + cls.members = [User.objects.create(username=f'eng{i:02d}') for i in range(12)] + for member in cls.members: + member.groups.add(cls.group) + + cls.policy = Policy.objects.create(name='Device changes') + PolicyRule.objects.create(policy=cls.policy, name='Two engineers', min_reviews=2).groups.set([cls.group]) + + cls.admin = User.objects.create(username='admin-viewer', is_superuser=True) + + def setUp(self): + self.branch = make_branch('display', self._testMethodName) + self.cr = ChangeRequest.objects.create(branch=self.branch, title='T', requester=self.requester) + ChangeRequestPolicy.objects.create(change_request=self.cr, policy=self.policy) + self.cr.refresh_from_db() + self.client.force_login(self.admin) + + def test_the_change_request_page_names_the_group(self): + html = self.client.get(self.cr.get_absolute_url()).content.decode() + + self.assertIn('Change Engineers', html) + for member in self.members: + self.assertNotIn(member.username, html) + + def test_the_branch_page_names_the_group(self): + from netbox_branching.choices import BranchStatusChoices + from netbox_branching.models import Branch + + Branch.objects.filter(pk=self.branch.pk).update(status=BranchStatusChoices.READY) + + html = self.client.get(self.branch.get_absolute_url()).content.decode() + + self.assertIn('Change Engineers', html) + for member in self.members: + self.assertNotIn(member.username, html) + + def test_an_empty_group_is_named_and_called_out(self): + """ + A rule pointing at a group with no members can never be satisfied. Naming the group is + the difference between knowing that and knowing what to fix. + """ + empty = Group.objects.create(name='Nobody Here') + PolicyRule.objects.create(policy=self.policy, name='A lead', min_reviews=1).groups.set([empty]) + + html = self.client.get(self.cr.get_absolute_url()).content.decode() + + self.assertIn('Nobody Here', html) + self.assertIn('can never be satisfied', html) + + def test_a_rule_naming_nobody_at_all_says_so(self): + PolicyRule.objects.create(policy=self.policy, name='Unassigned', min_reviews=1) + + html = self.client.get(self.cr.get_absolute_url()).content.decode() + + self.assertIn('can never be satisfied', html) + + def test_the_page_cost_does_not_grow_with_the_group(self): + """ + Naming the group rather than expanding it should also stop the page paying per member. + """ + from django.db import connection + from django.test.utils import CaptureQueriesContext + + with CaptureQueriesContext(connection) as small: + self.client.get(self.cr.get_absolute_url()) + + big = Group.objects.create(name='Everyone') + for i in range(40): + User.objects.create(username=f'extra{i:02d}').groups.add(big) + PolicyRule.objects.create(policy=self.policy, name='Anybody', min_reviews=1).groups.set([big]) + + with CaptureQueriesContext(connection) as large: + self.client.get(self.cr.get_absolute_url()) + + # One more rule costs a little; forty more people must cost nothing. + self.assertLessEqual(len(large.captured_queries) - len(small.captured_queries), 6) diff --git a/netbox_change_control/tests/test_scope.py b/netbox_change_control/tests/test_scope.py new file mode 100644 index 0000000..28a9677 --- /dev/null +++ b/netbox_change_control/tests/test_scope.py @@ -0,0 +1,244 @@ +""" +Tests for the policy scope following the branch. + +Which policies govern a change request is decided from the object types its branch touches. +A branch is not fixed at submission time: an author can keep editing inside it, and each edit +can bring in an object type no attached policy covers. + +If the governing set does not follow, the gate can be walked around. Open a request on a +branch touching only low-risk objects, collect the light approval that attracts, then add the +real change to the same branch. The approvals go stale and the status returns to Needs review, +but a policy that never attached asks for nothing, so the same reviewer approves a second time +and the work merges unseen by anybody with the authority to judge it. + +These tests pin both halves: the branch growing an object type must pull the policy in, and +the branch losing one must let it go. +""" + +from core.models import ObjectType +from django.contrib.contenttypes.models import ContentType +from django.test import TestCase +from netbox_branching.models import ChangeDiff +from users.models import Group, User + +from netbox_change_control.choices import ChangeRequestStatusChoices +from netbox_change_control.models import ChangeRequest, ChangeRequestPolicy, Policy, PolicyRule +from netbox_change_control.policy import sync_policies +from netbox_change_control.tests.base import approve, make_branch +from netbox_change_control.validators import require_approved_change_request + + +def touch(branch, model, object_id, repr_='thing', **kwargs): + """ + Record that the branch changes one object, the way branching does. + + ChangeDiff rows are always written one at a time by branching's own receiver, never in + bulk, so creating them here exercises the same post_save the plugin listens on. + """ + return ChangeDiff.objects.create( + branch=branch, + action=kwargs.pop('action', 'update'), + object_type=ContentType.objects.get_for_model(model), + object_id=object_id, + object_repr=repr_, + original=kwargs.pop('original', {'status': 'active'}), + modified=kwargs.pop('modified', {'status': 'active'}), + current=kwargs.pop('current', {'status': 'active'}), + ) + + +class ScopeFollowsTheBranchTest(TestCase): + """ + The light policy covers prefixes and asks for one engineer. The heavy policy covers + devices and asks for a lead as well. A branch that grows a device must pick up the heavy + one, whenever that growth happens. + """ + + @classmethod + def setUpTestData(cls): + from dcim.models import Device + from ipam.models import Prefix + + cls.engineers = Group.objects.create(name='Engineers') + cls.leads = Group.objects.create(name='Leads') + cls.engineer = User.objects.create(username='engineer') + cls.engineer.groups.add(cls.engineers) + cls.lead = User.objects.create(username='lead') + cls.lead.groups.add(cls.leads) + cls.author = User.objects.create(username='author') + + cls.light = Policy.objects.create(name='Prefix changes') + cls.light.object_types.set([ObjectType.objects.get_for_model(Prefix)]) + PolicyRule.objects.create(policy=cls.light, name='One engineer', min_reviews=1).groups.set([cls.engineers]) + + cls.heavy = Policy.objects.create(name='Device changes') + cls.heavy.object_types.set([ObjectType.objects.get_for_model(Device)]) + PolicyRule.objects.create(policy=cls.heavy, name='One lead', min_reviews=1).groups.set([cls.leads]) + + cls.Device = Device + cls.Prefix = Prefix + + def setUp(self): + self.branch = make_branch('scope', self._testMethodName) + touch(self.branch, self.Prefix, 1, '10.0.0.0/24') + self.cr = ChangeRequest.objects.create(branch=self.branch, title='T', requester=self.author) + self.cr.submit() + self.cr.refresh_from_db() + + def attached(self): + return sorted(p.name for p in self.cr.policies.all()) + + def test_the_fixture_starts_with_the_light_policy_only(self): + self.assertEqual(self.attached(), ['Prefix changes']) + + def test_a_branch_that_grows_a_new_object_type_picks_up_its_policy(self): + """ + The bug this file exists for. The device policy must attach on the edit, not on the + next sync and not never. + """ + touch(self.branch, self.Device, 1, 'core-switch') + + self.cr.refresh_from_db() + self.assertEqual(self.attached(), ['Device changes', 'Prefix changes']) + + def test_an_approval_given_before_the_growth_does_not_satisfy_the_new_policy(self): + """ + The whole point. One engineer was enough for a prefix change; it must not be enough + once the branch also rewrites a device. + """ + approve(self.cr, self.engineer) + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.APPROVED) + + touch(self.branch, self.Device, 1, 'core-switch') + + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.NEEDS_REVIEW) + self.assertFalse(self.cr.evaluate().satisfied) + + def test_re_approving_after_the_growth_still_needs_the_lead(self): + """ + The exploit end to end: the engineer approves twice and must still not get through. + """ + approve(self.cr, self.engineer) + touch(self.branch, self.Device, 1, 'core-switch') + + review = self.cr.reviews.get(reviewer=self.engineer) + review.save(refresh_snapshot=True) + self.cr.refresh_from_db() + + self.assertNotEqual(self.cr.status, ChangeRequestStatusChoices.APPROVED) + self.assertFalse(require_approved_change_request(self.branch).permitted) + + approve(self.cr, self.lead) + self.cr.refresh_from_db() + self.assertEqual(self.cr.status, ChangeRequestStatusChoices.APPROVED) + + def test_a_second_object_of_a_known_type_changes_nothing(self): + """ + The guard that keeps a bulk edit cheap must not also skip a real change of scope. + """ + touch(self.branch, self.Prefix, 2, '10.0.1.0/24') + + self.cr.refresh_from_db() + self.assertEqual(self.attached(), ['Prefix changes']) + + def test_a_policy_whose_object_type_leaves_the_branch_is_detached(self): + """ + Drift runs both ways. A policy asking for approvals the branch no longer needs is a + different failure, but it is the same missing re-match. + """ + diff = touch(self.branch, self.Device, 1, 'core-switch') + self.cr.refresh_from_db() + self.assertIn('Device changes', self.attached()) + + diff.delete() + sync_policies(self.cr) + + self.cr.refresh_from_db() + self.assertEqual(self.attached(), ['Prefix changes']) + + def test_a_disabled_policy_is_not_pulled_in(self): + self.heavy.enabled = False + self.heavy.save() + + touch(self.branch, self.Device, 1, 'core-switch') + + self.cr.refresh_from_db() + self.assertEqual(self.attached(), ['Prefix changes']) + + def test_a_completed_request_is_left_alone(self): + """ + A merged request is a record. Re-matching it would rewrite history. + """ + self.cr.status = ChangeRequestStatusChoices.COMPLETED + self.cr.save(update_fields=['status']) + + touch(self.branch, self.Device, 1, 'core-switch') + + self.cr.refresh_from_db() + self.assertEqual(self.attached(), ['Prefix changes']) + + +class MergeGateRematchesTest(TestCase): + """ + The gate must not depend on the receiver having fired. + + It is the only moment the answer decides anything, so it re-matches for itself. These + tests remove the binding behind its back, which is what any path the receiver misses + would look like. + """ + + @classmethod + def setUpTestData(cls): + from dcim.models import Device + from ipam.models import Prefix + + cls.engineers = Group.objects.create(name='Engineers') + cls.engineer = User.objects.create(username='engineer') + cls.engineer.groups.add(cls.engineers) + cls.author = User.objects.create(username='author') + + cls.light = Policy.objects.create(name='Prefix changes') + cls.light.object_types.set([ObjectType.objects.get_for_model(Prefix)]) + PolicyRule.objects.create(policy=cls.light, name='One engineer', min_reviews=1).groups.set([cls.engineers]) + + cls.heavy = Policy.objects.create(name='Device changes') + cls.heavy.object_types.set([ObjectType.objects.get_for_model(Device)]) + PolicyRule.objects.create(policy=cls.heavy, name='One lead', min_reviews=1).groups.set( + [Group.objects.create(name='Leads')] + ) + + cls.Device = Device + cls.Prefix = Prefix + + def setUp(self): + self.branch = make_branch('gate', self._testMethodName) + touch(self.branch, self.Prefix, 1, '10.0.0.0/24') + self.cr = ChangeRequest.objects.create(branch=self.branch, title='T', requester=self.author) + self.cr.submit() + approve(self.cr, self.engineer) + self.cr.refresh_from_db() + self.cr.checks.update(status='success') + + def test_an_approved_request_matching_its_policies_may_merge(self): + self.assertTrue(require_approved_change_request(self.branch).permitted) + + def test_the_gate_refuses_when_a_matching_policy_is_not_attached(self): + touch(self.branch, self.Device, 1, 'core-switch') + + # Behind the receiver's back, as any path it does not cover would leave things. + ChangeRequestPolicy.objects.filter(change_request=self.cr, policy=self.heavy).delete() + ChangeRequest.objects.filter(pk=self.cr.pk).update(status=ChangeRequestStatusChoices.APPROVED) + + indicator = require_approved_change_request(self.branch) + self.assertFalse(indicator.permitted) + + def test_the_gate_reattaches_the_policy_it_was_missing(self): + touch(self.branch, self.Device, 1, 'core-switch') + ChangeRequestPolicy.objects.filter(change_request=self.cr, policy=self.heavy).delete() + ChangeRequest.objects.filter(pk=self.cr.pk).update(status=ChangeRequestStatusChoices.APPROVED) + + require_approved_change_request(self.branch) + + self.assertIn('Device changes', {p.name for p in self.cr.policies.all()}) diff --git a/netbox_change_control/tests/test_search.py b/netbox_change_control/tests/test_search.py new file mode 100644 index 0000000..617f13d --- /dev/null +++ b/netbox_change_control/tests/test_search.py @@ -0,0 +1,147 @@ +""" +Global search. + +NetBox auto-imports a plugin's `search` module, and a model with no index registered there +never appears in the search box however well its own list page filters. Every model in this +plugin was in that state. + +It mattered most for `ChangeRequest.ref`: the field exists so a change can be found by the +ticket that spawned it, and the search box is the one place somebody types a ticket number. +""" + +from django.test import TestCase +from netbox.registry import registry +from netbox.search.backends import search_backend +from users.models import Group, User + +from netbox_change_control.choices import MergeCheckStatusChoices, ReviewDecisionChoices +from netbox_change_control.models import ( + ChangeComment, + ChangeRequest, + MergeCheck, + Policy, + PolicyRule, + Review, +) +from netbox_change_control.tests.base import make_branch + +INDEXED_MODELS = (ChangeComment, ChangeRequest, MergeCheck, Policy, PolicyRule, Review) + + +class EveryModelIsIndexedTest(TestCase): + def test_each_model_has_a_search_index(self): + """ + A model added later must be indexed too, or it silently drops out of search. + """ + missing = [ + model._meta.model_name + for model in INDEXED_MODELS + if f'netbox_change_control.{model._meta.model_name}' not in registry['search'] + ] + self.assertEqual(missing, [], 'these models have no search index registered') + + def test_the_indexed_fields_all_exist(self): + """ + A field name that does not resolve is cached as an attribute instead, silently, so a + typo produces an index that finds nothing. + """ + for model in INDEXED_MODELS: + indexer = registry['search'][f'netbox_change_control.{model._meta.model_name}'] + for name, _weight in indexer.fields: + with self.subTest(model=model._meta.model_name, field=name): + model._meta.get_field(name) + for name in indexer.display_attrs: + with self.subTest(model=model._meta.model_name, attr=name): + self.assertTrue( + hasattr(model, name) or model._meta.get_field(name), + f'{model.__name__}.{name} does not exist', + ) + + +class SearchFindsThingsTest(TestCase): + @classmethod + def setUpTestData(cls): + cls.requester = User.objects.create(username='requester') + cls.reviewer = User.objects.create(username='reviewer') + cls.group = Group.objects.create(name='Engineers') + + def setUp(self): + self.branch = make_branch('searchable', self._testMethodName) + self.cr = ChangeRequest.objects.create( + branch=self.branch, + ref='CHG0012345', + title='Upgrade the access switch', + description='replace a failing line card', + requester=self.requester, + ) + + def found(self, term): + return {(r.object._meta.model_name, r.object.pk) for r in search_backend.search(term)} + + def test_a_change_request_is_found_by_its_reference(self): + """ + The reason this file exists. A pipeline or a colleague has a ticket number and nothing + else. + """ + self.assertIn(('changerequest', self.cr.pk), self.found('CHG0012345')) + + def test_a_change_request_is_found_by_its_title(self): + self.assertIn(('changerequest', self.cr.pk), self.found('access switch')) + + def test_a_change_request_is_found_by_its_description(self): + self.assertIn(('changerequest', self.cr.pk), self.found('line card')) + + def test_a_change_request_is_found_by_its_branch_name_after_the_branch_is_gone(self): + """ + The stored branch name is indexed rather than the branch itself, because a request + outliving its branch is exactly when search is the only way left to find it. + """ + name = self.branch.name + self.branch.delete() + self.cr.refresh_from_db() + self.cr.save() + + self.assertTrue(self.cr.branch_deleted) + self.assertIn(('changerequest', self.cr.pk), self.found(name)) + + def test_a_policy_is_found_by_name(self): + policy = Policy.objects.create(name='Circuit changes', description='customer facing') + self.assertIn(('policy', policy.pk), self.found('Circuit changes')) + + def test_a_policy_rule_is_found_by_name(self): + policy = Policy.objects.create(name='Device changes') + rule = PolicyRule.objects.create(policy=policy, name='Two engineers', min_reviews=2) + self.assertIn(('policyrule', rule.pk), self.found('Two engineers')) + + def test_a_review_is_found_by_its_comment(self): + review = Review.objects.create( + change_request=self.cr, + reviewer=self.reviewer, + decision=ReviewDecisionChoices.REJECT, + comment='the loopback address is wrong', + ) + self.assertIn(('review', review.pk), self.found('loopback address')) + + def test_a_check_is_found_by_its_summary(self): + check = MergeCheck.objects.create( + change_request=self.cr, + name='ci-pipeline', + label='CI pipeline', + status=MergeCheckStatusChoices.FAILURE, + summary='config render failed on core-sw-1', + ) + self.assertIn(('mergecheck', check.pk), self.found('config render failed')) + + def test_a_comment_is_found_by_its_text_and_by_what_it_was_about(self): + comment = ChangeComment.objects.create( + change_request=self.cr, + author=self.reviewer, + text='are we sure about this rack?', + change_label='dmi01-akron-rtr01', + ) + self.assertIn(('changecomment', comment.pk), self.found('sure about this rack')) + self.assertIn(('changecomment', comment.pk), self.found('dmi01-akron-rtr01')) + + def test_an_unrelated_term_finds_nothing_of_ours(self): + ours = {name for name, _pk in self.found('zzz-nothing-matches-this')} + self.assertEqual(ours & {m._meta.model_name for m in INDEXED_MODELS}, set()) diff --git a/netbox_change_control/tests/test_templates.py b/netbox_change_control/tests/test_templates.py new file mode 100644 index 0000000..4ab238e --- /dev/null +++ b/netbox_change_control/tests/test_templates.py @@ -0,0 +1,190 @@ +""" +Rendering details that are wrong in ways nobody reports. + +A badge with an invalid colour class, a select that quietly resets, a timestamp in a different +format from the one next to it. None of these throws, so none of them shows up in a test that +only asserts a 200. +""" + +import re +from pathlib import Path + +from django.contrib.contenttypes.models import ContentType +from django.test import TestCase +from netbox_branching.models import ChangeDiff +from users.models import Group, ObjectPermission, User + +from netbox_change_control.choices import ReviewDecisionChoices +from netbox_change_control.models import ChangeRequest, Review +from netbox_change_control.tests.base import ChangeControlTestCase, make_policy + +TEMPLATE_DIR = Path(__file__).resolve().parent.parent / 'templates' + +# Tabler ships these, and NetBox's compiled stylesheet carries no others. A colour outside the +# set renders a badge with no background at all, which is why `text-bg-grey` went unnoticed. +TABLER_BADGE_COLOURS = { + 'azure', + 'black', + 'blue', + 'cyan', + 'dark', + 'danger', + 'gray', + 'green', + 'indigo', + 'info', + 'light', + 'lime', + 'muted', + 'orange', + 'pink', + 'primary', + 'purple', + 'red', + 'secondary', + 'success', + 'teal', + 'warning', + 'white', + 'yellow', +} + + +class BadgeColourTest(TestCase): + def test_every_badge_colour_is_one_tabler_ships(self): + offenders = [] + for template in TEMPLATE_DIR.rglob('*.html'): + for colour in re.findall(r'text-bg-([a-z]+)', template.read_text()): + if colour not in TABLER_BADGE_COLOURS: + offenders.append(f'{template.relative_to(TEMPLATE_DIR)}: text-bg-{colour}') + + self.assertEqual(sorted(set(offenders)), [], 'badge colours NetBox does not define') + + +def grant(user, actions, *models, label='perm'): + """ + The pages under test are permission-gated, and these tests are about what they render + rather than who may see them. + """ + permission = ObjectPermission.objects.create(name=f'{label}-{user.pk}', actions=list(actions)) + permission.object_types.set(ContentType.objects.get_for_model(m) for m in models) + permission.users.add(user) + return permission + + +def grant_reviewer(user): + """ + View the request, and be able to review it: the form is replaced by an explanation for a + user who cannot, so without `add_review` there is no select to inspect at all. + """ + grant(user, ['view'], ChangeRequest, label='view') + grant(user, ['view', 'add'], Review, label='review') + + +class ReviewFormTest(ChangeControlTestCase): + """ + The form has to open on the reviewer's standing decision. + + It prefilled the comment but reset the decision to Approve, so a reviewer coming back to + amend a "Request changes" was silently offered an approval instead. + """ + + branch_prefix = 'reviewform' + policy_checks = () + + def setUp(self): + super().setUp() + grant_reviewer(self.reviewer) + self.client.force_login(self.reviewer) + + def selected_option(self, html): + match = re.search(r'