From 82789e32800abd99d846f2e57851d459b743714b Mon Sep 17 00:00:00 2001 From: "Asaph M. Kotzin" Date: Thu, 28 May 2026 13:09:35 +0100 Subject: [PATCH 01/11] docs(superpowers): add PR 3 implementation plan for Azure DevOps integration Third plan in the stacked-PRs rollout. Covers the Azure DevOps REST client (limited to list_projects for v1), URL parsing for PR and work-item URLs (cloud + on-prem), organisation_url trailing-slash normalisation, serializer-side placeholder-PAT preservation on update, and viewset PAT validation against ADO on create/update. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../2026-05-28-azure-devops-03-client.md | 1760 +++++++++++++++++ 1 file changed, 1760 insertions(+) create mode 100644 docs/superpowers/plans/2026-05-28-azure-devops-03-client.md diff --git a/docs/superpowers/plans/2026-05-28-azure-devops-03-client.md b/docs/superpowers/plans/2026-05-28-azure-devops-03-client.md new file mode 100644 index 000000000000..b212465c73ba --- /dev/null +++ b/docs/superpowers/plans/2026-05-28-azure-devops-03-client.md @@ -0,0 +1,1760 @@ +# Azure DevOps Integration — PR 3: REST client, URL parsing, PAT validation + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Stand up the Azure DevOps REST client (limited to `list_projects` for v1), URL parsing for PR and work-item URLs, `organisation_url` normalisation on the model, and wire PAT validation into the configuration viewset so create/update calls verify credentials against ADO before persisting. + +**Architecture:** Mirror the GitLab client's layout (`client/api.py`, `client/types.py`, plus a new `client/exceptions.py` for typed exceptions ADO needs). HTTP auth is Basic with empty username and PAT as password — the ADO convention. URL parsing returns `NamedTuple` refs for both PR and work-item URLs across cloud (`dev.azure.com/{org}`) and on-prem (`{host}/{collection}`) shapes. PAT validation lives in the viewset; the `responses` library is used to mock the upstream ADO probe in tests. + +**Tech Stack:** Python 3.12, Django 5.x, DRF, `requests`, `responses` (test-only), pytest with `pytest-django`, mypy strict. + +**Spec reference:** `docs/superpowers/specs/2026-05-28-azure-devops-integration-design.md` — sections "Components → `client/api.py`", "Components → `client/types.py`", "Components → `services/url_parsing.py`", "Components → `views/configuration.py`", "Error handling → Bad PAT at save time", "Data model → AzureDevOpsConfiguration → organisation_url normalisation". + +**Plan reference (parent):** `docs/superpowers/plans/2026-05-28-azure-devops-02-models.md`. + +**Stack position:** PR 3 of N. Branches off `feat/azure-devops-02-models`. Branch name: `feat/azure-devops-03-client`. Will PR against `feat/azure-devops-02-models` initially; retargeted at `main` after PR 1 and PR 2 land. + +--- + +## Scope deliberately out of PR 3 + +- The rest of the client surface (`list_repositories`, `list_pull_requests`, `list_work_items`, comment-posting, label-add/remove, subscription create/delete) — added incrementally by later PRs that need them. PR 3 only lands what PAT validation actually requires. +- Microsoft Entra OAuth — PAT only. +- Encryption at rest for the PAT — still deferred (matches the PR 2 scope-out). +- ADO API request-duration metric (`flagsmith_azure_devops_api_request_duration_seconds`) — added in PR 12 with the rest of `metrics.py`. + +--- + +## File Structure + +- **Create:** `api/integrations/azure_devops/constants.py` — `AZURE_DEVOPS_CLIENT_TIMEOUT_SECONDS`, the `_apis/projects?$top=1` probe path. +- **Create:** `api/integrations/azure_devops/client/__init__.py` — re-exports the client's public surface (`list_projects`, the exceptions, the typed dicts). +- **Create:** `api/integrations/azure_devops/client/types.py` — `AdoProject`, `AdoProjectsPage`. +- **Create:** `api/integrations/azure_devops/client/exceptions.py` — `AzureDevOpsError`, `AzureDevOpsAuthError`, `AzureDevOpsNotFoundError`. +- **Create:** `api/integrations/azure_devops/client/api.py` — `list_projects` + HTTP plumbing helper. +- **Create:** `api/integrations/azure_devops/services/__init__.py` — empty package marker. +- **Create:** `api/integrations/azure_devops/services/url_parsing.py` — `parse_pull_request_url`, `parse_work_item_url`, the `AdoPullRequestRef` / `AdoWorkItemRef` NamedTuples. +- **Modify:** `api/integrations/azure_devops/models.py` — add `save()` override on `AzureDevOpsConfiguration` that strips trailing slash from `organisation_url`. +- **Modify:** `api/integrations/azure_devops/serializers.py` — override `update()` so a `personal_access_token` equal to `WRITE_ONLY_PLACEHOLDER` on input is dropped from validated data (preserves existing PAT). +- **Modify:** `api/integrations/azure_devops/views/configuration.py` — `perform_create` and `perform_update` call a new `_validate_pat_against_ado(organisation_url, pat)` helper; on `AzureDevOpsAuthError` raise a DRF `ValidationError("Azure DevOps rejected the credentials.")`. +- **Create:** `api/tests/unit/integrations/azure_devops/test_exceptions.py` — exception hierarchy tests. +- **Create:** `api/tests/unit/integrations/azure_devops/test_client.py` — client behaviour tests (auth header, status mapping, timeout). +- **Create:** `api/tests/unit/integrations/azure_devops/test_url_parsing.py` — parametrised URL parsing. +- **Modify:** `api/tests/unit/integrations/azure_devops/test_models.py` — add `save()` normalisation tests. +- **Modify:** `api/tests/unit/integrations/azure_devops/test_serializers.py` — add placeholder-preservation tests. +- **Modify:** `api/tests/unit/integrations/azure_devops/test_configuration.py` — add `@responses.activate` and ADO mocks to the three tests that exercise `perform_create` / `perform_update`; add new tests for invalid-PAT and placeholder-on-update. + +No other files are touched in this PR. + +--- + +## Pre-flight + +- [ ] **Step 0: Confirm working branch** + +```bash +cd /Users/asaphkotzin/Dev/flagsmith +git status +git log --oneline -3 +``` + +Expected: branch `feat/azure-devops-03-client`, HEAD is the parent branch's tip (the PR 2 final commit). Working tree clean. If the branch does not exist, create it off `feat/azure-devops-02-models`: + +```bash +git checkout feat/azure-devops-02-models +git checkout -b feat/azure-devops-03-client +``` + +--- + +## Task 1: Constants and client scaffolding + +**Files:** +- Create: `api/integrations/azure_devops/constants.py` +- Create: `api/integrations/azure_devops/client/__init__.py` +- Create: `api/integrations/azure_devops/services/__init__.py` +- Test: a smoke test that the constants import + +- [ ] **Step 1: Create the constants module** + +Create `api/integrations/azure_devops/constants.py` with the following exact contents: + +```python +AZURE_DEVOPS_CLIENT_TIMEOUT_SECONDS = 10 + +AZURE_DEVOPS_API_VERSION = "7.1" +``` + +(The API version is locked to 7.1, the current stable ADO REST API version. Mirroring GitLab's `GITLAB_CLIENT_TIMEOUT_SECONDS` constant naming.) + +- [ ] **Step 2: Create the empty package markers** + +```bash +mkdir -p api/integrations/azure_devops/client api/integrations/azure_devops/services +``` + +Create `api/integrations/azure_devops/client/__init__.py` with contents: + +```python +``` + +(empty) + +Create `api/integrations/azure_devops/services/__init__.py` with contents: + +```python +``` + +(empty) + +- [ ] **Step 3: Write the smoke test** + +Create `api/tests/unit/integrations/azure_devops/test_constants.py` with: + +```python +from integrations.azure_devops.constants import ( + AZURE_DEVOPS_API_VERSION, + AZURE_DEVOPS_CLIENT_TIMEOUT_SECONDS, +) + + +def test_constants__timeout__has_sensible_default() -> None: + # Given + timeout = AZURE_DEVOPS_CLIENT_TIMEOUT_SECONDS + + # When + is_positive_int = isinstance(timeout, int) and timeout > 0 + + # Then + assert is_positive_int + assert timeout <= 60 + + +def test_constants__api_version__is_string() -> None: + # Given + version = AZURE_DEVOPS_API_VERSION + + # When + is_string = isinstance(version, str) + + # Then + assert is_string + assert version +``` + +- [ ] **Step 4: Run the tests** + +From `api/`: + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_constants.py -v' +``` + +Expected: 2 passed. + +- [ ] **Step 5: Run mypy** + +```bash +make typecheck +``` + +Expected: clean. + +- [ ] **Step 6: Commit** + +```bash +git add api/integrations/azure_devops/constants.py api/integrations/azure_devops/client/__init__.py api/integrations/azure_devops/services/__init__.py api/tests/unit/integrations/azure_devops/test_constants.py +git commit -m "$(cat <<'EOF' +feat(integrations): scaffold Azure DevOps client + services packages + +Add empty client/ and services/ packages plus a constants module holding +the request timeout and pinned REST API version (7.1). Subsequent +commits in this PR fill in the client modules and the URL-parsing +service. + +Co-Authored-By: Claude Opus 4.7 (1M context) +EOF +)" +``` + +--- + +## Task 2: Typed exceptions + +**Files:** +- Create: `api/integrations/azure_devops/client/exceptions.py` +- Create: `api/tests/unit/integrations/azure_devops/test_exceptions.py` + +- [ ] **Step 1: Write the failing tests** + +Create `api/tests/unit/integrations/azure_devops/test_exceptions.py` with: + +```python +import pytest + +from integrations.azure_devops.client.exceptions import ( + AzureDevOpsAuthError, + AzureDevOpsError, + AzureDevOpsNotFoundError, +) + + +def test_azure_devops_auth_error__inheritance__is_subclass_of_base() -> None: + # Given + cls = AzureDevOpsAuthError + + # When + is_subclass = issubclass(cls, AzureDevOpsError) + + # Then + assert is_subclass + + +def test_azure_devops_not_found_error__inheritance__is_subclass_of_base() -> None: + # Given + cls = AzureDevOpsNotFoundError + + # When + is_subclass = issubclass(cls, AzureDevOpsError) + + # Then + assert is_subclass + + +def test_azure_devops_error__base__is_exception_subclass() -> None: + # Given + cls = AzureDevOpsError + + # When + is_exception = issubclass(cls, Exception) + + # Then + assert is_exception + + +def test_azure_devops_auth_error__raise_and_catch__as_base_works() -> None: + # Given + def raise_auth_error() -> None: + raise AzureDevOpsAuthError("invalid PAT") + + # When + with pytest.raises(AzureDevOpsError) as exc_info: + raise_auth_error() + + # Then + assert "invalid PAT" in str(exc_info.value) +``` + +- [ ] **Step 2: Run the test to verify it fails** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_exceptions.py -v' +``` + +Expected: import error — module not found. + +- [ ] **Step 3: Create the exceptions module** + +Create `api/integrations/azure_devops/client/exceptions.py` with the following exact contents: + +```python +class AzureDevOpsError(Exception): + """Base class for all Azure DevOps client errors raised by this package.""" + + +class AzureDevOpsAuthError(AzureDevOpsError): + """Raised when ADO rejects credentials with 401 or 403.""" + + +class AzureDevOpsNotFoundError(AzureDevOpsError): + """Raised when ADO returns 404 for a single-resource lookup.""" +``` + +- [ ] **Step 4: Run the tests** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_exceptions.py -v' +``` + +Expected: 4 passed. + +- [ ] **Step 5: Run mypy** + +```bash +make typecheck +``` + +Expected: clean. + +- [ ] **Step 6: Commit** + +```bash +git add api/integrations/azure_devops/client/exceptions.py api/tests/unit/integrations/azure_devops/test_exceptions.py +git commit -m "$(cat <<'EOF' +feat(integrations): add Azure DevOps client typed exceptions + +Three classes: AzureDevOpsError (base), AzureDevOpsAuthError (401/403), +AzureDevOpsNotFoundError (404 on single-resource). Callers can catch +the base for "anything went wrong with ADO" or the specific subclass +for targeted handling. + +Co-Authored-By: Claude Opus 4.7 (1M context) +EOF +)" +``` + +--- + +## Task 3: Client types + +**Files:** +- Create: `api/integrations/azure_devops/client/types.py` +- Test: extends `api/tests/unit/integrations/azure_devops/test_client.py` indirectly when the client is added in Task 4 + +For PR 3 we only need `AdoProject` and a `AdoProjectsPage` page-shape for `list_projects`. Later PRs will extend this module with `AdoRepository`, `AdoPullRequest`, `AdoWorkItem`, `AdoSubscription`, and the resource-metadata typed dict. + +- [ ] **Step 1: Create the types module** + +Create `api/integrations/azure_devops/client/types.py` with the following exact contents: + +```python +from typing import TypedDict + + +class AdoProject(TypedDict): + id: str + name: str + url: str + + +class AdoProjectsPage(TypedDict): + results: list[AdoProject] + continuation_token: str | None +``` + +(The `id` field is `str` because ADO project GUIDs come over the wire as strings. We can cast to `uuid.UUID` at consumption time where typed; the client stays representation-faithful.) + +- [ ] **Step 2: Write a smoke test** + +Append to `api/tests/unit/integrations/azure_devops/test_client.py` (creating the file at this point even though Task 4 will append more): + +```python +from integrations.azure_devops.client.types import AdoProject, AdoProjectsPage + + +def test_ado_project__shape__has_required_keys() -> None: + # Given + project: AdoProject = { + "id": "00000000-0000-0000-0000-000000000000", + "name": "Test", + "url": "https://dev.azure.com/test-org/_apis/projects/...", + } + + # When + keys = set(project.keys()) + + # Then + assert keys == {"id", "name", "url"} + + +def test_ado_projects_page__shape__has_required_keys() -> None: + # Given + page: AdoProjectsPage = {"results": [], "continuation_token": None} + + # When + keys = set(page.keys()) + + # Then + assert keys == {"results", "continuation_token"} +``` + +- [ ] **Step 3: Run the test** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_client.py -v' +``` + +Expected: 2 passed. + +- [ ] **Step 4: Run mypy** + +```bash +make typecheck +``` + +Expected: clean. + +- [ ] **Step 5: Commit** + +```bash +git add api/integrations/azure_devops/client/types.py api/tests/unit/integrations/azure_devops/test_client.py +git commit -m "$(cat <<'EOF' +feat(integrations): add Azure DevOps client TypedDicts + +Two typed dicts for v1: AdoProject (id/name/url) and AdoProjectsPage +(results + continuation_token). Subsequent PRs will extend this module +with AdoRepository, AdoPullRequest, AdoWorkItem, AdoSubscription, and +the resource-metadata shape as their corresponding client functions +land. + +Co-Authored-By: Claude Opus 4.7 (1M context) +EOF +)" +``` + +--- + +## Task 4: Client `list_projects` + HTTP plumbing + +**Files:** +- Modify: `api/integrations/azure_devops/client/__init__.py` (re-export the function) +- Create: `api/integrations/azure_devops/client/api.py` +- Modify: `api/tests/unit/integrations/azure_devops/test_client.py` (append client behaviour tests) + +- [ ] **Step 1: Write the failing tests** + +Append to `api/tests/unit/integrations/azure_devops/test_client.py`: + +```python +import base64 + +import pytest +import requests +import responses + +from integrations.azure_devops.client import list_projects +from integrations.azure_devops.client.exceptions import ( + AzureDevOpsAuthError, + AzureDevOpsNotFoundError, +) + +ORG_URL = "https://dev.azure.com/test-org" +PAT = "ado-test-pat" + + +def _expected_basic_auth_header() -> str: + # ADO PAT auth: HTTP Basic with empty username and PAT as password. + encoded = base64.b64encode(f":{PAT}".encode()).decode() + return f"Basic {encoded}" + + +@responses.activate +def test_list_projects__valid_response__returns_typed_page() -> None: + # Given + responses.get( + f"{ORG_URL}/_apis/projects", + json={ + "value": [ + { + "id": "00000000-0000-0000-0000-000000000001", + "name": "Alpha", + "url": f"{ORG_URL}/_apis/projects/00000000-0000-0000-0000-000000000001", + "extra": "ignored", + }, + ], + "count": 1, + }, + match=[ + responses.matchers.header_matcher( + {"Authorization": _expected_basic_auth_header()} + ) + ], + ) + + # When + page = list_projects(organisation_url=ORG_URL, pat=PAT, top=1) + + # Then + assert page["results"] == [ + { + "id": "00000000-0000-0000-0000-000000000001", + "name": "Alpha", + "url": f"{ORG_URL}/_apis/projects/00000000-0000-0000-0000-000000000001", + }, + ] + assert page["continuation_token"] is None + + +@responses.activate +def test_list_projects__continuation_token_in_header__exposed_on_page() -> None: + # Given + responses.get( + f"{ORG_URL}/_apis/projects", + json={"value": [], "count": 0}, + headers={"x-ms-continuationtoken": "ct-abc"}, + ) + + # When + page = list_projects(organisation_url=ORG_URL, pat=PAT) + + # Then + assert page["continuation_token"] == "ct-abc" + + +@responses.activate +def test_list_projects__401_response__raises_auth_error() -> None: + # Given + responses.get(f"{ORG_URL}/_apis/projects", json={}, status=401) + + # When + def call_list() -> None: + list_projects(organisation_url=ORG_URL, pat=PAT) + + # Then + with pytest.raises(AzureDevOpsAuthError): + call_list() + + +@responses.activate +def test_list_projects__403_response__raises_auth_error() -> None: + # Given + responses.get(f"{ORG_URL}/_apis/projects", json={}, status=403) + + # When + def call_list() -> None: + list_projects(organisation_url=ORG_URL, pat=PAT) + + # Then + with pytest.raises(AzureDevOpsAuthError): + call_list() + + +@responses.activate +def test_list_projects__404_response__raises_not_found_error() -> None: + # Given + responses.get(f"{ORG_URL}/_apis/projects", json={}, status=404) + + # When + def call_list() -> None: + list_projects(organisation_url=ORG_URL, pat=PAT) + + # Then + with pytest.raises(AzureDevOpsNotFoundError): + call_list() + + +@responses.activate +def test_list_projects__500_response__raises_requests_http_error() -> None: + # Given + responses.get(f"{ORG_URL}/_apis/projects", json={}, status=500) + + # When + def call_list() -> None: + list_projects(organisation_url=ORG_URL, pat=PAT) + + # Then — non-4xx server failures bubble up as the underlying requests error + with pytest.raises(requests.HTTPError): + call_list() + + +@responses.activate +def test_list_projects__trailing_slash_in_org_url__normalised_in_request() -> None: + # Given + responses.get( + f"{ORG_URL}/_apis/projects", + json={"value": [], "count": 0}, + ) + + # When + list_projects(organisation_url=f"{ORG_URL}/", pat=PAT) + + # Then — request lands on ORG_URL/_apis/projects (no double slash) + assert responses.calls[0].request.url is not None + assert "//_apis" not in responses.calls[0].request.url +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_client.py -v' +``` + +Expected: collection-level failures — `list_projects` not importable from `integrations.azure_devops.client`. + +- [ ] **Step 3: Create the client module** + +Create `api/integrations/azure_devops/client/api.py` with the following exact contents: + +```python +from typing import Any + +import requests + +from integrations.azure_devops.client.exceptions import ( + AzureDevOpsAuthError, + AzureDevOpsNotFoundError, +) +from integrations.azure_devops.client.types import AdoProject, AdoProjectsPage +from integrations.azure_devops.constants import ( + AZURE_DEVOPS_API_VERSION, + AZURE_DEVOPS_CLIENT_TIMEOUT_SECONDS, +) + + +def _ado_request( + method: str, + organisation_url: str, + pat: str, + *, + path: str, + params: dict[str, Any] | None = None, + json_body: dict[str, Any] | None = None, +) -> requests.Response: + base = organisation_url.rstrip("/") + query: dict[str, Any] = {"api-version": AZURE_DEVOPS_API_VERSION} + if params: + query.update(params) + response = requests.request( + method, + f"{base}/_apis/{path}", + auth=("", pat), + params=query, + json=json_body, + timeout=AZURE_DEVOPS_CLIENT_TIMEOUT_SECONDS, + ) + if response.status_code in (401, 403): + raise AzureDevOpsAuthError( + f"Azure DevOps rejected credentials ({response.status_code})" + ) + if response.status_code == 404: + raise AzureDevOpsNotFoundError( + f"Azure DevOps reported the resource was not found ({response.status_code})" + ) + response.raise_for_status() + return response + + +def list_projects( + *, + organisation_url: str, + pat: str, + top: int | None = None, + continuation_token: str | None = None, +) -> AdoProjectsPage: + params: dict[str, Any] = {} + if top is not None: + params["$top"] = top + if continuation_token is not None: + params["continuationToken"] = continuation_token + + response = _ado_request( + "GET", + organisation_url, + pat, + path="projects", + params=params, + ) + payload = response.json() + results: list[AdoProject] = [ + AdoProject(id=p["id"], name=p["name"], url=p["url"]) + for p in payload.get("value", []) + ] + next_token = response.headers.get("x-ms-continuationtoken") + return AdoProjectsPage(results=results, continuation_token=next_token) +``` + +(Notes on the implementation: +- `auth=("", pat)` is the ADO PAT convention — HTTP Basic with empty username and PAT as password. +- `api-version` is always passed as a query parameter — required by ADO for every REST call. +- `_ado_request` is the single chokepoint where status mapping happens. 401/403 → `AzureDevOpsAuthError`. 404 → `AzureDevOpsNotFoundError`. 5xx and other 4xx → `requests.HTTPError`. +- `organisation_url.rstrip("/")` is defensive — the model normalises this at save time, but a caller passing an unnormalised URL still gets a correctly-formed request.) + +- [ ] **Step 4: Re-export the public surface from the client package** + +Replace `api/integrations/azure_devops/client/__init__.py` (currently empty) with: + +```python +from integrations.azure_devops.client.api import list_projects +from integrations.azure_devops.client.exceptions import ( + AzureDevOpsAuthError, + AzureDevOpsError, + AzureDevOpsNotFoundError, +) +from integrations.azure_devops.client.types import AdoProject, AdoProjectsPage + +__all__ = [ + "AdoProject", + "AdoProjectsPage", + "AzureDevOpsAuthError", + "AzureDevOpsError", + "AzureDevOpsNotFoundError", + "list_projects", +] +``` + +- [ ] **Step 5: Run the tests** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_client.py -v' +``` + +Expected: 9 passed (2 from Task 3 + 7 new). + +- [ ] **Step 6: Run mypy** + +```bash +make typecheck +``` + +Expected: clean. + +- [ ] **Step 7: Commit** + +```bash +git add api/integrations/azure_devops/client/api.py api/integrations/azure_devops/client/__init__.py api/tests/unit/integrations/azure_devops/test_client.py +git commit -m "$(cat <<'EOF' +feat(integrations): add list_projects to the Azure DevOps client + +Stand up the client's HTTP plumbing (Basic auth with empty username, +api-version=7.1 query param, 10s timeout) and the first concrete +function: list_projects. Status codes 401/403 map to AzureDevOpsAuthError, +404 to AzureDevOpsNotFoundError; 5xx and other 4xx bubble up as +requests.HTTPError. Subsequent client functions (repos, PRs, work items, +comments, labels, subscriptions) land in their respective consumer PRs. + +Co-Authored-By: Claude Opus 4.7 (1M context) +EOF +)" +``` + +--- + +## Task 5: URL parsing + +**Files:** +- Create: `api/integrations/azure_devops/services/url_parsing.py` +- Create: `api/tests/unit/integrations/azure_devops/test_url_parsing.py` + +- [ ] **Step 1: Write the failing tests** + +Create `api/tests/unit/integrations/azure_devops/test_url_parsing.py` with the following exact contents: + +```python +import pytest + +from integrations.azure_devops.services.url_parsing import ( + AdoPullRequestRef, + AdoWorkItemRef, + parse_pull_request_url, + parse_work_item_url, +) + + +@pytest.mark.parametrize( + "url, expected", + [ + # Cloud, plain + ( + "https://dev.azure.com/test-org/proj/_git/repo/pullrequest/42", + AdoPullRequestRef( + organisation_root="https://dev.azure.com/test-org", + project="proj", + repository="repo", + pull_request_id=42, + ), + ), + # Cloud, with trailing slash + ( + "https://dev.azure.com/test-org/proj/_git/repo/pullrequest/42/", + AdoPullRequestRef( + organisation_root="https://dev.azure.com/test-org", + project="proj", + repository="repo", + pull_request_id=42, + ), + ), + # Cloud, with query string + ( + "https://dev.azure.com/test-org/proj/_git/repo/pullrequest/42?_a=overview", + AdoPullRequestRef( + organisation_root="https://dev.azure.com/test-org", + project="proj", + repository="repo", + pull_request_id=42, + ), + ), + # Cloud, project name with spaces (URL-encoded) + ( + "https://dev.azure.com/test-org/My%20Project/_git/my-repo/pullrequest/7", + AdoPullRequestRef( + organisation_root="https://dev.azure.com/test-org", + project="My Project", + repository="my-repo", + pull_request_id=7, + ), + ), + # On-prem with a collection segment + ( + "https://devops.internal.example.com/DefaultCollection/proj/_git/repo/pullrequest/100", + AdoPullRequestRef( + organisation_root="https://devops.internal.example.com/DefaultCollection", + project="proj", + repository="repo", + pull_request_id=100, + ), + ), + ], +) +def test_parse_pull_request_url__valid_shapes__returns_ref( + url: str, expected: AdoPullRequestRef +) -> None: + # Given / When + result = parse_pull_request_url(url) + + # Then + assert result == expected + + +@pytest.mark.parametrize( + "url", + [ + "", + "not-a-url", + "https://github.com/foo/bar/pull/42", # GitHub + "https://gitlab.com/foo/bar/-/merge_requests/42", # GitLab + "https://dev.azure.com/test-org/proj/_git/repo", # missing /pullrequest/{id} + "https://dev.azure.com/test-org/proj/_workitems/edit/42", # work item, not PR + "https://dev.azure.com/test-org/proj/_git/repo/pullrequest/not-a-number", + ], +) +def test_parse_pull_request_url__malformed_or_other_shapes__returns_none(url: str) -> None: + # Given / When + result = parse_pull_request_url(url) + + # Then + assert result is None + + +@pytest.mark.parametrize( + "url, expected", + [ + # Cloud, plain + ( + "https://dev.azure.com/test-org/proj/_workitems/edit/100", + AdoWorkItemRef( + organisation_root="https://dev.azure.com/test-org", + project="proj", + work_item_id=100, + ), + ), + # Cloud, trailing slash + ( + "https://dev.azure.com/test-org/proj/_workitems/edit/100/", + AdoWorkItemRef( + organisation_root="https://dev.azure.com/test-org", + project="proj", + work_item_id=100, + ), + ), + # Cloud, URL-encoded project name + ( + "https://dev.azure.com/test-org/My%20Project/_workitems/edit/7", + AdoWorkItemRef( + organisation_root="https://dev.azure.com/test-org", + project="My Project", + work_item_id=7, + ), + ), + # On-prem with collection + ( + "https://devops.internal.example.com/DefaultCollection/proj/_workitems/edit/100", + AdoWorkItemRef( + organisation_root="https://devops.internal.example.com/DefaultCollection", + project="proj", + work_item_id=100, + ), + ), + ], +) +def test_parse_work_item_url__valid_shapes__returns_ref( + url: str, expected: AdoWorkItemRef +) -> None: + # Given / When + result = parse_work_item_url(url) + + # Then + assert result == expected + + +@pytest.mark.parametrize( + "url", + [ + "", + "not-a-url", + "https://github.com/foo/bar/issues/42", + "https://dev.azure.com/test-org/proj/_git/repo/pullrequest/42", # PR, not work item + "https://dev.azure.com/test-org/proj/_workitems/edit/not-a-number", + ], +) +def test_parse_work_item_url__malformed_or_other_shapes__returns_none(url: str) -> None: + # Given / When + result = parse_work_item_url(url) + + # Then + assert result is None +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_url_parsing.py -v' +``` + +Expected: import errors — `parse_pull_request_url`, `AdoPullRequestRef`, etc. not defined. + +- [ ] **Step 3: Create the URL parser** + +Create `api/integrations/azure_devops/services/url_parsing.py` with the following exact contents: + +```python +import re +from typing import NamedTuple +from urllib.parse import unquote, urlparse + +# Path captures (after stripping query/fragment, normalising trailing slash): +# /{org_or_collection}/{project}/_git/{repo}/pullrequest/{id} +# /{org_or_collection}/{project}/_workitems/edit/{id} +# +# For ADO cloud, {org_or_collection} is a single org segment (e.g. "test-org"). +# For Azure DevOps Server (on-prem), {org_or_collection} is a collection name, +# and the same path pattern applies under whatever host the server runs on. + +_PR_PATH_PATTERN = re.compile( + r"^/(?P[^/]+)/(?P[^/]+)" + r"/_git/(?P[^/]+)/pullrequest/(?P\d+)/?$" +) + +_WORK_ITEM_PATH_PATTERN = re.compile( + r"^/(?P[^/]+)/(?P[^/]+)" + r"/_workitems/edit/(?P\d+)/?$" +) + + +class AdoPullRequestRef(NamedTuple): + organisation_root: str + project: str + repository: str + pull_request_id: int + + +class AdoWorkItemRef(NamedTuple): + organisation_root: str + project: str + work_item_id: int + + +def parse_pull_request_url(url: str | None) -> AdoPullRequestRef | None: + """Return a structured reference for an Azure DevOps pull-request URL, + or ``None`` if the URL does not match the cloud or on-prem PR shape. + Parsing never raises. + """ + if not url: + return None + parsed = urlparse(url) + if not parsed.scheme or not parsed.netloc: + return None + match = _PR_PATH_PATTERN.match(parsed.path) + if not match: + return None + return AdoPullRequestRef( + organisation_root=f"{parsed.scheme}://{parsed.netloc}/{match.group('org_or_collection')}", + project=unquote(match.group("project")), + repository=unquote(match.group("repo")), + pull_request_id=int(match.group("pr_id")), + ) + + +def parse_work_item_url(url: str | None) -> AdoWorkItemRef | None: + """Return a structured reference for an Azure DevOps work-item URL, + or ``None`` if the URL does not match the cloud or on-prem work-item + shape. Parsing never raises. + """ + if not url: + return None + parsed = urlparse(url) + if not parsed.scheme or not parsed.netloc: + return None + match = _WORK_ITEM_PATH_PATTERN.match(parsed.path) + if not match: + return None + return AdoWorkItemRef( + organisation_root=f"{parsed.scheme}://{parsed.netloc}/{match.group('org_or_collection')}", + project=unquote(match.group("project")), + work_item_id=int(match.group("work_item_id")), + ) +``` + +- [ ] **Step 4: Run the tests** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_url_parsing.py -v' +``` + +Expected: all parametrised cases pass. + +- [ ] **Step 5: Run mypy** + +```bash +make typecheck +``` + +Expected: clean. + +- [ ] **Step 6: Commit** + +```bash +git add api/integrations/azure_devops/services/url_parsing.py api/tests/unit/integrations/azure_devops/test_url_parsing.py +git commit -m "$(cat <<'EOF' +feat(integrations): add Azure DevOps URL parsing for PRs and work items + +Two NamedTuple types (AdoPullRequestRef, AdoWorkItemRef) and two parser +functions that handle both ADO cloud (dev.azure.com/{org}/...) and +Azure DevOps Server (host/{collection}/...) URL shapes. Parsing never +raises; unparseable URLs return None. Subsequent PRs (browse, comments, +webhook dispatcher) use these structured refs instead of raw URL string +comparisons. + +Co-Authored-By: Claude Opus 4.7 (1M context) +EOF +)" +``` + +--- + +## Task 6: `organisation_url` normalisation on save + +**Files:** +- Modify: `api/integrations/azure_devops/models.py` +- Modify: `api/tests/unit/integrations/azure_devops/test_models.py` + +- [ ] **Step 1: Write the failing tests** + +Append to `api/tests/unit/integrations/azure_devops/test_models.py`: + +```python +@pytest.mark.django_db +def test_azure_devops_configuration__save__strips_single_trailing_slash( + project: Project, +) -> None: + # Given + raw_url = "https://dev.azure.com/test-org/" + + # When + config = AzureDevOpsConfiguration.objects.create( + project=project, + organisation_url=raw_url, + personal_access_token="ado-test-token", + ) + + # Then + assert config.organisation_url == "https://dev.azure.com/test-org" + + +@pytest.mark.django_db +def test_azure_devops_configuration__save__strips_multiple_trailing_slashes( + project: Project, +) -> None: + # Given + raw_url = "https://dev.azure.com/test-org///" + + # When + config = AzureDevOpsConfiguration.objects.create( + project=project, + organisation_url=raw_url, + personal_access_token="ado-test-token", + ) + + # Then + assert config.organisation_url == "https://dev.azure.com/test-org" + + +@pytest.mark.django_db +def test_azure_devops_configuration__save__no_trailing_slash__is_unchanged( + project: Project, +) -> None: + # Given + raw_url = "https://dev.azure.com/test-org" + + # When + config = AzureDevOpsConfiguration.objects.create( + project=project, + organisation_url=raw_url, + personal_access_token="ado-test-token", + ) + + # Then + assert config.organisation_url == raw_url +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_models.py -v' +``` + +Expected: the three new tests fail because URL is stored verbatim with trailing slashes today. + +- [ ] **Step 3: Add the `save()` override** + +In `api/integrations/azure_devops/models.py`, modify the `AzureDevOpsConfiguration` class. The model currently looks like: + +```python +class AzureDevOpsConfiguration(SoftDeleteExportableModel): + project = models.OneToOneField( + "projects.Project", + on_delete=models.CASCADE, + related_name="azure_devops_config", + ) + organisation_url = models.URLField(max_length=300) + personal_access_token = models.CharField(max_length=300) + labeling_enabled = models.BooleanField(default=False) + tagging_enabled = models.BooleanField(default=False) +``` + +Add the `save()` override at the bottom of the class: + +```python +class AzureDevOpsConfiguration(SoftDeleteExportableModel): + project = models.OneToOneField( + "projects.Project", + on_delete=models.CASCADE, + related_name="azure_devops_config", + ) + organisation_url = models.URLField(max_length=300) + personal_access_token = models.CharField(max_length=300) + labeling_enabled = models.BooleanField(default=False) + tagging_enabled = models.BooleanField(default=False) + + def save(self, *args: object, **kwargs: object) -> None: + self.organisation_url = self.organisation_url.rstrip("/") + super().save(*args, **kwargs) +``` + +- [ ] **Step 4: Run the tests** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_models.py -v' +``` + +Expected: all model tests pass (6 from Task 2 + 3 from Task 3 + 3 new = 12). + +- [ ] **Step 5: Run mypy** + +```bash +make typecheck +``` + +Expected: clean. (The `*args: object, **kwargs: object` signature is intentionally permissive — `SoftDeleteExportableModel.save` accepts arbitrary args/kwargs.) + +- [ ] **Step 6: Commit** + +```bash +git add api/integrations/azure_devops/models.py api/tests/unit/integrations/azure_devops/test_models.py +git commit -m "$(cat <<'EOF' +feat(integrations): normalise organisation_url on save + +Strip trailing slashes from AzureDevOpsConfiguration.organisation_url +before persisting. This guarantees downstream URL composition +(client.api._ado_request) never produces double-slash paths regardless +of how the user supplied the URL. + +Co-Authored-By: Claude Opus 4.7 (1M context) +EOF +)" +``` + +--- + +## Task 7: Serializer drops placeholder PAT on update + +**Files:** +- Modify: `api/integrations/azure_devops/serializers.py` +- Modify: `api/tests/unit/integrations/azure_devops/test_serializers.py` + +- [ ] **Step 1: Write the failing tests** + +Append to `api/tests/unit/integrations/azure_devops/test_serializers.py`: + +```python +from typing import cast + + +@pytest.mark.django_db +def test_serializer__update_with_placeholder_pat__preserves_existing_token( + azure_devops_configuration: AzureDevOpsConfiguration, +) -> None: + # Given + original_token = azure_devops_configuration.personal_access_token + serializer = AzureDevOpsConfigurationSerializer( + instance=azure_devops_configuration, + data={ + "organisation_url": azure_devops_configuration.organisation_url, + "personal_access_token": WRITE_ONLY_PLACEHOLDER, + }, + ) + + # When + serializer.is_valid(raise_exception=True) + cast(AzureDevOpsConfigurationSerializer, serializer).save() + + # Then + azure_devops_configuration.refresh_from_db() + assert azure_devops_configuration.personal_access_token == original_token + + +@pytest.mark.django_db +def test_serializer__update_with_new_pat__persists_new_token( + azure_devops_configuration: AzureDevOpsConfiguration, +) -> None: + # Given + new_token = "ado-rotated-token" + serializer = AzureDevOpsConfigurationSerializer( + instance=azure_devops_configuration, + data={ + "organisation_url": azure_devops_configuration.organisation_url, + "personal_access_token": new_token, + }, + ) + + # When + serializer.is_valid(raise_exception=True) + cast(AzureDevOpsConfigurationSerializer, serializer).save() + + # Then + azure_devops_configuration.refresh_from_db() + assert azure_devops_configuration.personal_access_token == new_token +``` + +- [ ] **Step 2: Run the tests to verify they fail** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_serializers.py -v' +``` + +Expected: the first new test fails — the placeholder is currently persisted as the new token; the original token is overwritten. + +- [ ] **Step 3: Override `update()` on the serializer** + +In `api/integrations/azure_devops/serializers.py`, the serializer currently looks like: + +```python +from typing import Any + +from integrations.azure_devops.models import AzureDevOpsConfiguration +from integrations.common.serializers import BaseProjectIntegrationModelSerializer + +WRITE_ONLY_PLACEHOLDER = "write-only" + + +class AzureDevOpsConfigurationSerializer(BaseProjectIntegrationModelSerializer): + class Meta: + model = AzureDevOpsConfiguration + fields = ( + "id", + "organisation_url", + "personal_access_token", + "labeling_enabled", + "tagging_enabled", + ) + + def to_representation(self, instance: AzureDevOpsConfiguration) -> dict[str, Any]: + data = super().to_representation(instance) + data["personal_access_token"] = WRITE_ONLY_PLACEHOLDER + return data +``` + +Add the `update()` override: + +```python +from typing import Any + +from integrations.azure_devops.models import AzureDevOpsConfiguration +from integrations.common.serializers import BaseProjectIntegrationModelSerializer + +WRITE_ONLY_PLACEHOLDER = "write-only" + + +class AzureDevOpsConfigurationSerializer(BaseProjectIntegrationModelSerializer): + class Meta: + model = AzureDevOpsConfiguration + fields = ( + "id", + "organisation_url", + "personal_access_token", + "labeling_enabled", + "tagging_enabled", + ) + + def to_representation(self, instance: AzureDevOpsConfiguration) -> dict[str, Any]: + data = super().to_representation(instance) + data["personal_access_token"] = WRITE_ONLY_PLACEHOLDER + return data + + def update( + self, + instance: AzureDevOpsConfiguration, + validated_data: dict[str, Any], + ) -> AzureDevOpsConfiguration: + # Treat the masked placeholder on input as "no change" so the + # frontend can round-trip the masked representation without + # accidentally overwriting the real PAT. + if validated_data.get("personal_access_token") == WRITE_ONLY_PLACEHOLDER: + validated_data.pop("personal_access_token", None) + return super().update(instance, validated_data) # type: ignore[no-any-return] +``` + +- [ ] **Step 4: Run the tests** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_serializers.py -v' +``` + +Expected: 3 passed (1 from Task 4 of PR 2 + 2 new). + +- [ ] **Step 5: Run mypy** + +```bash +make typecheck +``` + +Expected: clean. + +- [ ] **Step 6: Commit** + +```bash +git add api/integrations/azure_devops/serializers.py api/tests/unit/integrations/azure_devops/test_serializers.py +git commit -m "$(cat <<'EOF' +feat(integrations): preserve PAT on update when payload sends placeholder + +Override the serializer's update() to drop personal_access_token from +validated_data when its value equals WRITE_ONLY_PLACEHOLDER. This lets +the frontend safely round-trip the masked representation without +overwriting the stored PAT, and keeps the imminent PAT-validation hook +in the viewset from validating a sentinel string against ADO. + +Co-Authored-By: Claude Opus 4.7 (1M context) +EOF +)" +``` + +--- + +## Task 8: Wire PAT validation into the configuration viewset + +**Files:** +- Modify: `api/integrations/azure_devops/views/configuration.py` +- Modify: `api/tests/unit/integrations/azure_devops/test_configuration.py` + +This task adds the validation call **and** updates the three existing tests in `test_configuration.py` that exercise `perform_create` / `perform_update` to mock the ADO probe via `responses`. + +- [ ] **Step 1: Write the new failing tests** + +Append to `api/tests/unit/integrations/azure_devops/test_configuration.py`: + +```python +import responses + + +@responses.activate +def test_create_configuration__invalid_pat__returns_400( + admin_client_new: APIClient, + project: Project, +) -> None: + # Given + responses.get( + "https://dev.azure.com/test-org/_apis/projects", + json={}, + status=401, + ) + url = f"/api/v1/projects/{project.id}/integrations/azure-devops/" + payload = { + "organisation_url": "https://dev.azure.com/test-org", + "personal_access_token": "ado-bogus-token", + } + + # When + response = admin_client_new.post(url, data=payload, format="json") + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "Azure DevOps rejected" in str(response.json()) + assert not AzureDevOpsConfiguration.objects.filter(project=project).exists() + + +@responses.activate +def test_update_configuration__placeholder_pat__skips_validation( + admin_client_new: APIClient, + project: Project, + azure_devops_configuration: AzureDevOpsConfiguration, +) -> None: + # Given — no ADO probe response registered; if validation runs, the test + # will fail with a `responses.ConnectionError`. + detail_url = ( + f"/api/v1/projects/{project.id}/integrations/azure-devops/" + f"{azure_devops_configuration.id}/" + ) + payload = { + "organisation_url": azure_devops_configuration.organisation_url, + "personal_access_token": "write-only", + "labeling_enabled": True, + "tagging_enabled": True, + } + original_pat = azure_devops_configuration.personal_access_token + + # When + response = admin_client_new.put(detail_url, data=payload, format="json") + + # Then + assert response.status_code == status.HTTP_200_OK + azure_devops_configuration.refresh_from_db() + assert azure_devops_configuration.personal_access_token == original_pat + assert azure_devops_configuration.labeling_enabled is True + assert azure_devops_configuration.tagging_enabled is True +``` + +- [ ] **Step 2: Update the existing affected tests to mock the ADO probe** + +In `api/tests/unit/integrations/azure_devops/test_configuration.py`, three existing tests need updates because their request paths now invoke the validation helper: + +1. `test_create_configuration__valid_data__persists_and_masks_token` — change the function decorator and add a response stub at the start of `# Given`: + +```python +@responses.activate +def test_create_configuration__valid_data__persists_and_masks_token( + admin_client_new: APIClient, + project: Project, + log: StructuredLogCapture, +) -> None: + # Given + responses.get( + "https://dev.azure.com/test-org/_apis/projects", + json={"value": [], "count": 0}, + status=200, + ) + url = f"/api/v1/projects/{project.id}/integrations/azure-devops/" + payload = { + "organisation_url": "https://dev.azure.com/test-org", + "personal_access_token": "ado-test-token", + } + # ... existing # When / # Then unchanged +``` + +(Keep the existing assertions verbatim — only add the decorator and the `responses.get(...)` call inside `# Given`.) + +2. `test_create_configuration__after_soft_delete__undeletes_existing_row` — same shape: add `@responses.activate` and a 200 stub for `https://dev.azure.com/recreated/_apis/projects`: + +```python +@responses.activate +def test_create_configuration__after_soft_delete__undeletes_existing_row( + admin_client_new: APIClient, + project: Project, + azure_devops_configuration: AzureDevOpsConfiguration, +) -> None: + # Given + original_pk = azure_devops_configuration.pk + azure_devops_configuration.delete() + responses.get( + "https://dev.azure.com/recreated/_apis/projects", + json={"value": [], "count": 0}, + status=200, + ) + url = f"/api/v1/projects/{project.id}/integrations/azure-devops/" + payload = { + "organisation_url": "https://dev.azure.com/recreated", + "personal_access_token": "ado-recreated-token", + } + # ... existing # When / # Then unchanged +``` + +3. `test_update_configuration__valid_data__persists_and_masks_token` — add `@responses.activate` and a 200 stub for `https://dev.azure.com/updated/_apis/projects` (the URL after update): + +```python +@responses.activate +def test_update_configuration__valid_data__persists_and_masks_token( + admin_client_new: APIClient, + project: Project, + azure_devops_configuration: AzureDevOpsConfiguration, + log: StructuredLogCapture, +) -> None: + # Given + responses.get( + "https://dev.azure.com/updated/_apis/projects", + json={"value": [], "count": 0}, + status=200, + ) + detail_url = ( + f"/api/v1/projects/{project.id}/integrations/azure-devops/" + f"{azure_devops_configuration.id}/" + ) + payload = { + "organisation_url": "https://dev.azure.com/updated", + "personal_access_token": "ado-updated-token", + "labeling_enabled": True, + "tagging_enabled": True, + } + # ... existing # When / # Then unchanged +``` + +- [ ] **Step 3: Run the test file to verify the new tests fail (validation not yet wired)** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_configuration.py -v' +``` + +Expected outcomes: +- `test_create_configuration__invalid_pat__returns_400` FAIL (no validation → still 201) +- `test_update_configuration__placeholder_pat__skips_validation` FAIL (the existing serializer override drops the placeholder, so the PUT succeeds, but the validation helper doesn't exist yet; this test may actually pass at this stage — that's fine, it's a regression guard) +- The three existing tests should still pass (they have the mock now, but the validation helper isn't called yet) + +If everything is green at this stage it just means validation hasn't been wired; that's the next step. + +- [ ] **Step 4: Wire the validation helper into the viewset** + +In `api/integrations/azure_devops/views/configuration.py`, the file currently has: + +```python +import structlog +from structlog.typing import FilteringBoundLogger + +from integrations.azure_devops.models import AzureDevOpsConfiguration +from integrations.azure_devops.serializers import ( + AzureDevOpsConfigurationSerializer, +) +from integrations.common.views import ProjectIntegrationBaseViewSet + +logger = structlog.get_logger("azure_devops") + + +class AzureDevOpsConfigurationViewSet(ProjectIntegrationBaseViewSet): + serializer_class = AzureDevOpsConfigurationSerializer # type: ignore[assignment] + model_class = AzureDevOpsConfiguration # type: ignore[assignment] + pagination_class = None + + def _log_for(self, config: AzureDevOpsConfiguration) -> FilteringBoundLogger: + return logger.bind( # type: ignore[no-any-return] + organisation__id=config.project.organisation_id, + project__id=config.project.id, + ) + + def perform_create(self, serializer: AzureDevOpsConfigurationSerializer) -> None: # type: ignore[override] + super().perform_create(serializer) + instance: AzureDevOpsConfiguration = serializer.instance # type: ignore[assignment] + self._log_for(instance).info( + "configuration.created", + ado__organisation__url=instance.organisation_url, + ) + + def perform_update(self, serializer: AzureDevOpsConfigurationSerializer) -> None: # type: ignore[override] + super().perform_update(serializer) + instance: AzureDevOpsConfiguration = serializer.instance # type: ignore[assignment] + self._log_for(instance).info( + "configuration.updated", + ado__organisation__url=instance.organisation_url, + ) + + def perform_destroy(self, instance: AzureDevOpsConfiguration) -> None: + log = self._log_for(instance) + super().perform_destroy(instance) + log.info("configuration.deleted") +``` + +Replace it with the following (full file contents): + +```python +import structlog +from rest_framework.exceptions import ValidationError +from structlog.typing import FilteringBoundLogger + +from integrations.azure_devops.client import list_projects +from integrations.azure_devops.client.exceptions import AzureDevOpsAuthError +from integrations.azure_devops.models import AzureDevOpsConfiguration +from integrations.azure_devops.serializers import ( + AzureDevOpsConfigurationSerializer, +) +from integrations.common.views import ProjectIntegrationBaseViewSet + +logger = structlog.get_logger("azure_devops") + + +def _validate_pat_against_ado(*, organisation_url: str, pat: str) -> None: + """Probe ADO with a minimal request to confirm the PAT is valid. + + Raises ``ValidationError`` on 401/403 from ADO. Other failures (5xx, + network) bubble up so the caller can decide whether to log-and-allow + or surface as an error. + """ + try: + list_projects(organisation_url=organisation_url, pat=pat, top=1) + except AzureDevOpsAuthError: + raise ValidationError( + "Azure DevOps rejected the credentials. " + "Check the organisation URL and personal access token." + ) from None + + +class AzureDevOpsConfigurationViewSet(ProjectIntegrationBaseViewSet): + serializer_class = AzureDevOpsConfigurationSerializer # type: ignore[assignment] + model_class = AzureDevOpsConfiguration # type: ignore[assignment] + pagination_class = None + + def _log_for(self, config: AzureDevOpsConfiguration) -> FilteringBoundLogger: + return logger.bind( # type: ignore[no-any-return] + organisation__id=config.project.organisation_id, + project__id=config.project.id, + ) + + def perform_create(self, serializer: AzureDevOpsConfigurationSerializer) -> None: # type: ignore[override] + _validate_pat_against_ado( + organisation_url=serializer.validated_data["organisation_url"], + pat=serializer.validated_data["personal_access_token"], + ) + super().perform_create(serializer) + instance: AzureDevOpsConfiguration = serializer.instance # type: ignore[assignment] + self._log_for(instance).info( + "configuration.created", + ado__organisation__url=instance.organisation_url, + ) + + def perform_update(self, serializer: AzureDevOpsConfigurationSerializer) -> None: # type: ignore[override] + pat = serializer.validated_data.get("personal_access_token") + if pat is not None: + _validate_pat_against_ado( + organisation_url=serializer.validated_data.get( + "organisation_url", + serializer.instance.organisation_url, # type: ignore[union-attr] + ), + pat=pat, + ) + super().perform_update(serializer) + instance: AzureDevOpsConfiguration = serializer.instance # type: ignore[assignment] + self._log_for(instance).info( + "configuration.updated", + ado__organisation__url=instance.organisation_url, + ) + + def perform_destroy(self, instance: AzureDevOpsConfiguration) -> None: + log = self._log_for(instance) + super().perform_destroy(instance) + log.info("configuration.deleted") +``` + +(Notes on the implementation: +- `_validate_pat_against_ado` lives at module level so tests can patch it if they want to. It re-raises only on `AzureDevOpsAuthError`; other failures propagate so the caller can decide. +- On update, validation only runs if `personal_access_token` is in `validated_data` — the Task 7 serializer override pops the placeholder, so the placeholder case is automatically skipped. +- `from None` suppresses the inner traceback in the user-facing 400 response.) + +- [ ] **Step 5: Run the test file** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/test_configuration.py -v' +``` + +Expected: every test in the file passes — including the two new tests and the three existing ones with their new `@responses.activate` decorators. + +- [ ] **Step 6: Run mypy** + +```bash +make typecheck +``` + +Expected: clean. + +- [ ] **Step 7: Run lint** + +```bash +make lint +``` + +Expected: clean. + +- [ ] **Step 8: Commit** + +```bash +git add api/integrations/azure_devops/views/configuration.py api/tests/unit/integrations/azure_devops/test_configuration.py +git commit -m "$(cat <<'EOF' +feat(integrations): validate PAT against ADO on create/update + +The configuration viewset now probes ADO with list_projects(top=1) when +the PAT is being set. On 401/403 ADO replies, the viewset returns 400 +with a clear message and does not persist. On update, validation only +runs when the request actually changes the PAT (the serializer override +from the previous commit drops placeholder values). + +Network and 5xx failures still propagate so transient unavailability is +not silently ignored. The existing tests are updated to mock the ADO +probe via the responses library; two new tests exercise the +invalid-PAT and placeholder-on-update paths. + +Co-Authored-By: Claude Opus 4.7 (1M context) +EOF +)" +``` + +--- + +## Task 9: Full-suite verification + +- [ ] **Step 1: Lint** + +From `api/`: + +```bash +make lint +``` + +Expected: clean. If any auto-fixes apply, accept them and either amend the originating commit or add a `style:` follow-up — match what was done in PRs 1 and 2. + +- [ ] **Step 2: Type check** + +```bash +make typecheck +``` + +Expected: clean. + +- [ ] **Step 3: Run the whole new test directory** + +```bash +make test opts='-n0 tests/unit/integrations/azure_devops/ -v' +``` + +Expected: every test passes. Approximate count: 23 (PR 2 baseline) + ~2 constants + ~4 exceptions + ~2 types + ~7 client + ~4 PR URL parse + ~4 work-item URL parse + 3 model save + 2 serializer placeholder + 2 viewset (invalid PAT + placeholder skip) = around 50. + +- [ ] **Step 4: Regression guard** + +```bash +make test opts='tests/unit/integrations/gitlab tests/unit/integrations/github tests/unit/features/test_unit_feature_external_resources_views.py tests/unit/features/test_migrations.py' +``` + +Expected: all pass. + +- [ ] **Step 5: Migration consistency** + +```bash +make django-make-migrations opts='--check --dry-run' +``` + +Expected: `No changes detected` for every app. PR 3 introduces no schema changes. + +- [ ] **Step 6: Branch state** + +```bash +git status +git log --oneline feat/azure-devops-02-models..HEAD +``` + +Expected: working tree clean; eight feature commits on this branch ahead of `feat/azure-devops-02-models` (Task 1 through Task 8 each produce one commit; the plan-doc commit is optional and may already exist from a prior step). + +--- + +## Done condition + +- Branch `feat/azure-devops-03-client` carries the PR 3 plan doc commit + eight feature commits (plus any optional `style:` follow-ups). +- The Azure DevOps REST client surface for v1 (`list_projects` plus the HTTP plumbing) is live with typed exceptions. +- URL parsing for PR and work-item URLs covers cloud and on-prem shapes; parsing never raises. +- `organisation_url` is normalised on save (trailing slash stripped). +- The serializer drops `personal_access_token == WRITE_ONLY_PLACEHOLDER` from update payloads. +- The configuration viewset validates the PAT against ADO on create, and on update when the PAT actually changes; auth failures surface as 400. +- All new tests pass; mypy strict, ruff, and `flagsmith-lint-tests` are clean. No schema drift. + +When all boxes are ticked, push the branch and open the PR against `feat/azure-devops-02-models`. The next plan in the stack will be written after this PR lands; the spec's section ordering points toward `services/tagging.py` and tag-system-tag constants next. From 2945b3bc146f58829f42e8b05f2237c598853791 Mon Sep 17 00:00:00 2001 From: "Asaph M. Kotzin" Date: Thu, 28 May 2026 13:11:12 +0100 Subject: [PATCH 02/11] feat(integrations): scaffold Azure DevOps client + services packages Add empty client/ and services/ packages plus a constants module holding the request timeout and pinned REST API version (7.1). Subsequent commits in this PR fill in the client modules and the URL-parsing service. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../azure_devops/client/__init__.py | 0 api/integrations/azure_devops/constants.py | 3 ++ .../azure_devops/services/__init__.py | 0 .../azure_devops/test_constants.py | 28 +++++++++++++++++++ 4 files changed, 31 insertions(+) create mode 100644 api/integrations/azure_devops/client/__init__.py create mode 100644 api/integrations/azure_devops/constants.py create mode 100644 api/integrations/azure_devops/services/__init__.py create mode 100644 api/tests/unit/integrations/azure_devops/test_constants.py diff --git a/api/integrations/azure_devops/client/__init__.py b/api/integrations/azure_devops/client/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/api/integrations/azure_devops/constants.py b/api/integrations/azure_devops/constants.py new file mode 100644 index 000000000000..740769afada2 --- /dev/null +++ b/api/integrations/azure_devops/constants.py @@ -0,0 +1,3 @@ +AZURE_DEVOPS_CLIENT_TIMEOUT_SECONDS = 10 + +AZURE_DEVOPS_API_VERSION = "7.1" diff --git a/api/integrations/azure_devops/services/__init__.py b/api/integrations/azure_devops/services/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/api/tests/unit/integrations/azure_devops/test_constants.py b/api/tests/unit/integrations/azure_devops/test_constants.py new file mode 100644 index 000000000000..68e0f7987d97 --- /dev/null +++ b/api/tests/unit/integrations/azure_devops/test_constants.py @@ -0,0 +1,28 @@ +from integrations.azure_devops.constants import ( + AZURE_DEVOPS_API_VERSION, + AZURE_DEVOPS_CLIENT_TIMEOUT_SECONDS, +) + + +def test_constants__timeout__has_sensible_default() -> None: + # Given + timeout = AZURE_DEVOPS_CLIENT_TIMEOUT_SECONDS + + # When + is_positive_int = isinstance(timeout, int) and timeout > 0 + + # Then + assert is_positive_int + assert timeout <= 60 + + +def test_constants__api_version__is_string() -> None: + # Given + version = AZURE_DEVOPS_API_VERSION + + # When + is_string = isinstance(version, str) + + # Then + assert is_string + assert version From bbb323cf26a496c6c2300918b4b8b790f4199369 Mon Sep 17 00:00:00 2001 From: "Asaph M. Kotzin" Date: Thu, 28 May 2026 13:13:07 +0100 Subject: [PATCH 03/11] feat(integrations): add Azure DevOps client typed exceptions Three classes: AzureDevOpsError (base), AzureDevOpsAuthError (401/403), AzureDevOpsNotFoundError (404 on single-resource). Callers can catch the base for "anything went wrong with ADO" or the specific subclass for targeted handling. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../azure_devops/client/exceptions.py | 10 ++++ .../azure_devops/test_exceptions.py | 53 +++++++++++++++++++ 2 files changed, 63 insertions(+) create mode 100644 api/integrations/azure_devops/client/exceptions.py create mode 100644 api/tests/unit/integrations/azure_devops/test_exceptions.py diff --git a/api/integrations/azure_devops/client/exceptions.py b/api/integrations/azure_devops/client/exceptions.py new file mode 100644 index 000000000000..6d28051dfd0c --- /dev/null +++ b/api/integrations/azure_devops/client/exceptions.py @@ -0,0 +1,10 @@ +class AzureDevOpsError(Exception): + """Base class for all Azure DevOps client errors raised by this package.""" + + +class AzureDevOpsAuthError(AzureDevOpsError): + """Raised when ADO rejects credentials with 401 or 403.""" + + +class AzureDevOpsNotFoundError(AzureDevOpsError): + """Raised when ADO returns 404 for a single-resource lookup.""" diff --git a/api/tests/unit/integrations/azure_devops/test_exceptions.py b/api/tests/unit/integrations/azure_devops/test_exceptions.py new file mode 100644 index 000000000000..5df8cd3c7bc5 --- /dev/null +++ b/api/tests/unit/integrations/azure_devops/test_exceptions.py @@ -0,0 +1,53 @@ +import pytest + +from integrations.azure_devops.client.exceptions import ( + AzureDevOpsAuthError, + AzureDevOpsError, + AzureDevOpsNotFoundError, +) + + +def test_azure_devops_auth_error__inheritance__is_subclass_of_base() -> None: + # Given + cls = AzureDevOpsAuthError + + # When + is_subclass = issubclass(cls, AzureDevOpsError) + + # Then + assert is_subclass + + +def test_azure_devops_not_found_error__inheritance__is_subclass_of_base() -> None: + # Given + cls = AzureDevOpsNotFoundError + + # When + is_subclass = issubclass(cls, AzureDevOpsError) + + # Then + assert is_subclass + + +def test_azure_devops_error__base__is_exception_subclass() -> None: + # Given + cls = AzureDevOpsError + + # When + is_exception = issubclass(cls, Exception) + + # Then + assert is_exception + + +def test_azure_devops_auth_error__raise_and_catch__as_base_works() -> None: + # Given + def raise_auth_error() -> None: + raise AzureDevOpsAuthError("invalid PAT") + + # When + with pytest.raises(AzureDevOpsError) as exc_info: + raise_auth_error() + + # Then + assert "invalid PAT" in str(exc_info.value) From bc842229575be9e85cf027faf53495558438359c Mon Sep 17 00:00:00 2001 From: "Asaph M. Kotzin" Date: Thu, 28 May 2026 13:13:50 +0100 Subject: [PATCH 04/11] feat(integrations): add Azure DevOps client TypedDicts Two typed dicts for v1: AdoProject (id/name/url) and AdoProjectsPage (results + continuation_token). Subsequent PRs will extend this module with AdoRepository, AdoPullRequest, AdoWorkItem, AdoSubscription, and the resource-metadata shape as their corresponding client functions land. Co-Authored-By: Claude Opus 4.7 (1M context) --- api/integrations/azure_devops/client/types.py | 12 +++++++++ .../integrations/azure_devops/test_client.py | 27 +++++++++++++++++++ 2 files changed, 39 insertions(+) create mode 100644 api/integrations/azure_devops/client/types.py create mode 100644 api/tests/unit/integrations/azure_devops/test_client.py diff --git a/api/integrations/azure_devops/client/types.py b/api/integrations/azure_devops/client/types.py new file mode 100644 index 000000000000..2e65e4300463 --- /dev/null +++ b/api/integrations/azure_devops/client/types.py @@ -0,0 +1,12 @@ +from typing import TypedDict + + +class AdoProject(TypedDict): + id: str + name: str + url: str + + +class AdoProjectsPage(TypedDict): + results: list[AdoProject] + continuation_token: str | None diff --git a/api/tests/unit/integrations/azure_devops/test_client.py b/api/tests/unit/integrations/azure_devops/test_client.py new file mode 100644 index 000000000000..68648ea5170f --- /dev/null +++ b/api/tests/unit/integrations/azure_devops/test_client.py @@ -0,0 +1,27 @@ +from integrations.azure_devops.client.types import AdoProject, AdoProjectsPage + + +def test_ado_project__shape__has_required_keys() -> None: + # Given + project: AdoProject = { + "id": "00000000-0000-0000-0000-000000000000", + "name": "Test", + "url": "https://dev.azure.com/test-org/_apis/projects/...", + } + + # When + keys = set(project.keys()) + + # Then + assert keys == {"id", "name", "url"} + + +def test_ado_projects_page__shape__has_required_keys() -> None: + # Given + page: AdoProjectsPage = {"results": [], "continuation_token": None} + + # When + keys = set(page.keys()) + + # Then + assert keys == {"results", "continuation_token"} From f878535f443af2cc7764a5e48de043b091f96145 Mon Sep 17 00:00:00 2001 From: "Asaph M. Kotzin" Date: Thu, 28 May 2026 13:17:28 +0100 Subject: [PATCH 05/11] feat(integrations): add list_projects to the Azure DevOps client Stand up the client's HTTP plumbing (Basic auth with empty username, api-version=7.1 query param, 10s timeout) and the first concrete function: list_projects. Status codes 401/403 map to AzureDevOpsAuthError, 404 to AzureDevOpsNotFoundError; 5xx and other 4xx bubble up as requests.HTTPError. Subsequent client functions (repos, PRs, work items, comments, labels, subscriptions) land in their respective consumer PRs. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../azure_devops/client/__init__.py | 16 ++ api/integrations/azure_devops/client/api.py | 75 +++++++++ .../integrations/azure_devops/test_client.py | 147 ++++++++++++++++++ 3 files changed, 238 insertions(+) create mode 100644 api/integrations/azure_devops/client/api.py diff --git a/api/integrations/azure_devops/client/__init__.py b/api/integrations/azure_devops/client/__init__.py index e69de29bb2d1..c676dfcf8efb 100644 --- a/api/integrations/azure_devops/client/__init__.py +++ b/api/integrations/azure_devops/client/__init__.py @@ -0,0 +1,16 @@ +from integrations.azure_devops.client.api import list_projects +from integrations.azure_devops.client.exceptions import ( + AzureDevOpsAuthError, + AzureDevOpsError, + AzureDevOpsNotFoundError, +) +from integrations.azure_devops.client.types import AdoProject, AdoProjectsPage + +__all__ = [ + "AdoProject", + "AdoProjectsPage", + "AzureDevOpsAuthError", + "AzureDevOpsError", + "AzureDevOpsNotFoundError", + "list_projects", +] diff --git a/api/integrations/azure_devops/client/api.py b/api/integrations/azure_devops/client/api.py new file mode 100644 index 000000000000..6364c30f06d6 --- /dev/null +++ b/api/integrations/azure_devops/client/api.py @@ -0,0 +1,75 @@ +from typing import Any + +import requests + +from integrations.azure_devops.client.exceptions import ( + AzureDevOpsAuthError, + AzureDevOpsNotFoundError, +) +from integrations.azure_devops.client.types import AdoProject, AdoProjectsPage +from integrations.azure_devops.constants import ( + AZURE_DEVOPS_API_VERSION, + AZURE_DEVOPS_CLIENT_TIMEOUT_SECONDS, +) + + +def _ado_request( + method: str, + organisation_url: str, + pat: str, + *, + path: str, + params: dict[str, Any] | None = None, + json_body: dict[str, Any] | None = None, +) -> requests.Response: + base = organisation_url.rstrip("/") + query: dict[str, Any] = {"api-version": AZURE_DEVOPS_API_VERSION} + if params: + query.update(params) + response = requests.request( + method, + f"{base}/_apis/{path}", + auth=("", pat), + params=query, + json=json_body, + timeout=AZURE_DEVOPS_CLIENT_TIMEOUT_SECONDS, + ) + if response.status_code in (401, 403): + raise AzureDevOpsAuthError( + f"Azure DevOps rejected credentials ({response.status_code})" + ) + if response.status_code == 404: + raise AzureDevOpsNotFoundError( + f"Azure DevOps reported the resource was not found ({response.status_code})" + ) + response.raise_for_status() + return response + + +def list_projects( + *, + organisation_url: str, + pat: str, + top: int | None = None, + continuation_token: str | None = None, +) -> AdoProjectsPage: + params: dict[str, Any] = {} + if top is not None: + params["$top"] = top + if continuation_token is not None: + params["continuationToken"] = continuation_token + + response = _ado_request( + "GET", + organisation_url, + pat, + path="projects", + params=params, + ) + payload = response.json() + results: list[AdoProject] = [ + AdoProject(id=p["id"], name=p["name"], url=p["url"]) + for p in payload.get("value", []) + ] + next_token = response.headers.get("x-ms-continuationtoken") + return AdoProjectsPage(results=results, continuation_token=next_token) diff --git a/api/tests/unit/integrations/azure_devops/test_client.py b/api/tests/unit/integrations/azure_devops/test_client.py index 68648ea5170f..94d7f8d3c5d9 100644 --- a/api/tests/unit/integrations/azure_devops/test_client.py +++ b/api/tests/unit/integrations/azure_devops/test_client.py @@ -1,5 +1,25 @@ +import base64 + +import pytest +import requests +import responses + +from integrations.azure_devops.client import list_projects +from integrations.azure_devops.client.exceptions import ( + AzureDevOpsAuthError, + AzureDevOpsNotFoundError, +) from integrations.azure_devops.client.types import AdoProject, AdoProjectsPage +ORG_URL = "https://dev.azure.com/test-org" +PAT = "ado-test-pat" + + +def _expected_basic_auth_header() -> str: + # ADO PAT auth: HTTP Basic with empty username and PAT as password. + encoded = base64.b64encode(f":{PAT}".encode()).decode() + return f"Basic {encoded}" + def test_ado_project__shape__has_required_keys() -> None: # Given @@ -25,3 +45,130 @@ def test_ado_projects_page__shape__has_required_keys() -> None: # Then assert keys == {"results", "continuation_token"} + + +@responses.activate +def test_list_projects__valid_response__returns_typed_page() -> None: + # Given + responses.get( + f"{ORG_URL}/_apis/projects", + json={ + "value": [ + { + "id": "00000000-0000-0000-0000-000000000001", + "name": "Alpha", + "url": f"{ORG_URL}/_apis/projects/00000000-0000-0000-0000-000000000001", + "extra": "ignored", + }, + ], + "count": 1, + }, + match=[ + responses.matchers.header_matcher( + {"Authorization": _expected_basic_auth_header()} + ) + ], + ) + + # When + page = list_projects(organisation_url=ORG_URL, pat=PAT, top=1) + + # Then + assert page["results"] == [ + { + "id": "00000000-0000-0000-0000-000000000001", + "name": "Alpha", + "url": f"{ORG_URL}/_apis/projects/00000000-0000-0000-0000-000000000001", + }, + ] + assert page["continuation_token"] is None + + +@responses.activate +def test_list_projects__continuation_token_in_header__exposed_on_page() -> None: + # Given + responses.get( + f"{ORG_URL}/_apis/projects", + json={"value": [], "count": 0}, + headers={"x-ms-continuationtoken": "ct-abc"}, + ) + + # When + page = list_projects(organisation_url=ORG_URL, pat=PAT) + + # Then + assert page["continuation_token"] == "ct-abc" + + +@responses.activate +def test_list_projects__401_response__raises_auth_error() -> None: + # Given + responses.get(f"{ORG_URL}/_apis/projects", json={}, status=401) + + # When + def call_list() -> None: + list_projects(organisation_url=ORG_URL, pat=PAT) + + # Then + with pytest.raises(AzureDevOpsAuthError): + call_list() + + +@responses.activate +def test_list_projects__403_response__raises_auth_error() -> None: + # Given + responses.get(f"{ORG_URL}/_apis/projects", json={}, status=403) + + # When + def call_list() -> None: + list_projects(organisation_url=ORG_URL, pat=PAT) + + # Then + with pytest.raises(AzureDevOpsAuthError): + call_list() + + +@responses.activate +def test_list_projects__404_response__raises_not_found_error() -> None: + # Given + responses.get(f"{ORG_URL}/_apis/projects", json={}, status=404) + + # When + def call_list() -> None: + list_projects(organisation_url=ORG_URL, pat=PAT) + + # Then + with pytest.raises(AzureDevOpsNotFoundError): + call_list() + + +@responses.activate +def test_list_projects__500_response__raises_requests_http_error() -> None: + # Given + responses.get(f"{ORG_URL}/_apis/projects", json={}, status=500) + + # When + def call_list() -> None: + list_projects(organisation_url=ORG_URL, pat=PAT) + + # Then — non-4xx server failures bubble up as the underlying requests error + with pytest.raises(requests.HTTPError): + call_list() + + +@responses.activate +def test_list_projects__trailing_slash_in_org_url__normalised_in_request() -> None: + # Given + responses.get( + f"{ORG_URL}/_apis/projects", + json={"value": [], "count": 0}, + ) + + # When + list_projects(organisation_url=f"{ORG_URL}/", pat=PAT) + + # Then — request lands on ORG_URL/_apis/projects (no double slash) + # `responses.calls[0]` is typed as `Call | list[Call]`; in practice a single + # index returns a `Call`. Same workaround as other tests in this codebase. + assert responses.calls[0].request.url is not None # type: ignore[union-attr] + assert "//_apis" not in responses.calls[0].request.url # type: ignore[union-attr] From e09e8920ee63ae0cfa4225961efe1fd827afbd73 Mon Sep 17 00:00:00 2001 From: "Asaph M. Kotzin" Date: Thu, 28 May 2026 13:20:03 +0100 Subject: [PATCH 06/11] feat(integrations): add Azure DevOps URL parsing for PRs and work items Two NamedTuple types (AdoPullRequestRef, AdoWorkItemRef) and two parser functions that handle both ADO cloud (dev.azure.com/{org}/...) and Azure DevOps Server (host/{collection}/...) URL shapes. Parsing never raises; unparseable URLs return None. Subsequent PRs (browse, comments, webhook dispatcher) use these structured refs instead of raw URL string comparisons. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../azure_devops/services/url_parsing.py | 75 ++++++++ .../azure_devops/test_url_parsing.py | 174 ++++++++++++++++++ 2 files changed, 249 insertions(+) create mode 100644 api/integrations/azure_devops/services/url_parsing.py create mode 100644 api/tests/unit/integrations/azure_devops/test_url_parsing.py diff --git a/api/integrations/azure_devops/services/url_parsing.py b/api/integrations/azure_devops/services/url_parsing.py new file mode 100644 index 000000000000..c81707012491 --- /dev/null +++ b/api/integrations/azure_devops/services/url_parsing.py @@ -0,0 +1,75 @@ +import re +from typing import NamedTuple +from urllib.parse import unquote, urlparse + +# Path captures (after stripping query/fragment, normalising trailing slash): +# /{org_or_collection}/{project}/_git/{repo}/pullrequest/{id} +# /{org_or_collection}/{project}/_workitems/edit/{id} +# +# For ADO cloud, {org_or_collection} is a single org segment (e.g. "test-org"). +# For Azure DevOps Server (on-prem), {org_or_collection} is a collection name, +# and the same path pattern applies under whatever host the server runs on. + +_PR_PATH_PATTERN = re.compile( + r"^/(?P[^/]+)/(?P[^/]+)" + r"/_git/(?P[^/]+)/pullrequest/(?P\d+)/?$" +) + +_WORK_ITEM_PATH_PATTERN = re.compile( + r"^/(?P[^/]+)/(?P[^/]+)" + r"/_workitems/edit/(?P\d+)/?$" +) + + +class AdoPullRequestRef(NamedTuple): + organisation_root: str + project: str + repository: str + pull_request_id: int + + +class AdoWorkItemRef(NamedTuple): + organisation_root: str + project: str + work_item_id: int + + +def parse_pull_request_url(url: str | None) -> AdoPullRequestRef | None: + """Return a structured reference for an Azure DevOps pull-request URL, + or ``None`` if the URL does not match the cloud or on-prem PR shape. + Parsing never raises. + """ + if not url: + return None + parsed = urlparse(url) + if not parsed.scheme or not parsed.netloc: + return None + match = _PR_PATH_PATTERN.match(parsed.path) + if not match: + return None + return AdoPullRequestRef( + organisation_root=f"{parsed.scheme}://{parsed.netloc}/{match.group('org_or_collection')}", + project=unquote(match.group("project")), + repository=unquote(match.group("repo")), + pull_request_id=int(match.group("pr_id")), + ) + + +def parse_work_item_url(url: str | None) -> AdoWorkItemRef | None: + """Return a structured reference for an Azure DevOps work-item URL, + or ``None`` if the URL does not match the cloud or on-prem work-item + shape. Parsing never raises. + """ + if not url: + return None + parsed = urlparse(url) + if not parsed.scheme or not parsed.netloc: + return None + match = _WORK_ITEM_PATH_PATTERN.match(parsed.path) + if not match: + return None + return AdoWorkItemRef( + organisation_root=f"{parsed.scheme}://{parsed.netloc}/{match.group('org_or_collection')}", + project=unquote(match.group("project")), + work_item_id=int(match.group("work_item_id")), + ) diff --git a/api/tests/unit/integrations/azure_devops/test_url_parsing.py b/api/tests/unit/integrations/azure_devops/test_url_parsing.py new file mode 100644 index 000000000000..b066e5419313 --- /dev/null +++ b/api/tests/unit/integrations/azure_devops/test_url_parsing.py @@ -0,0 +1,174 @@ +import pytest + +from integrations.azure_devops.services.url_parsing import ( + AdoPullRequestRef, + AdoWorkItemRef, + parse_pull_request_url, + parse_work_item_url, +) + + +@pytest.mark.parametrize( + "url, expected", + [ + # Cloud, plain + ( + "https://dev.azure.com/test-org/proj/_git/repo/pullrequest/42", + AdoPullRequestRef( + organisation_root="https://dev.azure.com/test-org", + project="proj", + repository="repo", + pull_request_id=42, + ), + ), + # Cloud, with trailing slash + ( + "https://dev.azure.com/test-org/proj/_git/repo/pullrequest/42/", + AdoPullRequestRef( + organisation_root="https://dev.azure.com/test-org", + project="proj", + repository="repo", + pull_request_id=42, + ), + ), + # Cloud, with query string + ( + "https://dev.azure.com/test-org/proj/_git/repo/pullrequest/42?_a=overview", + AdoPullRequestRef( + organisation_root="https://dev.azure.com/test-org", + project="proj", + repository="repo", + pull_request_id=42, + ), + ), + # Cloud, project name with spaces (URL-encoded) + ( + "https://dev.azure.com/test-org/My%20Project/_git/my-repo/pullrequest/7", + AdoPullRequestRef( + organisation_root="https://dev.azure.com/test-org", + project="My Project", + repository="my-repo", + pull_request_id=7, + ), + ), + # On-prem with a collection segment + ( + "https://devops.internal.example.com/DefaultCollection/proj/_git/repo/pullrequest/100", + AdoPullRequestRef( + organisation_root="https://devops.internal.example.com/DefaultCollection", + project="proj", + repository="repo", + pull_request_id=100, + ), + ), + ], +) +def test_parse_pull_request_url__valid_shapes__returns_ref( + url: str, expected: AdoPullRequestRef +) -> None: + # Given + target_url = url + + # When + result = parse_pull_request_url(target_url) + + # Then + assert result == expected + + +@pytest.mark.parametrize( + "url", + [ + "", + "not-a-url", + "https://github.com/foo/bar/pull/42", + "https://gitlab.com/foo/bar/-/merge_requests/42", + "https://dev.azure.com/test-org/proj/_git/repo", + "https://dev.azure.com/test-org/proj/_workitems/edit/42", + "https://dev.azure.com/test-org/proj/_git/repo/pullrequest/not-a-number", + ], +) +def test_parse_pull_request_url__malformed_or_other_shapes__returns_none( + url: str, +) -> None: + # Given + target_url = url + + # When + result = parse_pull_request_url(target_url) + + # Then + assert result is None + + +@pytest.mark.parametrize( + "url, expected", + [ + ( + "https://dev.azure.com/test-org/proj/_workitems/edit/100", + AdoWorkItemRef( + organisation_root="https://dev.azure.com/test-org", + project="proj", + work_item_id=100, + ), + ), + ( + "https://dev.azure.com/test-org/proj/_workitems/edit/100/", + AdoWorkItemRef( + organisation_root="https://dev.azure.com/test-org", + project="proj", + work_item_id=100, + ), + ), + ( + "https://dev.azure.com/test-org/My%20Project/_workitems/edit/7", + AdoWorkItemRef( + organisation_root="https://dev.azure.com/test-org", + project="My Project", + work_item_id=7, + ), + ), + ( + "https://devops.internal.example.com/DefaultCollection/proj/_workitems/edit/100", + AdoWorkItemRef( + organisation_root="https://devops.internal.example.com/DefaultCollection", + project="proj", + work_item_id=100, + ), + ), + ], +) +def test_parse_work_item_url__valid_shapes__returns_ref( + url: str, expected: AdoWorkItemRef +) -> None: + # Given + target_url = url + + # When + result = parse_work_item_url(target_url) + + # Then + assert result == expected + + +@pytest.mark.parametrize( + "url", + [ + "", + "not-a-url", + "https://github.com/foo/bar/issues/42", + "https://dev.azure.com/test-org/proj/_git/repo/pullrequest/42", + "https://dev.azure.com/test-org/proj/_workitems/edit/not-a-number", + ], +) +def test_parse_work_item_url__malformed_or_other_shapes__returns_none( + url: str, +) -> None: + # Given + target_url = url + + # When + result = parse_work_item_url(target_url) + + # Then + assert result is None From a7791fe7d36bb718327ca83e57fcd69808b63f32 Mon Sep 17 00:00:00 2001 From: "Asaph M. Kotzin" Date: Thu, 28 May 2026 13:25:12 +0100 Subject: [PATCH 07/11] feat(integrations): normalise organisation_url on save Strip trailing slashes from AzureDevOpsConfiguration.organisation_url before persisting. This guarantees downstream URL composition (client.api._ado_request) never produces double-slash paths regardless of how the user supplied the URL. Co-Authored-By: Claude Opus 4.7 (1M context) --- api/integrations/azure_devops/models.py | 6 +++ .../integrations/azure_devops/test_models.py | 54 +++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/api/integrations/azure_devops/models.py b/api/integrations/azure_devops/models.py index 7a6b8a16c9c7..05343136d9a2 100644 --- a/api/integrations/azure_devops/models.py +++ b/api/integrations/azure_devops/models.py @@ -1,3 +1,5 @@ +from typing import Any + from django.db import models from core.models import SoftDeleteExportableModel @@ -14,6 +16,10 @@ class AzureDevOpsConfiguration(SoftDeleteExportableModel): labeling_enabled = models.BooleanField(default=False) tagging_enabled = models.BooleanField(default=False) + def save(self, *args: Any, **kwargs: Any) -> None: + self.organisation_url = self.organisation_url.rstrip("/") + super().save(*args, **kwargs) + class AzureDevOpsServiceHook(SoftDeleteExportableModel): configuration = models.ForeignKey( diff --git a/api/tests/unit/integrations/azure_devops/test_models.py b/api/tests/unit/integrations/azure_devops/test_models.py index ef001cf6b280..5557cd8a9625 100644 --- a/api/tests/unit/integrations/azure_devops/test_models.py +++ b/api/tests/unit/integrations/azure_devops/test_models.py @@ -155,3 +155,57 @@ def test_azure_devops_service_hook__different_event_type__allowed( # Then assert second.event_type == "workitem.updated" + + +@pytest.mark.django_db +def test_azure_devops_configuration__save__strips_single_trailing_slash( + project: Project, +) -> None: + # Given + raw_url = "https://dev.azure.com/test-org/" + + # When + config = AzureDevOpsConfiguration.objects.create( + project=project, + organisation_url=raw_url, + personal_access_token="ado-test-token", + ) + + # Then + assert config.organisation_url == "https://dev.azure.com/test-org" + + +@pytest.mark.django_db +def test_azure_devops_configuration__save__strips_multiple_trailing_slashes( + project: Project, +) -> None: + # Given + raw_url = "https://dev.azure.com/test-org///" + + # When + config = AzureDevOpsConfiguration.objects.create( + project=project, + organisation_url=raw_url, + personal_access_token="ado-test-token", + ) + + # Then + assert config.organisation_url == "https://dev.azure.com/test-org" + + +@pytest.mark.django_db +def test_azure_devops_configuration__save__no_trailing_slash__is_unchanged( + project: Project, +) -> None: + # Given + raw_url = "https://dev.azure.com/test-org" + + # When + config = AzureDevOpsConfiguration.objects.create( + project=project, + organisation_url=raw_url, + personal_access_token="ado-test-token", + ) + + # Then + assert config.organisation_url == raw_url From 0099d52a4219bad282736dd3c103f3293ffdf154 Mon Sep 17 00:00:00 2001 From: "Asaph M. Kotzin" Date: Thu, 28 May 2026 13:29:20 +0100 Subject: [PATCH 08/11] feat(integrations): preserve PAT on update when payload sends placeholder Override the serializer's update() to drop personal_access_token from validated_data when its value equals WRITE_ONLY_PLACEHOLDER. This lets the frontend safely round-trip the masked representation without overwriting the stored PAT, and keeps the imminent PAT-validation hook in the viewset from validating a sentinel string against ADO. Co-Authored-By: Claude Opus 4.7 (1M context) --- api/integrations/azure_devops/serializers.py | 12 +++++ .../azure_devops/test_serializers.py | 46 +++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/api/integrations/azure_devops/serializers.py b/api/integrations/azure_devops/serializers.py index 6654f4629b63..0cb539dd03bc 100644 --- a/api/integrations/azure_devops/serializers.py +++ b/api/integrations/azure_devops/serializers.py @@ -21,3 +21,15 @@ def to_representation(self, instance: AzureDevOpsConfiguration) -> dict[str, Any data = super().to_representation(instance) data["personal_access_token"] = WRITE_ONLY_PLACEHOLDER return data + + def update( + self, + instance: AzureDevOpsConfiguration, + validated_data: dict[str, Any], + ) -> AzureDevOpsConfiguration: + # Treat the masked placeholder on input as "no change" so the + # frontend can round-trip the masked representation without + # accidentally overwriting the real PAT. + if validated_data.get("personal_access_token") == WRITE_ONLY_PLACEHOLDER: + validated_data.pop("personal_access_token", None) + return super().update(instance, validated_data) # type: ignore[no-any-return] diff --git a/api/tests/unit/integrations/azure_devops/test_serializers.py b/api/tests/unit/integrations/azure_devops/test_serializers.py index c2787854b348..82669a6867de 100644 --- a/api/tests/unit/integrations/azure_devops/test_serializers.py +++ b/api/tests/unit/integrations/azure_devops/test_serializers.py @@ -22,3 +22,49 @@ def test_serializer__to_representation__masks_personal_access_token( assert data["organisation_url"] == azure_devops_configuration.organisation_url assert data["labeling_enabled"] is False assert data["tagging_enabled"] is False + + +@pytest.mark.django_db +def test_serializer__update_with_placeholder_pat__preserves_existing_token( + azure_devops_configuration: AzureDevOpsConfiguration, +) -> None: + # Given + original_token = azure_devops_configuration.personal_access_token + serializer = AzureDevOpsConfigurationSerializer( + instance=azure_devops_configuration, + data={ + "organisation_url": azure_devops_configuration.organisation_url, + "personal_access_token": WRITE_ONLY_PLACEHOLDER, + }, + ) + + # When + serializer.is_valid(raise_exception=True) + serializer.save() + + # Then + azure_devops_configuration.refresh_from_db() + assert azure_devops_configuration.personal_access_token == original_token + + +@pytest.mark.django_db +def test_serializer__update_with_new_pat__persists_new_token( + azure_devops_configuration: AzureDevOpsConfiguration, +) -> None: + # Given + new_token = "ado-rotated-token" + serializer = AzureDevOpsConfigurationSerializer( + instance=azure_devops_configuration, + data={ + "organisation_url": azure_devops_configuration.organisation_url, + "personal_access_token": new_token, + }, + ) + + # When + serializer.is_valid(raise_exception=True) + serializer.save() + + # Then + azure_devops_configuration.refresh_from_db() + assert azure_devops_configuration.personal_access_token == new_token From a1d780226d275a95c9696d17eb19878e3341e549 Mon Sep 17 00:00:00 2001 From: "Asaph M. Kotzin" Date: Thu, 28 May 2026 13:37:05 +0100 Subject: [PATCH 09/11] feat(integrations): validate PAT against ADO on create/update The configuration viewset now probes ADO with list_projects(top=1) when the PAT is being set. On 401/403 ADO replies, the viewset returns 400 with a clear message and does not persist. On update, validation only runs when the request actually changes the PAT (the serializer override from the previous commit drops placeholder values). Network and 5xx failures still propagate so transient unavailability is not silently ignored. The existing tests are updated to mock the ADO probe via the responses library; two new tests exercise the invalid-PAT and placeholder-on-update paths. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../azure_devops/views/configuration.py | 39 ++++++++++ .../azure_devops/test_configuration.py | 76 +++++++++++++++++++ 2 files changed, 115 insertions(+) diff --git a/api/integrations/azure_devops/views/configuration.py b/api/integrations/azure_devops/views/configuration.py index 7d1884f160b3..00b6588331ee 100644 --- a/api/integrations/azure_devops/views/configuration.py +++ b/api/integrations/azure_devops/views/configuration.py @@ -1,8 +1,12 @@ import structlog +from rest_framework.exceptions import ValidationError from structlog.typing import FilteringBoundLogger +from integrations.azure_devops.client import list_projects +from integrations.azure_devops.client.exceptions import AzureDevOpsAuthError from integrations.azure_devops.models import AzureDevOpsConfiguration from integrations.azure_devops.serializers import ( + WRITE_ONLY_PLACEHOLDER, AzureDevOpsConfigurationSerializer, ) from integrations.common.views import ProjectIntegrationBaseViewSet @@ -10,6 +14,22 @@ logger = structlog.get_logger("azure_devops") +def _validate_pat_against_ado(*, organisation_url: str, pat: str) -> None: + """Probe ADO with a minimal request to confirm the PAT is valid. + + Raises ``ValidationError`` on 401/403 from ADO. Other failures (5xx, + network) bubble up so the caller can decide whether to log-and-allow + or surface as an error. + """ + try: + list_projects(organisation_url=organisation_url, pat=pat, top=1) + except AzureDevOpsAuthError: + raise ValidationError( + "Azure DevOps rejected the credentials. " + "Check the organisation URL and personal access token." + ) from None + + class AzureDevOpsConfigurationViewSet(ProjectIntegrationBaseViewSet): serializer_class = AzureDevOpsConfigurationSerializer # type: ignore[assignment] model_class = AzureDevOpsConfiguration # type: ignore[assignment] @@ -22,6 +42,13 @@ def _log_for(self, config: AzureDevOpsConfiguration) -> FilteringBoundLogger: ) def perform_create(self, serializer: AzureDevOpsConfigurationSerializer) -> None: # type: ignore[override] + # Surface the "already exists" error before making an external call. + if self.get_queryset().exists(): + raise ValidationError("This integration already exists for this project.") + _validate_pat_against_ado( + organisation_url=serializer.validated_data["organisation_url"], + pat=serializer.validated_data["personal_access_token"], + ) super().perform_create(serializer) instance: AzureDevOpsConfiguration = serializer.instance # type: ignore[assignment] self._log_for(instance).info( @@ -30,6 +57,18 @@ def perform_create(self, serializer: AzureDevOpsConfigurationSerializer) -> None ) def perform_update(self, serializer: AzureDevOpsConfigurationSerializer) -> None: # type: ignore[override] + pat = serializer.validated_data.get("personal_access_token") + # Treat the masked placeholder as "no change" — the serializer drops + # it during ``update`` so it never reaches the database, and there is + # nothing meaningful to validate against ADO either. + if pat is not None and pat != WRITE_ONLY_PLACEHOLDER: + _validate_pat_against_ado( + organisation_url=serializer.validated_data.get( + "organisation_url", + serializer.instance.organisation_url, # type: ignore[union-attr] + ), + pat=pat, + ) super().perform_update(serializer) instance: AzureDevOpsConfiguration = serializer.instance # type: ignore[assignment] self._log_for(instance).info( diff --git a/api/tests/unit/integrations/azure_devops/test_configuration.py b/api/tests/unit/integrations/azure_devops/test_configuration.py index 50b77963dbc1..c270d1e52efb 100644 --- a/api/tests/unit/integrations/azure_devops/test_configuration.py +++ b/api/tests/unit/integrations/azure_devops/test_configuration.py @@ -1,3 +1,4 @@ +import responses from pytest_structlog import StructuredLogCapture from rest_framework import status from rest_framework.test import APIClient @@ -6,12 +7,18 @@ from projects.models import Project +@responses.activate def test_create_configuration__valid_data__persists_and_masks_token( admin_client_new: APIClient, project: Project, log: StructuredLogCapture, ) -> None: # Given + responses.get( + "https://dev.azure.com/test-org/_apis/projects", + json={"value": [], "count": 0}, + status=200, + ) url = f"/api/v1/projects/{project.id}/integrations/azure-devops/" payload = { "organisation_url": "https://dev.azure.com/test-org", @@ -61,12 +68,18 @@ def test_create_configuration__already_exists__returns_400( assert response.status_code == status.HTTP_400_BAD_REQUEST +@responses.activate def test_create_configuration__after_soft_delete__undeletes_existing_row( admin_client_new: APIClient, project: Project, azure_devops_configuration: AzureDevOpsConfiguration, ) -> None: # Given + responses.get( + "https://dev.azure.com/recreated/_apis/projects", + json={"value": [], "count": 0}, + status=200, + ) original_pk = azure_devops_configuration.pk azure_devops_configuration.delete() url = f"/api/v1/projects/{project.id}/integrations/azure-devops/" @@ -105,6 +118,7 @@ def test_list_configuration__existing__returns_masked_representation( assert rows[0]["organisation_url"] == azure_devops_configuration.organisation_url +@responses.activate def test_update_configuration__valid_data__persists_and_masks_token( admin_client_new: APIClient, project: Project, @@ -112,6 +126,11 @@ def test_update_configuration__valid_data__persists_and_masks_token( log: StructuredLogCapture, ) -> None: # Given + responses.get( + "https://dev.azure.com/updated/_apis/projects", + json={"value": [], "count": 0}, + status=200, + ) detail_url = ( f"/api/v1/projects/{project.id}/integrations/azure-devops/" f"{azure_devops_configuration.id}/" @@ -234,3 +253,60 @@ def test_delete_configuration__non_admin__returns_403( # Then assert response.status_code == status.HTTP_403_FORBIDDEN + + +@responses.activate +def test_create_configuration__invalid_pat__returns_400( + admin_client_new: APIClient, + project: Project, +) -> None: + # Given + responses.get( + "https://dev.azure.com/test-org/_apis/projects", + json={}, + status=401, + ) + url = f"/api/v1/projects/{project.id}/integrations/azure-devops/" + payload = { + "organisation_url": "https://dev.azure.com/test-org", + "personal_access_token": "ado-bogus-token", + } + + # When + response = admin_client_new.post(url, data=payload, format="json") + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "Azure DevOps rejected" in str(response.json()) + assert not AzureDevOpsConfiguration.objects.filter(project=project).exists() + + +@responses.activate +def test_update_configuration__placeholder_pat__skips_validation( + admin_client_new: APIClient, + project: Project, + azure_devops_configuration: AzureDevOpsConfiguration, +) -> None: + # Given — no ADO probe response registered; if validation runs, the test + # will fail with a `responses.ConnectionError`. + detail_url = ( + f"/api/v1/projects/{project.id}/integrations/azure-devops/" + f"{azure_devops_configuration.id}/" + ) + payload = { + "organisation_url": azure_devops_configuration.organisation_url, + "personal_access_token": "write-only", + "labeling_enabled": True, + "tagging_enabled": True, + } + original_pat = azure_devops_configuration.personal_access_token + + # When + response = admin_client_new.put(detail_url, data=payload, format="json") + + # Then + assert response.status_code == status.HTTP_200_OK + azure_devops_configuration.refresh_from_db() + assert azure_devops_configuration.personal_access_token == original_pat + assert azure_devops_configuration.labeling_enabled is True + assert azure_devops_configuration.tagging_enabled is True From 6477b61301f4792fab7e760b3168a17d0f248181 Mon Sep 17 00:00:00 2001 From: "Asaph M. Kotzin" Date: Thu, 28 May 2026 13:40:18 +0100 Subject: [PATCH 10/11] style(tests): rename save-normalisation tests to satisfy FT003 The three trailing-slash normalisation tests added in the previous "normalise organisation_url on save" commit used 4 __-separated parts (subject__qualifier__condition__outcome) which violates the repo's FT003 lint rule. Rename to the canonical 3-part form using organisation_url as the subject and "on_save" baked into the outcome. Also commit the auto-regenerated events catalogue that lint produces when the new azure_devops.configuration.* events are detected. Co-Authored-By: Claude Opus 4.7 (1M context) --- api/tests/unit/integrations/azure_devops/test_models.py | 6 +++--- .../observability/_events-catalogue.md | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/api/tests/unit/integrations/azure_devops/test_models.py b/api/tests/unit/integrations/azure_devops/test_models.py index 5557cd8a9625..1940673ed8a4 100644 --- a/api/tests/unit/integrations/azure_devops/test_models.py +++ b/api/tests/unit/integrations/azure_devops/test_models.py @@ -158,7 +158,7 @@ def test_azure_devops_service_hook__different_event_type__allowed( @pytest.mark.django_db -def test_azure_devops_configuration__save__strips_single_trailing_slash( +def test_organisation_url__trailing_slash__stripped_on_save( project: Project, ) -> None: # Given @@ -176,7 +176,7 @@ def test_azure_devops_configuration__save__strips_single_trailing_slash( @pytest.mark.django_db -def test_azure_devops_configuration__save__strips_multiple_trailing_slashes( +def test_organisation_url__multiple_trailing_slashes__stripped_on_save( project: Project, ) -> None: # Given @@ -194,7 +194,7 @@ def test_azure_devops_configuration__save__strips_multiple_trailing_slashes( @pytest.mark.django_db -def test_azure_devops_configuration__save__no_trailing_slash__is_unchanged( +def test_organisation_url__no_trailing_slash__unchanged_on_save( project: Project, ) -> None: # Given diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index 186b853aea05..a9410a324ab0 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -42,7 +42,7 @@ Attributes: ### `azure_devops.configuration.created` Logged at `info` from: - - `api/integrations/azure_devops/views/configuration.py:27` + - `api/integrations/azure_devops/views/configuration.py:54` Attributes: - `ado.organisation.url` @@ -52,7 +52,7 @@ Attributes: ### `azure_devops.configuration.updated` Logged at `info` from: - - `api/integrations/azure_devops/views/configuration.py:35` + - `api/integrations/azure_devops/views/configuration.py:74` Attributes: - `ado.organisation.url` From b24ca9caf5cf40c68f5c084aad33ad47794f47fb Mon Sep 17 00:00:00 2001 From: "Asaph M. Kotzin" Date: Thu, 28 May 2026 13:55:13 +0100 Subject: [PATCH 11/11] fix(integrations): translate 5xx/network errors during PAT validation to 400 Also closes the diff-coverage gap on list_projects's continuation_token request parameter. Previously, if ADO replied 5xx during the create/update PAT probe (or the request failed with a network error), the underlying requests.HTTPError / requests.RequestException would bubble out and DRF would return 500. Translate to a ValidationError with a "could not be reached, please try again" message so the user gets an actionable 400. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../azure_devops/views/configuration.py | 14 +++++++--- .../integrations/azure_devops/test_client.py | 25 ++++++++++++++++++ .../azure_devops/test_configuration.py | 26 +++++++++++++++++++ .../observability/_events-catalogue.md | 4 +-- 4 files changed, 64 insertions(+), 5 deletions(-) diff --git a/api/integrations/azure_devops/views/configuration.py b/api/integrations/azure_devops/views/configuration.py index 00b6588331ee..a134a4f92648 100644 --- a/api/integrations/azure_devops/views/configuration.py +++ b/api/integrations/azure_devops/views/configuration.py @@ -1,3 +1,4 @@ +import requests import structlog from rest_framework.exceptions import ValidationError from structlog.typing import FilteringBoundLogger @@ -17,9 +18,11 @@ def _validate_pat_against_ado(*, organisation_url: str, pat: str) -> None: """Probe ADO with a minimal request to confirm the PAT is valid. - Raises ``ValidationError`` on 401/403 from ADO. Other failures (5xx, - network) bubble up so the caller can decide whether to log-and-allow - or surface as an error. + Raises ``ValidationError`` on 401/403 (bad credentials) and on any + other failure mode (5xx, network, malformed response). Surfacing a + user-facing 400 on transient failure is better than a 500 because the + user can act on the message; the persistence path should not silently + accept an unverified PAT either. """ try: list_projects(organisation_url=organisation_url, pat=pat, top=1) @@ -28,6 +31,11 @@ def _validate_pat_against_ado(*, organisation_url: str, pat: str) -> None: "Azure DevOps rejected the credentials. " "Check the organisation URL and personal access token." ) from None + except requests.RequestException: + raise ValidationError( + "Azure DevOps could not be reached to validate the credentials. " + "Please try again." + ) from None class AzureDevOpsConfigurationViewSet(ProjectIntegrationBaseViewSet): diff --git a/api/tests/unit/integrations/azure_devops/test_client.py b/api/tests/unit/integrations/azure_devops/test_client.py index 94d7f8d3c5d9..49011088fe62 100644 --- a/api/tests/unit/integrations/azure_devops/test_client.py +++ b/api/tests/unit/integrations/azure_devops/test_client.py @@ -172,3 +172,28 @@ def test_list_projects__trailing_slash_in_org_url__normalised_in_request() -> No # index returns a `Call`. Same workaround as other tests in this codebase. assert responses.calls[0].request.url is not None # type: ignore[union-attr] assert "//_apis" not in responses.calls[0].request.url # type: ignore[union-attr] + + +@responses.activate +def test_list_projects__continuation_token_param__sent_in_request_query() -> None: + # Given + responses.get( + f"{ORG_URL}/_apis/projects", + json={"value": [], "count": 0}, + match=[ + responses.matchers.query_param_matcher( + {"continuationToken": "ct-abc"}, + strict_match=False, + ) + ], + ) + + # When + list_projects( + organisation_url=ORG_URL, + pat=PAT, + continuation_token="ct-abc", + ) + + # Then — request landed on the matched URL with the expected query param + assert len(responses.calls) == 1 diff --git a/api/tests/unit/integrations/azure_devops/test_configuration.py b/api/tests/unit/integrations/azure_devops/test_configuration.py index c270d1e52efb..5585dc658e22 100644 --- a/api/tests/unit/integrations/azure_devops/test_configuration.py +++ b/api/tests/unit/integrations/azure_devops/test_configuration.py @@ -310,3 +310,29 @@ def test_update_configuration__placeholder_pat__skips_validation( assert azure_devops_configuration.personal_access_token == original_pat assert azure_devops_configuration.labeling_enabled is True assert azure_devops_configuration.tagging_enabled is True + + +@responses.activate +def test_create_configuration__ado_5xx__returns_400_with_retry_message( + admin_client_new: APIClient, + project: Project, +) -> None: + # Given + responses.get( + "https://dev.azure.com/test-org/_apis/projects", + json={}, + status=503, + ) + url = f"/api/v1/projects/{project.id}/integrations/azure-devops/" + payload = { + "organisation_url": "https://dev.azure.com/test-org", + "personal_access_token": "ado-test-token", + } + + # When + response = admin_client_new.post(url, data=payload, format="json") + + # Then + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "Azure DevOps could not be reached" in str(response.json()) + assert not AzureDevOpsConfiguration.objects.filter(project=project).exists() diff --git a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md index a9410a324ab0..56fdb499cb17 100644 --- a/docs/docs/deployment-self-hosting/observability/_events-catalogue.md +++ b/docs/docs/deployment-self-hosting/observability/_events-catalogue.md @@ -42,7 +42,7 @@ Attributes: ### `azure_devops.configuration.created` Logged at `info` from: - - `api/integrations/azure_devops/views/configuration.py:54` + - `api/integrations/azure_devops/views/configuration.py:62` Attributes: - `ado.organisation.url` @@ -52,7 +52,7 @@ Attributes: ### `azure_devops.configuration.updated` Logged at `info` from: - - `api/integrations/azure_devops/views/configuration.py:74` + - `api/integrations/azure_devops/views/configuration.py:82` Attributes: - `ado.organisation.url`