Fix/refactor choices #586

EdoStorm96 · 2023-11-23T12:07:47Z

Fixes #580. Refactoring all the choices in the models to use Django's special enumeration types. Had to change the code in lots of places, but really nothing major has happened. The most important changes happened in the models. When performing makemigrations --dry-run no changes are detected. A few things to pay attention to:

I removed DESIGNS from the Studies model entirely, as these seem to not be used anywhere.
I had to slightly adjust the list comprehension on line 105 of reviews/forms.py. It works fine like this, i guess. Although I do wonder if there is a better solution with the new enumeration types ...
Also note the changes in get_continuation_display(). Again, this works, but I wonder if there could be a better solution?
Lastly, keep in mind the new syntax that this change requires, in the future, eg.:
- No longer: Review.SUPERVISOR, but: Review.Stages.SUPERVISOR
- No longer: choices = STAGES, but choices = self.Stages.choices in models or Review.Stages.choices in forms
- No longer: Import YES, NO, DOUBT, YES_NO_DOUBT, but import YesNoDoubt

tymees

Nice, this makes my brain very happy.

Didn't do a full test or anything, but did do a double check if you got everything. I found 4 issues; I provided change-suggestions for you as easy fixes. If you fix them, it's approved :)

The other comments are just suggestions :)

I removed DESIGNS from the Studies model entirely, as these seem to not be used anywhere.

I did try to see if they were ever used, as far as I can tell they never were... At least not for 7 years lol.

tymees · 2023-11-23T14:55:01Z

reviews/utils/review_utils.py

@@ -338,7 +338,7 @@ def auto_review(proposal: Proposal):
        if study.legally_incapable:
            reasons.append(_('De aanvraag bevat het gebruik van wilsonbekwame volwassenen.'))

-        if study.deception in [YES, DOUBT]:
+        if study.deception in [YesNoDoubt.YES, DOUBT]:


Suggested change

if study.deception in [YesNoDoubt.YES, DOUBT]:

if study.deception in [YesNoDoubt.YES, YesNoDoubt.DOUBT]:

tymees · 2023-11-23T14:55:13Z

reviews/utils/review_utils.py

@@ -353,11 +353,11 @@ def auto_review(proposal: Proposal):
        for task in Task.objects.filter(session__study=study):
            reasons.extend(auto_review_task(study, task))

-        if study.stressful in [YES, DOUBT]:
+        if study.stressful in [YesNoDoubt.YES, DOUBT]:


Suggested change

if study.stressful in [YesNoDoubt.YES, DOUBT]:

if study.stressful in [YesNoDoubt.YES, YesNoDoubt.DOUBT]:

tymees · 2023-11-23T14:55:26Z

reviews/utils/review_utils.py

            reasons.append(_('De onderzoeker geeft aan dat (of twijfelt erover of) het onderzoek op onderdelen of \
 als geheel zodanig belastend is dat deze ondanks de verkregen informed consent vragen zou kunnen oproepen.'))

-        if study.risk in [YES, DOUBT]:
+        if study.risk in [YesNoDoubt.YES, DOUBT]:


Suggested change

if study.risk in [YesNoDoubt.YES, DOUBT]:

if study.risk in [YesNoDoubt.YES, YesNoDoubt.DOUBT]:

tymees · 2023-11-23T15:02:11Z

reviews/models.py

-            self.continuation,
-            self.continuation
-        )
+        continuation = dict(self.Continuations.choices)[self.continuation]


Suggested change

continuation = dict(self.Continuations.choices)[self.continuation]

continuation = self.Continuations(self.continuation).label

This is the proper way to get the label for a choice :)

Ahh, I knew something like this had to be possible! Glad to know the syntax.

tymees · 2023-11-23T15:04:22Z

proposals/utils/pdf_diff_logic.py

+        from main.models import YesNoDoubt

-        d = dict(YES_NO_DOUBT)
+        d = dict(YesNoDoubt.choices)
        return d[value]


No need for the dict cast:

from main.models import YesNoDoubt return YesNoDoubt(value).label

tymees · 2023-11-23T15:20:10Z

reviews/forms.py

@@ -102,7 +102,7 @@ def __init__(self, *args, **kwargs):
        allow_long_route_continuation = kwargs.pop('allow_long_route_continuation', False)
        super(ReviewCloseForm, self).__init__(*args, **kwargs)
        if not allow_long_route_continuation:
-            self.fields['continuation'].choices = [x for x in Review.CONTINUATIONS if x[0] != Review.LONG_ROUTE]
+            self.fields['continuation'].choices = [x for x in Review.Continuations.choices if x != Review.Continuations.LONG_ROUTE]


This does not work?:

>>> [x for x in Review.Continuations.choices if x != Review.Continuations.LONG_ROUTE] [(0, 'Goedkeuring door FETC-GW'), (1, 'Revisie noodzakelijk'), (2, 'Afwijzing door FETC-GW'), (3, 'Open review met lange (4-weken) route'), (4, 'Laat opnieuw beoordelen door METC'), (5, 'Positief advies van FETC-GW, post-hoc'), (6, 'Negatief advies van FETC-GW, post-hoc'), (7, 'Niet verder in behandeling genomen')]

I think it should be this:

Suggested change

self.fields['continuation'].choices = [x for x in Review.Continuations.choices if x != Review.Continuations.LONG_ROUTE]

