-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add export for participant requests #3578
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
from django.db.models import Prefetch | ||
from django_filters.rest_framework import DjangoFilterBackend | ||
from rest_framework import mixins | ||
from rest_framework.settings import api_settings | ||
from rest_framework.viewsets import GenericViewSet | ||
|
||
from grandchallenge.api.permissions import IsAuthenticated | ||
from grandchallenge.challenges.models import Challenge | ||
from grandchallenge.core.guardian import filter_by_permission | ||
from grandchallenge.participants.models import ( | ||
RegistrationQuestion, | ||
RegistrationQuestionAnswer, | ||
RegistrationRequest, | ||
) | ||
from grandchallenge.participants.renderers import ( | ||
RegistrationRequestCSVRenderer, | ||
) | ||
from grandchallenge.participants.serializers import ( | ||
RegistrationRequestSerializer, | ||
) | ||
|
||
|
||
class RegistrationRequestViewSet(mixins.ListModelMixin, GenericViewSet): | ||
serializer_class = RegistrationRequestSerializer | ||
permission_classes = [IsAuthenticated] | ||
filter_backends = [DjangoFilterBackend] | ||
filterset_fields = ["challenge__short_name"] | ||
renderer_classes = ( | ||
*api_settings.DEFAULT_RENDERER_CLASSES, | ||
RegistrationRequestCSVRenderer, | ||
) | ||
|
||
def get_queryset(self): | ||
viewable_challenges = filter_by_permission( | ||
queryset=Challenge.objects.all(), | ||
user=self.request.user, | ||
codename="change_challenge", | ||
accept_user_perms=False, | ||
) | ||
|
||
# Permission check on challenges | ||
queryset = RegistrationRequest.objects.filter( | ||
challenge__in=viewable_challenges | ||
) | ||
|
||
viewable_questions = filter_by_permission( | ||
queryset=RegistrationQuestion.objects.filter( | ||
challenge__in=viewable_challenges | ||
).all(), | ||
user=self.request.user, | ||
codename="view_registrationquestion", | ||
accept_user_perms=False, | ||
) | ||
|
||
# Prefetch with filter for viewable questions | ||
queryset = queryset.prefetch_related( | ||
Prefetch( | ||
"registration_question_answers", | ||
RegistrationQuestionAnswer.objects.filter( | ||
question__in=viewable_questions | ||
), | ||
), | ||
"registration_question_answers__question", | ||
) | ||
|
||
return queryset.select_related( | ||
"user__user_profile", | ||
).order_by("created") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import json | ||
|
||
from rest_framework.settings import api_settings | ||
from rest_framework.utils.encoders import JSONEncoder | ||
from rest_framework_csv.renderers import PaginatedCSVRenderer | ||
|
||
|
||
class RegistrationRequestCSVRenderer(PaginatedCSVRenderer): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than flattening stuff with a special renderer why not serialize the answers instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was 50/50. I had the answers as a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's what it is though - it is a JSON field so in the future may not necessarily be a string. I think it is best to take the same approach as the evaluation serialisation and serialise things on the answer level, rather than the registration request. Permissions are already implemented for that so is less impact. You would then get some duplicate data that is serialised along with each answer (e.g. the username, the request status, the question text) but that is okay for now, it's not much. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean to have a CSV row per answer? If so, what about registrees that did not answer any questions? IMHO they should be included in any challenge registration overview. Plus, there are no permissions for the
Are you referring to the permissions for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Empty answers are saved for users that don't answer, right? The permissions weren't added because they weren't needed. Now they are needed if you want to add this view in line with the existing framework. We should do that rather than trying to fight it with custom renderers specific to one particular field. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Empty answers are indeed saved (by design), but if there are no questions at the time of sign-up, the registree would now show up in an answer-level serialization. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then add another viewset, serialiser and permissions for the registration requests and link to those from the answers. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. The current plan (somewhere in the next cooldown week) is to have two routes:
|
||
def flatten_dict(self, d): | ||
partial_flat_dict = {} | ||
for key, item in d.items(): | ||
if key == "registration_question_answers": | ||
partial_flat_dict[key] = ( | ||
self.flatten_registration_question_answers(item) | ||
) | ||
|
||
d.update(partial_flat_dict) | ||
return super().flatten_dict(d) | ||
|
||
def flatten_registration_question_answers(self, answers): | ||
""" | ||
Answers are potentially rich JSON objects, | ||
and should not be traversed during tablization | ||
""" | ||
for answer in answers: | ||
answer["answer"] = json.dumps( | ||
answer["answer"], | ||
cls=JSONEncoder, | ||
ensure_ascii=not api_settings.UNICODE_JSON, | ||
allow_nan=not api_settings.STRICT_JSON, | ||
) | ||
return self.flatten_item(answers) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
from rest_framework.fields import CharField, SerializerMethodField | ||
from rest_framework.serializers import ModelSerializer | ||
|
||
from grandchallenge.participants.models import ( | ||
RegistrationQuestion, | ||
RegistrationQuestionAnswer, | ||
RegistrationRequest, | ||
) | ||
from grandchallenge.profiles.serializers import UserProfileSerializer | ||
|
||
|
||
class RegistrationQuestionSerializer(ModelSerializer): | ||
|
||
class Meta: | ||
model = RegistrationQuestion | ||
fields = ( | ||
"question_text", | ||
"question_help_text", | ||
"required", | ||
) | ||
|
||
|
||
class RegistrationQuestionAnswerSerializer(ModelSerializer): | ||
|
||
question = RegistrationQuestionSerializer() | ||
|
||
class Meta: | ||
model = RegistrationQuestionAnswer | ||
fields = ( | ||
"question", | ||
"answer", | ||
) | ||
|
||
|
||
class RegistrationRequestSerializer(ModelSerializer): | ||
challenge = CharField(read_only=True, source="challenge.short_name") | ||
user = UserProfileSerializer(read_only=True, source="user.user_profile") | ||
registration_status = SerializerMethodField(read_only=True) | ||
registration_question_answers = RegistrationQuestionAnswerSerializer( | ||
many=True | ||
) | ||
|
||
class Meta: | ||
model = RegistrationRequest | ||
fields = ( | ||
"challenge", | ||
"user", | ||
"created", | ||
"changed", | ||
"registration_status", | ||
"registration_question_answers", | ||
) | ||
|
||
def get_registration_status(self, obj): | ||
return obj.get_status_display() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,139 @@ | ||
import pytest | ||
from guardian.shortcuts import remove_perm | ||
|
||
from grandchallenge.participants.models import ( | ||
RegistrationQuestionAnswer, | ||
RegistrationRequest, | ||
) | ||
from tests.factories import ( | ||
ChallengeFactory, | ||
RegistrationQuestionFactory, | ||
RegistrationRequestFactory, | ||
UserFactory, | ||
) | ||
from tests.utils import get_view_for_user | ||
|
||
|
||
@pytest.mark.django_db | ||
def test_registration_request_api_list(client): | ||
ch = ChallengeFactory() | ||
admin, participant, non_part_user = UserFactory.create_batch(3) | ||
ch.add_admin(admin) | ||
ch.add_participant(participant) | ||
|
||
rq_0 = RegistrationQuestionFactory(challenge=ch, question_text="Foo") | ||
rq_1 = RegistrationQuestionFactory(challenge=ch, question_text="No Foo") | ||
|
||
remove_perm( | ||
"view_registrationquestion", ch.admins_group, rq_1 | ||
) # non-permitted viewable question | ||
|
||
RegistrationRequestFactory( | ||
challenge=ch, user=participant, status=RegistrationRequest.ACCEPTED | ||
) # Note: has no answer to the questions, should still show in the export | ||
|
||
rr_1 = RegistrationRequestFactory(challenge=ch, user=non_part_user) | ||
|
||
RegistrationQuestionAnswer.objects.create( | ||
question=rq_0, | ||
registration_request=rr_1, | ||
answer="bar", | ||
) | ||
RegistrationQuestionAnswer.objects.create( | ||
question=rq_1, | ||
registration_request=rr_1, | ||
answer="no bar", | ||
) | ||
|
||
response = get_view_for_user( | ||
viewname="api:registration-request-list", | ||
client=client, | ||
user=admin, | ||
) | ||
|
||
assert response.status_code == 200, "Admin can query api for list" | ||
|
||
result = response.json() | ||
|
||
assert result["count"] == 2 | ||
|
||
# Check content | ||
registration_request = result["results"][-1] | ||
|
||
assert "created" in registration_request | ||
assert "changed" in registration_request | ||
|
||
assert registration_request["challenge"] == ch.short_name | ||
assert registration_request["registration_status"] == "Pending" | ||
assert ( | ||
registration_request["user"]["user"]["username"] | ||
== non_part_user.username | ||
) | ||
|
||
question_answers = registration_request["registration_question_answers"] | ||
assert len(question_answers) == 1 | ||
assert question_answers[0]["question"]["question_text"] == "Foo" | ||
assert question_answers[0]["answer"] == "bar" | ||
|
||
|
||
@pytest.mark.django_db | ||
def test_registration_request_api_list_filtering(client): | ||
ch_0 = ChallengeFactory() | ||
admin_0, participant_0, non_part_user = UserFactory.create_batch(3) | ||
ch_0.add_admin(admin_0) | ||
ch_0.add_participant(participant_0) | ||
|
||
RegistrationRequestFactory(challenge=ch_0, user=participant_0) | ||
RegistrationRequestFactory(challenge=ch_0, user=non_part_user) | ||
|
||
# Create another challenge | ||
ch_1 = ChallengeFactory() | ||
admin_ch1, participant_1 = UserFactory.create_batch(2) | ||
ch_1.add_admin(admin_ch1) | ||
ch_1.add_participant(participant_1) | ||
RegistrationRequestFactory(challenge=ch_1, user=participant_1) | ||
|
||
# First admin is admin of both | ||
ch_1.add_admin(admin_0) | ||
|
||
for usr in (participant_0, non_part_user): | ||
response = get_view_for_user( | ||
viewname="api:registration-request-list", | ||
client=client, | ||
user=usr, | ||
) | ||
|
||
assert response.status_code == 200, "Non admins can query api" | ||
assert response.json()["count"] == 0, "Non admins get no challenges" | ||
|
||
response = get_view_for_user( | ||
viewname="api:registration-request-list", | ||
client=client, | ||
user=admin_ch1, | ||
) | ||
assert ( | ||
response.json()["count"] == 1 | ||
), "Alt admin can only see one challenge" | ||
|
||
response = get_view_for_user( | ||
viewname="api:registration-request-list", | ||
client=client, | ||
user=admin_0, | ||
) | ||
|
||
assert response.status_code == 200, "Admin is allowed to see list" | ||
assert ( | ||
response.json()["count"] == 3 | ||
), "Admin can see both challenge' requests" | ||
|
||
response = get_view_for_user( | ||
viewname="api:registration-request-list", | ||
data={"challenge__short_name": ch_1.short_name}, | ||
client=client, | ||
user=admin_0, | ||
) | ||
assert response.status_code == 200, "Admin is allowed to see list" | ||
result = response.json() | ||
|
||
assert result["count"] == 1, "Admin can filter for a challenge" | ||
assert result["results"][0]["challenge"] == ch_1.short_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have object based permissions, otherwise this is all public information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are permission checks in the get_queryset. Currently, we don't have any
"view_registrationrequest"
permissions being assigned to the challenge.admins_groups. Should we? If so, I'll create a PR + manual call to have them added first, before doing anything that relies on them.