From 5b0d512b456e5e4984035c8f228b421aa59ba835 Mon Sep 17 00:00:00 2001
From: Niklas Mohrin <dev@niklasmohrin.de>
Date: Mon, 21 Feb 2022 23:43:37 +0100
Subject: [PATCH] Fix importer error-merge error (#1714)
* tdd, we all agree
* cold fix
* Remove `merge_dictionaries_of_sets`
---
evap/staff/fixtures/excel_files_test_data.py | 8 +++++++
evap/staff/importers.py | 24 ++++++++++++++------
evap/staff/tests/test_importers.py | 11 +++++++++
evap/staff/tests/test_tools.py | 13 +----------
evap/staff/tools.py | 9 --------
5 files changed, 37 insertions(+), 28 deletions(-)
diff --git a/evap/staff/fixtures/excel_files_test_data.py b/evap/staff/fixtures/excel_files_test_data.py
index 08d9204ad..2ab82fc43 100644
--- a/evap/staff/fixtures/excel_files_test_data.py
+++ b/evap/staff/fixtures/excel_files_test_data.py
@@ -122,6 +122,14 @@ test_enrollment_data_degree_merge_filedata = {
]
}
+test_enrollment_data_error_merge_filedata = {
+ 'MA Belegungen': [
+ ['Degree', 'Student last name', 'Student first name', 'Student email address', 'Course kind', 'Course is graded', 'Course name (de)', 'Course name (en)', 'Responsible title', 'Responsible last name', 'Responsible first name', 'Responsible email address'],
+ ['Grandmaster', 'Quid', 'Bastius', 'bastius.quid@external.example.com', 'jaminar', 'probably not', 'Bauen', 'Build', '', 'Sed', 'Diam', '345@external.example.com'],
+ ['Beginner,Bachelor', 'Lorem', 'Ipsum', 'ipsum.lorem@institution.example.com', 'jaminar', 'probably not', 'Bauen', 'Build', '', 'Sed', 'Diam', '345@external.example.com'],
+ ],
+}
+
test_enrollment_data_import_names_filedata = {
'MA Belegungen': [
['Degree', 'Student last name', 'Student first name', 'Student email address', 'Course kind', 'Course is graded', 'Course name (de)', 'Course name (en)', 'Responsible title', 'Responsible last name', 'Responsible first name', 'Responsible email address'],
diff --git a/evap/staff/importers.py b/evap/staff/importers.py
index 1fa1972e6..a71adcb7e 100644
--- a/evap/staff/importers.py
+++ b/evap/staff/importers.py
@@ -14,7 +14,7 @@ from django.utils.translation import gettext_lazy
from evap.evaluation.models import Contribution, Course, CourseType, Degree, Evaluation, UserProfile
from evap.evaluation.tools import clean_email
-from evap.staff.tools import ImportType, create_user_list_html_string_for_message, merge_dictionaries_of_sets
+from evap.staff.tools import ImportType, create_user_list_html_string_for_message
def sorted_messages(messages):
@@ -402,18 +402,25 @@ class EnrollmentImporter(ExcelImporter):
self.evaluations[evaluation_id] = evaluation_data
self.names_de.add(evaluation_data.name_de)
else:
- if evaluation_data.equals_except_for_degrees(self.evaluations[evaluation_id]):
+ known_data = self.evaluations[evaluation_id]
+ if evaluation_data.equals_except_for_degrees(known_data):
self.warnings[ImporterWarning.DEGREE].append(
_(
'Sheet "{}", row {}: The course\'s "{}" degree differs from it\'s degree in a previous row.'
" Both degrees have been set for the course."
).format(sheetname, row + 1, evaluation_data.name_en)
)
- self.evaluations[evaluation_id].degrees |= evaluation_data.degrees
- self.evaluations[evaluation_id].errors = merge_dictionaries_of_sets(
- self.evaluations[evaluation_id].errors, evaluation_data.errors
- )
- elif evaluation_data != self.evaluations[evaluation_id]:
+
+ known_data.degrees |= evaluation_data.degrees
+
+ assert evaluation_data.errors.keys() <= {"degrees", "course_type", "is_graded"}
+ assert known_data.errors.get("course_type") == evaluation_data.errors.get("course_type")
+ assert known_data.errors.get("is_graded") == evaluation_data.errors.get("is_graded")
+
+ degree_errors = known_data.errors.get("degrees", set()) | evaluation_data.errors.get("degrees", set())
+ if len(degree_errors) > 0:
+ known_data.errors["degrees"] = degree_errors
+ elif evaluation_data != known_data:
self.errors[ImporterError.COURSE].append(
_('Sheet "{}", row {}: The course\'s "{}" data differs from it\'s data in a previous row.').format(
sheetname, row + 1, evaluation_data.name_en
@@ -439,6 +446,9 @@ class EnrollmentImporter(ExcelImporter):
self.errors[ImporterError.COURSE].append(
_("Course {} does already exist in this semester.").format(evaluation_data.name_de)
)
+
+ assert evaluation_data.errors.keys() <= {"degrees", "course_type", "is_graded"}
+
if "degrees" in evaluation_data.errors:
missing_degree_names |= evaluation_data.errors["degrees"]
if "course_type" in evaluation_data.errors:
diff --git a/evap/staff/tests/test_importers.py b/evap/staff/tests/test_importers.py
index 2d7b26e9b..dd9edafd5 100644
--- a/evap/staff/tests/test_importers.py
+++ b/evap/staff/tests/test_importers.py
@@ -243,6 +243,17 @@ class TestEnrollmentImporter(TestCase):
course = Course.objects.get(name_de="Bauen")
self.assertSetEqual(set(course.degrees.all()), set(Degree.objects.filter(name_de__in=["Master", "Bachelor"])))
+ def test_errors_are_merged(self):
+ excel_content = excel_data.create_memory_excel_file(excel_data.test_enrollment_data_error_merge_filedata)
+ __, warnings, errors = EnrollmentImporter.process(
+ excel_content, self.semester, self.vote_start_datetime, self.vote_end_date, test_run=False
+ )
+ self.assertIn("Both degrees have been set for the course", "".join(warnings[ImporterWarning.DEGREE]))
+ self.assertIn("is probably not, but must be", "".join(errors[ImporterError.IS_GRADED]))
+ self.assertIn("jaminar", "".join(errors[ImporterError.COURSE_TYPE_MISSING]))
+ self.assertIn("Beginner", "".join(errors[ImporterError.DEGREE_MISSING]))
+ self.assertIn("Grandmaster", "".join(errors[ImporterError.DEGREE_MISSING]))
+
def test_course_type_and_degrees_are_retrieved_with_import_names(self):
excel_content = excel_data.create_memory_excel_file(excel_data.test_enrollment_data_import_names_filedata)
diff --git a/evap/staff/tests/test_tools.py b/evap/staff/tests/test_tools.py
index 7a3ba2b87..e4a343607 100644
--- a/evap/staff/tests/test_tools.py
+++ b/evap/staff/tests/test_tools.py
@@ -7,12 +7,7 @@ from model_bakery import baker
from evap.evaluation.models import Contribution, Course, Evaluation, UserProfile
from evap.evaluation.tests.tools import WebTest
from evap.rewards.models import RewardPointGranting, RewardPointRedemption
-from evap.staff.tools import (
- delete_navbar_cache_for_users,
- merge_dictionaries_of_sets,
- merge_users,
- remove_user_from_represented_and_ccing_users,
-)
+from evap.staff.tools import delete_navbar_cache_for_users, merge_users, remove_user_from_represented_and_ccing_users
class NavbarCacheTest(WebTest):
@@ -262,9 +257,3 @@ class RemoveUserFromRepresentedAndCCingUsersTest(TestCase):
self.assertEqual([set(user1.delegates.all()), set(user1.cc_users.all())], [{delete_user}, {delete_user}])
self.assertEqual([set(user2.delegates.all()), set(user2.cc_users.all())], [{delete_user}, {delete_user}])
self.assertEqual(len(messages), 4)
-
-
-class MiscellaneousToolsTest(TestCase):
- def test_merge_dictionaries_of_sets(self):
- self.assertEqual(merge_dictionaries_of_sets({"a": set([1])}, {"b": set([2])}), {"a": set([1]), "b": set([2])})
- self.assertEqual(merge_dictionaries_of_sets({"a": set([1])}, {"a": set([2])}), {"a": set([1, 2])})
diff --git a/evap/staff/tools.py b/evap/staff/tools.py
index fd82ddef9..c620845e7 100644
--- a/evap/staff/tools.py
+++ b/evap/staff/tools.py
@@ -1,7 +1,6 @@
import os
from datetime import date, datetime, timedelta
from enum import Enum
-from typing import Any, Dict, Set
from django.conf import settings
from django.contrib import messages
@@ -369,11 +368,3 @@ def remove_user_from_represented_and_ccing_users(user, ignored_users=None, test_
cc_user.cc_users.remove(user)
remove_messages.append(_("Removed {} from the CC users of {}.").format(user.full_name, cc_user.full_name))
return remove_messages
-
-
-def merge_dictionaries_of_sets(a: Dict[Any, Set], b: Dict[Any, Set]) -> Dict[Any, Set]:
- return {
- **a,
- **b,
- **({key: (a[key] | b[key]) for key in a if key in b}),
- }
--
GitLab