shared flow endpoints need to not be blocked by wrong locks - #179
Conversation
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughRequest metadata now records whether previews originate from add-data or assign-entities flows. Shared datamanager endpoints use this value to apply the matching service lock, with updated preview handling, database migration, and acceptance coverage. ChangesFlow-aware datamanager requests
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
pooleycodes
changed the title
# Conflicts: # application/db/models.py
|
📝 Documentation reminder This PR changes 173 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.
🧹 Nitpick comments (2)
application/blueprints/datamanager/controllers/request_meta.py (1)
18-34: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNo validation constrains
source_flowto its two known values.Neither the write path nor the schema enforces
source_flow ∈ {"add_data", "assign_entities"}. Sincepreview.py/router.pytreat anything other than exactly"assign_entities"as"add_data", a typo or a future third flow would silently mis-select the process lock with no error surfaced.
application/blueprints/datamanager/controllers/request_meta.py#L18-L34: validatesource_flowagainst the known literals inrecord_source_flow(raise or log on unexpected values) before persisting.application/db/models.py#L27-L30: consider aCHECKconstraint (via a follow-up migration) restrictingsource_flowto the same known values as defence-in-depth at the schema level.🤖 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/request_meta.py` around lines 18 - 34, Validate source_flow in record_source_flow against only "add_data" and "assign_entities" before reading or persisting metadata, and raise or log an unexpected value instead of silently treating it as add_data. Also add a database CHECK constraint for RequestMeta.source_flow in application/db/models.py covering the same two literals, with the corresponding follow-up migration.tests/acceptance/blueprints/datamanager/test_flagged_resources.py (1)
163-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_seed_source_flow's use ofmerge()diverges from production's field-preserving update.
record_source_flowin production preserves otherRequestMetafields on update; this helper'smerge()would null them out if reused against a row with pre-existingbranch_sha/endpoints_to_retire. Not a problem for the current fresh-ID tests, but a trap if reused later.🤖 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 `@tests/acceptance/blueprints/datamanager/test_flagged_resources.py` around lines 163 - 167, Update _seed_source_flow to use the same field-preserving source-flow update behavior as production’s record_source_flow instead of merging a partial RequestMeta object. Ensure existing branch_sha and endpoints_to_retire values remain unchanged while source_flow is recorded, then preserve the existing commit 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.
Nitpick comments:
In `@application/blueprints/datamanager/controllers/request_meta.py`:
- Around line 18-34: Validate source_flow in record_source_flow against only
"add_data" and "assign_entities" before reading or persisting metadata, and
raise or log an unexpected value instead of silently treating it as add_data.
Also add a database CHECK constraint for RequestMeta.source_flow in
application/db/models.py covering the same two literals, with the corresponding
follow-up migration.
In `@tests/acceptance/blueprints/datamanager/test_flagged_resources.py`:
- Around line 163-167: Update _seed_source_flow to use the same field-preserving
source-flow update behavior as production’s record_source_flow instead of
merging a partial RequestMeta object. Ensure existing branch_sha and
endpoints_to_retire values remain unchanged while source_flow is recorded, then
preserve the existing commit behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fee29ea6-2f0d-4d62-b9e3-15f9408bacd0
📒 Files selected for processing (9)
application/blueprints/datamanager/controllers/flagged_resources.pyapplication/blueprints/datamanager/controllers/form.pyapplication/blueprints/datamanager/controllers/preview.pyapplication/blueprints/datamanager/controllers/request_meta.pyapplication/blueprints/datamanager/router.pyapplication/db/models.pymigrations/versions/b9c0d1e2f3a4_add_request_meta_source_flow.pytests/acceptance/blueprints/datamanager/test_flagged_resources.pytests/unit/blueprints/datamanager/controllers/test_add.py
2 participants
What type of PR is this? (check all applicable)
Description
The entities preview is used by both add data and assign entities flows, however as it lives under add data, if you lock that process assign entities is unable to proceed.
Not a perfect solution, but RequestMeta now stores the source flow to check
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Lock add-data process, and try to go all the way through assign entities, you should no longer be stopped.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above. Please refer to the Digital Land Testing Guidance for more information.
have not been included
[optional] Are there any post deployment tasks we need to perform?
[optional] Are there any dependencies on other PRs or Work?
Summary by CodeRabbit
New Features
Bug Fixes
Tests