diff --git a/TODO.md b/TODO.md index 44cd4817..32a22900 100644 --- a/TODO.md +++ b/TODO.md @@ -3,12 +3,10 @@ ## Top - Before new release + -- Fix order of forms that source / destination fields are at the top again -- test job_utils - -- move mixins stuff over to consumer and delete the stuff in slective transfer view (we never post there) -- exclude autoreload when tests are saved (Custom Filter in server command watched files) -- Test canceled task/job in test_workers.py - -- Test locked of queued task - -- Fix pyright (wait for new release) - Rename C_STORE to C-STORE and so on in dimse connector - Replace AssertionError with assert diff --git a/adit/batch_query/utils/query_utils.py b/adit/batch_query/utils/query_utils.py index 58541849..b2cc62e2 100644 --- a/adit/batch_query/utils/query_utils.py +++ b/adit/batch_query/utils/query_utils.py @@ -23,6 +23,7 @@ class QueryExecutor: def __init__(self, query_task: BatchQueryTask) -> None: self.query_task = query_task + # Make sure source is accessible by the user source_node = DicomNode.objects.accessible_by_user(self.query_task.job.owner, "source").get( pk=self.query_task.source.pk ) diff --git a/adit/core/templates/core/js_required_hint.html b/adit/core/templates/core/js_required_hint.html new file mode 100644 index 00000000..f026b6ac --- /dev/null +++ b/adit/core/templates/core/js_required_hint.html @@ -0,0 +1,8 @@ +{% extends "core/core_layout.html" %} +{% block heading %} +

JavaScript Required

