From 12e4991bfce6c9f601b3a83795b6605636437068 Mon Sep 17 00:00:00 2001 From: Michael Villeneuve Date: Tue, 7 Jan 2025 11:25:39 +0100 Subject: [PATCH 1/8] wip: Fixing proposal tests --- .../fixtures/testing/test_proposals.json | 125 ++++++++++-------- proposals/forms.py | 76 +++++++---- proposals/tests/proposal_submission_tests.py | 40 +++--- proposals/views/proposal_views.py | 1 + 4 files changed, 144 insertions(+), 98 deletions(-) diff --git a/proposals/fixtures/testing/test_proposals.json b/proposals/fixtures/testing/test_proposals.json index e49b7a3f3..376524553 100644 --- a/proposals/fixtures/testing/test_proposals.json +++ b/proposals/fixtures/testing/test_proposals.json @@ -3,23 +3,23 @@ "model": "proposals.proposal", "pk": 1, "fields": { - "reference_number": "23-003-01", + "reference_number": "24-009-01", "reviewing_committee": 3, - "institution": 2, - "date_start": "2025-01-01", - "title": "Fixture proposal", - "summary": "asdf", + "institution": 4, + "date_start": "2024-12-27", + "title": "Normal test proposal", + "summary": "fsefsdfes", "other_applicants": false, "other_stakeholders": false, "stakeholders": "", - "translated_forms": true, - "translated_forms_languages": "asdf", - "funding_details": "", - "funding_name": "", - "comments": "asdf", - "inform_local_staff": true, - "embargo": true, - "embargo_end_date": "2024-09-01", + "translated_forms": false, + "translated_forms_languages": null, + "funding_details": "fsefdsf", + "funding_name": "esfdegf", + "comments": "aaaaa", + "inform_local_staff": null, + "embargo": false, + "embargo_end_date": null, "in_archive": false, "is_pre_assessment": false, "pre_assessment_pdf": "", @@ -28,23 +28,28 @@ "pre_approval_pdf": "", "in_course": false, "is_exploration": false, - "pdf": "FETC-AK-23-003-01-Proposal.pdf", - "studies_similar": false, - "studies_number": 3, + "pdf": "", + "studies_similar": true, + "studies_number": 1, "status": 1, "status_review": null, - "dmp_file": "FETC-AK-23-003-01-DMP.pdf", + "privacy_officer": false, + "dmp_file": "", "confirmation_comments": "", - "date_created": "2023-11-28T12:59:17.670Z", - "date_modified": "2023-11-28T13:36:55.297Z", + "date_created": "2024-12-19T14:52:13.624Z", + "date_modified": "2024-12-23T10:29:26.363Z", "date_submitted_supervisor": null, "date_reviewed_supervisor": null, - "date_submitted": "2023-11-28T13:36:55.296Z", + "date_submitted": null, "date_reviewed": null, "date_confirmed": null, "has_minor_revision": false, "minor_revision_description": null, - "self_assessment": "asdf", + "self_assessment": "gsdfges", + "knowledge_security": "N", + "knowledge_security_details": "", + "researcher_risk": "N", + "researcher_risk_details": "", "relation": 5, "student_program": "", "student_context": null, @@ -55,7 +60,7 @@ "parent": null, "is_revision": false, "funding": [ - 1 + 3 ], "applicants": [ 5 @@ -66,48 +71,53 @@ "model": "proposals.proposal", "pk": 2, "fields": { - "reference_number": "23-004-01", + "reference_number": "24-010-01", "reviewing_committee": 3, - "institution": 2, - "date_start": "2024-08-01", - "title": "Fixture Preassessment", + "institution": 4, + "date_start": "2042-12-25", + "title": "Preassessment test proposal", "summary": "", "other_applicants": false, - "other_stakeholders": true, - "stakeholders": "asdf", + "other_stakeholders": false, + "stakeholders": "", "translated_forms": null, "translated_forms_languages": null, "funding_details": "", - "funding_name": "", - "comments": "asdf", + "funding_name": "aaaaa", + "comments": "asdfda", "inform_local_staff": null, - "embargo": true, - "embargo_end_date": "2024-11-01", + "embargo": null, + "embargo_end_date": null, "in_archive": false, "is_pre_assessment": true, - "pre_assessment_pdf": "test_pdf.pdf", + "pre_assessment_pdf": "FETC-AK-24-010-01-Name-Preassessment.pdf", "is_pre_approved": null, "pre_approval_institute": null, "pre_approval_pdf": "", "in_course": false, "is_exploration": false, - "pdf": "FETC-AK-23-004-01-Proposal.pdf", + "pdf": "", "studies_similar": null, "studies_number": 1, "status": 1, "status_review": null, + "privacy_officer": null, "dmp_file": "", "confirmation_comments": "", - "date_created": "2023-11-28T14:10:12.708Z", - "date_modified": "2023-11-28T14:10:40.554Z", + "date_created": "2024-12-23T10:30:40.272Z", + "date_modified": "2024-12-23T10:35:22.000Z", "date_submitted_supervisor": null, "date_reviewed_supervisor": null, - "date_submitted": "2023-11-28T14:10:40.554Z", + "date_submitted": null, "date_reviewed": null, "date_confirmed": null, "has_minor_revision": false, "minor_revision_description": null, "self_assessment": "asdf", + "knowledge_security": "", + "knowledge_security_details": "", + "researcher_risk": "", + "researcher_risk_details": "", "relation": 5, "student_program": "", "student_context": null, @@ -117,7 +127,9 @@ "supervisor": null, "parent": null, "is_revision": false, - "funding": [], + "funding": [ + 2 + ], "applicants": [ 5 ] @@ -127,48 +139,53 @@ "model": "proposals.proposal", "pk": 3, "fields": { - "reference_number": "23-005-01", + "reference_number": "24-011-01", "reviewing_committee": 3, - "institution": 2, - "date_start": "2024-08-01", - "title": "Fixture Preapproval", - "summary": "asdf", + "institution": 4, + "date_start": "2046-12-21", + "title": "Preapproved test pdf", + "summary": "asd", "other_applicants": false, "other_stakeholders": false, "stakeholders": "", - "translated_forms": null, + "translated_forms": false, "translated_forms_languages": null, "funding_details": "", "funding_name": "", - "comments": "asdf", + "comments": "asd", "inform_local_staff": null, - "embargo": true, - "embargo_end_date": "2025-01-10", + "embargo": false, + "embargo_end_date": null, "in_archive": false, "is_pre_assessment": false, "pre_assessment_pdf": "", "is_pre_approved": true, - "pre_approval_institute": "asdf", - "pre_approval_pdf": "test_pdf.pdf", + "pre_approval_institute": "asd", + "pre_approval_pdf": "FETC-AK-24-011-01-Name-Pre_Approval.pdf", "in_course": false, "is_exploration": false, - "pdf": "FETC-AK-23-005-01-Proposal.pdf", + "pdf": "", "studies_similar": null, "studies_number": 1, "status": 1, "status_review": null, + "privacy_officer": true, "dmp_file": "", "confirmation_comments": "", - "date_created": "2023-11-28T14:28:19.045Z", - "date_modified": "2023-11-28T14:28:31.971Z", + "date_created": "2024-12-23T11:15:55.495Z", + "date_modified": "2024-12-23T11:17:02.358Z", "date_submitted_supervisor": null, "date_reviewed_supervisor": null, - "date_submitted": "2023-11-28T14:28:31.971Z", + "date_submitted": null, "date_reviewed": null, "date_confirmed": null, "has_minor_revision": false, "minor_revision_description": null, - "self_assessment": "asdf", + "self_assessment": "asd", + "knowledge_security": "", + "knowledge_security_details": "", + "researcher_risk": "", + "researcher_risk_details": "", "relation": 5, "student_program": "", "student_context": null, diff --git a/proposals/forms.py b/proposals/forms.py index 5feb90cb1..51e5e95c8 100644 --- a/proposals/forms.py +++ b/proposals/forms.py @@ -832,7 +832,8 @@ def __init__(self, *args, **kwargs): self.request = kwargs.pop("request", None) # Needed for validation - self.final_validation = kwargs.pop("final_validation", None) + final_validation = kwargs.pop("final_validation", None) + self.final_validation = None super(ProposalSubmitForm, self).__init__(*args, **kwargs) @@ -847,6 +848,13 @@ def __init__(self, *args, **kwargs): del self.fields["embargo"] del self.fields["embargo_end_date"] + self.final_validation = final_validation + + # If we have an existing instance and we're not POSTing, + # run an initial clean + if self.instance.pk and "data" not in kwargs: + self.initial_clean() + def clean(self): """ Check if the Proposal is complete: @@ -857,53 +865,65 @@ def clean(self): cleaned_data = super(ProposalSubmitForm, self).clean() + if ( + check_local_facilities(self.proposal) + and cleaned_data["inform_local_staff"] is None + ): + self.add_error("inform_local_staff", _("Dit veld is verplicht.")) + if ( not self.instance.is_practice() and not "js-redirect-submit" in self.request.POST and not "save_back" in self.request.POST ): - if ( - check_local_facilities(self.proposal) - and cleaned_data["inform_local_staff"] is None - ): - self.add_error("inform_local_staff", _("Dit veld is verplicht.")) - - if cleaned_data["embargo"] is None: - self.add_error("embargo", _("Dit veld is verplicht.")) + self.validate_embargo(cleaned_data) - embargo_end_date = cleaned_data["embargo_end_date"] - two_years_from_now = timezone.now().date() + timezone.timedelta(days=730) - - if "embargo" in cleaned_data: - if cleaned_data["embargo"] is True and not embargo_end_date: - self.add_error( - "embargo_end_date", - _("Vul een datum in waarop de embargo afloopt."), - ) - - if embargo_end_date is not None and embargo_end_date > two_years_from_now: - self.add_error( - "embargo_end_date", - _( - "De embargo-periode kan maximaal 2 jaar zijn. Kies een datum binnen 2 jaar van vandaag." - ), - ) # final_validation is a kwarg that should only be given by the # actual submit view, not the stepper. if not self.final_validation: return - # For final validation, i.e. if a user is actually submitting,we + # For final validation, i.e. if a user is actually submitting, we # instantiate a new stepper to reflect changes # made to the current instance, which may not yet be saved. from proposals.utils.stepper import Stepper - validator = Stepper(self.instance, request=self.request) + # And then we append stepper errors to show them to the user + # on the submit page if validator.get_form_errors(): self.add_error( None, _("Aanvraag bevat nog foutmeldingen"), ) + def validate_embargo(self, cleaned_data): + + # Only check embargo and end date if form was initialized with it. + if "embargo" not in self.fields: + return + + self.mark_soft_required(cleaned_data, "embargo") + + if "embargo" in cleaned_data: + self.check_dependency( + cleaned_data, + "embargo", + "embargo_end_date", + error_message=_("Vul een datum in waarop de embargo afloopt."), + ) + embargo_end_date = cleaned_data.get("embargo_end_date", None) + if embargo_end_date: + two_years_from_now = ( + timezone.now().date() + timezone.timedelta(days=730) + ) + if embargo_end_date > two_years_from_now: + self.add_error( + "embargo_end_date", + _( + "De embargo-periode kan maximaal 2 jaar zijn. " + "Kies een datum binnen 2 jaar van vandaag." + ), + ) + class KnowledgeSecurityForm(SoftValidationMixin, ConditionalModelForm): diff --git a/proposals/tests/proposal_submission_tests.py b/proposals/tests/proposal_submission_tests.py index 40a215baa..7f90cec3e 100644 --- a/proposals/tests/proposal_submission_tests.py +++ b/proposals/tests/proposal_submission_tests.py @@ -5,7 +5,7 @@ from django.test import TestCase from main.tests import BaseViewTestCase -from proposals.models import Proposal, Relation +from proposals.models import Proposal, Relation, Wmo from reviews.models import Review from proposals.views.proposal_views import ( @@ -77,6 +77,12 @@ def setup_proposal(self): Please note, this currently uses a mish-mash of both fixtures and programmatically created users and objects. """ + self.wmo = Wmo( + status=0, + metc="N", + is_medical="N", + ) + self.wmo.save() self.proposal = Proposal.objects.get(pk=1) self.proposal.applicants.add(self.user) self.proposal.save() @@ -156,21 +162,22 @@ def test_submission_unsupervised(self): self.get_post_data(), user=self.user, ) - self.assertIn(page.status_code, [302, 200]) - self.refresh() - # Post-submission tests - self.assertNotEqual( - self.proposal.status, - self.proposal.Statuses.DRAFT, - ) - self.assertNotEqual( - self.proposal.latest_review(), - None, - ) - self.assertEqual( - self.proposal.latest_review().stage, - Review.Stages.ASSIGNMENT, - ) + if True: + self.assertIn(page.status_code, [302, 200]) + self.refresh() + # Post-submission tests + self.assertNotEqual( + self.proposal.status, + self.proposal.Statuses.DRAFT, + ) + self.assertNotEqual( + self.proposal.latest_review(), + None, + ) + self.assertEqual( + self.proposal.latest_review().stage, + Review.Stages.ASSIGNMENT, + ) def test_proposal_supervised(self): """ @@ -225,6 +232,7 @@ class PreassessmentSubmitTestCase( def setUp(self): super().setUp() self.proposal = self.pre_assessment + self.proposal.wmo = self.wmo class PreapprovedSubmitTestCase( diff --git a/proposals/views/proposal_views.py b/proposals/views/proposal_views.py index 3c27885c4..73b47c444 100644 --- a/proposals/views/proposal_views.py +++ b/proposals/views/proposal_views.py @@ -756,6 +756,7 @@ def form_valid(self, form): if ( "save_back" not in self.request.POST and "js-redirect-submit" not in self.request.POST + and not form.instance.is_practice() ): start_review(proposal) return success_response From c66e8418a389cc38ab0c59ad1e7e17807d967cd6 Mon Sep 17 00:00:00 2001 From: Edo Storm Date: Tue, 7 Jan 2025 15:20:51 +0100 Subject: [PATCH 2/8] fix: use the test pdf for tests --- proposals/fixtures/testing/test_proposals.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/proposals/fixtures/testing/test_proposals.json b/proposals/fixtures/testing/test_proposals.json index 376524553..d50eceba0 100644 --- a/proposals/fixtures/testing/test_proposals.json +++ b/proposals/fixtures/testing/test_proposals.json @@ -90,7 +90,7 @@ "embargo_end_date": null, "in_archive": false, "is_pre_assessment": true, - "pre_assessment_pdf": "FETC-AK-24-010-01-Name-Preassessment.pdf", + "pre_assessment_pdf": "test_pdf.pdf", "is_pre_approved": null, "pre_approval_institute": null, "pre_approval_pdf": "", @@ -161,7 +161,7 @@ "pre_assessment_pdf": "", "is_pre_approved": true, "pre_approval_institute": "asd", - "pre_approval_pdf": "FETC-AK-24-011-01-Name-Pre_Approval.pdf", + "pre_approval_pdf": "test_pdf.pdf", "in_course": false, "is_exploration": false, "pdf": "", From c83f1081f4e9c27cc565dc61195c2bc0f60ccb2e Mon Sep 17 00:00:00 2001 From: Edo Storm Date: Tue, 7 Jan 2025 15:21:22 +0100 Subject: [PATCH 3/8] fix: bug concerning PreAss Wmo in tests --- proposals/tests/proposal_submission_tests.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/proposals/tests/proposal_submission_tests.py b/proposals/tests/proposal_submission_tests.py index 7f90cec3e..7502e99fb 100644 --- a/proposals/tests/proposal_submission_tests.py +++ b/proposals/tests/proposal_submission_tests.py @@ -232,7 +232,13 @@ class PreassessmentSubmitTestCase( def setUp(self): super().setUp() self.proposal = self.pre_assessment - self.proposal.wmo = self.wmo + wmo = Wmo( + status=0, + metc="N", + is_medical="N", + ) + wmo.save() + self.proposal.wmo = wmo class PreapprovedSubmitTestCase( From 38947e04b7580143cbb3dccbe1c5c73bdd3d1dba Mon Sep 17 00:00:00 2001 From: Edo Storm Date: Tue, 7 Jan 2025 15:25:50 +0100 Subject: [PATCH 4/8] fix: remove if True --- proposals/tests/proposal_submission_tests.py | 32 ++++++++++---------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/proposals/tests/proposal_submission_tests.py b/proposals/tests/proposal_submission_tests.py index 7502e99fb..6d297d042 100644 --- a/proposals/tests/proposal_submission_tests.py +++ b/proposals/tests/proposal_submission_tests.py @@ -162,22 +162,22 @@ def test_submission_unsupervised(self): self.get_post_data(), user=self.user, ) - if True: - self.assertIn(page.status_code, [302, 200]) - self.refresh() - # Post-submission tests - self.assertNotEqual( - self.proposal.status, - self.proposal.Statuses.DRAFT, - ) - self.assertNotEqual( - self.proposal.latest_review(), - None, - ) - self.assertEqual( - self.proposal.latest_review().stage, - Review.Stages.ASSIGNMENT, - ) + + self.assertIn(page.status_code, [302, 200]) + self.refresh() + # Post-submission tests + self.assertNotEqual( + self.proposal.status, + self.proposal.Statuses.DRAFT, + ) + self.assertNotEqual( + self.proposal.latest_review(), + None, + ) + self.assertEqual( + self.proposal.latest_review().stage, + Review.Stages.ASSIGNMENT, + ) def test_proposal_supervised(self): """ From c793bf91f897d88abb7a0622a723849c2b147749 Mon Sep 17 00:00:00 2001 From: Edo Storm Date: Tue, 7 Jan 2025 15:35:55 +0100 Subject: [PATCH 5/8] style: black --- proposals/forms.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/proposals/forms.py b/proposals/forms.py index 51e5e95c8..588987857 100644 --- a/proposals/forms.py +++ b/proposals/forms.py @@ -886,6 +886,7 @@ def clean(self): # instantiate a new stepper to reflect changes # made to the current instance, which may not yet be saved. from proposals.utils.stepper import Stepper + validator = Stepper(self.instance, request=self.request) # And then we append stepper errors to show them to the user # on the submit page @@ -912,9 +913,7 @@ def validate_embargo(self, cleaned_data): ) embargo_end_date = cleaned_data.get("embargo_end_date", None) if embargo_end_date: - two_years_from_now = ( - timezone.now().date() + timezone.timedelta(days=730) - ) + two_years_from_now = timezone.now().date() + timezone.timedelta(days=730) if embargo_end_date > two_years_from_now: self.add_error( "embargo_end_date", From 31ac676648bff8ba01a01e880c995f5b85b13b53 Mon Sep 17 00:00:00 2001 From: Edo Storm Date: Wed, 8 Jan 2025 10:03:43 +0100 Subject: [PATCH 6/8] feat: comment about final_validation --- proposals/forms.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/proposals/forms.py b/proposals/forms.py index 588987857..ad7f44c84 100644 --- a/proposals/forms.py +++ b/proposals/forms.py @@ -831,7 +831,8 @@ def __init__(self, *args, **kwargs): # Needed for POST data self.request = kwargs.pop("request", None) - # Needed for validation + # we save this variable for later, but want it + # set for None for this part of the validation. final_validation = kwargs.pop("final_validation", None) self.final_validation = None From 54360abb61beb04b70330da52c8c19530862b05d Mon Sep 17 00:00:00 2001 From: Edo Storm Date: Wed, 8 Jan 2025 10:03:59 +0100 Subject: [PATCH 7/8] feat: explicitly add wmo to proposal in tests --- proposals/tests/proposal_submission_tests.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/proposals/tests/proposal_submission_tests.py b/proposals/tests/proposal_submission_tests.py index 6d297d042..d59e7f91a 100644 --- a/proposals/tests/proposal_submission_tests.py +++ b/proposals/tests/proposal_submission_tests.py @@ -85,7 +85,8 @@ def setup_proposal(self): self.wmo.save() self.proposal = Proposal.objects.get(pk=1) self.proposal.applicants.add(self.user) - self.proposal.save() + self.proposal.wmo = self.wmo + self.proposal.save() self.pre_assessment = Proposal.objects.get(pk=2) self.pre_assessment.applicants.add(self.user) self.pre_assessment.save() From 790b5d0e57511424fc0d10093cc4b99d69f7fadf Mon Sep 17 00:00:00 2001 From: Edo Storm Date: Wed, 8 Jan 2025 10:04:35 +0100 Subject: [PATCH 8/8] style: black --- proposals/forms.py | 2 +- proposals/tests/proposal_submission_tests.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/proposals/forms.py b/proposals/forms.py index ad7f44c84..41d14c10d 100644 --- a/proposals/forms.py +++ b/proposals/forms.py @@ -831,7 +831,7 @@ def __init__(self, *args, **kwargs): # Needed for POST data self.request = kwargs.pop("request", None) - # we save this variable for later, but want it + # we save this variable for later, but want it # set for None for this part of the validation. final_validation = kwargs.pop("final_validation", None) self.final_validation = None diff --git a/proposals/tests/proposal_submission_tests.py b/proposals/tests/proposal_submission_tests.py index d59e7f91a..c88f4ff64 100644 --- a/proposals/tests/proposal_submission_tests.py +++ b/proposals/tests/proposal_submission_tests.py @@ -86,7 +86,7 @@ def setup_proposal(self): self.proposal = Proposal.objects.get(pk=1) self.proposal.applicants.add(self.user) self.proposal.wmo = self.wmo - self.proposal.save() + self.proposal.save() self.pre_assessment = Proposal.objects.get(pk=2) self.pre_assessment.applicants.add(self.user) self.pre_assessment.save()