Skip to content

Big cleanup of old processes - #178

Merged
pooleycodes merged 4 commits into
mainfrom
177-remove-dead-and-obsolete-code-and-add-documentation-update-checks-for-the-manage-service
Jul 28, 2026
Merged

Big cleanup of old processes#178
pooleycodes merged 4 commits into
mainfrom
177-remove-dead-and-obsolete-code-and-add-documentation-update-checks-for-the-manage-service

Conversation

@pooleycodes

@pooleycodes pooleycodes commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Tidy-up of the Manage service down to the two live operator workflows (Add data and
Assign entities), plus supporting docs and a docs-update CI check.

  • Removed dead code — the source, dataset, endpoint, schema, report, and publisher
    blueprints were registered but unreachable from the live UI; removed them along with their
    templates, data_access query modules, static JS bundles, and the old cross-blueprint navigation.
  • Slimmed the database — trimmed the ORM to ServiceLock + RequestMeta, retired the
    flask data / flask publish CLIs and application/publish/, dropped the now-unused PyGithub
    dependency, and added a migration that drops the 24 obsolete config tables.
  • Docs — added docs/datamanager/add-data.md, consolidated the stale-check into github-add.md,
    and brought the Assign Entities architecture doc alongside. Added a non-blocking GitHub Action
    that reminds a PR to update docs/ when it changes more than 100 lines of code.
  • Merged latest main.

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Service should operate identical to before

Added/updated tests?

  • [] No

[optional] Are there any post deployment tasks we need to perform?

The migration drops the retired config tables (flask db upgrade runs on deploy). This is
destructive and intentional — see docs/README.md.

Summary by CodeRabbit

  • Changes
    • Streamlined the app around the Add data and Assign entities workflows.
    • Removed legacy pages and navigation for datasets, sources, reporting, schemas, organisations, and related management features.
    • Removed legacy UI components including charts, filters, forms, and client-side interactions.
  • Documentation
    • Added and updated operator workflow documentation, including GitHub integration and stale-assessment guard behaviour.
    • Updated “Getting started” instructions to skip local load/drop steps.
  • Maintenance
    • Retired legacy data-loading/publishing command-line operations and removed related Makefile targets.
    • Updated bundles/dependencies and included a database change that drops the retired config tables.
  • Chores
    • Added a non-blocking pull-request reminder to update docs after substantial changes.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

This change narrows the application to active datamanager workflows, removes obsolete interfaces and configuration infrastructure, adds an irreversible database migration, updates dependencies and build configuration, documents active flows, and introduces a non-blocking documentation reminder workflow.

Changes

Manage service consolidation

