Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions isic/ingest/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from isic.ingest.models.lesion import get_lesion_count_for_user
from isic.ingest.services.accession import create_accession
from isic.ingest.services.accession.review import bulk_create_accession_reviews
from isic.ingest.services.contributor import compute_contributor_merge_impact
from isic.ingest.tasks import update_metadata_task

lesion_router = Router()
Expand Down Expand Up @@ -265,6 +266,45 @@ def contributor_list(request: HttpRequest):
)


class MergeImpactUserOut(Schema):
id: int
name: str
email: str
has_engagement_profile: bool


class MergeImpactContributorOut(Schema):
id: int
institution_name: str
accession_count: int
published_image_count: int


class ContributorMergeImpactOut(Schema):
dest_contributor: MergeImpactContributorOut
src_contributor: MergeImpactContributorOut
users_gaining_access_to_dest: list[MergeImpactUserOut]
users_gaining_access_to_src: list[MergeImpactUserOut]
engagement_profiles_repointed: list[MergeImpactUserOut]


@contributor_router.get(
"/merge-impact/",
response={200: ContributorMergeImpactOut, 400: dict},
summary="Describe the access impact of merging two contributors.",
include_in_schema=False,
auth=is_staff,
)
def contributor_merge_impact(request: HttpRequest, dest_contributor: int, src_contributor: int):
if dest_contributor == src_contributor:
return 400, {"error": "The two contributors must be different."}

return 200, compute_contributor_merge_impact(
dest_contributor=get_object_or_404(Contributor, id=dest_contributor),
src_contributor=get_object_or_404(Contributor, id=src_contributor),
)


