Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/4 small issues #579

Merged
merged 6 commits into from
Nov 20, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified locale/en/LC_MESSAGES/django.mo
Binary file not shown.
740 changes: 270 additions & 470 deletions locale/en/LC_MESSAGES/django.po

Large diffs are not rendered by default.

12 changes: 11 additions & 1 deletion proposals/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -574,8 +574,18 @@ class ProposalDataManagementForm(SoftValidationMixin, forms.ModelForm):
class Meta:
model = Proposal
fields = ['avg_understood', 'dmp_file']
widgets = {
'avg_understood': forms.RadioSelect(choices=YES_NO),
}

def clean(self):
cleaned_data = super(ProposalDataManagementForm, self).clean()

_soft_validation_fields = ['avg_understood']
if cleaned_data['avg_understood'] is None:
self.add_error(
'avg_understood',
_('Dit veld is verplicht om verder te gaan.')
)

class ProposalUpdateDataManagementForm(forms.ModelForm):
class Meta:
Expand Down
19 changes: 19 additions & 0 deletions proposals/migrations/0050_alter_proposal_avg_understood.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Generated by Django 3.2.20 on 2023-11-15 10:56

from django.db import migrations, models
import proposals.validators


class Migration(migrations.Migration):

dependencies = [
('proposals', '0049_alter_proposal_supervisor'),
]

operations = [
migrations.AlterField(
model_name='proposal',
name='avg_understood',
field=models.BooleanField(blank=True, default=None, null=True, validators=[proposals.validators.AVGUnderstoodValidator], verbose_name='Ik heb mijn aanvraag en de documenten voor deelnemers besproken met de privacy officer.'),
),
]
7 changes: 4 additions & 3 deletions proposals/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -400,9 +400,10 @@ class Proposal(models.Model):
)

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],
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)


Expand Down
1 change: 1 addition & 0 deletions proposals/templates/proposals/proposal_pdf.html
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
{% blocktrans with title=proposal.title reference_number=proposal.reference_number submitter=proposal.created_by.get_full_name trimmed %}
FETC-GW - <em>{{ title }}</em> (referentienummer {{ reference_number }}, ingediend door {{ submitter }})
{% endblocktrans %}
- {% trans proposal.reviewing_committee.name %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{% if proposal.is_revision %}
<br>
<strong>
Expand Down
2 changes: 1 addition & 1 deletion proposals/utils/pdf_diff_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -963,7 +963,7 @@ class DMPFileSection(PageBreakMixin, BaseSection):

section_title = _("Data Management Plan")

row_fields = ["dmp_file"]
row_fields = ["dmp_file", "avg_understood"]


class EmbargoSection(BaseSection):
Expand Down
8 changes: 5 additions & 3 deletions proposals/validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,11 @@ def __call__(self, value):


def AVGUnderstoodValidator(value):

if value != True:
# 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'
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

29 changes: 29 additions & 0 deletions reviews/migrations/0013_is_commission_review_field.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Generated by Django 3.2.20 on 2023-11-14 09:46

from django.db import migrations, models

def update_is_commssion_review(apps, schema_editor):

Review = apps.get_model('reviews', 'Review')

for review in Review.objects.all():
#Hardcoded this to account for possible future changes to stages
SUPERVISOR_STAGE = 0
if review.stage == SUPERVISOR_STAGE:
review.is_commission_review = False
review.save()

class Migration(migrations.Migration):

dependencies = [
('reviews', '0012_auto_20211213_1503'),
]

operations = [
migrations.AddField(
model_name='review',
name='is_commission_review',
field=models.BooleanField(default=True),
),
migrations.RunPython(update_is_commssion_review),
]
4 changes: 4 additions & 0 deletions reviews/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ class Review(models.Model):
date_end = models.DateTimeField(blank=True, null=True)
date_should_end = models.DateField(blank=True, null=True)

is_commission_review = models.BooleanField(
default=True
)

proposal = models.ForeignKey(Proposal, on_delete=models.CASCADE)

def update_go(self, last_decision=None):
Expand Down
2 changes: 1 addition & 1 deletion reviews/templates/reviews/review_detail_sidebar.html
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ <h3>
</li>
{% if review.proposal.supervisor %}
<li>
{% trans "Supervisor" %}: {{ review.proposal.supervisor }}
{% trans "Supervisor" %}: {{ review.proposal.supervisor.get_full_name }}
</li>
{% endif %}
<li>
Expand Down
3 changes: 3 additions & 0 deletions reviews/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ def test_start_supervisor_review(self):
# If the Relation on a Proposal requires a supervisor, a Review for the supervisor should be started.
review = start_review(self.proposal)
self.assertEqual(review.stage, Review.SUPERVISOR)
self.assertEqual(review.is_commission_review, False)
self.assertEqual(Decision.objects.filter(reviewer=self.supervisor).count(), 1)
self.assertEqual(Decision.objects.filter(review=review).count(), 1)
self.assertEqual(review.decision_set.count(), 1)
Expand All @@ -170,6 +171,7 @@ def test_start_review(self):

review = start_review(self.proposal)
self.assertEqual(review.stage, Review.ASSIGNMENT)
self.assertEqual(review.is_commission_review, True)
self.assertEqual(Decision.objects.filter(reviewer=self.secretary).count(), 1)
self.assertEqual(Decision.objects.filter(review=review).count(), 1)
self.assertEqual(review.decision_set.count(), 1)
Expand All @@ -196,6 +198,7 @@ def test_decision_supervisor(self):
decision.save()
review.refresh_from_db()
self.assertEqual(review.go, True)
self.assertEqual(review.is_commission_review, False)

self.assertEqual(len(mail.outbox), 2)
self.check_subject_lines(mail.outbox)
Expand Down
1 change: 1 addition & 0 deletions reviews/utils/review_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ def start_supervisor_phase(proposal):
"""
review = Review.objects.create(proposal=proposal, date_start=timezone.now())
review.stage = Review.SUPERVISOR
review.is_commission_review = False
review.save()

proposal.date_submitted_supervisor = timezone.now()
Expand Down
Loading