fix: clear submission type marker after submission#5147
fix: clear submission type marker after submission#5147shivamsingh-007 wants to merge 5 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
…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
- 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.
The submission type radio buttons (CLI / file upload) stayed visually checked after a submission, while the phase selection correctly reset. This happened because the radio buttons had no ng-model binding, so Angular couldn't reset their checked state. Added ng-model to both radio buttons and reset the model + boolean flags on submission success and error. Fixes Cloud-CV#4351 Signed-off-by: Shivam Singh <shivam.singh.koyad2006@gmail.com>
WalkthroughChallenge and phase date validation now applies in models and serializers, with expanded boundary tests. Submission type controls now bind to shared state and clear after successful or failed submissions. ChangesChallenge date validation
Submission state cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. 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 Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/challenges/test_serializers.py (1)
323-356: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd serializer-level negative tests for the new date validation.
ChallengePhaseCreateSerializeralready enforces the date boundaries, but this file still only covers the happy path. Addserializer.is_valid()failures for the invalid start/end date cases and assert the expected errors so the API-facing layer is covered.🤖 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 `@tests/unit/challenges/test_serializers.py` around lines 323 - 356, Add negative tests alongside test_internal_challenge_defaults_created_phase_to_days_14 for ChallengePhaseCreateSerializer covering start_date and end_date values outside the challenge boundaries. Assert serializer.is_valid() returns false and verify the expected field-specific validation errors, while keeping the existing valid internal-phase test unchanged.Source: Path instructions
🤖 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 367-377: Remove the start/end date ValidationError checks from
Challenge.save() and the corresponding ChallengePhase.save() implementation.
Keep persistence methods limited to calling the superclass save, and consolidate
the date validation in the existing serializer validation methods, reusing a
shared validator if appropriate to avoid duplication across ChallengeSerializer,
ChallengePhaseSerializer, and ChallengePhaseCreateSerializer.
In `@apps/challenges/serializers.py`:
- Around line 163-201: The validation rules are duplicated across
ChallengePhaseSerializer.validate(), ChallengePhaseCreateSerializer.validate(),
and ChallengePhase.save(). Extract the shared challenge-boundary and start/end
date checks into one reusable helper, then have both serializer validate methods
call it while preserving their current inputs and ValidationError behavior; use
the consolidated helper from save() as well.
---
Nitpick comments:
In `@tests/unit/challenges/test_serializers.py`:
- Around line 323-356: Add negative tests alongside
test_internal_challenge_defaults_created_phase_to_days_14 for
ChallengePhaseCreateSerializer covering start_date and end_date values outside
the challenge boundaries. Assert serializer.is_valid() returns false and verify
the expected field-specific validation errors, while keeping the existing valid
internal-phase test unchanged.
🪄 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: fa782d84-1e8c-4942-8fc3-389eaf20cb91
📒 Files selected for processing (6)
apps/challenges/models.pyapps/challenges/serializers.pyfrontend/src/js/controllers/challengeCtrl.jsfrontend/src/views/web/challenge/submission.htmltests/unit/challenges/test_models.pytests/unit/challenges/test_serializers.py
| 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." | ||
| ) | ||
| super(Challenge, self).save(*args, **kwargs) | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n## models.py relevant sections\n'
sed -n '330,410p' apps/challenges/models.py
printf '\n---\n'
sed -n '580,690p' apps/challenges/models.py
printf '\n## serializers/search\n'
rg -n "class (ChallengeSerializer|ChallengePhaseSerializer|ChallengePhaseCreateSerializer)|validate\(" apps/challenges -n
printf '\n## exception handling search\n'
rg -n "exception_handler|ValidationError|DRF.*exception|REST_FRAMEWORK" -S .Repository: Cloud-CV/EvalAI
Length of output: 13692
🏁 Script executed:
set -euo pipefail
printf '\n## REST_FRAMEWORK config\n'
sed -n '140,220p' settings/common.py
printf '\n## Challenge serializer validate() methods\n'
sed -n '1,240p' apps/challenges/serializers.py
printf '\n---\n'
sed -n '480,560p' apps/challenges/serializers.pyRepository: Cloud-CV/EvalAI
Length of output: 13913
🏁 Script executed:
set -euo pipefail
sed -n '150,210p' settings/common.py
printf '\n---\n'
sed -n '1,240p' apps/challenges/serializers.py
printf '\n---\n'
sed -n '480,560p' apps/challenges/serializers.pyRepository: Cloud-CV/EvalAI
Length of output: 13203
Move challenge date validation out of save() Challenge.save() and ChallengePhase.save() raise django.core.exceptions.ValidationError directly, so callers that bypass serializer validation can surface an unhandled 500. The same date checks are already duplicated in ChallengeSerializer, ChallengePhaseSerializer, and ChallengePhaseCreateSerializer, so this logic is split across five places and will drift.
🤖 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 367 - 377, Remove the start/end date
ValidationError checks from Challenge.save() and the corresponding
ChallengePhase.save() implementation. Keep persistence methods limited to
calling the superclass save, and consolidate the date validation in the existing
serializer validation methods, reusing a shared validator if appropriate to
avoid duplication across ChallengeSerializer, ChallengePhaseSerializer, and
ChallengePhaseCreateSerializer.
| def validate(self, attrs): | ||
| start_date = attrs.get( | ||
| "start_date", getattr(self.instance, "start_date", None) | ||
| ) | ||
| end_date = attrs.get( | ||
| "end_date", getattr(self.instance, "end_date", None) | ||
| ) | ||
| challenge = attrs.get( | ||
| "challenge", getattr(self.instance, "challenge", None) | ||
| ) | ||
| if challenge: | ||
| csd = challenge.start_date | ||
| ced = challenge.end_date | ||
| if start_date and csd and start_date < csd: | ||
| raise serializers.ValidationError( | ||
| "Phase start date must be on or after " | ||
| "challenge start date." | ||
| ) | ||
| if start_date and ced and start_date >= ced: | ||
| raise serializers.ValidationError( | ||
| "Phase start date must be before " | ||
| "challenge end date." | ||
| ) | ||
| if end_date and csd and end_date <= csd: | ||
| raise serializers.ValidationError( | ||
| "Phase end date must be after " | ||
| "challenge start date." | ||
| ) | ||
| if end_date and ced and end_date > ced: | ||
| 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." | ||
| ) | ||
| return attrs | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
ChallengePhaseSerializer.validate() and ChallengePhaseCreateSerializer.validate() are identical.
Both methods implement the exact same four challenge-boundary checks plus the self start_date >= end_date check, verbatim. This — combined with the same logic re-implemented again in ChallengePhase.save() — means the validation rules now live in 3 places in this file alone (5 counting the models). See the consolidated comment for a suggested extraction.
Also applies to: 496-534
🤖 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 163 - 201, The validation rules
are duplicated across ChallengePhaseSerializer.validate(),
ChallengePhaseCreateSerializer.validate(), and ChallengePhase.save(). Extract
the shared challenge-boundary and start/end date checks into one reusable
helper, then have both serializer validate methods call it while preserving
their current inputs and ValidationError behavior; use the consolidated helper
from save() as well.
Summary
Fix submission type marker persisting on UI after submission completes.
Related Issue
Fixes #4351
What changed
ng-model="challenge.submissionType"to both CLI and file-upload radio buttons insubmission.htmlvm.submissionType = nullinitialization inchallengeCtrl.jsisSubmissionUsingCli,isSubmissionUsingFile,isSubmissionUsingUrl, andsubmissionTypetonullon both submission success and errorWhy this approach
The radio buttons had no model binding, so AngularJS couldn't control their checked state. Once checked, they stayed checked regardless of submission lifecycle. This is the same pattern already used for phase selection (is_phase_selected).
Testing done
Checklist
Summary by CodeRabbit
Bug Fixes
Tests