Feature/revise study end questions 4 #707

EdoStorm96 · 2024-09-25T14:50:05Z

fixes #546. This was an olde branch I had worked on. This implementation is not entirely what we settled on, as we initially wanted to also have a small table representing the trajectories on the Knowledge Security page if there were multiple trajectories. I do still have a template lying around for this somewhere ... But I don't think it is the most important thing now. At least we have the updated questions/texts in here now.

As far as I can tell, everything here is working well. Though there are a lot of changes, nothing too crazy happening.

miggol

Lots of small remarks but nothing major, excellent turnaround

miggol · 2024-09-27T08:52:30Z

proposals/models.py

@@ -497,6 +497,42 @@ class PracticeReasons(models.IntegerChoices):
            MaxWordsValidator(SELF_ASSESSMENT_MAX_WORDS),
        ],
    )
+    knowledge_security = models.CharField(
+        _("Zijn er kwesties rondom kennisveiligheid?"),


Many of these strings remain untranslated

miggol · 2024-09-27T09:33:28Z

proposals/forms.py

@@ -862,7 +919,7 @@ class Meta:
    _soft_validation_fields = ["translated_forms", "translated_forms_languages"]

    def clean(self):
-        cleaned_data = super(TranslatedConsentForms, self).clean()
+        cleaned_data = super(TranslatedConsentForm, self).clean()


The TranslatedConsentFormsView still uses a plural which is a tiny bit confusing

miggol · 2024-09-27T09:43:51Z

proposals/utils/checkers.py

+
+    def check(self):
+        if self.stepper.has_multiple_studies():
+            self.title = _("Trajecten afronding")


miggol · 2024-09-27T10:08:47Z

proposals/utils/pdf_diff_logic.py

+        obj = self.obj
+
+        rows_to_remove = []
+        for x in range(0, len(self.row_fields), 2):


This is such an unnecessarily complicated way to remove at most two fields. But it's kind of funny as well

Yeah, I have kindoff a love/hate relationship with this ... I often start thinking of a way to get rid of it, but then end up spending too much time on it (again ...) and I just move on. I'll get around to it someday ;p

miggol · 2024-09-27T10:29:06Z

studies/forms.py

            "risk",
            "risk_details",
        ]
        widgets = {
            "deception": BootstrapRadioSelect(),
            "negativity": BootstrapRadioSelect(),
-            "stressful": BootstrapRadioSelect(),


This field is still mentioned in _soft_validation_fields

miggol · 2024-09-27T10:41:41Z

studies/forms.py

+            "risk",
+        )
+        for field in self.base_fields:
+            self.fields[field].empty_label = None


I don't know if this does anything if we set choices explicitly anyway

miggol · 2024-09-27T10:45:17Z

studies/forms.py

-        self.fields["stressful"].choices = YesNoDoubt.choices
-        self.fields["risk"].empty_label = None
-        self.fields["risk"].choices = YesNoDoubt.choices
+        self.base_fields = (


Because we reference base_fields in clean() we should make sure we define it before calling super().__init__(). Or else the super init might call clean before base_fields is defined, resulting in an error.

I can't get this to actually happen though, so it might depend on MRO. But we know it's possible.

miggol · 2024-09-27T10:57:36Z

reviews/tests.py

        self.study.save()

        reasons = auto_review(self.proposal)
        self.assertEqual(len(reasons), 6)

-        self.study.risk = YesNoDoubt.YES
+        self.study.researcher_risk = YesNoDoubt.YES


researcher_risk is a proposal attribute

Nice that you thought of updating the tests though! But also remember to test your tests :)

EdoStorm96 · 2024-09-30T15:08:00Z

So, I've addressed your comments. The translations were a lot of work, because a lot of stuff was behind (#705 ...). I promise to keep translations up-to-date from now on 😔.

Most of the translations regarding this branch I've pulled from my old branch. Other things, which I've previously forgotten, I've just translated to my best intuition ... This should bring most of the translations for major/4 up to date.

EdoStorm96 · 2024-10-01T08:57:25Z

Ok, I've also adressed a dead link on index.html here. I ran into this during the translations. fixes #710.

EdoStorm96 · 2024-10-01T13:00:58Z

Woops, so I missed the issue with the tests you pointed out, which turned me onto the other issue with the tests, that we discussed earlier.

miggol

With every PR things are looking better and better, on both the code and application side. Having a hard time keeping up with all the improvements.

I wanted to mention that the diff page needs a new lick of paint but I see there's already an issue for that (#706).

Because this branch as is does have breaking errors I'm not approving right now but I expect the fixes to be simple.

miggol · 2024-10-02T09:36:02Z

