Restrict automated changes to new courses and evaluations #2600
Conversation
…orter as long as they are in preparation
richardebeling
left a comment
richardebeling
left a comment
There was a problem hiding this comment.
Change generally looks good to me, but I'm not yet convinced that we want to add the course to self.courses_by_gguid
|
|
||
| if course and course.evaluations.exclude(state=Evaluation.State.NEW).exists(): | ||
| self.statistics.attempted_course_changes.add(course) | ||
| self.courses_by_gguid[data["gguid"]] = course |
There was a problem hiding this comment.
In ~632, we have
# Don't import if course was skipped
course = self.courses_by_gguid.get(event["relatedevents"][0]["gguid"])
if course is None:
continueDoes adding the course to courses_by_gguid here make sense, given that this will make the logic below think the course was imported?
There was a problem hiding this comment.
If we don't add it, no evaluations will be imported/updated for the course at all, which is not what we want. Here, no changes will be made to the course in the database, and the importer will continue processing the evaluations as usual.
There was a problem hiding this comment.
Skipping is intended for course types that we don't want to import in the first place.
There was a problem hiding this comment.
can you add a short comment about this on the line here?
niklasmohrin
left a comment
niklasmohrin
left a comment
There was a problem hiding this comment.
looks good, except for what richard said
| importer = self._import() | ||
|
|
||
| evaluation = Evaluation.objects.get(pk=evaluation.pk) | ||
| self.assertFalse(evaluation.is_rewarded) | ||
| self.assertEqual(evaluation.course.name_en, "Change") |
There was a problem hiding this comment.
Does the following work, or does it also break because of #2576 ?
| importer = self._import() | |
| evaluation = Evaluation.objects.get(pk=evaluation.pk) | |
| self.assertFalse(evaluation.is_rewarded) | |
| self.assertEqual(evaluation.course.name_en, "Change") | |
| with assert_no_database_modifications(): | |
| importer = self._import() |
There was a problem hiding this comment.
yes, we have an empty log entry there
…n#2600) changes to courses and evaluations should only be made by the cms importer as long as they are in preparation
3 participants
changes to courses and evaluations should only be made by the cms importer as long as they are in preparation