Add a permissions warning on contributor merge - #1576
Open
danlamanna wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe change adds contributor merge-impact computation, a staff-only API endpoint, autocomplete selection events, Alpine form integration, impact messaging, and unit, API, and browser tests. ChangesContributor merge impact
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Staff
participant autocompleteInput
participant contributorMergeImpact
participant contributor_merge_impact
participant compute_contributor_merge_impact
Staff->>autocompleteInput: select destination and source contributors
autocompleteInput->>contributorMergeImpact: dispatch autocomplete-selection
contributorMergeImpact->>contributor_merge_impact: request merge impact
contributor_merge_impact->>compute_contributor_merge_impact: compute contributor effects
compute_contributor_merge_impact-->>contributor_merge_impact: return impact data
contributor_merge_impact-->>contributorMergeImpact: return API response
contributorMergeImpact-->>Staff: display merge-impact alert
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@isic/ingest/static/ingest/contributor_merge_impact.js`:
- Around line 16-34: Update refresh() to clear impact when starting every valid
request and track the request or selection identity so responses from superseded
fetches cannot update impact. Preserve loading cleanup for the active request,
and ensure changing or clearing selections leaves stale results cleared. Add a
browser test covering a selection change or clear before the first response
resolves.
🪄 Autofix
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 59388345-19d1-475e-bcdd-713c6be18ffa
📒 Files selected for processing (11)
isic/ingest/api.pyisic/ingest/services/contributor/__init__.pyisic/ingest/static/ingest/autocomplete.jsisic/ingest/static/ingest/contributor_merge_impact.jsisic/ingest/templates/ingest/contributor_merge.htmlisic/ingest/templates/ingest/partials/autocomplete_field.htmlisic/ingest/templates/ingest/partials/contributor_merge_impact.htmlisic/ingest/templates/ingest/partials/merge_impact_users.htmlisic/ingest/tests/test_api_contributor.pyisic/ingest/tests/test_merge.pyisic/ingest/tests/test_merge_contributors_browser.py
Comment on lines
+16
to
+34
| async refresh() { | ||
| const dest = this.selections[destField] || ''; | ||
| const src = this.selections[srcField] || ''; | ||
|
|
||
| // merging a contributor into itself is rejected by the form, so there's nothing to warn about | ||
| if (!dest || !src || dest === src) { | ||
| this.impact = null; | ||
| this.loading = false; | ||
| return; | ||
| } | ||
|
|
||
| this.loading = true; | ||
| const params = new URLSearchParams({ dest_contributor: dest, src_contributor: src }); | ||
| try { | ||
| const response = await fetch(`${impactUrl}?${params}`); | ||
| this.impact = response.ok ? await response.json() : null; | ||
| } finally { | ||
| this.loading = false; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent stale merge-impact responses from updating the current selection.
refresh() does not associate a response with the selection that started the request. If a user changes or clears a contributor while fetch() is pending, an older response can set impact after the current selection has changed. The warning can then show incorrect access effects.
Clear impact when a new valid request starts. Ignore results from superseded requests. Add a browser test that changes or clears a selection before the first response completes.
Proposed fix
function contributorMergeImpact({ impactUrl, destField, srcField }) {
return {
selections: {},
impact: null,
loading: false,
+ refreshVersion: 0,
async refresh() {
+ const refreshVersion = ++this.refreshVersion;
const dest = this.selections[destField] || '';
const src = this.selections[srcField] || '';
if (!dest || !src || dest === src) {
this.impact = null;
this.loading = false;
return;
}
this.loading = true;
+ this.impact = null;
const params = new URLSearchParams({ dest_contributor: dest, src_contributor: src });
try {
const response = await fetch(`${impactUrl}?${params}`);
- this.impact = response.ok ? await response.json() : null;
+ const impact = response.ok ? await response.json() : null;
+ if (refreshVersion === this.refreshVersion) {
+ this.impact = impact;
+ }
} finally {
- this.loading = false;
+ if (refreshVersion === this.refreshVersion) {
+ this.loading = false;
+ }
}
},📝 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.
Suggested change
| async refresh() { | |
| const dest = this.selections[destField] || ''; | |
| const src = this.selections[srcField] || ''; | |
| // merging a contributor into itself is rejected by the form, so there's nothing to warn about | |
| if (!dest || !src || dest === src) { | |
| this.impact = null; | |
| this.loading = false; | |
| return; | |
| } | |
| this.loading = true; | |
| const params = new URLSearchParams({ dest_contributor: dest, src_contributor: src }); | |
| try { | |
| const response = await fetch(`${impactUrl}?${params}`); | |
| this.impact = response.ok ? await response.json() : null; | |
| } finally { | |
| this.loading = false; | |
| } | |
| async refresh() { | |
| const refreshVersion = ++this.refreshVersion; | |
| const dest = this.selections[destField] || ''; | |
| const src = this.selections[srcField] || ''; | |
| // merging a contributor into itself is rejected by the form, so there's nothing to warn about | |
| if (!dest || !src || dest === src) { | |
| this.impact = null; | |
| this.loading = false; | |
| return; | |
| } | |
| this.loading = true; | |
| this.impact = null; | |
| const params = new URLSearchParams({ dest_contributor: dest, src_contributor: src }); | |
| try { | |
| const response = await fetch(`${impactUrl}?${params}`); | |
| const impact = response.ok ? await response.json() : null; | |
| if (refreshVersion === this.refreshVersion) { | |
| this.impact = impact; | |
| } | |
| } finally { | |
| if (refreshVersion === this.refreshVersion) { | |
| this.loading = false; | |
| } | |
| } |
🤖 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 `@isic/ingest/static/ingest/contributor_merge_impact.js` around lines 16 - 34,
Update refresh() to clear impact when starting every valid request and track the
request or selection identity so responses from superseded fetches cannot update
impact. Preserve loading cleanup for the active request, and ensure changing or
clearing selections leaves stale results cleared. Add a browser test covering a
selection change or clear before the first response resolves.
|
❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
Member
Author
|
@coderabbitai resolve merge conflict |
|
✅ Merge conflicts resolved successfully! Resolved 2 conflict file(s). Commit: 11 file operation(s)
View agent analysis |
Resolved conflicts in: - isic/ingest/services/contributor/__init__.py (content) - isic/ingest/static/ingest/autocomplete.js (content) Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 2 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Member
Author
|
@coderabbitai fix-ci |
|
✅ CI fix PR created: Follow
3 PR-caused check(s)Showing 2 errors from 2 check run(s) out of ~3 total. Deferred 1 check run(s) (GitHub Actions: ci / 0_test.txt — test). These may resolve after fixing the shown errors, or may need a follow-up run.
1 file(s) modified
View agent analysis |
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
1 participant
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.
Summary by CodeRabbit
New Features
Bug Fixes
Tests