Skip to content
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

chore: better submission status #421

Merged
merged 8 commits into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
55 changes: 48 additions & 7 deletions backend/api/serializers/project_serializer.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
from api.logic.parse_zip_files import parse_zip
from api.models.group import Group
from api.models.project import Project
from api.models.submission import Submission
from api.models.submission import Submission, ExtraCheckResult, StructureCheckResult, StateEnum
from api.serializers.course_serializer import CourseSerializer
from django.core.files.uploadedfile import InMemoryUploadedFile
from django.utils import timezone
Expand All @@ -14,28 +14,69 @@
class SubmissionStatusSerializer(serializers.Serializer):
non_empty_groups = serializers.IntegerField(read_only=True)
groups_submitted = serializers.IntegerField(read_only=True)
submissions_passed = serializers.IntegerField(read_only=True)
structure_checks_passed = serializers.IntegerField(read_only=True)
extra_checks_passed = serializers.IntegerField(read_only=True)

def to_representation(self, instance: Project):
"""Return the submission status of the project"""
if not isinstance(instance, Project):
raise ValidationError(gettext("project.errors.invalid_instance"))

non_empty_groups = instance.groups.filter(students__isnull=False).count()
groups_submitted = Submission.objects.filter(group__project=instance).count()
submissions_passed = Submission.objects.filter(group__project=instance, is_valid=True).count()
non_empty_groups = Group.objects.filter(project=instance, students__isnull=False).distinct().count()

groups_submitted_ids = Submission.objects.filter(group__project=instance).values_list('group__id', flat=True)
unique_groups = set(groups_submitted_ids)
groups_submitted = len(unique_groups)

# The total amount of groups with at least one submission should never exceed the total number of non empty groups
# (the seeder does not account for this restriction)
if (groups_submitted > non_empty_groups):
EwoutV marked this conversation as resolved.
Show resolved Hide resolved
non_empty_groups = groups_submitted

passed_structure_checks_submission_ids = StructureCheckResult.objects.filter(
submission__group__project=instance,
submission__is_valid=True,
result=StateEnum.SUCCESS
).values_list('submission__id', flat=True)

passed_structure_checks_group_ids = Submission.objects.filter(
id__in=passed_structure_checks_submission_ids
).values_list('group_id', flat=True)

unique_groups = set(passed_structure_checks_group_ids)
structure_checks_passed = len(unique_groups)

passed_extra_checks_submission_ids = ExtraCheckResult.objects.filter(
submission__group__project=instance,
submission__is_valid=True,
result=StateEnum.SUCCESS
).values_list('submission__id', flat=True)

passed_extra_checks_group_ids = Submission.objects.filter(
id__in=passed_extra_checks_submission_ids
).values_list('group_id', flat=True)

unique_groups = set(passed_extra_checks_group_ids)
extra_checks_passed = len(unique_groups)

# The total number of passed extra checks combined with the number of passed structure checks
# can never exceed the total number of submissions (the seeder does not account for this restriction)
if (structure_checks_passed + extra_checks_passed > groups_submitted):
extra_checks_passed = groups_submitted - structure_checks_passed

return {
"non_empty_groups": non_empty_groups,
"groups_submitted": groups_submitted,
"submissions_passed": submissions_passed,
"structure_checks_passed": structure_checks_passed,
"extra_checks_passed": extra_checks_passed
}

class Meta:
fields = [
"non_empty_groups",
"groups_submitted",
"submissions_passed",
"structure_checks_passed",
"extra_checks_passed"
]


Expand Down
4 changes: 2 additions & 2 deletions backend/api/tests/test_project.py
Original file line number Diff line number Diff line change
Expand Up @@ -802,7 +802,7 @@ def test_submission_status_non_empty_groups(self):
# Only two of the three created groups contain at least one student
self.assertEqual(
content_json["status"],
{"non_empty_groups": 2, "groups_submitted": 0, "submissions_passed": 0},
{"non_empty_groups": 2, "groups_submitted": 0, "extra_checks_passed": 0, "structure_checks_passed": 0},
)

