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

Bug withdrawn application backend #1594

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

Snorre98
Copy link
Contributor

@Snorre98 Snorre98 commented Nov 3, 2024

⭐ alot lines added are comments and doc strings! ;-) 400 lines are tests

👁️ seed script was viloating validation in the refactored RecruitmentApplication model

What

This PR refactors the RecruitmentApplication model. I found a bug which caused the applicants prioitiy of positions to "come out of sync". This PR fixes this bug by adding constraints to the RecruitmentApplication model, as well as refactors the save and clean methodes.

Why

First of all; when an application was withdrawn the "active applications" count was not updated. Therefor it as possible to set position priority on a scale of "all applications" including withdrawn applications.
This would cause the priority to not be deterministic. By refershing the page, a new query would be made which fethced the correct priority sequence, but by reprioritizing the priority sequence could come out of sync again.

New validation and helper methodes in RecruitmentApplication

RecruitmentApplication became extencive because I created helper methodes for almost everything. I also added some validation which should have been implemented before, like validation constrains for reprioritization deadline and priority deadline for recruitment admins.

The logic for update_applicant_state is unchanged.

get_total_interviews is unchanged

get_total_applications is unchanged

Views and tests

Views

Some of the views related to fetching applications for applicant where also refactored.

  • RecruitmentApplicationForApplicantView now only gives non-withdrawn applications. There is a view dedicated to getting withdrawn applications.

  • RecruitmentApplicationWithdrawApplicantView was extended with a get methode

  • RecruitmentApplicationApplicantPriorityView was refactored to work with the refactored RecruitmentApplication model better

Tests

  • test_withdrawn_application_priority_handling test that priorities are properly managed when applications are withdrawn
  • test_reapplying_after_withdrawal_priority test that reapplying after withdrawal gets correct priority
  • test_priority_constraints_with_withdrawn_applications test that priorities stay within bounds of active applications only
  • test_multiple_withdrawals_and_priorities test complex scenario with multiple withdrawals and reapplications
  • test_recruiter_priority_update_before_deadline test that recruiter can update priority before the deadline
  • test_recruiter_priority_update_after_deadline test that recruiter cannot update priority after the deadline
  • test_deadline_validation_with_multiple_applications test deadline validation with multiple applications
  • test_applicant_update_application_priority_before_deadline Test that applicant can update their application priorities before the deadline
  • test_applicant_update_application_priority_after_deadline Test that applicant cannot update their application priorities after the applicant reprioritization deadline
  • test_applicant_priority_remains_sequential Test that application priorities remain sequential (1,2,3...) when updated
  • test_deadline_validation_constraints Test application validation against recruitment timeline deadlines
  • test_priority_validation Test validation of application priority assignments
  • test_reorder_priorities_edge_cases Test edge cases in priority reordering logic

Replaces backend part of #1542

closes #1535

@Snorre98 Snorre98 self-assigned this Nov 3, 2024
@Snorre98 Snorre98 marked this pull request as ready for review November 3, 2024 05:13
Copy link
Contributor

@Mathias-a Mathias-a left a comment

Choose a reason for hiding this comment

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

idk se på litt

Comment on lines +293 to +294

# Core application fields
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this even mean?

Comment on lines +341 to +342
# Track original withdrawn state to detect changes
self._original_withdrawn = self.withdrawn if self.pk else False
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Comment on lines +432 to +438
def _is_withdrawal(self) -> bool:
"""
Determines if this save operation represents a new withdrawal.
Returns:
bool: True if this is a new withdrawal, False otherwise
"""
return self.pk and self.withdrawn and not self._original_withdrawn
Copy link
Contributor

Choose a reason for hiding this comment

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

Denne syntes jeg er rar. Er det ikke mulig å sammenlikne args med nåværende tilstand?

@magsyg
Copy link
Contributor

magsyg commented Dec 10, 2024

Hvordan er denne over 1000 linjer lang wut

@magsyg
Copy link
Contributor

magsyg commented Dec 10, 2024

Hmmm ser ganske greit ut, men burde nok egt vært flere issues, ser ut som dette er en stor refactor av application og modulasering med _ functionene

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.

[BUG] withdrawn application showing up for applicatn
3 participants