better context in retire endpoints and unretire option - #180
Merged
pooleycodes merged 8 commits intoJul 29, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
WalkthroughThe datamanager now enriches endpoint selections with metadata and log status, supports retire and unretire selections, persists both lists, displays an unretire preview, and passes unretire endpoints to the asynchronous workflow. ChangesEndpoint lifecycle management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant TransformForm
participant DatamanagerRouter
participant RequestMeta
participant PreviewController
participant GithubWorkflow
Operator->>TransformForm: select retire or unretire endpoints
TransformForm->>DatamanagerRouter: submit endpoint selections
DatamanagerRouter->>RequestMeta: persist endpoint lists
PreviewController->>RequestMeta: load endpoint lists
PreviewController->>GithubWorkflow: dispatch retire and unretire endpoints
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
|
📝 Documentation reminder This PR changes 318 lines of code (threshold 100) but does not update anything in This is a non-blocking reminder — it will not prevent merging. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
application/blueprints/datamanager/controllers/transform.py (1)
446-447: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSequential remote calls for endpoint info and log summary.
get_endpoint_info_for_hashesandget_endpoint_log_summary_for_hashesare independent lookups run back-to-back on every check-transform render. The log-summary function's own docstring warns thelogtable query is slow, so serialising these two round-trips adds avoidable latency to a page load.Consider running them concurrently (e.g.
concurrent.futures.ThreadPoolExecutor) to overlap the two HTTP calls.🤖 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/blueprints/datamanager/controllers/transform.py` around lines 446 - 447, Update the check-transform rendering flow around get_endpoint_info_for_hashes and get_endpoint_log_summary_for_hashes to execute these independent lookups concurrently, using the project’s established concurrency approach such as concurrent.futures.ThreadPoolExecutor. Collect both results before continuing, and preserve the existing endpoint_data and log_data behavior.
🤖 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/blueprints/datamanager/router.py`:
- Around line 258-292: The POST handler must stop and return an error when
_current_endpoint_hash(request_id) cannot resolve the request, rather than
filtering and persisting retirement changes. Check the helper result immediately
after calling _current_endpoint_hash, distinguish the None failure case, and
return the handler’s established error response before calculating to_retire or
to_unretire.
- Around line 253-268: The endpoint change calculations around
_current_endpoint_hash must use server-side endpoint hashes and retirement state
as the source of truth, rather than trusting presented_endpoints or
currently_retired from the request. Resolve the allowed endpoint set and
currently retired hashes for request_id, intersect submitted retire_endpoints
with allowed hashes, and compute to_retire and to_unretire only from that
validated state before persistence or workflow forwarding.
In `@application/blueprints/datamanager/services/endpoint.py`:
- Around line 60-71: Validate every value in hashes as a locally verified
SHA-256 hexadecimal digest before constructing hash_list in the endpoint query
flow. Reject or safely handle any invalid value before it reaches the SQL
interpolation, while preserving the existing query behavior for valid hashes and
removing the injection risk identified in the hash_list construction.
In `@application/blueprints/datamanager/services/github.py`:
- Line 281: Update the endpoints_to_unretire parameter and its matching list
parameter to use an explicit nullable type, such as list[str] | None, while
retaining the existing None default and behavior.
In `@tests/integration/blueprints/datamanager/test_datamanager_integration.py`:
- Around line 1132-1137: Add ("presented_endpoints", current_hash) to the test
case’s expected entries alongside the existing presented_endpoints values,
ensuring current_hash is included in the intersection and the current-endpoint
protection filter is genuinely exercised.
In `@tests/unit/blueprints/datamanager/controllers/test_transform.py`:
- Around line 211-217: Update both patch targets from
get_endpoint_urls_for_hashes to get_endpoint_info_for_hashes:
tests/unit/blueprints/datamanager/controllers/test_transform.py lines 211-217
and tests/acceptance/blueprints/datamanager/test_flagged_resources.py lines
440-452. Keep the existing mocked return values and test behavior unchanged.
---
Nitpick comments:
In `@application/blueprints/datamanager/controllers/transform.py`:
- Around line 446-447: Update the check-transform rendering flow around
get_endpoint_info_for_hashes and get_endpoint_log_summary_for_hashes to execute
these independent lookups concurrently, using the project’s established
concurrency approach such as concurrent.futures.ThreadPoolExecutor. Collect both
results before continuing, and preserve the existing endpoint_data and log_data
behavior.
🪄 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: ab1fc2ba-4c33-4800-89af-77856236e53f
📒 Files selected for processing (13)
application/blueprints/datamanager/controllers/preview.pyapplication/blueprints/datamanager/controllers/transform.pyapplication/blueprints/datamanager/router.pyapplication/blueprints/datamanager/services/endpoint.pyapplication/blueprints/datamanager/services/github.pyapplication/db/models.pyapplication/templates/datamanager/check-transform.htmlapplication/templates/datamanager/entities_preview.htmlmigrations/versions/b8c9d0e1f2a3_add_request_meta_endpoints_to_unretire.pytests/acceptance/blueprints/datamanager/test_flagged_resources.pytests/integration/blueprints/datamanager/test_datamanager_integration.pytests/unit/blueprints/datamanager/controllers/test_transform.pytests/unit/blueprints/datamanager/services/test_endpoint.py
…-improve-retire-endpoint-section
pooleycodes
marked this pull request as ready for review
July 29, 2026 13:12
Contributor
No documentation update or its not needed? |
pooleycodes
deleted the
2620-transform-page---improve-retire-endpoint-section
branch
July 29, 2026 15:11
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this? (check all applicable)
Description
Improves the Retire endpoints section on the check-transform page and adds an unretire option, to make it easier to manage org/datasets that have multiple active endpoints (e.g. BFL / DC data).
Ships together with the downstream config change digital-land/config#2851, which adds
unretire_endpointshandling tobin/add_data.py(clearsend-dateinendpoint.csv/source.csv, the inverse of retire).Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Added/updated tests?
have not been included
[optional] Are there any post deployment tasks we need to perform?
No - config PR already in
[optional] Are there any dependencies on other PRs or Work?
No
Summary by CodeRabbit
Summary by CodeRabbit