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

Temp/embargo and tests wip #835

Merged
merged 8 commits into from
Jan 8, 2025
Merged

Conversation

EdoStorm96
Copy link
Contributor

@EdoStorm96 EdoStorm96 commented Jan 7, 2025

Make tests pass :)

Just some final issues to ensure major/4 can pass tests ...

Besides the stuff you did already, I made the PreAss and PreAppr proposals use the test_pdf.pdf file, when applicable, and solved the issue with adding a WMO object to PreAss proposals in the test db.

PS. I am not quite sure what self.wmo on line 80 of proposal_submission_tests.py is doing now ... As I don´t see it being referenced anywhere ... But I tried removing it and it broke the tests ...

@EdoStorm96 EdoStorm96 requested a review from miggol January 7, 2025 14:24
Copy link
Contributor

@miggol miggol left a comment

Choose a reason for hiding this comment

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

Approved with some minor requests.

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I intended to add a comment along the lines of

"we save this variable for later, but want it set for None for this part of the validation"

Because as it stands this is kind of confusing.

Comment on lines +80 to +85
self.wmo = Wmo(
status=0,
metc="N",
is_medical="N",
)
self.wmo.save()
Copy link
Contributor

Choose a reason for hiding this comment

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

This wmo must be implicitly getting used for the regular self.proposal.wmo without getting explicitly assigned. I don't know why (it's very confusing), but perhaps it would be better to add self.proposal.wmo = self.wmo down here:

    def setup_proposal(self):
        """
        Load our test proposals from a fixture, then add our user as
        an applicant to each of them.

        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.wmo = self.wmo
        self.proposal.save()

Right before the proposal gets saved. Just to make it explicit and less magic.

@EdoStorm96 EdoStorm96 merged commit 300591d into major/4 Jan 8, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants