Add date validation constraints for challenge and challenge phase models#5140
Open
shivamsingh-007 wants to merge 4 commits into
Open
Add date validation constraints for challenge and challenge phase models#5140shivamsingh-007 wants to merge 4 commits into
shivamsingh-007 wants to merge 4 commits into
Conversation
- Add ValidationError import for model-level validation - Override Challenge.save() to enforce start_date < end_date - Override ChallengePhase.save() with 5 rules: - Phase start >= challenge start - Phase start < challenge end - Phase end > challenge start - Phase end <= challenge end - Phase start < phase end - Add 9 tests for Challenge and ChallengePhase date validation - Fix serializer test setUp to use stable challenge dates
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds date validation to Challenge and ChallengePhase save paths, adds serializer validation for the same date constraints, and updates model and serializer tests to match the new rules. ChangesDate validation for challenges and phases
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Serializer
participant Challenge
participant ChallengePhase
Client->>Serializer: submit challenge or phase data
Serializer->>Serializer: validate date ordering and bounds
Serializer->>Challenge: save() when challenge data is valid
Serializer->>ChallengePhase: save() when phase data is valid
Challenge-->>Client: ValidationError or persisted model
ChallengePhase-->>Client: ValidationError or persisted model
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds model-level date range validation for Challenge and ChallengePhase to prevent persisting inconsistent lifecycle windows (e.g., start >= end, or phase dates outside challenge dates), and updates unit tests to cover the new constraints and avoid timestamp drift.
Changes:
- Add
save()-time validations forChallengeandChallengePhasedate ranges. - Add/adjust unit tests to cover the new validation rules and stabilize time-dependent setup.
- Fix serializer tests to reuse challenge dates to avoid microsecond drift triggering the new validations.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| apps/challenges/models.py | Adds save()-time validations for challenge/phase date windows. |
| tests/unit/challenges/test_models.py | Adds unit tests for new validation rules and pins setup time to a single timestamp. |
| tests/unit/challenges/test_serializers.py | Updates serializer tests to reuse challenge dates and adds assertions to avoid drift-related failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+358
to
+362
| @@ -358,6 +359,17 @@ def is_active(self): | |||
| return True | |||
| return False | |||
|
|
|||
| def save(self, *args, **kwargs): | |||
Comment on lines
601
to
605
| @@ -591,6 +603,48 @@ def is_active(self): | |||
| return False | |||
|
|
|||
| def save(self, *args, **kwargs): | |||
Comment on lines
+366
to
+370
| and self.start_date >= self.end_date | ||
| ): | ||
| raise ValidationError( | ||
| "Challenge start date must be before end date." | ||
| ) |
Comment on lines
+612
to
+615
| ): | ||
| raise ValidationError( | ||
| "Phase start date must be on or after challenge start date." | ||
| ) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/challenges/models.py (2)
606-647: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated
start < endcheck into a shared helper.Both
Challenge.save()andChallengePhase.save()implement the same "start must be strictly before end" pattern independently. A small shared validator (e.g., a module-level function or mixin) would reduce duplication and centralize future rule changes.♻️ Example
+def _validate_date_order(start, end, message): + if start and end and start >= end: + raise ValidationError(message) + class Challenge(TimeStampedModel): ... def save(self, *args, **kwargs): - if ( - self.start_date - and self.end_date - and self.start_date >= self.end_date - ): - raise ValidationError( - "Challenge start date must be before end date." - ) + _validate_date_order( + self.start_date, + self.end_date, + "Challenge start date must be before end date.", + ) super(Challenge, self).save(*args, **kwargs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/challenges/models.py` around lines 606 - 647, Both Challenge.save() and ChallengePhase.save() duplicate the same strict start-before-end validation, so extract that rule into a shared helper or mixin and reuse it from both save paths. Update the validation logic around Challenge.save and ChallengePhase.save to call the shared validator, keeping the existing messages/behavior but centralizing the comparison so future rule changes happen in one place.
362-372: 🗄️ Data Integrity & Integration | 🔵 TrivialRetroactive validation may block edits to pre-existing challenges with invalid dates.
This validation runs on every
save()call, not just when dates change. Any existingChallengerow created before this PR withstart_date >= end_date(or similarly anyChallengePhaseviolating the new cross-field rules) will now fail to save even for unrelated field updates (e.g., togglingis_frozen), until its dates are fixed. Consider auditing existing rows for violations and/or only validating when the relevant date fields actually change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/challenges/models.py` around lines 362 - 372, The save-time cross-field validation in Challenge.save is retroactive and can block unrelated edits on rows that already have invalid dates. Update the logic so the date check only runs when start_date or end_date actually changes, or otherwise handle existing invalid data before enforcing the rule; apply the same approach to the new ChallengePhase validation paths so pre-existing records can still be edited safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/challenges/models.py`:
- Around line 362-372: Move the challenge date-range validation out of
Challenge.save so invalid dates are rejected before persistence instead of
raising ValidationError from the model path. Add the check at the
serializer/form boundary that handles Challenge creation/update, or if save()
must keep the guard, catch the ValidationError in Challenge.save and convert it
to the appropriate application-level error handling so
ChallengeSerializer.save() and admin saves do not surface a 500.
In `@tests/unit/challenges/test_models.py`:
- Around line 319-325: The test for challenge phase date ordering is currently
hitting the broader challenge-bound validation before the intended start/end
ordering check. Update test_save_raises_error_when_phase_start_after_phase_end
in test_models.py so the phase dates remain within the Challenge bounds from
BaseTestCase.setUp, while still making self.challenge_phase.start_date later
than self.challenge_phase.end_date. This ensures the ValidationError comes from
the phase ordering logic in ChallengePhase.save rather than the separate
challenge boundary check.
In `@tests/unit/challenges/test_serializers.py`:
- Around line 347-352: The test currently only checks that ChallengePhase dates
stay within the challenge bounds, but ChallengePhaseCreateSerializer should
preserve the exact dates from the challenge. Update the assertions in the
serializer test around ChallengePhaseCreateSerializer/ChallengePhase.save() to
compare challenge_phase.start_date and challenge_phase.end_date with
self.challenge.start_date and self.challenge.end_date using exact equality so
accidental mutation is detected.
---
Nitpick comments:
In `@apps/challenges/models.py`:
- Around line 606-647: Both Challenge.save() and ChallengePhase.save() duplicate
the same strict start-before-end validation, so extract that rule into a shared
helper or mixin and reuse it from both save paths. Update the validation logic
around Challenge.save and ChallengePhase.save to call the shared validator,
keeping the existing messages/behavior but centralizing the comparison so future
rule changes happen in one place.
- Around line 362-372: The save-time cross-field validation in Challenge.save is
retroactive and can block unrelated edits on rows that already have invalid
dates. Update the logic so the date check only runs when start_date or end_date
actually changes, or otherwise handle existing invalid data before enforcing the
rule; apply the same approach to the new ChallengePhase validation paths so
pre-existing records can still be edited safely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a96d55e-4593-448f-b408-4128f495df05
📒 Files selected for processing (3)
apps/challenges/models.pytests/unit/challenges/test_models.pytests/unit/challenges/test_serializers.py
…e ordering test - Guard Challenge.is_active and ChallengePhase.is_active against null dates (avoids TypeError when start_date or end_date is None) - Add validate() to ChallengeSerializer, ChallengePhaseSerializer, and ChallengePhaseCreateSerializer to catch invalid date ranges at the serializer boundary (avoids 500 from model ValidationError) - Fix test_save_raises_error_when_phase_start_after_phase_end to keep test dates within challenge bounds so it exercises the internal ordering check instead of short-circuiting on the boundary check
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/challenges/serializers.py (1)
159-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated validation logic between
ChallengePhaseSerializerandChallengePhaseCreateSerializer.The
validate()bodies at Lines 159-185 and 480-506 are identical. Extract into a shared module-level function or mixin to avoid the two copies drifting apart on future changes.♻️ Suggested refactor
def validate_phase_dates(attrs, instance=None): start_date = attrs.get("start_date", getattr(instance, "start_date", None)) end_date = attrs.get("end_date", getattr(instance, "end_date", None)) challenge = attrs.get("challenge", getattr(instance, "challenge", None)) if challenge: if start_date and challenge.start_date and start_date < challenge.start_date: raise serializers.ValidationError( "Phase start date must be on or after challenge start date." ) if start_date and challenge.end_date and start_date >= challenge.end_date: raise serializers.ValidationError( "Phase start date must be before challenge end date." ) if end_date and challenge.start_date and end_date <= challenge.start_date: raise serializers.ValidationError( "Phase end date must be after challenge start date." ) if end_date and challenge.end_date and end_date > challenge.end_date: raise serializers.ValidationError( "Phase end date must be on or before challenge end date." ) if start_date and end_date and start_date >= end_date: raise serializers.ValidationError( "Phase start date must be before end date." )Then in both serializers:
def validate(self, attrs): - ... + validate_phase_dates(attrs, self.instance) return attrsAlso applies to: 480-506
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/challenges/serializers.py` around lines 159 - 185, The validation logic is duplicated in ChallengePhaseSerializer.validate and ChallengePhaseCreateSerializer.validate, so extract the shared date checks into a single helper such as validate_phase_dates or a reusable mixin method. Update both serializers to delegate to that shared logic, keeping the existing challenge/start_date/end_date validations in one place so future changes only need to be made once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/challenges/serializers.py`:
- Line 164: Several conditional statements in the serializers are exceeding the
project’s 79-character limit and will fail black formatting checks. Reformat the
long `if` expressions in `apps/challenges/serializers.py` so they wrap cleanly
within 79 characters, especially the date-comparison guards in the relevant
serializer methods, without changing behavior. Use the affected conditional
blocks in the serializer logic as the places to update, and ensure the file
passes `black --check --line-length=79`.
- Around line 159-185: The phase serializer validation only checks values
present in attrs, so PATCH requests that omit start_date, end_date, or challenge
can bypass validation and later fail in ChallengePhase.save(). Update validate
in both phase serializers to merge missing fields from self.instance before
applying the date range checks, and keep the existing constraints enforced using
the resolved start_date, end_date, and challenge values.
- Around line 39-47: `ChallengeSerializer.validate` only checks `attrs`, so
PATCH requests can miss an invalid start/end date pair when one value already
exists on `self.instance`. Update the validation to merge incoming values with
the current instance values before comparing dates, and keep raising a DRF
`serializers.ValidationError` from `ChallengeSerializer.validate` instead of
letting `Challenge.save()` handle it.
---
Nitpick comments:
In `@apps/challenges/serializers.py`:
- Around line 159-185: The validation logic is duplicated in
ChallengePhaseSerializer.validate and ChallengePhaseCreateSerializer.validate,
so extract the shared date checks into a single helper such as
validate_phase_dates or a reusable mixin method. Update both serializers to
delegate to that shared logic, keeping the existing
challenge/start_date/end_date validations in one place so future changes only
need to be made once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: acb0db2a-1c00-4e5b-827c-1f008cd10f0d
📒 Files selected for processing (3)
apps/challenges/models.pyapps/challenges/serializers.pytests/unit/challenges/test_models.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/challenges/test_models.py
- apps/challenges/models.py
Comment on lines
+39
to
+46
| def validate(self, attrs): | ||
| start_date = attrs.get("start_date") | ||
| end_date = attrs.get("end_date") | ||
| if start_date and end_date and start_date >= end_date: | ||
| raise serializers.ValidationError( | ||
| "Challenge start date must be before end date." | ||
| ) | ||
| return attrs |
Comment on lines
+160
to
+162
| start_date = attrs.get("start_date") | ||
| end_date = attrs.get("end_date") | ||
| challenge = attrs.get("challenge") |
Comment on lines
+481
to
+483
| start_date = attrs.get("start_date") | ||
| end_date = attrs.get("end_date") | ||
| challenge = attrs.get("challenge") |
- Fall back to self.instance values when a field is absent from attrs during PATCH requests, so the date-range check catches invalid combinations where the other value exists on the existing instance - Shorten conditional lines with local vars (csd, ced) to comply with 79-char black limit - Split error message strings across lines for line-length compliance
…hods Shorter and cleaner than the explicit if/else guard. Adopts the exact pattern Copilot suggested.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Challenge and ChallengePhase models stored dates without any validation. This allowed
inconsistent date ranges (e.g., phase dates outside the challenge window, start after end)
to be persisted, leading to undefined behavior in submissions and challenge lifecycle
logic.
Changes
apps/challenges/models.pyValidationErrorfromdjango.core.exceptionsChallenge.save()to rejectstart_date >= end_dateChallengePhase.save()with five validation rules:tests/unit/challenges/test_models.pyBaseTestCase.setUp()to a singlenowtimestamp to avoid date drifttests/unit/challenges/test_serializers.pyChallengePhaseCreateSerializerTest.setUp()to referenceself.challenge.end_dateinstead of recomputing
timezone.now(), which drifted by microseconds and triggeredthe new validation
Testing
All 73 tests pass (40 model + 33 serializer):
Summary by CodeRabbit