@contributor_router.get(
"/{id}/",
response=ContributorDetailOut,
Expand Down
99 changes: 99 additions & 0 deletions isic/ingest/services/contributor/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
from dataclasses import dataclass

from django.contrib.auth.models import User
from django.db import transaction
from django.db.models import Count, Q

from isic.ingest.models.accession import Accession
from isic.ingest.models.contributor import Contributor


Expand Down Expand Up @@ -28,6 +32,101 @@ def create_contributor( # noqa: PLR0913
return contributor


@dataclass(frozen=True)
class MergeImpactUser:
id: int
name: str
email: str
has_engagement_profile: bool


@dataclass(frozen=True)
class MergeImpactContributor:
id: int
institution_name: str
accession_count: int
published_image_count: int


@dataclass(frozen=True)
class ContributorMergeImpact:
dest_contributor: MergeImpactContributor
src_contributor: MergeImpactContributor
# owners of the source that aren't owners of the destination yet, they gain access to
# everything the destination already has.
users_gaining_access_to_dest: list[MergeImpactUser]
# owners of the destination that aren't owners of the source, they gain access to the
# source's data once it moves under the destination.
users_gaining_access_to_src: list[MergeImpactUser]
engagement_profiles_repointed: list[MergeImpactUser]


def _merge_impact_contributor(contributor: Contributor) -> MergeImpactContributor:
counts = Accession.objects.filter(cohort__contributor=contributor).aggregate(
accession_count=Count("id"),
# mirrors AccessionQuerySet.published, an accession is published once it has an image
published_image_count=Count("id", filter=Q(image__isnull=False)),
)
return MergeImpactContributor(
id=contributor.pk,
institution_name=contributor.institution_name,
accession_count=counts["accession_count"],
published_image_count=counts["published_image_count"],
)


def _merge_impact_user(user: User, *, has_engagement_profile: bool) -> MergeImpactUser:
return MergeImpactUser(
id=user.pk,
name=user.get_full_name() or user.email,
email=user.email,
has_engagement_profile=has_engagement_profile,
)


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.
"""
dest_owners = list(dest_contributor.owners.order_by("email"))
src_owners = list(src_contributor.owners.order_by("email"))

engagement_user_ids = set(
User.objects.filter(
pk__in=[user.pk for user in dest_owners + src_owners],
engagement_profile__isnull=False,
).values_list("pk", flat=True)
)

def users_gaining_access(
owners: list[User], existing_owners: list[User]
) -> list[MergeImpactUser]:
existing_owner_ids = {user.pk for user in existing_owners}
return [
_merge_impact_user(user, has_engagement_profile=user.pk in engagement_user_ids)
for user in owners
if user.pk not in existing_owner_ids
]

return ContributorMergeImpact(
dest_contributor=_merge_impact_contributor(dest_contributor),
src_contributor=_merge_impact_contributor(src_contributor),
users_gaining_access_to_dest=users_gaining_access(src_owners, dest_owners),
users_gaining_access_to_src=users_gaining_access(dest_owners, src_owners),
engagement_profiles_repointed=[
_merge_impact_user(profile.user, has_engagement_profile=True)
for profile in src_contributor.engagement_profiles.select_related("user").order_by(
"user__email"
)
],
)


def merge_contributors(*, dest_contributor: Contributor, src_contributor: Contributor) -> None:
"""Merge a src_contributor into dest_contributor."""
with transaction.atomic():
Expand Down
30 changes: 29 additions & 1 deletion isic/ingest/static/ingest/autocomplete.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
// Alpine component backing the autocomplete form fields, e.g. the merge cohorts/contributors
// pages. suggestUrl returns a list of matches for a query, detailUrl returns a single object
// (by id) to preview, and labelKey names the field that's displayed for a match.
function autocompleteInput({ suggestUrl, detailUrl, labelKey = 'name', required = false }) {
function autocompleteInput({
suggestUrl,
detailUrl,
labelKey = 'name',
required = false,
fieldName = '',
}) {
return {
selectedId: '',
selectedDetail: null,
loadingSelectedDetail: false,
query: '',
suggestions: [],
loadingSuggestions: false,
rootEl: null,

// the selection lives in a hidden input, which the browser bars from constraint validation,
// so the visible search box carries the requirement instead. typing without picking a
Expand All @@ -18,6 +25,9 @@ function autocompleteInput({ suggestUrl, detailUrl, labelKey = 'name', required
},

async init() {
// notifySelection can run after the clicked suggestion has been removed from the DOM, and
// events dispatched from a detached element never bubble, so hold onto the root element.
this.rootEl = this.$el;
this.$watch('selectedDetail', (selection) =>
this.$dispatch('autocomplete-selected', { selection }),
);
Expand All @@ -27,17 +37,30 @@ function autocompleteInput({ suggestUrl, detailUrl, labelKey = 'name', required
await this.populateDetail();
this.query = this.label(this.selectedDetail);
}
this.notifySelection();
},

label(item) {
return item ? item[labelKey] : '';
},

// let an ancestor react to selections made across otherwise independent autocomplete fields,
// e.g. the contributor merge impact panel. pages without a listener are unaffected.
notifySelection() {
this.rootEl.dispatchEvent(
new CustomEvent('autocomplete-selection', {
detail: { name: fieldName, id: this.selectedId },
bubbles: true,
})
);
},

async select(item) {
this.selectedId = item.id;
this.query = this.label(item);
this.suggestions = [];
await this.populateDetail();
this.notifySelection();
},

async populateDetail() {
Expand All @@ -48,9 +71,14 @@ function autocompleteInput({ suggestUrl, detailUrl, labelKey = 'name', required
},

async fetchSuggestions() {
// typing invalidates the selection, but only the first keystroke actually changes it
const hadSelection = Boolean(this.selectedId);
this.selectedId = '';
this.selectedDetail = null;
this.suggestions = [];
if (hadSelection) {
this.notifySelection();
}
if (this.query.length >= 3) {
this.loadingSuggestions = true;
const response = await fetch(`${suggestUrl}?query=${encodeURIComponent(this.query)}`);
Expand Down
63 changes: 63 additions & 0 deletions isic/ingest/static/ingest/contributor_merge_impact.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
// Alpine component for the merge contributors page. It listens for selections from the two
// autocomplete fields, which are otherwise independent components, and loads a summary of who
// gains access to what once both contributors are chosen.
function contributorMergeImpact({ impactUrl, destField, srcField }) {
return {
selections: {},
impact: null,
loading: false,
requestCounter: 0,

onAutocompleteSelection({ name, id }) {
if (name !== destField && name !== srcField) return;
this.selections[name] = id || '';
this.refresh();
},

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

// Clear impact immediately and track this request
this.impact = null;
this.loading = true;
const thisRequest = ++this.requestCounter;
const params = new URLSearchParams({ dest_contributor: dest, src_contributor: src });
try {
const response = await fetch(`${impactUrl}?${params}`);
// Only update impact if this is still the most recent request
if (thisRequest === this.requestCounter) {
this.impact = response.ok ? await response.json() : null;
}
} finally {
// Only clear loading if this is still the most recent request
if (thisRequest === this.requestCounter) {
this.loading = false;
}
}
Comment on lines +17 to +44

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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.

},

get anyoneGainsAccess() {
return Boolean(
this.impact &&
(this.impact.users_gaining_access_to_dest.length ||
this.impact.users_gaining_access_to_src.length)
);
},

number(count) {
return new Intl.NumberFormat('en-US').format(count ?? 0);
},

pluralize(count, singular, plural) {
return count === 1 ? singular : plural;
},
};
}
7 changes: 6 additions & 1 deletion isic/ingest/templates/ingest/contributor_merge.html
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@
{% block head_extra %}
{{ block.super }}
<script src="{% static 'ingest/autocomplete.js' %}"></script>
<script src="{% static 'ingest/contributor_merge_impact.js' %}"></script>
{% endblock %}

{% block content %}
<form class="flex flex-col max-w-2xl mx-auto space-y-4" method="post">
<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)">
{% csrf_token %}

{{ form.non_field_errors }}
Expand All @@ -19,6 +22,8 @@
{% include 'ingest/partials/autocomplete_field.html' with field=form.contributor suggest_url=contributor_suggest_url detail_url=contributor_api_url label_key='institution_name' detail_template='ingest/partials/contributor_autocomplete_detail.html' %}
{% include 'ingest/partials/autocomplete_field.html' with field=form.contributor_to_merge suggest_url=contributor_suggest_url detail_url=contributor_api_url label_key='institution_name' detail_template='ingest/partials/contributor_autocomplete_detail.html' %}

{% include 'ingest/partials/contributor_merge_impact.html' %}

<button type="submit" class="btn btn-primary self-end">Merge Contributors</button>
</form>
{% endblock %}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
<fieldset class="fieldset" x-data="autocompleteInput({suggestUrl: '{{ suggest_url }}', detailUrl: '{{ detail_url }}', labelKey: '{{ label_key|default:"name" }}', required: {{ field.field.required|yesno:"true,false" }}})">
<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 }}'})">

<label class="fieldset-legend">{{ field.label }}</label>

<input
Expand Down
Loading
Loading