+{% endblock heading %} +{% block content %} + + ← Go Back +{% endblock content %} diff --git a/adit/core/utils/transfer_utils.py b/adit/core/utils/transfer_utils.py index 7b5208b1..9e90aa50 100644 --- a/adit/core/utils/transfer_utils.py +++ b/adit/core/utils/transfer_utils.py @@ -30,6 +30,7 @@ class TransferExecutor: def __init__(self, transfer_task: TransferTask) -> None: self.transfer_task = transfer_task + # Make sure source and destination are accessible by the user source_node = DicomNode.objects.accessible_by_user( self.transfer_task.job.owner, "source" ).get(pk=self.transfer_task.source.pk) diff --git a/adit/selective_transfer/consumers.py b/adit/selective_transfer/consumers.py index 7aebe69f..64b3cf9e 100644 --- a/adit/selective_transfer/consumers.py +++ b/adit/selective_transfer/consumers.py @@ -3,7 +3,8 @@ import logging import threading from concurrent.futures import ThreadPoolExecutor -from typing import Any +from datetime import datetime +from typing import Any, Iterator, cast from asgiref.sync import async_to_sync from channels.db import database_sync_to_async @@ -13,11 +14,13 @@ from django.template.loader import render_to_string from adit.accounts.models import User -from adit.core.utils.dicom_dataset import ResultDataset +from adit.core.models import DicomNode +from adit.core.utils.dicom_dataset import QueryDataset, ResultDataset from adit.core.utils.dicom_operator import DicomOperator +from adit.core.utils.job_utils import queue_pending_tasks from .forms import SelectiveTransferJobForm -from .mixins import SelectiveTransferJobCreateMixin +from .models import SelectiveTransferJob, SelectiveTransferTask from .utils.debounce import debounce from .views import SELECTIVE_TRANSFER_ADVANCED_OPTIONS_COLLAPSED @@ -47,7 +50,7 @@ def render_error_message(message) -> str: ) -class SelectiveTransferConsumer(SelectiveTransferJobCreateMixin, AsyncJsonWebsocketConsumer): +class SelectiveTransferConsumer(AsyncJsonWebsocketConsumer): async def connect(self): logger.debug("Connected to WebSocket client.") @@ -149,19 +152,24 @@ def _build_form_error_response(self, form: SelectiveTransferJobForm, message: st async def _make_query(self, form: SelectiveTransferJobForm, message_id: int) -> None: loop = asyncio.get_event_loop() await loop.run_in_executor( - self.pool, self._create_and_send_query_response, form, message_id + self.pool, self._generate_and_send_query_response, form, message_id ) - def _create_and_send_query_response( + def _generate_and_send_query_response( self, form: SelectiveTransferJobForm, message_id: int ) -> None: with lock: if message_id != self.current_message_id: return - # TODO: we have to query multiple servers here by creating multiple operators, - # also the source must be attached somehow - operator = self.create_source_operator(form) + # Make sure source is accessible by the user + source = cast(DicomNode, form.cleaned_data["source"]) + source_node = DicomNode.objects.accessible_by_user(self.user, "source").get( + pk=source.pk + ) + assert source_node.node_type == DicomNode.NodeType.SERVER + operator = DicomOperator(source_node.dicomserver) + self.query_operators.append(operator) try: @@ -191,6 +199,38 @@ def _create_and_send_query_response( return None + def query_studies( + self, operator: DicomOperator, form: SelectiveTransferJobForm, limit_results: int + ) -> Iterator[ResultDataset]: + data = form.cleaned_data + + if data["modality"] in settings.EXCLUDED_MODALITIES: + return [] + + studies = operator.find_studies( + QueryDataset.create( + PatientID=data["patient_id"], + PatientName=data["patient_name"], + PatientBirthDate=data["patient_birth_date"], + AccessionNumber=data["accession_number"], + StudyDate=data["study_date"], + ModalitiesInStudy=data["modality"], + ), + limit_results=limit_results, + ) + + def has_only_excluded_modalities(study: ResultDataset): + modalities_in_study = set(study.ModalitiesInStudy) + excluded_modalities = set(settings.EXCLUDED_MODALITIES) + not_excluded_modalities = list(modalities_in_study - excluded_modalities) + return len(not_excluded_modalities) == 0 + + for study in studies: + if has_only_excluded_modalities(study): + continue + + yield study + @debounce() def send_query_response( self, @@ -201,7 +241,11 @@ def send_query_response( # Rerender form to remove potential previous error messages rendered_form = render_crispy_form(form) - studies = self.sort_studies(studies) + studies = sorted( + studies, + key=lambda study: datetime.combine(study.StudyDate, study.StudyTime), + reverse=True, + ) rendered_query_results = render_to_string( "selective_transfer/_query_results.html", @@ -240,3 +284,40 @@ def build_transfer_response( {"transfer": True, "created_job": job}, ) return rendered_form + rendered_created_job + + def transfer_selected_studies( + self, user: User, form: SelectiveTransferJobForm, selected_studies: list[str] + ) -> SelectiveTransferJob: + if not selected_studies: + raise ValueError("At least one study to transfer must be selected.") + if len(selected_studies) > 10 and not user.is_staff: + raise ValueError("Maximum 10 studies per selective transfer are allowed.") + + form.instance.owner = user + job = form.save() + + pseudonym = form.cleaned_data["pseudonym"] + for selected_study in selected_studies: + study_data = selected_study.split("\\") + patient_id = study_data[0] + study_uid = study_data[1] + SelectiveTransferTask.objects.create( + job=job, + source=form.cleaned_data["source"], + destination=form.cleaned_data["destination"], + patient_id=patient_id, + study_uid=study_uid, + pseudonym=pseudonym, + ) + + if user.is_staff or settings.SELECTIVE_TRANSFER_UNVERIFIED: + job.status = SelectiveTransferJob.Status.PENDING + job.save() + + queue_pending_tasks( + job, + settings.SELECTIVE_TRANSFER_DEFAULT_PRIORITY, + settings.SELECTIVE_TRANSFER_URGENT_PRIORITY, + ) + + return job diff --git a/adit/selective_transfer/forms.py b/adit/selective_transfer/forms.py index 1897c4a9..affbe78a 100644 --- a/adit/selective_transfer/forms.py +++ b/adit/selective_transfer/forms.py @@ -56,7 +56,7 @@ class Meta: def __init__(self, *args, **kwargs): self.user = kwargs.pop("user") - self.action = kwargs.pop("action") + self.action = kwargs.pop("action", "") self.advanced_options_collapsed = kwargs.pop("advanced_options_collapsed") super().__init__(*args, **kwargs) diff --git a/adit/selective_transfer/mixins.py b/adit/selective_transfer/mixins.py index e58b0545..1cb332b0 100644 --- a/adit/selective_transfer/mixins.py +++ b/adit/selective_transfer/mixins.py @@ -1,111 +1,9 @@ -from datetime import datetime -from typing import Iterator - -from django.conf import settings - -from adit.accounts.models import User from adit.core.mixins import LockedMixin -from adit.core.utils.dicom_dataset import QueryDataset, ResultDataset -from adit.core.utils.dicom_operator import DicomOperator -from adit.core.utils.job_utils import queue_pending_tasks from .apps import SECTION_NAME -from .forms import SelectiveTransferJobForm -from .models import SelectiveTransferJob, SelectiveTransferSettings, SelectiveTransferTask +from .models import SelectiveTransferSettings class SelectiveTransferLockedMixin(LockedMixin): settings_model = SelectiveTransferSettings section_name = SECTION_NAME - - -class SelectiveTransferJobCreateMixin: - """A mixin shared by the "SelectiveTransferJobCreateView and the SelectiveTransferConsumer. - - It contains common logic to create a selective transfer job. The view has some rudimentary - functionality to make a query, display the result and create the job. The consumer - does this more interactively and is the preferred way, but depends on JavaScript and - WebSockets. The view is a fallback for browsers without JavaScript support. - TODO: As more and more things inside ADIT now depend on JavaScript, we should consider - to remove the view logic to query and create a job from the view and only use the consumer. - The view then just renders the initial form and the consumer does the rest. - """ - - def create_source_operator(self, form: SelectiveTransferJobForm) -> DicomOperator: - return DicomOperator(form.cleaned_data["source"].dicomserver) - - def query_studies( - self, operator: DicomOperator, form: SelectiveTransferJobForm, limit_results: int - ) -> Iterator[ResultDataset]: - data = form.cleaned_data - - if data["modality"] in settings.EXCLUDED_MODALITIES: - return [] - - studies = operator.find_studies( - QueryDataset.create( - PatientID=data["patient_id"], - PatientName=data["patient_name"], - PatientBirthDate=data["patient_birth_date"], - AccessionNumber=data["accession_number"], - StudyDate=data["study_date"], - ModalitiesInStudy=data["modality"], - ), - limit_results=limit_results, - ) - - def has_only_excluded_modalities(study: ResultDataset): - modalities_in_study = set(study.ModalitiesInStudy) - excluded_modalities = set(settings.EXCLUDED_MODALITIES) - not_excluded_modalities = list(modalities_in_study - excluded_modalities) - return len(not_excluded_modalities) == 0 - - for study in studies: - if has_only_excluded_modalities(study): - continue - - yield study - - def sort_studies(self, studies: list[ResultDataset]) -> list[ResultDataset]: - return sorted( - studies, - key=lambda study: datetime.combine(study.StudyDate, study.StudyTime), - reverse=True, - ) - - def transfer_selected_studies( - self, user: User, form: SelectiveTransferJobForm, selected_studies: list[str] - ) -> SelectiveTransferJob: - if not selected_studies: - raise ValueError("At least one study to transfer must be selected.") - if len(selected_studies) > 10 and not user.is_staff: - raise ValueError("Maximum 10 studies per selective transfer are allowed.") - - form.instance.owner = user - job = form.save() - - pseudonym = form.cleaned_data["pseudonym"] - for selected_study in selected_studies: - study_data = selected_study.split("\\") - patient_id = study_data[0] - study_uid = study_data[1] - SelectiveTransferTask.objects.create( - job=job, - source=form.cleaned_data["source"], - destination=form.cleaned_data["destination"], - patient_id=patient_id, - study_uid=study_uid, - pseudonym=pseudonym, - ) - - if user.is_staff or settings.SELECTIVE_TRANSFER_UNVERIFIED: - job.status = SelectiveTransferJob.Status.PENDING - job.save() - - queue_pending_tasks( - job, - settings.SELECTIVE_TRANSFER_DEFAULT_PRIORITY, - settings.SELECTIVE_TRANSFER_URGENT_PRIORITY, - ) - - return job diff --git a/adit/selective_transfer/views.py b/adit/selective_transfer/views.py index bd124247..ee3ae824 100644 --- a/adit/selective_transfer/views.py +++ b/adit/selective_transfer/views.py @@ -1,10 +1,10 @@ -from datetime import datetime from typing import Any from django.conf import settings -from django.http import HttpResponseBadRequest +from django.shortcuts import render from django.urls import reverse_lazy +from adit.core.types import AuthenticatedHttpRequest from adit.core.views import ( BaseUpdatePreferencesView, DicomJobCancelView, @@ -21,7 +21,7 @@ from .filters import SelectiveTransferJobFilter, SelectiveTransferTaskFilter from .forms import SelectiveTransferJobForm -from .mixins import SelectiveTransferJobCreateMixin, SelectiveTransferLockedMixin +from .mixins import SelectiveTransferLockedMixin from .models import SelectiveTransferJob, SelectiveTransferTask from .tables import SelectiveTransferJobTable, SelectiveTransferTaskTable @@ -51,28 +51,28 @@ class SelectiveTransferJobListView(SelectiveTransferLockedMixin, TransferJobList template_name = "selective_transfer/selective_transfer_job_list.html" -class SelectiveTransferJobCreateView( - SelectiveTransferLockedMixin, SelectiveTransferJobCreateMixin, DicomJobCreateView -): +class SelectiveTransferJobCreateView(SelectiveTransferLockedMixin, DicomJobCreateView): """A view class to render the selective transfer form. - POST (and the creation of the job) is not handled by this view because the - job itself is created by using the REST API and an AJAX call. - - TODO: Maybe we should only use this view to render the form and not for processing - it as normally we use the WebSocket consumer for that. It is a relict and a - fallback when no JavaScript or WebSockets are available. + The form data itself is not processed by this view but by using WebSockets (see + consumer.py). That way long running queries and cancellation can be used. """ form_class = SelectiveTransferJobForm template_name = "selective_transfer/selective_transfer_job_form.html" permission_required = "selective_transfer.add_selectivetransferjob" + request: AuthenticatedHttpRequest + + def post(self, request, *args, **kwargs): + return render( + request, + "core/js_required_hint.html", + {"hint": "Selective transfer requires JavaScript to work properly."}, + ) def get_form_kwargs(self) -> dict[str, Any]: kwargs = super().get_form_kwargs() - kwargs["action"] = self.request.POST.get("action") - preferences: dict[str, Any] = self.request.user.preferences kwargs["advanced_options_collapsed"] = preferences.get( SELECTIVE_TRANSFER_ADVANCED_OPTIONS_COLLAPSED, False @@ -103,46 +103,6 @@ def get_initial(self): return initial - def form_invalid(self, form): - error_message = "Please correct the form errors and search again." - return self.render_to_response( - self.get_context_data(form=form, error_message=error_message) - ) - - def form_valid(self, form: SelectiveTransferJobForm): - action = self.request.POST.get("action") - - if action == "query": - connector = self.create_source_operator(form) - limit = settings.SELECTIVE_TRANSFER_RESULT_LIMIT - studies = list(self.query_studies(connector, form, limit)) - studies = sorted( - studies, - key=lambda study: datetime.combine(study.StudyDate, study.StudyTime), - reverse=True, - ) - max_results_reached = len(studies) >= limit - return self.render_to_response( - self.get_context_data( - query=True, - query_results=studies, - max_results_reached=max_results_reached, - ) - ) - - if action == "transfer": - user = self.request.user - selected_studies = self.request.POST.getlist("selected_studies") - try: - job = self.transfer_selected_studies(user, form, selected_studies) - except ValueError as err: - return self.render_to_response( - self.get_context_data(transfer=True, error_message=str(err)) - ) - return self.render_to_response(self.get_context_data(transfer=True, created_job=job)) - - return HttpResponseBadRequest() - class SelectiveTransferJobDetailView(SelectiveTransferLockedMixin, DicomJobDetailView): table_class = SelectiveTransferTaskTable