self.fields['continuation'].choices = [x for x in Review.Continuations.choices if x[0] != Review.Continuations.LONG_ROUTE]

As for a better solution: there isn't any really that's clearly better. One alternative is a filter:

filter(lambda x: x[0] != Review.Continuations.LONG_ROUTE, Review.Continuations.choices)

But that's not really that much better.

You could also create a method in the new class to do the list comprehension there, but for this one usage that does not make much sense

I'm sorry. I just assumed ... Too bad that does not work. I just used your first suggestion.

miggol

Nice, I like the new choices. I reviewed as follows:

Ran tests
Tried to make a pdf
Migrated from scratch and loaded all fixtures
Finally, pointed out all the places my IDE told me there were undefined variables like DOUBT and NO_WMO

I found a few issues, mainly from the final step.

miggol · 2023-11-28T10:04:41Z

proposals/utils/pdf_diff_logic.py

Missed a couple of model.wmo.NO_WMO instances on lines 1017 and 1022

miggol · 2023-11-28T10:16:23Z

reviews/tests.py

Found some unchanged doubts and no's on lines 18 and 32.

These flagged up in python manage.py test. Usually you would see this in the PR on Github as well, but it seems there's some issue with the runners right now. If this persists/happens again you can always run the tests locally to check.

miggol · 2023-11-28T10:21:09Z

proposals/tests.py

-        wmo = Wmo.objects.create(proposal=proposal, metc=YES)
-        self.assertEqual(proposal.wmo.status, Wmo.WAITING)
+        wmo = Wmo.objects.create(proposal=proposal, metc=YesNoDoubt.YES)
+        self.assertEqual(proposal.wmo.status, Wmo.WMOStatuses.WAITING)
        wmo.metc = NO


Stubborn metc just can't take NO for an answer.

miggol · 2023-11-28T10:21:27Z

proposals/tests.py

@@ -279,22 +279,22 @@ def setUp(self):
        self.wmo = Wmo.objects.create(proposal=self.p1, metc=NO)


Undefined NO

miggol · 2023-11-28T10:22:37Z

proposals/views/wmo_views.py

-    is_metc = request.POST.get('metc') == YES
-    is_medical = request.POST.get('medical') == YES
+    is_metc = request.POST.get('metc') == YesNoDoubt.YES
+    is_medical = request.POST.get('medical') == YesNoDoubt.YES

    doubt = request.POST.get('metc') == DOUBT or request.POST.get('medical') == DOUBT


Undefined DOUBT

EdoStorm96 added 5 commits November 23, 2023 11:51

Refactor of all choices in reviews/models.py

1cbc8ec

Refactor of all choices in main/models.py (incl. YES_NO_DOUBT)

a325ff6

Refactor of all choices in proposals/models.py

f14001e

Removed DESIGNS from studies, because they are unused

af87f97

Undid some accidentally committed changes

d25a7bc

EdoStorm96 requested review from tymees and miggol November 23, 2023 12:07

tymees requested changes Nov 23, 2023

View reviewed changes

Ty's fixes

28411f1

miggol requested changes Nov 28, 2023

View reviewed changes

EdoStorm96 added 2 commits November 28, 2023 14:05

Things I missed ...

95f4f7e

resolve merge conflict

c885f2d

EdoStorm96 merged commit 466eb3a into develop Nov 28, 2023
2 checks passed

EdoStorm96 mentioned this pull request Nov 30, 2023

Fix/refactor more choices #589

Merged

EdoStorm96 deleted the fix/refactor_choices branch February 28, 2024 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/refactor choices #586

Fix/refactor choices #586

EdoStorm96 commented Nov 23, 2023

tymees left a comment

tymees Nov 23, 2023

tymees Nov 23, 2023

tymees Nov 23, 2023

tymees Nov 23, 2023

EdoStorm96 Nov 23, 2023

tymees Nov 23, 2023

tymees Nov 23, 2023

EdoStorm96 Nov 23, 2023

miggol left a comment

miggol Nov 28, 2023

miggol Nov 28, 2023

miggol Nov 28, 2023 •

edited

Loading

miggol Nov 28, 2023

miggol Nov 28, 2023

	if study.deception in [YesNoDoubt.YES, DOUBT]:
	if study.deception in [YesNoDoubt.YES, YesNoDoubt.DOUBT]:

	if study.stressful in [YesNoDoubt.YES, DOUBT]:
	if study.stressful in [YesNoDoubt.YES, YesNoDoubt.DOUBT]:

	if study.risk in [YesNoDoubt.YES, DOUBT]:
	if study.risk in [YesNoDoubt.YES, YesNoDoubt.DOUBT]:

	continuation = dict(self.Continuations.choices)[self.continuation]
	continuation = self.Continuations(self.continuation).label

	self.fields['continuation'].choices = [x for x in Review.Continuations.choices if x != Review.Continuations.LONG_ROUTE]
	self.fields['continuation'].choices = [x for x in Review.Continuations.choices if x[0] != Review.Continuations.LONG_ROUTE]

		@@ -279,22 +279,22 @@ def setUp(self):
		self.wmo = Wmo.objects.create(proposal=self.p1, metc=NO)

Fix/refactor choices #586

Fix/refactor choices #586

Conversation

EdoStorm96 commented Nov 23, 2023

tymees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miggol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miggol Nov 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miggol Nov 28, 2023 •

edited

Loading