diff --git a/application/blueprints/datamanager/controllers/flagged_resources.py b/application/blueprints/datamanager/controllers/flagged_resources.py index 5671c2d..da3ec12 100644 --- a/application/blueprints/datamanager/controllers/flagged_resources.py +++ b/application/blueprints/datamanager/controllers/flagged_resources.py @@ -348,7 +348,12 @@ def _get_resource_organisation(resource, dataset_id): def _submit_assign_entities_request( - dataset_input, resource, organisation=None, return_endpoint=None + dataset_input, + resource, + organisation=None, + return_endpoint=None, + excluded_references=None, + selected_redirects=None, ): dataset_id, collection_id = _resolve_dataset_and_collection(dataset_input) organisation = organisation or _get_resource_organisation(resource, dataset_id) @@ -365,6 +370,10 @@ def _submit_assign_entities_request( params["organisation"] = organisation if return_endpoint: params["return_endpoint"] = return_endpoint + if excluded_references is not None: + params["excluded_references"] = excluded_references + if selected_redirects is not None: + params["selected_redirects"] = selected_redirects preview_id = submit_request(params) record_branch_baseline(preview_id, params["github_branch"]) return preview_id diff --git a/application/blueprints/datamanager/controllers/preview.py b/application/blueprints/datamanager/controllers/preview.py index 927b7d2..7d722bc 100644 --- a/application/blueprints/datamanager/controllers/preview.py +++ b/application/blueprints/datamanager/controllers/preview.py @@ -1,6 +1,5 @@ import json import logging -from datetime import date from flask import ( render_template, @@ -102,8 +101,17 @@ def _load_json_list(value: str | None) -> list: return loaded if isinstance(loaded, list) else [] -def build_old_entity_redirect_table(entity_redirects: list[dict]) -> dict | None: - if not entity_redirects: +def _count_excluded_references(params: dict) -> int: + references = params.get("excluded_references") or [] + if not isinstance(references, list): + return 0 + return len( + {str(reference).strip() for reference in references if str(reference).strip()} + ) + + +def build_old_entity_redirect_table(old_entity_rows: list[dict]) -> dict | None: + if not old_entity_rows: return None columns = [ @@ -115,22 +123,30 @@ def build_old_entity_redirect_table(entity_redirects: list[dict]) -> dict | None "entry-date", "start-date", ] - entry_date = date.today().isoformat() rows = [] - for redirect in entity_redirects: + for old_entity in old_entity_rows: + if not isinstance(old_entity, dict): + continue row = { - "old-entity": str(redirect.get("old_entity", "")), - "status": "301", - "entity": str(redirect.get("entity", "")), - "notes": str( - redirect.get("notes") - or "Redirect duplicate entity selected in Assign Entities" + "old-entity": str( + old_entity.get("old-entity", "") or old_entity.get("old_entity", "") + ), + "status": str(old_entity.get("status", "") or ""), + "entity": str(old_entity.get("entity", "") or ""), + "notes": str(old_entity.get("notes", "") or ""), + "end-date": str( + old_entity.get("end-date", "") or old_entity.get("end_date", "") + ), + "entry-date": str( + old_entity.get("entry-date", "") or old_entity.get("entry_date", "") + ), + "start-date": str( + old_entity.get("start-date", "") or old_entity.get("start_date", "") ), - "end-date": "", - "entry-date": entry_date, - "start-date": "", } rows.append({"columns": {c: {"value": row[c]} for c in columns}}) + if not rows: + return None return { "columns": columns, @@ -260,10 +276,13 @@ def handle_entities_preview(request_id, req): endpoints_to_retire = ( _load_json_list(request_meta.endpoints_to_retire) if request_meta else [] ) - entity_redirects = ( - _load_json_list(request_meta.entity_redirects) if request_meta else [] + old_entity_rows = pipeline_summary.get("old-entity") or [] + old_entity_redirect_table_params = build_old_entity_redirect_table(old_entity_rows) + old_entity_redirect_count = ( + len(old_entity_redirect_table_params["rows"]) + if old_entity_redirect_table_params + else 0 ) - old_entity_redirect_table_params = build_old_entity_redirect_table(entity_redirects) existing_endpoints = ( source_summary_data.get("existing_endpoint_for_organisation_dataset") or [] ) @@ -306,9 +325,10 @@ def handle_entities_preview(request_id, req): source_flow=source_flow, return_url=return_url, retire_summary=retire_summary, - entity_redirects=entity_redirects, old_entity_redirect_table_params=old_entity_redirect_table_params, - new_count=int(pipeline_summary.get("new-in-resource") or 0), + old_entity_redirect_count=old_entity_redirect_count, + new_count=len(new_entities), + excluded_count=_count_excluded_references(params), existing_count=int(pipeline_summary.get("existing-in-resource") or 0), endpoint_already_exists=endpoint_already_exists, endpoint_url=endpoint_url, @@ -337,9 +357,6 @@ def handle_add_data_confirm( endpoints_to_retire = ( _load_json_list(request_meta.endpoints_to_retire) if request_meta else [] ) - entity_redirects = ( - _load_json_list(request_meta.entity_redirects) if request_meta else [] - ) # Stale-assessment guard: if the config branch has advanced for this collection # since the assessment was taken, the assigned entity numbers may now collide. @@ -386,7 +403,6 @@ def handle_add_data_confirm( triggered_by=f"{session.get('user', {}).get('login', 'unknown')}", github_branch=github_branch, endpoints_to_retire=endpoints_to_retire, - entity_redirects=entity_redirects, ) except GitHubWorkflowError as e: logger.exception(f"GitHub async workflow error: {e}") diff --git a/application/blueprints/datamanager/controllers/transform.py b/application/blueprints/datamanager/controllers/transform.py index b6bec4a..f1322e4 100644 --- a/application/blueprints/datamanager/controllers/transform.py +++ b/application/blueprints/datamanager/controllers/transform.py @@ -220,6 +220,54 @@ def _build_entities_data(resp_details: list, platform_entities: list) -> dict: return {"columns": columns, "rows": rows} +def _normalise_excluded_references(excluded_references) -> set: + """Return explicitly excluded references from async request params. + + A missing or empty param means no references were excluded from assignment. + """ + if not excluded_references: + return set() + if not isinstance(excluded_references, list): + return set() + return { + str(reference).strip() + for reference in excluded_references + if str(reference).strip() + } + + +def _entity_selection_form_value(reference: str) -> str: + return reference + + +def _add_assign_entities_selection_metadata( + entities_data: dict, + excluded_references, +) -> dict: + """Add checkbox metadata to Assign Entities rows. + + Only rows categorised as new can be assigned entity numbers. Async request + params contain references excluded from assignment, so every selectable row + starts checked unless its reference is explicitly excluded. + """ + excluded_reference_set = _normalise_excluded_references(excluded_references) + + for row in entities_data.get("rows", []): + fields = row.get("fields") or {} + reference = str(fields.get("reference", "")).strip() + can_select = row.get("category") == "new" and bool(reference) + selected = can_select and reference not in excluded_reference_set + row["entity_selection"] = { + "can_select": can_select, + "selected": selected, + "form_value": ( + _entity_selection_form_value(reference) if reference else "" + ), + } + + return entities_data + + def _entity_row_matches_search(row: dict, search_query: str) -> bool: if not search_query: return True @@ -235,26 +283,44 @@ def _entity_row_matches_filter(row: dict, category_filter: str) -> bool: return row.get("category") == category_filter -def _name_similarity_percent(value) -> float: - if value in (None, ""): - return 0 - try: - similarity = float(str(value).strip().rstrip("%")) - except (TypeError, ValueError): - return 0 - return similarity * 100 if 0 < similarity <= 1 else similarity +def _dedup_candidate_redirect_key(candidate: dict) -> tuple[str, str]: + return ( + str(candidate.get("old_entity", "") or "").strip(), + str(candidate.get("entity", "") or "").strip(), + ) -def _dedup_candidate_auto_select(candidate: dict) -> bool: - if candidate.get("old_entity_redirects"): - return False +def _old_entity_redirect_key(row: dict) -> tuple[str, str]: + return ( + str(row.get("old-entity", "") or row.get("old_entity", "") or "").strip(), + str(row.get("entity", "") or "").strip(), + ) - match_type = str(candidate.get("match_type", "")).replace(" ", "_").lower() - if match_type == "complete_match": - return True - if match_type != "single_match": - return False - return _name_similarity_percent(candidate.get("name_similarity")) > 85 + +def _dedup_candidate_selected_entity_reference(candidate: dict) -> str: + return str( + candidate.get("new_reference", "") or candidate.get("reference", "") or "" + ).strip() + + +def _dedup_candidate_selected_redirect_key(candidate: dict) -> tuple[str, str]: + """Return the key used to compare a Dedup candidate with selected_redirects.""" + return ( + _dedup_candidate_selected_entity_reference(candidate), + str(candidate.get("old_entity", "") or "").strip(), + ) + + +def _selected_redirect_key(redirect: dict) -> tuple[str, str]: + """Return the async selected_redirects key for a submitted redirect param.""" + return ( + str(redirect.get("reference", "") or "").strip(), + str( + redirect.get("old_entity_number", "") + or redirect.get("old_entity", "") + or "" + ).strip(), + ) def _dedup_candidate_form_value(candidate: dict) -> str: @@ -276,11 +342,46 @@ def _dedup_candidate_form_value(candidate: dict) -> str: ) -def _prepare_duplicate_candidates(candidates: list[dict]) -> list[dict]: +def _prepare_duplicate_candidates( + candidates: list[dict], + old_entity_rows: list[dict] | None = None, + organisation: str = "", + excluded_references=None, + selected_redirects=None, +) -> list[dict]: + """Prepare Dedup candidates for rendering and selection. + + Async's ``pipeline-summary.old-entity`` is the source of initial redirect + preselection. Rows present in old-entity but absent from request + ``selected_redirects`` are inferred to be async auto-selected and are locked + in the UI so users cannot untick redirects generated by async policy. + """ + preselected_redirects = { + key + for key in ( + _old_entity_redirect_key(row) + for row in (old_entity_rows or []) + if isinstance(row, dict) + ) + if all(key) + } + selected_redirect_keys = { + _selected_redirect_key(redirect) + for redirect in (selected_redirects or []) + if isinstance(redirect, dict) and all(_selected_redirect_key(redirect)) + } + excluded_reference_set = _normalise_excluded_references(excluded_references) return [ { **candidate, - "auto_select": _dedup_candidate_auto_select(candidate), + "auto_select": _dedup_candidate_redirect_key(candidate) + in preselected_redirects, + "redirect_locked": _dedup_candidate_redirect_key(candidate) + in preselected_redirects + and _dedup_candidate_selected_redirect_key(candidate) + not in selected_redirect_keys, + "redirect_can_select": _dedup_candidate_selected_entity_reference(candidate) + not in excluded_reference_set, "form_value": _dedup_candidate_form_value(candidate), } for candidate in candidates @@ -337,9 +438,12 @@ def _paginate_entity_data( entity_page: int, entity_search: str, entity_filter: str = "", + include_selection: bool = False, + excluded_references=None, ) -> tuple: entity_start_offset = (entity_page - 1) * _ROWS_PER_PAGE entities_data_full = _build_entities_data(all_resp_details, platform_entities) + # Counts cover every entity, independent of the current search/filter, so the # summary boxes always show the full picture. category_counts = _count_categories(entities_data_full["rows"]) @@ -366,6 +470,11 @@ def _paginate_entity_data( "columns": entities_data_full["columns"], "rows": entity_page_rows, } + if include_selection: + entities_data = _add_assign_entities_selection_metadata( + entities_data, + excluded_references, + ) return ( entities_data, has_next_entity_page, @@ -618,6 +727,7 @@ def handle_check_transform( """ params = req.get("params") or {} organisation_code = params.get("organisationName") or params.get("organisation", "") + excluded_references = params.get("excluded_references") dataset_id = params.get("dataset", "") is_assign_entities = transform_endpoint == "assign_entities.flagged_resource_detail" resource_hash = params.get("resource", "") @@ -664,7 +774,11 @@ def handle_check_transform( pipeline_summary = response_data.get("pipeline-summary") or {} show_dedup_tab = is_assign_entities and dataset_id == "conservation-area" duplicate_candidates = _prepare_duplicate_candidates( - pipeline_summary.get("duplicate-candidates") or [] if show_dedup_tab else [] + pipeline_summary.get("duplicate-candidates") or [] if show_dedup_tab else [], + pipeline_summary.get("old-entity") or [], + organisation=organisation_code, + excluded_references=excluded_references, + selected_redirects=params.get("selected_redirects"), ) # Calculate pagination for transformed facts and issue logs, and for entities. @@ -693,6 +807,8 @@ def handle_check_transform( entity_page, entity_search, entity_filter, + include_selection=is_assign_entities, + excluded_references=excluded_references, ) transformed_table = _build_transform_table(resp_details) issue_log_table = _build_issue_log_table(resp_details) @@ -746,6 +862,7 @@ def handle_check_transform( endpoint_url=endpoint_url, documentation_url=documentation_url, resource_hash=resource_hash, + is_assign_entities=is_assign_entities, show_dedup_tab=show_dedup_tab, duplicate_candidates=duplicate_candidates, planning_entity_base_url=( diff --git a/application/blueprints/datamanager/router.py b/application/blueprints/datamanager/router.py index 1578524..69b83fe 100644 --- a/application/blueprints/datamanager/router.py +++ b/application/blueprints/datamanager/router.py @@ -25,6 +25,7 @@ ) from .controllers.flagged_resources import ( REQUIRED_COLUMNS, + _submit_assign_entities_request, handle_flagged_resource_detail, handle_flagged_resource_submit, handle_flagged_resources_import, @@ -311,24 +312,105 @@ def flagged_resource_detail(request_id): return render_template("datamanager/error.html", message=e.message) +def _normalise_reference_values(values): + """Return submitted reference values as a de-duplicated, ordered list.""" + references = [] + seen = set() + for value in values or []: + reference = str(value or "").strip() + if not reference or reference in seen: + continue + references.append(reference) + seen.add(reference) + return references + + +def _merge_visible_excluded_references( + current_excluded_references, + visible_references, + selected_visible_references, +): + """Merge current exclusions with checkbox state from the visible page.""" + current_excluded = set(_normalise_reference_values(current_excluded_references)) + visible = set(_normalise_reference_values(visible_references)) + selected_visible = set(_normalise_reference_values(selected_visible_references)) + excluded = (current_excluded - visible) | (visible - selected_visible) + return sorted(excluded) + + +def _selected_redirects_for_async(redirects, excluded_references=None): + """Map validated Dedup rows to async selected_redirects params. + + Config-manager's Dedup form values use old/new entity field names, while + async expects ``reference`` and ``old_entity_number``. Redirects for + explicitly excluded references are dropped because async can only redirect + entities being assigned. + """ + excluded_references = set(_normalise_reference_values(excluded_references)) + selected_redirects = [] + seen = set() + for redirect_row in redirects: + reference = str( + redirect_row.get("new_reference") or redirect_row.get("reference") or "" + ).strip() + old_entity_number = str( + redirect_row.get("old_entity") + or redirect_row.get("old_entity_number") + or "" + ).strip() + if reference in excluded_references: + continue + key = (reference, old_entity_number) + if not all(key) or key in seen: + continue + selected_redirects.append( + { + "reference": reference, + "old_entity_number": old_entity_number, + } + ) + seen.add(key) + return selected_redirects + + def flagged_resource_detail_post(request_id): req = fetch_request(request_id) + params = req.get("params") or {} response_data = (req.get("response") or {}).get("data") or {} pipeline_summary = response_data.get("pipeline-summary") or {} + organisation = params.get("organisation") or params.get("organisationName") or None duplicate_candidates = pipeline_summary.get("duplicate-candidates") or [] redirects = parse_selected_redirects( request.form.getlist("entity_redirects"), duplicate_candidates ) - meta = db.session.get(RequestMeta, request_id) - if meta is None: - meta = RequestMeta( - request_id=request_id, - entity_redirects=json.dumps(redirects), + if request.form.get("entity_selection_changed") == "true": + excluded_references = _merge_visible_excluded_references( + params.get("excluded_references") or [], + request.form.getlist("visible_entity_references"), + request.form.getlist("selected_entity_references"), ) - db.session.add(meta) - else: - meta.entity_redirects = json.dumps(redirects) - db.session.commit() + try: + new_request_id = _submit_assign_entities_request( + params.get("dataset", ""), + params.get("resource", ""), + organisation=organisation, + return_endpoint=params.get("return_endpoint") + or "assign_entities.flagged_resources_start", + excluded_references=excluded_references, + selected_redirects=_selected_redirects_for_async( + redirects, excluded_references + ), + ) + except AsyncAPIError as e: + raise ControllerError( + f"Assign entities submission failed: {e.detail}" + ) from e + return redirect( + url_for( + "assign_entities.flagged_resource_detail", request_id=new_request_id + ) + ) + return redirect(url_for("datamanager.entities_preview", request_id=request_id)) diff --git a/application/blueprints/datamanager/services/github.py b/application/blueprints/datamanager/services/github.py index c98bc9f..fff8eb1 100644 --- a/application/blueprints/datamanager/services/github.py +++ b/application/blueprints/datamanager/services/github.py @@ -2,7 +2,6 @@ GitHub App service for authenticating and triggering workflows. """ -import json import time import logging import requests @@ -279,7 +278,6 @@ def trigger_add_data_async_workflow( triggered_by: str = "config-manager", github_branch: str = None, endpoints_to_retire: list = None, - entity_redirects: list = None, ) -> dict: """ Trigger the 'add-data-async-script' workflow in the digital-land/config repository. @@ -299,11 +297,6 @@ def trigger_add_data_async_workflow( "retire_endpoints": ( ",".join(endpoints_to_retire or []) if endpoints_to_retire else "" ), - "entity_redirects": ( - json.dumps(entity_redirects, separators=(",", ":")) - if entity_redirects - else "" - ), "environment": current_app.config.get("ENVIRONMENT"), }, } diff --git a/application/db/models.py b/application/db/models.py index 2186cc7..c443058 100644 --- a/application/db/models.py +++ b/application/db/models.py @@ -448,7 +448,6 @@ class RequestMeta(db.Model): endpoints_to_retire = db.Column( db.Text, nullable=True ) # JSON list of endpoint hashes - entity_redirects = db.Column(db.Text, nullable=True) # JSON list of old-entity rows branch_sha = db.Column( db.Text, nullable=True ) # config-manager-update HEAD SHA when the assessment was submitted diff --git a/application/templates/components/check-transform-base.html b/application/templates/components/check-transform-base.html index 6014bba..d683c59 100644 --- a/application/templates/components/check-transform-base.html +++ b/application/templates/components/check-transform-base.html @@ -131,6 +131,18 @@

{% block heading %}{% + {% if is_assign_entities %} + + {% endif %} {% for col in entities_data.columns %} {% endfor %} @@ -143,6 +155,22 @@

{% block heading %}{% {% elif row.category == 'new' %}{% set _row_bg = '#d4edda' %} {% else %}{% set _row_bg = '#d0e4f7' %}{% endif %}

+ {% if is_assign_entities %} + {% set selection = row.entity_selection or {} %} + + {% endif %} {% for col in entities_data.columns %} {% set _changed = row.changed_fields and col in row.changed_fields %}
+
+
+ + +
+
+ 		{{ col }}
+
+
+ {% if selection.can_select %} + + {% endif %} + + +
+
+ 		@@ -155,6 +183,17 @@

{% block heading %}{%

+ {% if is_assign_entities %} + {% set selected_entity_count = entities_data.rows | selectattr('entity_selection.selected') | list | length %} + {% set selectable_entity_count = entities_data.rows | selectattr('entity_selection.can_select') | list | length %} +

+ {% if selectable_entity_count %} + {{ selected_entity_count }} of {{ selectable_entity_count }} {{ 'entity' if selectable_entity_count == 1 else 'entities' }} selected for assignment + {% else %} + No entities available for assignment + {% endif %} +

+ {% endif %} {% elif entity_search or entity_filter %}

No entities match your search or filter.

{% else %} @@ -330,6 +369,135 @@

{% block heading %}{% {% endblock %} {% block pageScripts %}{{ super() }} + + {% if geometries %} {% endblock %} diff --git a/application/templates/datamanager/entities_preview.html b/application/templates/datamanager/entities_preview.html index 4a11abb..ae86b66 100644 --- a/application/templates/datamanager/entities_preview.html +++ b/application/templates/datamanager/entities_preview.html @@ -16,7 +16,7 @@

Preview and Confirm

Entity Summary

-
+
@@ -30,6 +30,14 @@

Entity Summary

{{ new_count }}
+ {% if source_flow == 'assign_entities' %} +
+
+ Rows that will NOT create new entities +
+
{{ excluded_count }}
+
+ {% endif %}
@@ -139,8 +147,8 @@

column.csv (new mappings)

Entity Org Summary

-

entity-organisation.csv

{% if entity_org_warning %} +

entity-organisation.csv

@@ -149,16 +157,19 @@

entity-organisation.csv
{% elif entity_org_overlap_info %} +

entity-organisation.csv

{{ entity_org_overlap_info }}
{% elif entity_org_error_warning %} +

entity-organisation.csv

{% elif has_entity_org and entity_org_table_params %} +

entity-organisation.csv

{{ table(entity_org_table_params) }}
@@ -199,6 +210,17 @@

Retire Summary

Old Entity Summary

+
+
+
+
+ Number of redirects +
+
{{ old_entity_redirect_count }}
+
+
+
+

old-entity.csv

{{ table(old_entity_redirect_table_params) }} diff --git a/docs/assign-entities/architecture.md b/docs/assign-entities/architecture.md new file mode 100644 index 0000000..0034bf6 --- /dev/null +++ b/docs/assign-entities/architecture.md @@ -0,0 +1,360 @@ +# Assign Entities - Architecture + +This document describes the Assign Entities process end to end: how config-manager starts async +processing, how entity and redirect selections are represented, what the preview shows, which +async result fields become config rows, and where to look when the flow behaves unexpectedly. + +Assign Entities is not a separate async request type. It is a specialised config-manager journey +that submits an async `add_data` request for an existing resource hash, then reuses the add-data +preview and GitHub commit flow once async has produced the final config rows. + +--- + +## Repositories involved + +| Repo | Responsibility | +| --- | --- | +| `config-manager` | User journey, selection UI, validation, async request creation, preview, GitHub dispatch | +| `async-request-backend` | Processes the resource, filters selected entities, assigns entity numbers, generates old-entity rows | +| `digital-land/config` | Fetches the completed async request and appends returned CSV rows | + +config-manager does not calculate entity number ranges and does not create `old-entity.csv` rows +itself. It passes selected references to async, then displays and commits whatever async returns. + +--- + +## Code map + +| Area | File | Role | +| --- | --- | --- | +| Routes | `application/blueprints/datamanager/router.py` | Assign Entities routes, selection POST handling, replacement async request | +| Start/import | `application/blueprints/datamanager/controllers/flagged_resources.py` | Direct resource/dataset entry, flagged-resources CSV import, async request submission | +| Results page | `application/blueprints/datamanager/controllers/transform.py` | Builds Entities and Dedup tab data from async response | +| Shared results template | `application/templates/components/check-transform-base.html` | Entities tab table, entity checkboxes, selection count, shared button state | +| Dedup template | `application/templates/datamanager/assign-entities-check-results.html` | Dedup tab, duplicate redirect checkboxes, hidden changed flag | +| Preview | `application/blueprints/datamanager/controllers/preview.py` | Displays async-generated lookup, entity-organisation, and old-entity rows | +| GitHub dispatch | `application/blueprints/datamanager/services/github.py` | Triggers the config repo workflow with the completed async request id | +| Redirect parsing | `application/blueprints/datamanager/services/duplicates.py` | Validates submitted Dedup checkbox values against async candidates | + +--- + +## Full process + +### 1. Start Assign Entities + +The user starts at `/assign-entities/` by entering a dataset/resource pair, or by uploading/pasting +a flagged-resources CSV. + +`controllers/flagged_resources.py` resolves: + +- `dataset` +- `collection` +- `resource` +- resource `organisation`, from resource metadata when possible +- shared `github_branch` from `CONFIG_REPO_BRANCH` + +It then submits an async request using `_submit_assign_entities_request`. + +```json +{ + "type": "add_data", + "resource": "resource-hash", + "dataset": "conservation-area", + "collection": "conservation-area", + "authoritative": true, + "github_branch": "config-manager-update", + "organisationName": "local-authority:ABC", + "organisation": "local-authority:ABC", + "return_endpoint": "assign_entities.flagged_resources_start" +} +``` + +At this point no explicit selection is sent, so async treats the request as "assign all new +entities". + +### 2. Show async result + +`router.py` fetches the request from async and delegates to `handle_check_transform` in +`controllers/transform.py`. + +The page is rendered from: + +- `response-details` rows fetched by `fetch_response_details(request_id)` +- platform entities from the planning data API, for comparison and row categories +- `response-details.transformed_row`, for Assign Entities rows +- request param `excluded_references`, for Assign Entities checked state +- `pipeline-summary.duplicate-candidates`, for the Dedup tab +- `pipeline-summary.old-entity`, for preselected duplicate redirects + +### 3. Select entities + +The Entities tab includes a checkbox column only in the Assign Entities flow. + +| Row category | Meaning | Checkbox | +| --- | --- | --- | +| `new` | Reference is not already on the platform | enabled | +| `changed` | Entity exists on the platform but differs from the resource | disabled | +| `in_both` | Entity exists and matches | disabled | +| `existing` | Entity exists on the platform only | disabled | + +Each enabled checkbox submits the row reference as its value: + +```text +REF-1 +``` + +The reference is the selection identity. Entity numbers are not used as the selection key because +the point of the flow is to decide which references should receive numbers. + +Initial checkbox state comes from async request params: + +- missing or empty `excluded_references`: all selectable `new` rows are checked +- non-empty `excluded_references`: matching references are unchecked + +Search and pagination render only a subset of rows. To avoid losing selections from other pages, +the template posts the visible row values separately and `router.py` merges the visible changes back +into the current full selection before it submits a replacement async request. + +### 4. Select duplicate redirects + +The Dedup tab is currently shown for Assign Entities `conservation-area` requests. + +Candidate rows come from `pipeline-summary.duplicate-candidates`. Initial checked state comes from +`pipeline-summary.old-entity`, not from a score threshold in config-manager. + +When an entity is not selected, any duplicate checkbox for that entity is disabled. This matters +because async can only redirect old entity numbers to new entity numbers it is assigning in the +same processed selection. + +Rows in `pipeline-summary.old-entity` that are not present in request param `selected_redirects` +are treated as async auto-selected. Those checkboxes stay checked and locked in the UI so the user +cannot remove redirects generated by async policy. + +### 5. Process entities + +Changing either the Entities tab or the Dedup tab changes the submit button text to +**Process entities**. + +On POST, `flagged_resource_detail_post`: + +1. Fetches the current async response. +2. Parses selected entity checkbox reference values. +3. Calculates excluded references from the visible rows and current request params. +4. Parses selected Dedup checkbox values against `duplicate-candidates`. +5. Merges visible entity selections with the current excluded references. +6. Filters Dedup redirects to exclude references that will not receive entity numbers. +7. Submits a replacement async request with `excluded_references` and `selected_redirects`. +8. Redirects to the new request's Assign Entities check-results page. + +The replacement request looks like: + +```json +{ + "type": "add_data", + "resource": "resource-hash", + "dataset": "conservation-area", + "collection": "conservation-area", + "authoritative": true, + "github_branch": "config-manager-update", + "organisationName": "local-authority:ABC", + "organisation": "local-authority:ABC", + "return_endpoint": "assign_entities.flagged_resources_start", + "excluded_references": [ + "REF-2" + ], + "selected_redirects": [ + { + "reference": "REF-1", + "old_entity_number": "100" + } + ] +} +``` + +`excluded_references` contains only references that should not receive entity numbers. +An empty list means nothing was excluded from assignment. + +### 6. Preview + +Once the user continues from the refreshed Assign Entities result, the add-data preview shows the +completed async output. It does not recalculate Assign Entities rows. + +| Preview section | Source | +| --- | --- | +| Rows that will create new entities | `pipeline-summary.new-entities` | +| Rows that will NOT create new entities | `params.excluded_references` | +| Entity organisation CSV | `pipeline-summary.entity-organisation` | +| Old Entity Summary / `old-entity.csv` | `pipeline-summary.old-entity` | + +The "Number of redirects" value is the number of rendered `old-entity` rows. + +### 7. Confirm and commit + +On confirmation, config-manager calls `trigger_add_data_async_workflow` with: + +- `request_id` +- `triggered_by` +- `github_branch` +- `retire_endpoints` +- `environment` + +The config repo workflow uses the `request_id` to fetch the completed async result and appends: + +- `pipeline/{collection}/lookup.csv` from `pipeline-summary.new-entities` +- `pipeline/{collection}/entity-organisation.csv` from `pipeline-summary.entity-organisation` +- `pipeline/{collection}/old-entity.csv` from `pipeline-summary.old-entity` + +--- + +## Entity number behaviour + +Entity number generation happens in async, before the result is returned to config-manager. + +Selection is applied before async adds lookup entries, so excluded references do not consume +numbers. That means selection should not create gaps in the assigned range. If gaps appear in a +preview, debug async's generated `pipeline-summary.new-entities` and the branch it assessed +against, not config-manager's preview code. + +`entity-organisation` is also async-owned. config-manager only renders the returned +`pipeline-summary.entity-organisation` rows and the config repo appends those rows when +authoritative and valid. + +--- + +## Gotchas + +### Empty excluded references means assign all + +Async treats missing or empty `excluded_references` as "exclude nothing". The UI can send +an empty list when every selectable reference is selected. + +### Transformed rows vs `new-entities` + +The Entities tab renders from transformed rows and platform comparison data. It expects async to +return transformed rows for all resource entities, selected or not. The preview uses +`pipeline-summary.new-entities`, which should contain only the rows async will actually assign. + +### Existing rows are visible but not selectable + +Existing, changed, and in-both rows keep their row category and colour semantics. Their checkboxes +are disabled because config-manager must only submit references eligible for new entity numbers. + +### Dedup selection depends on entity selection + +A Dedup checkbox can be disabled even when it is a good duplicate candidate. If the corresponding +new entity is not selected, the redirect cannot be submitted because async will not assign that new +entity number. + +### Auto-selected redirects are inferred + +config-manager treats old-entity rows returned by async but absent from request param +`selected_redirects` as auto-selected. Those rows are locked in the UI. This is an inference from +the async result and params, not a separate stored flag in config-manager. + +### Button text is stateful + +The shared results page changes "Continue to preview" to "Process entities" when either entity +selection or Dedup selection changes. If this does not happen, inspect the hidden +`entity_selection_changed` input and the checkbox data attributes in the rendered HTML. + +### Preview is not the selection editor + +The preview only shows async output. If a row is missing from preview, go back to the Assign +Entities check-results page and process a new selection. The preview should not be used to add or +remove selected references. + +### Branch state can make results stale + +Assign Entities uses the same stale-assessment protection as add-data. If the shared config branch +moves for the collection between assessment and confirmation, the confirm page can block and ask +the user to re-run. + +--- + +## Debugging + +### The page shows all entities selected after processing + +Check the replacement async request params: + +- Does `excluded_references` contain the references that were unchecked? +- Is it a list of reference strings? +- Do the reference values match the transformed row references? + +If `excluded_references` is missing or empty, async will process all new entities. + +### A selected row disappeared from the Entities tab + +Check whether async returned the row in `response-details.transformed_row`. The Assign Entities tab +expects transformed rows for all resource entities, including excluded candidates. The preview is +allowed to contain only assigned rows in `pipeline-summary.new-entities`. + +### A selected row does not appear in preview + +Inspect the completed async response: + +- `pipeline-summary.new-entities` +- `pipeline-summary.entity-organisation` +- `pipeline-summary.old-entity` +- `response.error` + +The preview uses those fields directly. If the row is not there, the issue is earlier than preview. + +### Dedup checkbox is disabled + +Check: + +- The candidate's `new_reference` or `reference` +- The request param `excluded_references` +- The rendered row's `redirect_can_select` +- Whether the row is locked because it is in `pipeline-summary.old-entity` but absent from + `selected_redirects` + +### Dedup selection was not sent to async + +Check the POST body from the Assign Entities page: + +- `entity_selection_changed` should be `true` +- selected duplicate checkboxes should post as `entity_redirects` +- parsed redirects must match a row in `pipeline-summary.duplicate-candidates` +- redirects for excluded references are deliberately filtered out + +### Preview old-entity count is wrong + +The count is the number of rows rendered from `pipeline-summary.old-entity`. Check the async result +first. + +### GitHub PR does not contain old-entity rows + +Check `pipeline-summary.old-entity` in the completed async result fetched by the config repo +workflow. + +### Entity numbers look stale or collide + +Check: + +- `github_branch` sent in the async request +- the `RequestMeta.branch_sha` baseline captured for the request +- whether the confirm-time stale check blocked or was skipped +- whether another add-data workflow changed `pipeline/{collection}/` while the page was open + +--- + +## Useful focused tests + +```bash +./.venv/bin/pytest tests/unit/blueprints/datamanager/controllers/test_transform.py -q +./.venv/bin/pytest tests/unit/blueprints/datamanager/controllers/test_add.py -q +./.venv/bin/pytest tests/acceptance/blueprints/datamanager/test_flagged_resources.py -q +``` + +Use the focused tests when changing the selection or preview flow. Run the broader datamanager +suite before merging changes that touch shared templates or GitHub dispatch behaviour. + +--- + +## Related + +- [Datamanager GitHub workflow](../datamanager/github-add.md) +- [Datamanager stale-assessment check](../datamanager/stale-check.md) +- [Datamanager blueprint architecture](../datamanager/architecture.md) diff --git a/docs/datamanager/architecture.md b/docs/datamanager/architecture.md index 00811fc..f581210 100644 --- a/docs/datamanager/architecture.md +++ b/docs/datamanager/architecture.md @@ -7,7 +7,8 @@ This document describes the structure of the `datamanager` blueprint and how to ## Directory layout > Docs for this blueprint live in `docs/datamanager/` (this file, plus `github-add.md` and -> `stale-check.md`). +> `stale-check.md`). Assign Entities has its own workflow docs in +> `docs/assign-entities/architecture.md`. ``` application/blueprints/datamanager/ diff --git a/docs/datamanager/github-add.md b/docs/datamanager/github-add.md index 4cb4691..6ddc7c3 100644 --- a/docs/datamanager/github-add.md +++ b/docs/datamanager/github-add.md @@ -34,7 +34,6 @@ The trigger lives in `services/github.py` (`trigger_add_data_async_workflow`, ca "triggered_by": "your-name-or-system", "branch": "config-manager-update", "retire_endpoints": "hash1,hash2", - "entity_redirects": "[{\"old_entity\":\"100\",\"entity\":\"200\"}]", "environment": "production" } } @@ -47,7 +46,6 @@ The trigger lives in `services/github.py` (`trigger_add_data_async_workflow`, ca | `client_payload.triggered_by` | no | Who/what triggered it (used in commit/PR content) | | `client_payload.branch` | no | Target branch — see [Branch behaviour](#branch-behaviour) | | `client_payload.retire_endpoints` | no | Comma-separated endpoint hashes to end-date | -| `client_payload.entity_redirects` | no | JSON list of old→new entity redirects | | `client_payload.environment` | no | Async API environment: `development` \| `staging` \| `production` (default `staging`) | The branch config-manager sends is its `CONFIG_REPO_BRANCH` setting — `config-manager-update` in @@ -111,7 +109,7 @@ URL is resolved from the `environment` in the payload: "data": { "endpoint-summary": { "new_endpoint_entry": {}, "endpoint_url_in_endpoint_csv": false }, "source-summary": { "new_source_entry": {}, "documentation_url_in_source_csv": false }, - "pipeline-summary": { "new-entities": [], "entity-organisation": [] } + "pipeline-summary": { "new-entities": [], "entity-organisation": [], "old-entity": [] } }, "error": null } @@ -127,7 +125,7 @@ URL is resolved from the `environment` in the payload: | `pipeline/{collection}/lookup.csv` | `pipeline-summary.new-entities` | array non-empty | | `pipeline/{collection}/column.csv` | `params.column_mapping` | mapping non-empty | | `pipeline/{collection}/entity-organisation.csv` | `pipeline-summary.entity-organisation` | `params.authoritative` true, and not an overlap/error | -| `pipeline/{collection}/old-entity.csv` | `client_payload.entity_redirects` | redirects provided | +| `pipeline/{collection}/old-entity.csv` | `pipeline-summary.old-entity` | array non-empty | Retiring endpoints (`retire_endpoints`) does not append rows — it sets `end-date` on the matching rows in `collection/{collection}/endpoint.csv` and `source.csv`. @@ -148,4 +146,6 @@ The workflow fails if `request_id` is empty, the request cannot be fetched, its - [stale-check.md](stale-check.md) — how config-manager avoids committing stale entity numbers when the branch advances between assessment and confirmation. +- [Assign Entities architecture](../assign-entities/architecture.md) — how Assign Entities + selections are sent to async. - [architecture.md](architecture.md) — the datamanager blueprint structure. diff --git a/migrations/versions/7b8c9d0e1f2a_drop_request_meta_entity_redirects.py b/migrations/versions/7b8c9d0e1f2a_drop_request_meta_entity_redirects.py new file mode 100644 index 0000000..1cc5da5 --- /dev/null +++ b/migrations/versions/7b8c9d0e1f2a_drop_request_meta_entity_redirects.py @@ -0,0 +1,26 @@ +"""drop request_meta entity_redirects column + +Revision ID: 7b8c9d0e1f2a +Revises: f6a7b8c9d0e1 +Create Date: 2026-07-22 00:00:00.000000 + +""" + +import sqlalchemy as sa +from alembic import op + +revision = "7b8c9d0e1f2a" +down_revision = "f6a7b8c9d0e1" +branch_labels = None +depends_on = None + + +def upgrade(): + op.drop_column("request_meta", "entity_redirects") + + +def downgrade(): + op.add_column( + "request_meta", + sa.Column("entity_redirects", sa.Text(), nullable=True), + ) diff --git a/tests/acceptance/blueprints/datamanager/test_flagged_resources.py b/tests/acceptance/blueprints/datamanager/test_flagged_resources.py index 2134c0e..287d790 100644 --- a/tests/acceptance/blueprints/datamanager/test_flagged_resources.py +++ b/tests/acceptance/blueprints/datamanager/test_flagged_resources.py @@ -1,4 +1,5 @@ import json +import re from datetime import datetime from io import BytesIO from unittest.mock import patch @@ -6,13 +7,23 @@ import responses as rsps from application.blueprints.base.views import ADD_DATA_LOCK, ASSIGN_ENTITIES_LOCK -from application.db.models import RequestMeta, ServiceLock +from application.db.models import ServiceLock from application.extensions import db from config.config import get_request_api_endpoint ASYNC_BASE = f"{get_request_api_endpoint()}/requests" +def _selected_entity_checkbox(response_data, reference): + response_text = response_data.decode() + match = re.search( + rf']*name="selected_entity_references"[^>]*value="{re.escape(reference)}"[^>]*>', + response_text, + ) + assert match, f"Could not find selected_entity_references checkbox for {reference}" + return match.group(0) + + CSV_INPUT = ( "dataset,resource,organisation,reference,status,entities_created,error_code,message\n" "tree,resource-a,local-authority:ABC,ref-1,error,12%,LARGE_NUMBER_OF_NEW_ENTITIES,Entity growth is 12%\n" @@ -382,7 +393,19 @@ def test_assign_entities_check_results_does_not_show_retire_endpoints(client): "source-summary": { "existing_endpoint_for_organisation_dataset": ["endpoint-a"] }, - "pipeline-summary": {"new-in-resource": 1}, + "pipeline-summary": { + "new-in-resource": 2, + "new-entities": [ + { + "organisation": "local-authority:ABC", + "reference": "ref-1", + }, + { + "organisation": "local-authority:ABC", + "reference": "ref-2", + }, + ], + }, } }, }, @@ -466,6 +489,106 @@ def test_assign_entities_check_results_does_not_show_retire_endpoints(client): assert b"retire_endpoints" not in response.data assert b'action="/assign-entities/check-results/assign-id-1"' in response.data assert b'form="duplicate-redirect-form"' in response.data + assert b"entity-select-all" in response.data + assert b'name="selected_entity_references"' in response.data + assert b'value="ref-2" form="duplicate-redirect-form" checked' in response.data + assert b"1 of 1 entity selected for assignment" in response.data + + +@rsps.activate +def test_assign_entities_check_results_uses_excluded_references_param(client): + rsps.add( + rsps.GET, + f"{ASYNC_BASE}/assign-selected-id", + json={ + "status": "COMPLETE", + "params": { + "dataset": "tree", + "organisation": "local-authority:ABC", + "resource": "resource-a", + "excluded_references": ["ref-1"], + }, + "response": { + "data": { + "source-summary": {}, + "pipeline-summary": { + "new-in-resource": 2, + "new-entities": [ + { + "entity": "2", + "organisation": "local-authority:ABC", + "reference": "ref-2", + } + ], + }, + } + }, + }, + status=200, + ) + rsps.add( + rsps.GET, + f"{ASYNC_BASE}/assign-selected-id/response-details", + json=[ + { + "entry_number": 1, + "transformed_row": [ + {"entity": "1", "field": "reference", "value": "ref-1"}, + {"entity": "1", "field": "name", "value": "Name 1"}, + ], + "issue_logs": [], + }, + { + "entry_number": 2, + "transformed_row": [ + {"entity": "2", "field": "reference", "value": "ref-2"}, + {"entity": "2", "field": "name", "value": "Name 2"}, + ], + "issue_logs": [], + }, + { + "entry_number": 3, + "transformed_row": [ + {"entity": "3", "field": "reference", "value": "existing-ref"}, + {"entity": "3", "field": "name", "value": "Existing"}, + ], + "issue_logs": [], + }, + ], + status=200, + ) + + transform_controller = "application.blueprints.datamanager.controllers.transform" + with patch(f"{transform_controller}.get_org_entity", return_value=90): + with patch(f"{transform_controller}.get_organisation_name"): + with patch(f"{transform_controller}.get_dataset_name", return_value="Tree"): + with patch( + f"{transform_controller}.get_entity_count_for_organisation_and_dataset", + return_value=1, + ): + with patch( + f"{transform_controller}.get_entities_for_organisation_and_dataset", + return_value=[ + { + "entity": "3", + "reference": "existing-ref", + "name": "Existing", + } + ], + ): + response = client.get( + "/assign-entities/check-results/assign-selected-id" + ) + + assert response.status_code == 200 + ref_1_checkbox = _selected_entity_checkbox(response.data, "ref-1") + ref_2_checkbox = _selected_entity_checkbox(response.data, "ref-2") + existing_checkbox = _selected_entity_checkbox(response.data, "existing-ref") + assert "checked" not in ref_1_checkbox + assert "disabled" not in ref_1_checkbox + assert "checked" in ref_2_checkbox + assert "disabled" in existing_checkbox + assert b"1 of 2 entities selected for assignment" in response.data @rsps.activate @@ -540,6 +663,13 @@ def test_assign_entities_check_results_shows_duplicate_candidates(client): "source-summary": {}, "pipeline-summary": { "new-in-resource": 1, + "old-entity": [ + { + "old-entity": "100", + "status": "301", + "entity": "200", + } + ], "duplicate-candidates": [ { "old_entity": "100", @@ -636,7 +766,7 @@ def test_assign_entities_check_results_shows_duplicate_candidates(client): b'href="/assign-entities/check-results/assign-duplicates-id?' b'entity_search=200#entities-table"' ) in response.data - assert b'type="hidden" name="entity_redirects"' in response.data + assert b'type="hidden" name="entity_redirects"' not in response.data assert ( b'id="entity-redirect-1" name="entity_redirects" type="checkbox"' in response.data @@ -655,9 +785,16 @@ def test_assign_entities_check_results_shows_duplicate_candidates(client): b" checked disabled" not in response.data ) assert b"old_entity" in response.data - assert b"checked disabled" in response.data + first_checkbox = re.search( + rb']*id="entity-redirect-1"[^>]*>', response.data + ).group(0) + assert b"checked" in first_checkbox + assert b"disabled" in first_checkbox assert b"entity-redirect-select-all" in response.data - assert b"entities selected for redirection" in response.data + assert b"1 of 2 entities selected for redirection" in response.data + assert b"JSON.parse(checkbox.value" not in response.data + assert b"function entitySelectionReference(checkbox)" in response.data + assert b"entitySelectAllCheckbox.addEventListener" in response.data @rsps.activate @@ -724,7 +861,7 @@ def test_assign_entities_check_results_hides_dedup_for_other_datasets(client): assert b'id="duplicates-table"' not in response.data -def test_assign_entities_check_results_post_stores_redirects(client): +def test_assign_entities_check_results_post_continues_without_storing_redirects(client): request_id = "assign-post-id" with patch( "application.blueprints.datamanager.router.fetch_request", @@ -776,18 +913,237 @@ def test_assign_entities_check_results_post_stores_redirects(client): assert response.headers["Location"].endswith( f"/datamanager/add-data/{request_id}/entities" ) - meta = db.session.get(RequestMeta, request_id) - assert json.loads(meta.entity_redirects) == [ + + +def test_assign_entities_check_results_post_resubmits_changed_entity_selection(client): + request_id = "assign-selection-id" + selected_value = "ref-2" + selected_redirect = json.dumps( { "old_entity": "100", "entity": "200", "dataset": "tree", "old_reference": "old-ref", - "new_reference": "new-ref", + "new_reference": "ref-2", "match_type": "complete_match", - "notes": "Redirect duplicate entity selected in Assign Entities", } - ] + ) + excluded_redirect = json.dumps( + { + "old_entity": "101", + "entity": "201", + "dataset": "tree", + "old_reference": "old-ref-1", + "new_reference": "ref-1", + "match_type": "complete_match", + } + ) + with patch( + "application.blueprints.datamanager.router.fetch_request", + return_value={ + "params": { + "dataset": "tree", + "resource": "resource-a", + "organisation": "local-authority:ABC", + "return_endpoint": "assign_entities.flagged_resources_summary", + }, + "response": { + "data": { + "pipeline-summary": { + "new-entities": [ + { + "organisation": "local-authority:ABC", + "reference": "ref-2", + }, + ], + "duplicate-candidates": [ + { + "old_entity": "100", + "entity": "200", + "dataset": "tree", + }, + { + "old_entity": "101", + "entity": "201", + "dataset": "tree", + }, + ], + } + } + }, + }, + ): + with patch( + "application.blueprints.datamanager.router._submit_assign_entities_request", + return_value="replacement-id", + ) as submit_request: + response = client.post( + f"/assign-entities/check-results/{request_id}", + data={ + "entity_selection_changed": "true", + "visible_entity_references": [ + "ref-1", + selected_value, + ], + "selected_entity_references": [selected_value], + "entity_redirects": [ + selected_redirect, + excluded_redirect, + json.dumps( + { + "old_entity": "999", + "entity": "200", + "dataset": "tree", + "new_reference": "ref-2", + } + ), + ], + }, + ) + + assert response.status_code == 302 + assert response.headers["Location"].endswith( + "/assign-entities/check-results/replacement-id" + ) + submit_request.assert_called_once_with( + "tree", + "resource-a", + organisation="local-authority:ABC", + return_endpoint="assign_entities.flagged_resources_summary", + excluded_references=["ref-1"], + selected_redirects=[ + { + "reference": "ref-2", + "old_entity_number": "100", + } + ], + ) + + +def test_assign_entities_check_results_post_continues_for_unchanged_entity_selection( + client, +): + request_id = "assign-unchanged-id" + selected_values = ["ref-1", "ref-2"] + with patch( + "application.blueprints.datamanager.router.fetch_request", + return_value={ + "params": { + "dataset": "tree", + "resource": "resource-a", + "organisation": "local-authority:ABC", + }, + "response": { + "data": { + "pipeline-summary": { + "new-entities": [ + { + "organisation": "local-authority:ABC", + "reference": "ref-1", + }, + { + "organisation": "local-authority:ABC", + "reference": "ref-2", + }, + ], + "duplicate-candidates": [], + } + } + }, + }, + ): + with patch( + "application.blueprints.datamanager.router._submit_assign_entities_request" + ) as submit_request: + response = client.post( + f"/assign-entities/check-results/{request_id}", + data={"selected_entity_references": selected_values}, + ) + + assert response.status_code == 302 + assert response.headers["Location"].endswith( + f"/datamanager/add-data/{request_id}/entities" + ) + submit_request.assert_not_called() + + +def test_assign_entities_check_results_post_resubmits_changed_redirect_selection( + client, +): + request_id = "assign-redirect-selection-id" + selected_values = ["ref-1", "ref-2"] + selected_redirect = json.dumps( + { + "old_entity": "100", + "entity": "200", + "dataset": "tree", + "new_reference": "ref-1", + } + ) + with patch( + "application.blueprints.datamanager.router.fetch_request", + return_value={ + "params": { + "dataset": "tree", + "resource": "resource-a", + "organisation": "local-authority:ABC", + }, + "response": { + "data": { + "pipeline-summary": { + "new-entities": [ + { + "organisation": "local-authority:ABC", + "reference": "ref-1", + }, + { + "organisation": "local-authority:ABC", + "reference": "ref-2", + }, + ], + "duplicate-candidates": [ + { + "old_entity": "100", + "entity": "200", + "dataset": "tree", + } + ], + } + } + }, + }, + ): + with patch( + "application.blueprints.datamanager.router._submit_assign_entities_request", + return_value="replacement-id", + ) as submit_request: + response = client.post( + f"/assign-entities/check-results/{request_id}", + data={ + "entity_selection_changed": "true", + "visible_entity_references": selected_values, + "selected_entity_references": selected_values, + "entity_redirects": [selected_redirect], + }, + ) + + assert response.status_code == 302 + assert response.headers["Location"].endswith( + "/assign-entities/check-results/replacement-id" + ) + submit_request.assert_called_once_with( + "tree", + "resource-a", + organisation="local-authority:ABC", + return_endpoint="assign_entities.flagged_resources_start", + excluded_references=[], + selected_redirects=[ + { + "reference": "ref-1", + "old_entity_number": "100", + } + ], + ) @rsps.activate diff --git a/tests/unit/blueprints/datamanager/controllers/test_add.py b/tests/unit/blueprints/datamanager/controllers/test_add.py index bfadbe5..b0e46ce 100644 --- a/tests/unit/blueprints/datamanager/controllers/test_add.py +++ b/tests/unit/blueprints/datamanager/controllers/test_add.py @@ -1,3 +1,4 @@ +import re from unittest.mock import patch from application.blueprints.datamanager.services.github import GitHubWorkflowError @@ -22,22 +23,35 @@ def test_renders_loading_template_when_pending(self, client): assert b"Preparing entities preview" in response.data def test_renders_old_entity_redirect_table(self, client): - db.session.add( - RequestMeta( - request_id="test-id", - entity_redirects=( - '[{"old_entity":"100","entity":"200",' - '"dataset":"conservation-area"}]' - ), - ) - ) - db.session.commit() result = { "status": "COMPLETE", - "params": {"dataset": "conservation-area", "authoritative": False}, + "params": { + "dataset": "conservation-area", + "authoritative": False, + "resource": "resource-a", + "excluded_references": ["not-selected", "not-selected", ""], + }, "response": { "data": { - "pipeline-summary": {"new-in-resource": 0}, + "pipeline-summary": { + "new-in-resource": 0, + "old-entity": [ + { + "old-entity": "100", + "status": "301", + "entity": "200", + }, + { + "old-entity": "101", + "status": "301", + "entity": "201", + }, + ], + "new-entities": [ + {"entity": "200", "reference": "new-ref"}, + {"entity": "201", "reference": "other-ref"}, + ], + }, "endpoint-summary": {}, "source-summary": {}, } @@ -52,8 +66,24 @@ def test_renders_old_entity_redirect_table(self, client): assert response.status_code == 200 assert b"old-entity.csv" in response.data assert b"100" in response.data + assert b"101" in response.data assert b"301" in response.data assert b"200" in response.data + assert b"Number of redirects" in response.data + assert re.search( + rb"Number of redirects.*?
2
", + response.data, + re.S, + ) + assert b"Rows that will create new entities" in response.data + assert b'
2
' in response.data + assert b"Rows that will" in response.data + assert b"NOT create new entities" in response.data + assert re.search( + rb"Rows that will.*?NOT create new entities.*?
1
", + response.data, + re.S, + ) class TestAddDataConfirmRoute: @@ -157,17 +187,7 @@ def test_returns_error_when_workflow_raises(self, client): assert response.status_code == 200 assert b"govuk-error-summary" in response.data - def test_confirm_passes_entity_redirects_to_workflow(self, client): - db.session.add( - RequestMeta( - request_id="confirm-redirect-id", - entity_redirects=( - '[{"old_entity":"100","entity":"200",' - '"dataset":"conservation-area"}]' - ), - ) - ) - db.session.commit() + def test_confirm_does_not_pass_entity_redirects_to_workflow(self, client): with client.session_transaction() as sess: sess["user"] = {"login": "test-user"} with patch( @@ -179,10 +199,4 @@ def test_confirm_passes_entity_redirects_to_workflow(self, client): ) assert response.status_code == 200 - assert trigger.call_args.kwargs["entity_redirects"] == [ - { - "old_entity": "100", - "entity": "200", - "dataset": "conservation-area", - } - ] + assert "entity_redirects" not in trigger.call_args.kwargs diff --git a/tests/unit/blueprints/datamanager/controllers/test_preview.py b/tests/unit/blueprints/datamanager/controllers/test_preview.py index 83efff4..e1aca49 100644 --- a/tests/unit/blueprints/datamanager/controllers/test_preview.py +++ b/tests/unit/blueprints/datamanager/controllers/test_preview.py @@ -1,10 +1,36 @@ from application.blueprints.datamanager.controllers.preview import ( _build_entity_organisation_summary, + build_old_entity_redirect_table, ) NEW_ENTITIES = [{"entity": "10100002", "reference": "REF001"}] +def test_old_entity_redirect_table_renders_null_values_as_empty_strings(): + table_params = build_old_entity_redirect_table( + [ + { + "old-entity": None, + "status": None, + "entity": None, + "notes": None, + "end-date": None, + "entry-date": None, + "start-date": None, + } + ] + ) + + row = table_params["rows"][0]["columns"] + assert row["old-entity"]["value"] == "" + assert row["status"]["value"] == "" + assert row["entity"]["value"] == "" + assert row["notes"]["value"] == "" + assert row["end-date"]["value"] == "" + assert row["entry-date"]["value"] == "" + assert row["start-date"]["value"] == "" + + def test_no_new_entities_hides_section(): result = _build_entity_organisation_summary([], True, {"entity-organisation": []}) diff --git a/tests/unit/blueprints/datamanager/controllers/test_transform.py b/tests/unit/blueprints/datamanager/controllers/test_transform.py index 6de7327..8b090ec 100644 --- a/tests/unit/blueprints/datamanager/controllers/test_transform.py +++ b/tests/unit/blueprints/datamanager/controllers/test_transform.py @@ -35,55 +35,120 @@ def test_dedup_candidate_form_value_keeps_existing_value(): ) -def test_prepare_duplicate_candidates_auto_selects_complete_matches(): +def test_prepare_duplicate_candidates_does_not_auto_select_complete_matches_without_old_entity(): candidates = _prepare_duplicate_candidates( [ { + "old_entity": "100", + "entity": "200", "match_type": "complete_match", "name_similarity": 10, } ] ) - assert candidates[0]["auto_select"] is True + assert candidates[0]["auto_select"] is False -def test_prepare_duplicate_candidates_does_not_auto_select_existing_redirects(): +def test_prepare_duplicate_candidates_auto_selects_old_entity_rows(): candidates = _prepare_duplicate_candidates( [ { + "old_entity": "100", + "entity": "200", "match_type": "complete_match", "old_entity_redirects": [ {"old-entity": "100", "entity": "300", "status": "301"} ], } - ] + ], + [{"old-entity": "100", "entity": "200", "status": "301"}], ) - assert candidates[0]["auto_select"] is False + assert candidates[0]["auto_select"] is True + assert candidates[0]["redirect_locked"] is True + assert candidates[0]["redirect_can_select"] is True + + +def test_prepare_duplicate_candidates_does_not_lock_selected_redirect_rows(): + candidates = _prepare_duplicate_candidates( + [ + { + "old_entity": "100", + "entity": "200", + "match_type": "complete_match", + "new_reference": "ref-1", + } + ], + [ + { + "old-entity": "100", + "entity": "200", + "status": "301", + } + ], + organisation="local-authority:ABC", + selected_redirects=[ + { + "reference": "ref-1", + "old_entity_number": "100", + } + ], + ) + + assert candidates[0]["auto_select"] is True + assert candidates[0]["redirect_locked"] is False -def test_prepare_duplicate_candidates_auto_selects_single_matches_over_threshold(): +def test_prepare_duplicate_candidates_does_not_auto_select_unmatched_old_entity_rows(): candidates = _prepare_duplicate_candidates( [ { + "old_entity": "100", + "entity": "200", "match_type": "single_match", "name_similarity": 86, } - ] + ], + [{"old-entity": "101", "entity": "200", "status": "301"}], ) - assert candidates[0]["auto_select"] is True + assert candidates[0]["auto_select"] is False -def test_prepare_duplicate_candidates_does_not_auto_select_single_matches_at_threshold(): +def test_prepare_duplicate_candidates_keeps_old_entity_field_alias(): candidates = _prepare_duplicate_candidates( [ { + "old_entity": "100", + "entity": "200", "match_type": "single_match", "name_similarity": 85, } - ] + ], + [{"old_entity": "100", "entity": "200", "status": "301"}], ) - assert candidates[0]["auto_select"] is False + assert candidates[0]["auto_select"] is True + + +def test_prepare_duplicate_candidates_disables_redirects_for_excluded_references(): + candidates = _prepare_duplicate_candidates( + [ + { + "old_entity": "100", + "entity": "200", + "new_reference": "ref-1", + }, + { + "old_entity": "101", + "entity": "201", + "new_reference": "ref-2", + }, + ], + organisation="local-authority:ABC", + excluded_references=["ref-1"], + ) + + assert candidates[0]["redirect_can_select"] is False + assert candidates[1]["redirect_can_select"] is True diff --git a/tests/unit/blueprints/datamanager/services/test_github.py b/tests/unit/blueprints/datamanager/services/test_github.py index 2b61bd3..f121d38 100644 --- a/tests/unit/blueprints/datamanager/services/test_github.py +++ b/tests/unit/blueprints/datamanager/services/test_github.py @@ -100,7 +100,7 @@ def test_returns_failure_on_non_204(self, app): assert result["success"] is False assert result["status_code"] == 422 - def test_includes_entity_redirects_in_payload(self, app): + def test_does_not_include_entity_redirects_in_payload(self, app): mock_dispatch = Mock() mock_dispatch.status_code = 204 @@ -120,21 +120,10 @@ def test_includes_entity_redirects_in_payload(self, app): "application.blueprints.datamanager.services.github.requests.post", return_value=mock_dispatch, ) as post: - trigger_add_data_async_workflow( - "request-123", - entity_redirects=[ - { - "old_entity": "100", - "entity": "200", - "dataset": "conservation-area", - } - ], - ) + trigger_add_data_async_workflow("request-123") payload = post.call_args.kwargs["json"] - assert payload["client_payload"]["entity_redirects"] == ( - '[{"old_entity":"100","entity":"200","dataset":"conservation-area"}]' - ) + assert "entity_redirects" not in payload["client_payload"] class TestGetBranchHeadSha: