IgnoredEvaluations, evaluation notes, CMS link icons#2602
IgnoredEvaluations, evaluation notes, CMS link icons#2602niklasmohrin merged 4 commits intoe-valuation:mainfrom
Conversation
janno42
force-pushed
the
importer-ignored-evaluations
branch
from
1cd7749 to
508eeac
Compare
janno42
force-pushed
the
importer-ignored-evaluations
branch
2 times, most recently
from
d1d2b02 to
a5e2878
Compare
niklasmohrin
left a comment
niklasmohrin
left a comment
There was a problem hiding this comment.
makes sense; haven't checked frontend stuff yet, here are some comments from looking at the code:
janno42
force-pushed
the
importer-ignored-evaluations
branch
from
0416880 to
b0adef0
Compare
richardebeling
left a comment
richardebeling
left a comment
There was a problem hiding this comment.
I have all files marked as "viewed" now, all code looked good to me at some point, but I'm not sure if everything is consistent within the "new app" approach now. Let me know if I should re-review or something
|
The new app is currently on a separate branch and almost ready for its own PR. I'm just waiting for this PR to be merged :) |
janno42
force-pushed
the
importer-ignored-evaluations
branch
2 times, most recently
from
bc945d7 to
131eef2
Compare
niklasmohrin
left a comment
niklasmohrin
left a comment
There was a problem hiding this comment.
still need to check with a test file
| from evap.evaluation.models_logging import LogEntry | ||
| from evap.staff.importers.json import ImportDict, JSONImporter, NameChange, WarningMessage, _clean_whitespaces | ||
|
|
||
| EXAMPLE_DATA: ImportDict = { |
There was a problem hiding this comment.
note (to self): depending on whether we merge this or #2634 first, we need to move the file and update the module name in the import then
| ) -> Evaluation | None: | ||
| # Don't import ignored evaluations again | ||
| if IgnoredEvaluation.objects.filter(cms_id=data["gguid"]).exists(): | ||
| return None |
There was a problem hiding this comment.
Not blocking, but we should add a note here that this evaluation was ignored; I guess a warning on self.statistics is a bit much, maybe ImportStatistics could get a new info_messages field?
I think some of our young contributors can figure this one out :)
janno42
force-pushed
the
importer-ignored-evaluations
branch
from
131eef2 to
f8d024d
Compare
janno42
force-pushed
the
importer-ignored-evaluations
branch
from
f8d024d to
11f19f5
Compare
3 participants
We need to keep track of which evaluations have been deleted so that we don't import them again ->
IgnoredEvaluationI'm happy to discuss the way it's implemented and naming; I don't have any strong feelings about them.
It would also help to have manager notes for evaluations, such as who said an evaluation could be deleted and when or any other things to remember for later preparation ->
Evaluation.notesI'd also like to see which courses and evaluations are linked to the CMS -> link icons