Feature/4 small issues #579

EdoStorm96 · 2023-11-15T09:16:03Z

This PR fixes #350, #575 and #578.

350: This was the biggest one. I added an is_commission_review field to the Review model, so that there is a simple check to see if a review is a supervisor review, besides the stage. Some things to note:
- Default is True, only get changed when creating a supervisor review.
- I wrote a data migration, which worked well for me, but have a look at that.
- I wrote some tests for this, which was a first for me. I guess this is fine? But I am open to suggestions.
575: Just make sure the supervisor's name, instead of solisID is displayed on the review detail page.
578: Added Chamber information to the PDF header. Right now, just using the acronyms, as this was requested by Desiree. It now looks like this, at the top of the page:

'FEtC-H - Looking at the elderly_rev (reference number 23-019-01, submitted by Justin Bieber) - LC'

The code in the template is a bit ugly ... I was kindoff struggling with keeping the chamber name translatable, but IMO this is fine.

EdoStorm96 · 2023-11-15T11:19:03Z

Ok, so I decided to also incorporate a fix for #491 here. I hope this PR is not getting too messy for you both ...

491:
So this alters how the avg_understood field works. It is now nullable, the widget for it has changed to a radioselect button, and it can be either yes or no, but not be left blank. This field is now also displayed in the pdf & diff.

