Add a permissions warning on contributor merge

[danlamanna]

No description provided.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a “merge impact” preview to the contributor merge workflow so staff can see which users would gain access (and which engagement defaults would be repointed) before submitting a merge.

Changes:

• Introduces compute_contributor_merge_impact service + API endpoint to compute/serve merge impact details.
• Adds UI partials + Alpine component to fetch/render merge impact live as autocomplete selections change.
• Expands test coverage with unit/API tests and a Playwright browser test for the new panel.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
isic/ingest/tests/test_merge_contributors_browser.py	Adds Playwright coverage for the new merge impact panel behavior.
isic/ingest/tests/test_merge.py	Adds unit tests for compute_contributor_merge_impact.
isic/ingest/tests/test_api_contributor.py	Adds API tests for merge impact endpoint + permissions.
isic/ingest/templates/ingest/partials/merge_impact_users.html	New partial for rendering user lists in the impact panel.
isic/ingest/templates/ingest/partials/contributor_merge_impact.html	New impact panel UI showing access changes and repointed engagement defaults.
isic/ingest/templates/ingest/partials/autocomplete_field.html	Passes field name into autocomplete Alpine component for cross-field coordination.
isic/ingest/templates/ingest/contributor_merge.html	Wires in impact panel and Alpine component on contributor merge page.
isic/ingest/static/ingest/contributor_merge_impact.js	New Alpine component that fetches and computes view state for merge impact.
isic/ingest/static/ingest/autocomplete.js	Dispatches selection-change events so pages can react to autocomplete changes.
isic/ingest/services/contributor/init.py	Adds merge impact dataclasses + computation logic.
isic/ingest/api.py	Adds a staff-only merge impact endpoint and response schemas.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (5)

isic/ingest/templates/ingest/partials/autocomplete_field.html:1

• This injects field.html_name into a JavaScript string context without JS-escaping. If the field name ever contains a quote/backslash, it can break the x-data expression and could enable XSS in worst cases. Use Django’s escapejs filter (or otherwise ensure safe encoding) for values embedded into JS strings.

<fieldset class="fieldset" x-data="autocompleteInput({suggestUrl: '{{ suggest_url }}', detailUrl: '{{ detail_url }}', labelKey: '{{ label_key|default:"name" }}', required: {{ field.field.required|yesno:"true,false" }}, fieldName: '{{ field.html_name }}'})">

isic/ingest/templates/ingest/contributor_merge.html:13

• Similar to the autocomplete partial, form.<...>.html_name is embedded directly into a JS string inside x-data. These should be JS-escaped (e.g., |escapejs) to prevent malformed Alpine expressions (and potential XSS) if a field name contains special characters.

  <form class="flex flex-col max-w-2xl mx-auto space-y-4" method="post"
        x-data="contributorMergeImpact({impactUrl: '{% url 'api:contributor_merge_impact' %}', destField: '{{ form.contributor.html_name }}', srcField: '{{ form.contributor_to_merge.html_name }}'})"
        @autocomplete-selection="onAutocompleteSelection($event.detail)">

isic/ingest/static/ingest/contributor_merge_impact.js:34

• If fetch() throws (network error / aborted request), this.impact is left unchanged, which can display stale impact data for a new selection. Consider adding a catch that clears this.impact (and optionally captures an error state/message) so the UI can’t show outdated results.

      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;
      }

isic/ingest/static/ingest/contributor_merge_impact.js:35

• Multiple refresh() calls can be in-flight at once (e.g., rapid changes across the two autocompletes). A slower earlier response can arrive last and overwrite this.impact with stale data. Consider using an AbortController per request or a monotonically increasing request id (only applying the latest response) to avoid out-of-order updates.

    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;
      }
    },

isic/ingest/services/contributor/init.py:71

• The docstring states the contributors must be different, but the function doesn’t enforce that precondition. Since this is a service-layer API (and can be called outside the view), consider adding an explicit check (e.g., raising a ValueError) or adjusting the docstring to reflect actual behavior.

def compute_contributor_merge_impact(
    *, dest_contributor: Contributor, src_contributor: Contributor
) -> ContributorMergeImpact:
    """
    Describe who gains access to what by merging src_contributor into dest_contributor.

    Access is granted exclusively by Contributor.owners, so merging exposes each contributor's
    data to the other's owners. Note that the two contributors must be different.
    """

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (4)

isic/ingest/static/ingest/contributor_merge_impact.js:33

• Concurrent refreshes are not correlated with the current selection. If an earlier request finishes after the user clears or changes a contributor, its response can restore a stale impact, and its finally can also clear loading while the latest request is still pending. The form can then present permissions for a different merge. Use an abort controller or monotonically increasing request token, and only update impact/loading for the latest refresh (including invalidating requests in the early-return branch).

      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;

isic/ingest/static/ingest/autocomplete.js:59

• The merge-impact event is emitted only after the unrelated detail request succeeds. If that request fails, the ancestor never learns about the valid selection and the permissions warning is never requested. Emit the selection immediately after assigning selectedId, before awaiting the preview lookup.

      await this.populateDetail();
      this.notifySelection();

isic/ingest/static/ingest/autocomplete.js:36

• The same dependency affects restored form values: when selectedId is present, a failed/slow detail lookup prevents the ancestor from learning the initial selection, so the warning is not restored. Notify immediately after reading the hidden input, then load its display details.

This issue also appears on line 58 of the same file.

      if (this.selectedId) {
        await this.populateDetail();
        this.query = this.label(this.selectedDetail);
      }
      this.notifySelection();

isic/ingest/templates/ingest/contributor_merge.html:13

• The form remains submittable before and while the asynchronous impact request runs, so on a slow response staff can complete the merge without ever seeing the new permissions warning. Gate submission on having a successfully loaded impact for the current two IDs (and surface a load error rather than silently enabling the merge); this also requires emitting autocomplete selections before the detail lookup completes.

  <form class="flex flex-col max-w-2xl mx-auto space-y-4" method="post"
        x-data="contributorMergeImpact({impactUrl: '{% url 'api:contributor_merge_impact' %}', destField: '{{ form.contributor.html_name }}', srcField: '{{ form.contributor_to_merge.html_name }}'})"
        @autocomplete-selection="onAutocompleteSelection($event.detail)">