Import merged evaluation - #2747
Conversation
niklasmohrin
left a comment
niklasmohrin
left a comment
There was a problem hiding this comment.
I have some trouble understanding how all the parts come together here. In the issue it says that we have an assumption that two things exist together when they don't really have to, but here it is not obvious how this is split into two now
There was a problem hiding this comment.
We should add a test for this behavior
There was a problem hiding this comment.
I would have expected test_import_merged_evaluations to fail in this case and don't really understand why it didn't.
| evaluation = Evaluation.objects.get(cms_evaluation_links__cms_id=data["gguid"]) | ||
| # get the course from the evaluation in case it was merged into a different course than defined in the | ||
| # import data | ||
| course = evaluation.course |
There was a problem hiding this comment.
Is there any chance that we can choose the correct course once before calling import_evaluation? I think it is hard to track what course belongs to what now. In the calling code in import_events we have some if course is None: continue and I am not sure how this should interact with this new assignment here (my intuition is that if the original course is None but we find a course here then we should still run the import).
If this is not possible, then one (or both) of the two course variables needs a new name, I guess the course before this is something like course_from_event and this here is like a selected_course? Although selected_course really should just always be evaluation.course, so I guess we could also just use this everywhere?
There was a problem hiding this comment.
I tried looking up the course outside of _import_evaluation, but that would require a larger refactoring. So, I renamed the parameter to cms_course, the default value for course that is overwritten if the evaluation already exists.
richardebeling
left a comment
richardebeling
left a comment
There was a problem hiding this comment.
code change looks good but I agree that a test would be nice
|
Turns out we already have |
niklasmohrin
left a comment
niklasmohrin
left a comment
There was a problem hiding this comment.
In person discussion:
- the usage of a course other than
evaluation.courseoutside of_import_evaluationis probably wrong - long term, we probably want to get rid of
course_by_gguidaltogether because this is really just an ad hoc mapping of evaluation's gguids to courses; cms doesn't distinguish between courses and evaluations - the notable change regarding the imported evaluation is the removal of the
course=coursewhen looking for linked evaluations
| other_evaluation = Evaluation.objects.get(cms_evaluation_links__cms_id="0x44") | ||
| self.assertEqual(evaluation.course, other_evaluation.course) |
There was a problem hiding this comment.
as we modified the cms link above, we now have evaluation.id == other_evaluation.id here, so this line always passes
3 participants
fix #2746; support importing evaluations that have been merged into another course