def test_submission_status_groups_submitted_and_passed_checks(self):
Expand Down Expand Up @@ -866,7 +866,7 @@ def test_submission_status_groups_submitted_and_passed_checks(self):

self.assertEqual(
content_json["status"],
{"non_empty_groups": 3, "groups_submitted": 2, "submissions_passed": 2},
{"non_empty_groups": 3, "groups_submitted": 2, "extra_checks_passed": 0, "structure_checks_passed": 0},
EwoutV marked this conversation as resolved.
Show resolved Hide resolved
)

def test_retrieve_list_submissions(self):
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/assets/lang/app/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,8 @@
"noSubmissions": "This project does not have any submissions",
"submissions": "Submission | Submissions",
"groups": "Group | Groups",
"testsSucceed": "Succeeded tests",
"structureTestsSucceed": "Succeeded structure tests",
"extraTestsSucceed": "Succeeded extra tests",
"testsFail": "Failed tests",
"submit": "Submit"
},
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/assets/lang/app/nl.json
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@
"noSubmissions": "Dit project heeft geen indieningen",
"submissions": "Indiening | Indieningen",
"groups": "Groep | Groepen",
"testsSucceed": "Geslaagde testen",
"structureTestsSucceed": "Geslaagde structuur testen",
"extraTestsSucceed": "Geslaagde extra testen",
"testsFail": "Gefaalde testen",
"submit": "Indienen"
},
Expand Down
16 changes: 12 additions & 4 deletions frontend/src/components/projects/ProjectMeter.vue
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,23 @@ const { t } = useI18n();
const meterItems = computed(() => {
const groups = props.project !== null ? props.project.status.non_empty_groups : 0;
const groupsSubmitted = props.project !== null ? props.project.status.groups_submitted : 0;
const submissionsPassed = props.project !== null ? props.project.status.submissions_passed : 0;
const submissionsFailed = groupsSubmitted - submissionsPassed;
const structureChecksPassed = props.project !== null ? props.project.status.structure_checks_passed : 0;
const extraChecksPassed = props.project !== null ? props.project.status.extra_checks_passed : 0;
const submissionsFailed = groupsSubmitted - structureChecksPassed;

return [
{
value: (submissionsPassed / groups) * 100,
value: (extraChecksPassed / groups) * 100,
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it possible to have extrachecks passed but structure checks not? I would change this to the min of those 2.

Copy link
Contributor Author

@francisvaut francisvaut May 16, 2024

Choose a reason for hiding this comment

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

That code is not extrachecks and structurechecks for a single submission, but for a whole project (PojectMeter).

Copy link
Contributor

Choose a reason for hiding this comment

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

You should round this to the nearest integer to account for representation errors, it could happen that the meter now displays 22.3333333333331%.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, the MeterGroup component already rounds these values by default.

color: '#749b68',
label: t('components.card.testsSucceed'),
label: t('components.card.extraTestsSucceed'),
icon: 'pi pi-check',
},
{
value: (structureChecksPassed / groups) * 100,
color: '#fa9746',
label: t('components.card.structureTestsSucceed'),
icon: 'pi pi-exclamation-circle',
},
{
value: (submissionsFailed / groups) * 100,
color: '#FF5445',
Expand Down
5 changes: 5 additions & 0 deletions frontend/src/test/unit/services/setup/delete_handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,9 @@ export const deleteHandlers = [
assistants.splice(index, 1);
return HttpResponse.json(assistants);
}),
http.delete(baseUrl + endpoints.structureChecks.retrieve.replace('{id}', ':id'), async ({ params }) => {
const index = structureChecks.findIndex((x) => x.id === params.id);
structureChecks.splice(index, 1);
return HttpResponse.json(structureChecks);
}),
];
17 changes: 0 additions & 17 deletions frontend/src/test/unit/services/setup/get_handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,27 +86,10 @@ export const getHandlers = [
const project = projects.find((x) => x.id === params.id);
const groupIds = project !== null && project !== undefined ? project.groups : [];
const submissionIds = project !== null && project !== undefined ? project.submissions : [];
const subGroupIds = Array.from(
new Set(submissions.filter((x) => submissionIds.includes(x.id)).map((x) => x.group)),
);

// Filter submissions for each subgroup and get the submission with the highest number
const subgroupSubmissions = subGroupIds.map((groupId) => {
const submissionsForGroup = submissions.filter((submission) => submission.group === groupId);
if (submissionsForGroup.length > 0) {
return submissionsForGroup.reduce((maxSubmission, currentSubmission) => {
return currentSubmission.submission_number > maxSubmission.submission_number
? currentSubmission
: maxSubmission;
});
} else {
return null;
}
});
return HttpResponse.json({
groups_submitted: new Set(submissions.filter((x) => submissionIds.includes(x.id)).map((x) => x.group)).size,
non_empty_groups: groups.filter((x) => groupIds.includes(x.id) && x.students.length > 0).length,
submissions_passed: subgroupSubmissions.filter((x) => x?.structureChecks_passed).length,
});
}),
http.get(baseUrl + endpoints.structureChecks.byProject.replace('{projectId}', ':id'), ({ params }) => {
Expand Down
15 changes: 13 additions & 2 deletions frontend/src/test/unit/services/setup/post_handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,18 +54,29 @@ export const postHandlers = [
faculties.push(newFaculty);
return HttpResponse.json(faculties);
}),
http.post(baseUrl + endpoints.groups.byProject.replace('{projectId}', ':id'), async ({ request, params }) => {
http.post(baseUrl + endpoints.groups.byProject.replace('{projectId}', ':id'), async ({ request }) => {
const buffer = await request.arrayBuffer();
const requestBody = new TextDecoder().decode(buffer);
const newGroup = JSON.parse(requestBody);
groups.push(newGroup);
return HttpResponse.json(groups);
}),
http.post(baseUrl + endpoints.projects.byCourse.replace('{courseId}', ':id'), async ({ request, params }) => {
http.post(baseUrl + endpoints.projects.byCourse.replace('{courseId}', ':id'), async ({ request }) => {
const buffer = await request.arrayBuffer();
const requestBody = new TextDecoder().decode(buffer);
const newProject = JSON.parse(requestBody);
projects.push(newProject);
return HttpResponse.json(projects);
}),
http.post(
baseUrl + endpoints.structureChecks.byProject.replace('{projectId}', ':id'),
async ({ request, params }) => {
const buffer = await request.arrayBuffer();
const requestBody = new TextDecoder().decode(buffer);
const newStructureCheck = JSON.parse(requestBody);
newStructureCheck.project = params.id;
structureChecks.push(newStructureCheck);
return HttpResponse.json(structureChecks);
},
),
];
50 changes: 50 additions & 0 deletions frontend/src/test/unit/services/structure_check.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-disable @typescript-eslint/no-unused-vars */
import { describe, it, expect } from 'vitest';
import { useStructureCheck } from '@/composables/services/structure_check.service.ts';
import { StructureCheck } from '@/types/StructureCheck';

const {
structureChecks,
Expand Down Expand Up @@ -59,3 +60,52 @@ describe('structureCheck', (): void => {
expect(structureChecks.value?.[3]?.blocked_extensions).toBeNull();
});
});

it('create structureCheck', async () => {
resetService();

const exampleStructureCheck = new StructureCheck(
'', // id
'structure_check_name', // name
[], // blocked extensions
[], // obligated extensions
null, // project
);

await getStructureCheckByProject('123456');
expect(structureChecks).not.toBeNull();
expect(Array.isArray(structureChecks.value)).toBe(true);
const prevLength = structureChecks.value?.length ?? 0;

await createStructureCheck(exampleStructureCheck, '123456');
await getStructureCheckByProject('123456');

expect(structureChecks).not.toBeNull();
expect(Array.isArray(structureChecks.value)).toBe(true);
expect(structureChecks.value?.length).toBe(prevLength + 1);

// Only check for fields that are sent to the backend
expect(structureChecks.value?.[prevLength]?.name).toBe('structure_check_name');
});

it('delete structureCheck', async () => {
resetService();

await getStructureCheckByProject('123456');
expect(structureChecks.value).not.toBeNull();
expect(Array.isArray(structureChecks.value)).toBe(true);
const prevLength = structureChecks.value?.length ?? 0;

let structureCheckId = '';
if (structureChecks.value?.[2]?.id !== undefined && structureChecks.value?.[2].id !== null) {
structureCheckId = structureChecks.value?.[2]?.id;
}

await deleteStructureCheck(structureCheckId);
await getStructureCheckByProject('123456');

expect(structureChecks).not.toBeNull();
expect(Array.isArray(structureChecks.value)).toBe(true);
expect(structureChecks.value?.length).toBe(prevLength - 1);
expect(structureChecks.value?.[2]?.id).not.toBe(structureCheckId);
});
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,6 @@ describe('submision_status', (): void => {
expect(submissionStatus.value).not.toBeNull();
expect(submissionStatus.value?.groups_submitted).toBe(1);
expect(submissionStatus.value?.non_empty_groups).toBe(2);
expect(submissionStatus.value?.submissions_passed).toBe(1);
// No need to check for structure_check_passed and extra_checks_passed since those queries are not implemented in the frontend
tyboro2002 marked this conversation as resolved.
Show resolved Hide resolved
});
});
5 changes: 3 additions & 2 deletions frontend/src/test/unit/types/data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,9 @@ export const structureCheckData = {

export const submissionStatusData = {
non_empty_groups: 5,
groups_submitted: 2,
submissions_passed: 1,
groups_submitted: 4,
structure_checks_passed: 3,
extra_checks_passed: 1,
};

export const submissionData = {
Expand Down
3 changes: 2 additions & 1 deletion frontend/src/test/unit/types/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,8 @@ export function createSubmissionStatus(submissionStatusData: any): SubmissionSta
return new SubmissionStatus(
submissionStatusData.non_empty_groups,
submissionStatusData.groups_submitted,
submissionStatusData.submissions_passed,
submissionStatusData.structure_checks_passed,
submissionStatusData.extra_checks_passed,
);
}

Expand Down
6 changes: 4 additions & 2 deletions frontend/src/test/unit/types/submissionStatus.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ describe('submissionStatus type', () => {
expect(submissionStatus).toBeInstanceOf(SubmissionStatus);
expect(submissionStatus.non_empty_groups).toBe(submissionStatusData.non_empty_groups);
expect(submissionStatus.groups_submitted).toBe(submissionStatusData.groups_submitted);
expect(submissionStatus.submissions_passed).toBe(submissionStatusData.submissions_passed);
expect(submissionStatus.structure_checks_passed).toBe(submissionStatusData.structure_checks_passed);
expect(submissionStatus.extra_checks_passed).toBe(submissionStatusData.extra_checks_passed);
});

it('create a submissionStatus instance from JSON data', () => {
Expand All @@ -21,6 +22,7 @@ describe('submissionStatus type', () => {
expect(submissionStatus).toBeInstanceOf(SubmissionStatus);
expect(submissionStatus.non_empty_groups).toBe(submissionStatusData.non_empty_groups);
expect(submissionStatus.groups_submitted).toBe(submissionStatusData.groups_submitted);
expect(submissionStatus.submissions_passed).toBe(submissionStatusData.submissions_passed);
expect(submissionStatus.structure_checks_passed).toBe(submissionStatusData.structure_checks_passed);
expect(submissionStatus.extra_checks_passed).toBe(submissionStatusData.extra_checks_passed);
});
});
6 changes: 4 additions & 2 deletions frontend/src/types/SubmisionStatus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ export class SubmissionStatus {
constructor(
public non_empty_groups: number,
public groups_submitted: number,
public submissions_passed: number,
public structure_checks_passed: number,
public extra_checks_passed: number,
) {}

/**
Expand All @@ -14,7 +15,8 @@ export class SubmissionStatus {
return new SubmissionStatus(
submissionStatus.non_empty_groups,
submissionStatus.groups_submitted,
submissionStatus.submissions_passed,
submissionStatus.structure_checks_passed,
submissionStatus.extra_checks_passed,
);
}
}