I did run into one issue here ... The AVGUnderstoodValidator function (the logic of which I updated in line with the field's new requirements) was seemingly not doing anything for me, so I tried removing it entirely. This seems to not be possible, when making migrations. So now it is just there, but not really doing anything? I created an error message in the form's clean method, which works fine, but could/should I remove the validator? Or is it best to keep it and get rid of my new error message? I did not really see the purpose of this validator ...

miggol

Right off the bat, I see that there's a new way to differentiate supervisor reviews (yay) but that we still don't differentiate between open and closed supervisor reviews. Could you add that functionality, such that concluded supervisor reviews are given the stage review.CLOSED? This logic lives in Review.update_go() currently.

I wanted to bring that up right away, because closing those reviews might bring up more required changes, and would also require changes to the data migration to close all old supervisor reviews.

I only had time to look at the #350 part of this PR so far. I will review this further at a later time.

tymees

Fully agree with Michael's notes about #350, the whole point was to be able to close those reviews as well. (The issue was created because of a different issue partly caused by supervisors reviews never closing).

I did take a look at the other issues. The fix for #575 is a automatic approve ;)
I do have comments for the others

tymees · 2023-11-15T11:54:36Z

proposals/templates/proposals/proposal_pdf.html

      FETC-GW - <em>{{ title }}</em> (referentienummer {{ reference_number }}, ingediend door {{ submitter }})
    {% endblocktrans %}
+    - {% trans proposal.reviewing_committee.name %}


IIRC Desiree wanted it to be part of the refnum. (Which we do actually in other places too)

It's fine if the committee name isn't translated; user's never get the refnum format anyway

tymees · 2023-11-15T11:57:50Z

proposals/validators.py

+# This does not seem to do anything, so I removed it from the model, however,
+# if I try to remove this validator entirely I get an error when making
+# migrations?
+    if value is None:
        raise forms.ValidationError(
-            _('Je dient kennis genomen te hebben van de AVG om jouw aanvraag in '
+            _('Je dient deze vraag in te vullen om jouw aanvraag in '
              'te dienen'), code='avg'
            )


I cannot reproduce your issue. Disabling your form validation works reliably. I did even test by changing the rule to fail on False and True to see if that works.

tymees · 2023-11-15T12:00:37Z

proposals/models.py

    avg_understood = models.BooleanField(
-        _('Ik heb kennis genomen van het bovenstaande en begrijp mijn verantwoordelijkheden ten opzichte van de AVG.'),
-        default=False,
-        null=False,
+        _('Ik heb mijn aanvraag en de documenten voor deelnemers besproken met de privacy officer.'),
+        default=None,
+        null=True,
+        blank=True,
        validators=[AVGUnderstoodValidator],
    )


I'm not sure if changing the label/help text of an existing, different question, is the way to go here. You're effectively rewriting history a bit.

It's less of a problem now that we have canonical PDF, as the record will not change (and thus, the proper history is preserved).

However, there are still two issues;
From a developer view, the variable name is confusing now.
From an applicants view, upon revision/copy/amendment we're going to fill in 'yes' to all existing proposals for this question. (As it was required to be true before). This is less than ideal, as ideally we want to force applicants to re-evaluate if they actually did/need to contact the privacy officer.

I'd remove the whole field and add another; that should also fix your migration error ;)

tymees · 2023-11-15T12:09:59Z

One last thing: you only added the is_committee_review variable. It would be nice if a app-wide pass was made to actually use it in QuerySets over the old annoying method

EdoStorm96 · 2023-11-16T10:20:48Z

Ok, so I implemented the changes you both requested, some notes:

I updated the Review.update_go() method as Michael suggested to close supervisor reviews. I now use this method on all past reviews in the data migration, to update their stage when relevant. See review migration 0013.
So I did end up replacing field avg_understood with privacy_officer, as Ty suggested. This does indeed make more sense.
To get rid of the AVGUnderstoodValidator I removed it and all references to it from the repo. However, this caused a problem, because it got referenced in proposal migration 0034. As suggested in the Django docs, I just placed this function in this migration (as an empty function) and now all seems to work fine. (Despite this solution looking shady to me ...)

tymees

Some minor tweaks, and it should be good to go :)

tymees · 2023-11-16T12:24:58Z

reviews/migrations/0013_is_commission_review_field.py

@@ -11,6 +11,7 @@ def update_is_commssion_review(apps, schema_editor):
        SUPERVISOR_STAGE = 0
        if review.stage == SUPERVISOR_STAGE:
            review.is_commission_review = False
+            review.update_go()


In theory this should work... However, the production DB isn't as clean as we would like.

I'd feel better if the code was something like this:

if review.go: review.stage = CLOSED

It's a lot simpler, and should be safer.

tymees · 2023-11-16T12:25:40Z

reviews/templates/reviews/review_detail_sidebar.html

@@ -90,7 +90,7 @@ <h3>
                </em>
            </li>
        {% endif %}
-        {% if review.stage == review.SUPERVISOR %}
+        {% if review.is_commission_review == False %}


It's cleaner to write {% if not review.is_commission_review %}

tymees · 2023-11-16T12:25:54Z

reviews/views.py

@@ -556,7 +556,7 @@ def form_valid(self, form):

        # Don't notify the secretary if this is a supervisor decision.
        # If it was a GO they the secretary will be notified anyway
-        if not review.stage == review.SUPERVISOR:
+        if review.is_commission_review == True:


== True is redundant

tymees · 2023-11-16T12:26:42Z

proposals/templates/proposals/proposal_pdf.html

+    {% blocktrans with title=proposal.title reference_number=proposal.reference_number submitter=proposal.created_by.get_full_name reviewing_committee=proposal.reviewing_committee.name trimmed %}
+      FETC-GW - <em>{{ title }}</em> (referentienummer {{ reference_number }}-{{reviewing_committee}}, ingediend door {{ submitter }})


Hate to be that guy... But conventionally we place the committee before the rest of the refnum.

Sorry, should've clarified that in my original comment

EdoStorm96 · 2023-11-16T13:22:34Z

All right, so I've implemented Ty's minor tweaks, but, more importantly, I realized that it should be is_committee_review and not is_commission_review. This is a typo that occurred when discussing the original issue (#350) and was then adopted in this discussion and also by me ;p I re-ran the migrations on my branch, so that there is no evidence of this. I think it should be all good now!

EdoStorm96 added 3 commits November 14, 2023 15:35

Added is_commission_review field to Review model

6cdfacb

Supervisor name in review detail sidebar

667860e

Added chamber info to PDF

53bfe72

EdoStorm96 requested review from tymees and miggol November 15, 2023 09:16

This was linked to issues Nov 15, 2023

Supervisor and reviewer: no solisid but full name #575

Closed

PDF - chamber information missing #578

Closed

EdoStorm96 marked this pull request as draft November 15, 2023 10:35

fixes #491

c3c459e

EdoStorm96 linked an issue Nov 15, 2023 that may be closed by this pull request

Privacy officer consulted? yes/no #491

Closed

EdoStorm96 changed the title ~~Feature/3 small issues~~ Feature/4 small issues Nov 15, 2023

EdoStorm96 marked this pull request as ready for review November 15, 2023 11:22

miggol requested changes Nov 15, 2023

View reviewed changes

tymees requested changes Nov 15, 2023

View reviewed changes

Implemented requested changes on PR #579

7ef30ec

EdoStorm96 requested review from tymees and miggol November 16, 2023 10:21

tymees requested changes Nov 16, 2023

View reviewed changes

changed is_commission_review to is_committee_review + minor tweaks

75ab28a

EdoStorm96 requested a review from tymees November 16, 2023 13:23

tymees approved these changes Nov 16, 2023

View reviewed changes

EdoStorm96 merged commit b46a24c into develop Nov 20, 2023
2 checks passed

EdoStorm96 deleted the feature/3_small_issues branch November 21, 2023 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/4 small issues #579

Feature/4 small issues #579

EdoStorm96 commented Nov 15, 2023

EdoStorm96 commented Nov 15, 2023

miggol left a comment

tymees left a comment

tymees Nov 15, 2023

tymees Nov 15, 2023

tymees Nov 15, 2023

tymees commented Nov 15, 2023

EdoStorm96 commented Nov 16, 2023

tymees left a comment

tymees Nov 16, 2023

tymees Nov 16, 2023

tymees Nov 16, 2023

tymees Nov 16, 2023

EdoStorm96 commented Nov 16, 2023

		{% blocktrans with title=proposal.title reference_number=proposal.reference_number submitter=proposal.created_by.get_full_name reviewing_committee=proposal.reviewing_committee.name trimmed %}
		FETC-GW - <em>{{ title }}</em> (referentienummer {{ reference_number }}-{{reviewing_committee}}, ingediend door {{ submitter }})

Feature/4 small issues #579

Feature/4 small issues #579

Conversation

EdoStorm96 commented Nov 15, 2023

EdoStorm96 commented Nov 15, 2023

miggol left a comment

Choose a reason for hiding this comment

tymees left a comment

Choose a reason for hiding this comment

tymees Nov 15, 2023

Choose a reason for hiding this comment

tymees Nov 15, 2023

Choose a reason for hiding this comment

tymees Nov 15, 2023

Choose a reason for hiding this comment

tymees commented Nov 15, 2023

EdoStorm96 commented Nov 16, 2023

tymees left a comment

Choose a reason for hiding this comment

tymees Nov 16, 2023

Choose a reason for hiding this comment

tymees Nov 16, 2023

Choose a reason for hiding this comment

tymees Nov 16, 2023

Choose a reason for hiding this comment

tymees Nov 16, 2023

Choose a reason for hiding this comment

EdoStorm96 commented Nov 16, 2023