Layer / File(s) Summary
Retire legacy application surfaces
application/blueprints/report/views.py, application/db/models.py, application/factory.py, migrations/versions/..., requirements/*, rollup.config.js, Makefile, README.md
Legacy reporting and configuration models, commands, blueprints, build targets, dependencies, and bundles are removed; the factory retains active blueprint wiring and a migration drops obsolete tables and the publication_status type.
Preserve datamanager request and check flows
application/blueprints/datamanager/..., application/templates/datamanager/...
Datamanager URL construction is localised, async API calls use shared endpoint configuration, and check-transform pages render entity, transformed, issue-log, documentation, pipeline, pagination, map, and tooltip states.
Document active workflows and enforce documentation reminders
.github/workflows/docs-check.yml, docs/README.md, docs/datamanager/*
Active service and datamanager workflows are documented, and pull requests receive a non-blocking reminder when application code changes exceed the configured threshold without documentation changes.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

Suggested reviewers: gibahjoe

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 36.36% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title broadly matches the PR’s large-scale removal of legacy code and workflows, even though it is high-level.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 177-remove-dead-and-obsolete-code-and-add-documentation-update-checks-for-the-manage-service

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
migrations/versions/a7b8c9d0e1f2_drop_config_tables.py (1)

57-70: 🗄️ Data Integrity & Integration | 🔵 Trivial

Treat this as a release-level destructive migration.

CASCADE permanently deletes the configuration data, and downgrade() cannot restore it. Run it only after a verified backup/restore rehearsal, and ensure deployment rollback tooling will not attempt an automatic downgrade.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@migrations/versions/a7b8c9d0e1f2_drop_config_tables.py` around lines 57 - 70,
Document this migration as release-level and destructive, explicitly requiring a
verified backup/restore rehearsal before running it. Update deployment or
rollback configuration associated with upgrade/downgrade handling so automatic
downgrade attempts are disabled, while preserving the existing irreversible
downgrade behavior in downgrade().
docs/datamanager/add-data.md (1)

14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a language specifier to fenced code blocks (MD040). Both new docs use plain ``` fences for illustrative, non-code content, which markdownlint-cli2 flags in each case.

  • docs/datamanager/add-data.md#L14-L16: annotate the flow-steps diagram fence, e.g. ```text.
  • docs/datamanager/assign-entities.md#L15-L17: annotate the flow-steps diagram fence, e.g. ```text.
  • docs/datamanager/assign-entities.md#L28-L30: annotate the CSV-columns fence, e.g. ```text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/datamanager/add-data.md` around lines 14 - 16, Annotate all three plain
fenced blocks with a text language specifier: the flow-steps fence in
docs/datamanager/add-data.md (lines 14-16), the flow-steps fence in
docs/datamanager/assign-entities.md (lines 15-17), and the CSV-columns fence in
docs/datamanager/assign-entities.md (lines 28-30).

Source: Linters/SAST tools

application/templates/datamanager/components/check-transform-base.html (1)

42-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate pagination-building logic.

The pagination_params block (42-60) and entity_pagination_params block (62-80) are near-identical, differing only by the page variable name and the extra entity_search/entity_filter query args. Consider extracting a Jinja macro (taking the endpoint, page var, has-next flag, and extra url_for kwargs) to avoid the two copies drifting apart on future edits.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@application/templates/datamanager/components/check-transform-base.html`
around lines 42 - 80, Extract the duplicated pagination construction from the
pagination_params and entity_pagination_params blocks into a reusable Jinja
macro. Parameterize the macro with the page value, next-page flag, endpoint, and
optional query arguments so it generates equivalent items, previous/next links,
and ellipses for both standard and entity pagination; replace both inline blocks
with macro calls while preserving their current URL parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/docs-check.yml:
- Around line 26-34: Harden the checkout and diff setup by adding
persist-credentials: false to the actions/checkout@v6 step. In the “Check
whether a substantial code change skipped docs” step, pass github.base_ref
through an env variable named BASE_REF and update the shell commands to use
$BASE_REF instead of directly interpolating the GitHub expression in run:.

---

Nitpick comments:
In `@application/templates/datamanager/components/check-transform-base.html`:
- Around line 42-80: Extract the duplicated pagination construction from the
pagination_params and entity_pagination_params blocks into a reusable Jinja
macro. Parameterize the macro with the page value, next-page flag, endpoint, and
optional query arguments so it generates equivalent items, previous/next links,
and ellipses for both standard and entity pagination; replace both inline blocks
with macro calls while preserving their current URL parameters.

In `@docs/datamanager/add-data.md`:
- Around line 14-16: Annotate all three plain fenced blocks with a text language
specifier: the flow-steps fence in docs/datamanager/add-data.md (lines 14-16),
the flow-steps fence in docs/datamanager/assign-entities.md (lines 15-17), and
the CSV-columns fence in docs/datamanager/assign-entities.md (lines 28-30).

In `@migrations/versions/a7b8c9d0e1f2_drop_config_tables.py`:
- Around line 57-70: Document this migration as release-level and destructive,
explicitly requiring a verified backup/restore rehearsal before running it.
Update deployment or rollback configuration associated with upgrade/downgrade
handling so automatic downgrade attempts are disabled, while preserving the
existing irreversible downgrade behavior in downgrade().
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: cea917ef-8d92-4091-a260-b354c6fca2dc

📥 Commits

Reviewing files that changed from the base of the PR and between bb527b3 and 4e35a7b.

📒 Files selected for processing (98)
  • .github/workflows/docs-check.yml
  • Makefile
  • README.md
  • application/blueprints/datamanager/config.py
  • application/blueprints/datamanager/controllers/transform.py
  • application/blueprints/datamanager/services/async_api.py
  • application/blueprints/datamanager/utils/__init__.py
  • application/blueprints/dataset/__init__.py
  • application/blueprints/dataset/forms.py
  • application/blueprints/dataset/views.py
  • application/blueprints/endpoint/__init__.py
  • application/blueprints/endpoint/views.py
  • application/blueprints/publisher/views.py
  • application/blueprints/report/views.py
  • application/blueprints/schema/__init__.py
  • application/blueprints/schema/views.py
  • application/blueprints/source/__init__.py
  • application/blueprints/source/forms.py
  • application/blueprints/source/source.html
  • application/blueprints/source/views.py
  • application/commands.py
  • application/data_access/endpoint/endpoint_queries.py
  • application/data_access/odp_summaries/conformance.py
  • application/data_access/odp_summaries/issue.py
  • application/data_access/odp_summaries/status.py
  • application/data_access/odp_summaries/utils.py
  • application/data_access/overview/api_queries.py
  • application/data_access/overview/datasette_queries.py
  • application/data_access/overview/entity_queries.py
  • application/data_access/overview/issue_summary.py
  • application/data_access/overview/source_and_resource_queries.py
  • application/data_access/overview/utils.py
  • application/data_access/summary_queries.py
  • application/data_commands.py
  • application/db/models.py
  • application/factory.py
  • application/publish/__init__.py
  • application/publish/models.py
  • application/spec_helpers.py
  • application/templates/components/filter-group/macro.jinja
  • application/templates/components/helpers.jinja
  • application/templates/components/resource-card.html
  • application/templates/components/rule-table-header.html
  • application/templates/components/source-card.html
  • application/templates/content_type/index.html
  • application/templates/datamanager/assign-entities-check-results.html
  • application/templates/datamanager/check-transform.html
  • application/templates/datamanager/components/check-transform-base.html
  • application/templates/dataset/dataset.html
  • application/templates/dataset/editrule.html
  • application/templates/dataset/index.html
  • application/templates/dataset/performance.html
  • application/templates/dataset/rules.html
  • application/templates/dataset/sources.html
  • application/templates/layouts/layout.html
  • application/templates/macro/primary-nav.html
  • application/templates/macro/resources-sources-by-month.html
  • application/templates/overview/high-charts-scripts.html
  • application/templates/overview/main-dataset-table.html
  • application/templates/overview/performance.html
  • application/templates/partials/primary-navigation.html
  • application/templates/reporting/endpoint_details.html
  • application/templates/reporting/odp_conformance_summary.html
  • application/templates/reporting/odp_issue_summary.html
  • application/templates/reporting/odp_status_summary.html
  • application/templates/reporting/overview.html
  • application/templates/schema/index.html
  • application/templates/schema/schema.html
  • application/templates/source/archive.html
  • application/templates/source/create.html
  • application/templates/source/edit.html
  • application/templates/source/finish.html
  • application/templates/source/index.html
  • application/templates/source/search.html
  • application/templates/source/source.html
  • application/templates/source/summary.html
  • application/templates/tables/org-table-on-dataset.html
  • config/allowed-users.md
  • docs/README.md
  • docs/datamanager/add-data.md
  • docs/datamanager/architecture.md
  • docs/datamanager/assign-entities.md
  • docs/datamanager/github-add.md
  • docs/datamanager/stale-check.md
  • migrations/versions/a7b8c9d0e1f2_drop_config_tables.py
  • requirements/requirements.in
  • requirements/requirements.txt
  • rollup.config.js
  • src/javascripts/app-background-check.js
  • src/javascripts/app-resource-mapping.js
  • src/javascripts/application-add-source.js
  • src/javascripts/components/collapsible-section.js
  • src/javascripts/components/resource-check.js
  • src/javascripts/components/select-datasets.js
  • src/javascripts/reporting-summary.js
  • src/javascripts/utilities/DOM-helpers.js
  • src/javascripts/utilities/fetchWithArgs.js
  • src/javascripts/utilities/timeseries-chart.js
💤 Files with no reviewable changes (79)
  • application/templates/source/create.html
  • docs/datamanager/stale-check.md
  • application/templates/overview/main-dataset-table.html
  • application/templates/source/archive.html
  • application/templates/components/filter-group/macro.jinja
  • application/templates/dataset/sources.html
  • application/templates/tables/org-table-on-dataset.html
  • application/templates/reporting/endpoint_details.html
  • application/templates/components/rule-table-header.html
  • application/templates/layouts/layout.html
  • application/templates/dataset/index.html
  • src/javascripts/app-resource-mapping.js
  • application/templates/overview/performance.html
  • application/data_access/overview/api_queries.py
  • application/templates/content_type/index.html
  • application/templates/source/search.html
  • application/templates/dataset/editrule.html
  • application/data_access/overview/utils.py
  • application/templates/source/finish.html
  • application/templates/partials/primary-navigation.html
  • application/templates/macro/primary-nav.html
  • application/templates/components/helpers.jinja
  • src/javascripts/utilities/fetchWithArgs.js
  • src/javascripts/utilities/timeseries-chart.js
  • application/templates/reporting/odp_status_summary.html
  • application/templates/components/resource-card.html
  • src/javascripts/utilities/DOM-helpers.js
  • application/templates/reporting/odp_issue_summary.html
  • application/templates/schema/schema.html
  • application/blueprints/dataset/forms.py
  • application/blueprints/schema/views.py
  • application/templates/overview/high-charts-scripts.html
  • src/javascripts/app-background-check.js
  • application/data_access/endpoint/endpoint_queries.py
  • src/javascripts/components/select-datasets.js
  • application/templates/dataset/rules.html
  • application/blueprints/endpoint/views.py
  • config/allowed-users.md
  • application/templates/macro/resources-sources-by-month.html
  • application/templates/schema/index.html
  • application/blueprints/datamanager/config.py
  • application/templates/components/source-card.html
  • application/commands.py
  • application/data_access/overview/entity_queries.py
  • src/javascripts/application-add-source.js
  • application/data_access/odp_summaries/utils.py
  • application/templates/source/summary.html
  • application/templates/source/source.html
  • application/data_access/overview/datasette_queries.py
  • application/templates/source/index.html
  • rollup.config.js
  • application/data_access/odp_summaries/status.py
  • src/javascripts/components/collapsible-section.js
  • application/blueprints/source/source.html
  • application/templates/dataset/performance.html
  • application/data_access/summary_queries.py
  • application/data_access/odp_summaries/issue.py
  • application/templates/reporting/overview.html
  • application/blueprints/publisher/views.py
  • src/javascripts/components/resource-check.js
  • application/templates/dataset/dataset.html
  • application/data_access/odp_summaries/conformance.py
  • application/templates/reporting/odp_conformance_summary.html
  • README.md
  • application/publish/models.py
  • src/javascripts/reporting-summary.js
  • application/spec_helpers.py
  • application/data_access/overview/issue_summary.py
  • application/data_access/overview/source_and_resource_queries.py
  • application/blueprints/source/views.py
  • Makefile
  • application/data_commands.py
  • application/blueprints/source/forms.py
  • application/blueprints/dataset/views.py
  • application/blueprints/datamanager/utils/init.py
  • application/templates/source/edit.html
  • application/factory.py
  • application/blueprints/report/views.py
  • application/db/models.py

Comment thread .github/workflows/docs-check.yml Outdated
…bsolete-code-and-add-documentation-update-checks-for-the-manage-service

# Conflicts:
#	docs/datamanager/architecture.md
#	docs/datamanager/github-add.md
@pooleycodes pooleycodes changed the title big cleanup - rem old code Big cleanup of old processes Jul 27, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
application/templates/datamanager/assign-entities-check-results.html (1)

62-62: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Remove the duplicate form IDs.

Line 170 adds a second duplicate-redirect-form and entity-selection-changed ID already defined at Lines 61–62. This makes form="duplicate-redirect-form" and getElementById(...) resolution dependent on DOM order. Remove the empty second form; the existing form already contains the submission fields.

Proposed fix
-      <form method="post" action="{{ url_for('assign_entities.flagged_resource_detail_post', request_id=request_id) }}" id="duplicate-redirect-form">
-        <input type="hidden" name="entity_selection_changed" id="entity-selection-changed" value="false">
-      </form>

Also applies to: 170-178

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@application/templates/datamanager/assign-entities-check-results.html` at line
62, Remove the duplicate empty form near the later occurrence of
duplicate-redirect-form and entity-selection-changed. Keep the original form and
its existing submission fields, ensuring only one element uses each ID so
form="duplicate-redirect-form" and getElementById(...) resolve consistently.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@application/templates/datamanager/components/check-transform-base.html`:
- Around line 444-482: Update updateEntitySelection so it does not return when
entityCheckboxes is empty; only skip logic that requires rendered checkboxes.
Continue calculating current selection and currentRedirectSelection, then update
entitySelectionChanged and entitySelectionSubmit so duplicate-checkbox changes
still mark the selection changed and set the correct CTA on empty entity pages.

In `@docs/datamanager/add-data.md`:
- Line 27: Update the flow diagram fenced block in add-data.md to use the text
language identifier, and remove the blank line inside the blockquote near the
shared transform.py note. Apply the same formatting correction at the additional
referenced occurrence, preserving the diagram and blockquote content.

---

Outside diff comments:
In `@application/templates/datamanager/assign-entities-check-results.html`:
- Line 62: Remove the duplicate empty form near the later occurrence of
duplicate-redirect-form and entity-selection-changed. Keep the original form and
its existing submission fields, ensuring only one element uses each ID so
form="duplicate-redirect-form" and getElementById(...) resolve consistently.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 57f4b3bd-7b92-42e1-8441-6ed15de264d9

📥 Commits

Reviewing files that changed from the base of the PR and between 4e35a7b and 03f8415.

📒 Files selected for processing (11)
  • .github/workflows/docs-check.yml
  • application/blueprints/datamanager/controllers/transform.py
  • application/db/models.py
  • application/templates/datamanager/assign-entities-check-results.html
  • application/templates/datamanager/components/check-transform-base.html
  • docs/README.md
  • docs/datamanager/add-data.md
  • docs/datamanager/architecture.md
  • docs/datamanager/assign-entities.md
  • docs/datamanager/github-add.md
  • migrations/versions/a7b8c9d0e1f2_drop_config_tables.py
💤 Files with no reviewable changes (2)
  • docs/datamanager/architecture.md
  • application/db/models.py
🚧 Files skipped from review as they are similar to previous changes (5)
  • docs/README.md
  • migrations/versions/a7b8c9d0e1f2_drop_config_tables.py
  • application/blueprints/datamanager/controllers/transform.py
  • .github/workflows/docs-check.yml
  • docs/datamanager/github-add.md

Comment thread docs/datamanager/add-data.md

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
application/templates/datamanager/assign-entities-check-results.html (1)

62-62: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Remove the duplicate form IDs.

Line 170 adds a second duplicate-redirect-form and entity-selection-changed ID already defined at Lines 61–62. This makes form="duplicate-redirect-form" and getElementById(...) resolution dependent on DOM order. Remove the empty second form; the existing form already contains the submission fields.

Proposed fix
-      <form method="post" action="{{ url_for('assign_entities.flagged_resource_detail_post', request_id=request_id) }}" id="duplicate-redirect-form">
-        <input type="hidden" name="entity_selection_changed" id="entity-selection-changed" value="false">
-      </form>

Also applies to: 170-178

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@application/templates/datamanager/assign-entities-check-results.html` at line
62, Remove the duplicate empty form near the later occurrence of
duplicate-redirect-form and entity-selection-changed. Keep the original form and
its existing submission fields, ensuring only one element uses each ID so
form="duplicate-redirect-form" and getElementById(...) resolve consistently.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@application/templates/datamanager/components/check-transform-base.html`:
- Around line 444-482: Update updateEntitySelection so it does not return when
entityCheckboxes is empty; only skip logic that requires rendered checkboxes.
Continue calculating current selection and currentRedirectSelection, then update
entitySelectionChanged and entitySelectionSubmit so duplicate-checkbox changes
still mark the selection changed and set the correct CTA on empty entity pages.

In `@docs/datamanager/add-data.md`:
- Line 27: Update the flow diagram fenced block in add-data.md to use the text
language identifier, and remove the blank line inside the blockquote near the
shared transform.py note. Apply the same formatting correction at the additional
referenced occurrence, preserving the diagram and blockquote content.

---

Outside diff comments:
In `@application/templates/datamanager/assign-entities-check-results.html`:
- Line 62: Remove the duplicate empty form near the later occurrence of
duplicate-redirect-form and entity-selection-changed. Keep the original form and
its existing submission fields, ensuring only one element uses each ID so
form="duplicate-redirect-form" and getElementById(...) resolve consistently.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 57f4b3bd-7b92-42e1-8441-6ed15de264d9

📥 Commits

Reviewing files that changed from the base of the PR and between 4e35a7b and 03f8415.

📒 Files selected for processing (11)
  • .github/workflows/docs-check.yml
  • application/blueprints/datamanager/controllers/transform.py
  • application/db/models.py
  • application/templates/datamanager/assign-entities-check-results.html
  • application/templates/datamanager/components/check-transform-base.html
  • docs/README.md
  • docs/datamanager/add-data.md
  • docs/datamanager/architecture.md
  • docs/datamanager/assign-entities.md
  • docs/datamanager/github-add.md
  • migrations/versions/a7b8c9d0e1f2_drop_config_tables.py
💤 Files with no reviewable changes (2)
  • docs/datamanager/architecture.md
  • application/db/models.py
🚧 Files skipped from review as they are similar to previous changes (5)
  • docs/README.md
  • migrations/versions/a7b8c9d0e1f2_drop_config_tables.py
  • application/blueprints/datamanager/controllers/transform.py
  • .github/workflows/docs-check.yml
  • docs/datamanager/github-add.md
🛑 Comments failed to post (1)
application/templates/datamanager/components/check-transform-base.html (1)

444-482: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not skip Dedup state when the current Entities page is empty.

The early return prevents duplicate-checkbox changes from updating entity_selection_changed or the CTA whenever no entity rows are rendered (for example, an empty filtered page). The POST can then continue to preview without processing the changed redirects.

Proposed fix
 function updateEntitySelection() {
-  if (!entitySelectedCount || !entityCheckboxes.length) return;
   var count = 0;
   var selectableCount = 0;
   var selectableSelectedCount = 0;
   var currentSelection = [];
   entityCheckboxes.forEach(function(checkbox) {
     // ...
   });
-  entitySelectedCount.textContent = selectableCount
-    ? count + ' of ' + selectableCount + ' ' + (selectableCount === 1 ? 'entity' : 'entities') + ' selected for assignment'
-    : 'No entities available for assignment';
+  if (entitySelectedCount) {
+    entitySelectedCount.textContent = selectableCount
+      ? count + ' of ' + selectableCount + ' ' + (selectableCount === 1 ? 'entity' : 'entities') + ' selected for assignment'
+      : 'No entities available for assignment';
+  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

  function updateEntitySelection() {
    var count = 0;
    var selectableCount = 0;
    var selectableSelectedCount = 0;
    var currentSelection = [];
    entityCheckboxes.forEach(function(checkbox) {
      var row = checkbox.closest('.govuk-table__row');
      if (row && row.dataset.originalBackground === undefined) {
        row.dataset.originalBackground = row.style.backgroundColor || '';
      }
      if (!checkbox.disabled) {
        selectableCount += 1;
        if (checkbox.checked) selectableSelectedCount += 1;
      }
      if (checkbox.checked && !checkbox.disabled) {
        count += 1;
        currentSelection.push(checkbox.value);
        if (row) row.style.backgroundColor = darkenRowColour(row.dataset.originalBackground);
      } else if (row && checkbox.disabled === false) {
        row.style.backgroundColor = row.dataset.originalBackground || '';
      }
    });
    if (entitySelectedCount) {
      entitySelectedCount.textContent = selectableCount
        ? count + ' of ' + selectableCount + ' ' + (selectableCount === 1 ? 'entity' : 'entities') + ' selected for assignment'
        : 'No entities available for assignment';
    }
    if (entitySelectAllCheckbox) {
      entitySelectAllCheckbox.checked = selectableCount > 0 && selectableSelectedCount === selectableCount;
      entitySelectAllCheckbox.indeterminate = selectableSelectedCount > 0 && selectableSelectedCount < selectableCount;
      entitySelectAllCheckbox.disabled = selectableCount === 0;
    }
    var selectionChanged = currentSelection.sort().join('|') !== initialEntitySelection
      || currentRedirectSelection() !== initialRedirectSelection;
    if (entitySelectionChanged) {
      entitySelectionChanged.value = selectionChanged ? 'true' : 'false';
    }
    if (entitySelectionSubmit) {
      entitySelectionSubmit.textContent = selectionChanged ? 'Process entities' : 'Continue to preview';
    }
🧰 Tools
🪛 HTMLHint (1.9.2)

[error] 471-471: Special characters must be escaped : [ > ].

(spec-char-escape)


[error] 472-472: Special characters must be escaped : [ > ].

(spec-char-escape)


[error] 472-472: Special characters must be escaped : [ < ].

(spec-char-escape)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@application/templates/datamanager/components/check-transform-base.html`
around lines 444 - 482, Update updateEntitySelection so it does not return when
entityCheckboxes is empty; only skip logic that requires rendered checkboxes.
Continue calculating current selection and currentRedirectSelection, then update
entitySelectionChanged and entitySelectionSubmit so duplicate-checkbox changes
still mark the selection changed and set the correct CTA on empty entity pages.

@pooleycodes
pooleycodes marked this pull request as ready for review July 27, 2026 12:19

@gibahjoe gibahjoe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pooleycodes
pooleycodes merged commit 92507b4 into main Jul 28, 2026
9 checks passed
@pooleycodes
pooleycodes deleted the 177-remove-dead-and-obsolete-code-and-add-documentation-update-checks-for-the-manage-service branch July 28, 2026 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove Dead and Obsolete Code and Add Documentation Update Checks for the Manage Service

2 participants