studies/forms.py

        """

+        self.base_fields = (


Okay whoops, there's another issue with this. Turns out that self.base_fields is a reserved attribute used in BaseModelForm which in fact messes with form initialization.

I assume that's just a coincidence and you did not intend to name it that way specifically, so renaming self.base_fields to something like self.choice_fields should resolve the resulting AttributeError.

miggol · 2024-10-02T09:38:38Z

studies/forms.py

@@ -29,7 +29,7 @@
 class StudyForm(SoftValidationMixin, ConditionalModelForm):

    age_groups_header = TemplatedFormTextField(
-        header=_("De leeftijdsgroup van je deelnemers"), header_element="h4"
+        header=_("De leeftijdsgroep van je deelnemers"), header_element="h4"


I had such a hard time discerning what actually changed here

miggol · 2024-10-02T09:41:56Z

media/test_pdf.pdf

miggol · 2024-10-02T10:16:44Z

reviews/tests.py

        self.study.save()

        reasons = auto_review(self.proposal)
        self.assertEqual(len(reasons), 6)

-        self.study.risk = YesNoDoubt.YES
+        self.study.proposal.researcher_risk = YesNoDoubt.YES
        self.study.save()


The tests seem to pass fine without saving the object. But if you could save the proposal object here instead of the study at least it will be internally consistent, should there be a case (in the future) where saving does matter.

miggol · 2024-10-02T10:20:07Z

studies/models.py

Ah yes, way better like this. Backslashes stress me out

miggol · 2024-10-02T10:33:21Z

proposals/utils/pdf_diff_logic.py

@@ -1029,15 +1051,17 @@ def create_context_pdf(context, model):
                    sections.append(StudyOverviewSection(study))
                    sections.append(InformedConsentFormsSection(study.documents))

-                extra_documents = get_extra_documents(model)
+                sections.append(KnowledgeSecuritySection(proposal))


When generating a PDF for a proposal with sessions, line 1050 causes an error. I suspect this is just a mismatched indent and that the line just needs one more indentation level.

This doesn't seem to be related to this PR in particular, so I couldn't select the specific line in this review. But if you could quickly fix it here that would be great.

miggol

Looking good

EdoStorm96 added 10 commits September 24, 2024 15:35

feat: rename TranslatedConsentForm

2f46c2f

feat: add fields to proposal and make migration

045e437

feat: modify study model and migration

b3ed947

feat: make KnowledgeSecurityForm

40e7235

feat: update StudyEndForm

fb6b7d8

feat: add KnowledgeSecurityView

1b538c3

feat: implement pdf_diff_logic for new questions

6446f56

feat: update auto_review & tests

1515411

fix: woops, some ugly string stuff

8c523b5

fix: rename KnowledgeSecurityChecker

33be1ae

EdoStorm96 requested a review from miggol September 25, 2024 14:50

style: steez

712270f

EdoStorm96 linked an issue Sep 25, 2024 that may be closed by this pull request

Extra question re knowledge security and safety researchers #546

Open

EdoStorm96 marked this pull request as ready for review September 25, 2024 14:57

miggol requested changes Sep 27, 2024

View reviewed changes

EdoStorm96 added 5 commits September 30, 2024 16:46

fix: update a lot of translations

d75c387

fix: rename TranslatedConsentFormsView to TranslatedConsentView

d00f26c

fix: remove stressful from soft_validation_fields

72634a5

fix: improvements to the StudyEndForm

4d094e1

style: blck

a2a2fa9

EdoStorm96 requested a review from miggol September 30, 2024 15:08

EdoStorm96 linked an issue Sep 30, 2024 that may be closed by this pull request

Translations #705

Closed

EdoStorm96 added 2 commits October 1, 2024 10:16

fix: update link on index.html

e3f9788

style: djlint

17a6d1d

EdoStorm96 linked an issue Oct 1, 2024 that may be closed by this pull request

fix dead links on index.html #710

Closed

EdoStorm96 added 3 commits October 1, 2024 14:21

fix: tests

6c0c319

Merge branch 'major/4' into feature/revise_study_end_questions_4

44ecaca

fix: add test_pdf.pdf for tests

b3f7861

miggol requested changes Oct 2, 2024

View reviewed changes

EdoStorm96 added 3 commits October 2, 2024 14:58

fix: indentation TasksOverviewSection

a29de10

fix: save proposal in AutoReviewTests

703ddfe

fix: rename self.choice_fields attribute of StudyEndForm

41e6347

EdoStorm96 requested a review from miggol October 2, 2024 13:24

miggol approved these changes Oct 8, 2024

View reviewed changes

EdoStorm96 merged commit 262cc6a into major/4 Oct 8, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/revise study end questions 4 #707

Feature/revise study end questions 4 #707

EdoStorm96 commented Sep 25, 2024

miggol left a comment

miggol Sep 27, 2024

miggol Sep 27, 2024

miggol Sep 27, 2024

miggol Sep 27, 2024

EdoStorm96 Sep 30, 2024

miggol Sep 27, 2024

miggol Sep 27, 2024

miggol Sep 27, 2024

miggol Sep 27, 2024

EdoStorm96 commented Sep 30, 2024

EdoStorm96 commented Oct 1, 2024

EdoStorm96 commented Oct 1, 2024

miggol left a comment

miggol Oct 2, 2024

miggol Oct 2, 2024

miggol Oct 2, 2024

miggol Oct 2, 2024

miggol Oct 2, 2024

miggol Oct 2, 2024

miggol left a comment

Feature/revise study end questions 4 #707

Feature/revise study end questions 4 #707

Conversation

EdoStorm96 commented Sep 25, 2024

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

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

EdoStorm96 commented Sep 30, 2024

EdoStorm96 commented Oct 1, 2024

EdoStorm96 commented Oct 1, 2024

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

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