Remove Completed Field from Driver Assignments

[kingMonkeh]

…update tests

JIRA ticket link

Remove Completed Field from Driver Assignments

Implementation description

• Drop completed column
• Run alembic migration
• Update tests

Steps to test

1. Run migration, check that column is actually gone

What should reviewers focus on?

• The migration and model

Checklist

• My PR name is descriptive and in imperative tense
• My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
• I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

[claude]

PR Review: Remove Completed Field from Driver Assignments

Thank you for the PR! I've reviewed the changes and found both strengths and critical issues that need to be addressed before merging.

---

🔴 Critical Issues

1. Test Failure in test_models.py (Line 463)

The test still references the removed completed field:

assert assignment_default.completed is False  # Default value

This line was not removed in your PR and will cause the test to fail immediately. This appears to be an oversight.

Required Fix: Remove line 463 from backend/python/tests/test_models.py

2. Breaking Changes in Production Code (Multiple Locations)

The completed field is still actively used in production code that was not updated in this PR:

File: backend/python/app/services/jobs/driver_history_jobs.py

• Line 54: Queries for incomplete assignments using DriverAssignment.completed.is_(False)
• Line 86: Sets assignment.completed = True
• Line 27: Documentation mentions "Marks assignments as completed"

File: backend/python/app/services/jobs/email_reminder_jobs.py

• Line 54: Filters by DriverAssignment.completed.is_(False)

File: backend/python/app/seed_database.py

• Line 640: Creates assignments with completed=drive_date_obj < today

Impact: These files will break immediately after the migration runs, causing runtime errors in scheduled jobs and database seeding.

Required Fix: Update all these files to remove references to the completed field. You'll need to decide on alternative logic:

• For driver history jobs: Either remove the completion tracking entirely or use a different mechanism
• For email reminders: Remove the filter or find another way to identify which assignments need reminders
• For seed database: Remove the completed parameter

---

⚠️ High Priority Issues

3. Migration Downgrade Could Cause Data Loss

The downgrade function adds the completed column back with nullable=False:

op.add_column('driver_assignments', sa.Column('completed', sa.BOOLEAN(), autoincrement=False, nullable=False))

Problem: If you downgrade after having driver assignments in the database, this will fail because you can't add a non-nullable column to a table with existing rows without providing a default value.

Recommendation: Update the downgrade function to include a server default:

op.add_column('driver_assignments', sa.Column('completed', sa.BOOLEAN(), server_default='false', autoincrement=False, nullable=False))

4. Questionable Test Change in test_driver_assignment_service.py

The test now updates time to None and asserts it becomes None:

update_data = DriverAssignmentUpdate(time=None)
# ...
assert updated_assignment.time is None

Problem:

• The time field is non-optional in DriverAssignmentBase (time: datetime = Field())
• Setting it to None likely violates the database schema
• This doesn't test meaningful update functionality

Recommendation: Use a different field for the update test, or update to a valid datetime value:

update_data = DriverAssignmentUpdate(time=datetime(2024, 1, 15, 10, 0))
assert updated_assignment.time == datetime(2024, 1, 15, 10, 0)

---

✅ What Was Done Well

1. Clean model changes - The removal from the model classes is correct and thorough
2. Migration structure - The migration file itself is properly structured
3. Consistent commit - All model changes are in a single atomic commit

---

📋 Required Actions Before Merge

• Remove line 463 from test_models.py (the assert assignment_default.completed is False line)
• Update driver_history_jobs.py to remove all completed field references
• Update email_reminder_jobs.py to remove the completed filter
• Update seed_database.py to remove the completed parameter
• Fix the test in test_driver_assignment_service.py to test a valid update
• Add server_default='false' to the migration downgrade function
• Run all tests to verify everything passes
• Consider updating the JIRA ticket with the additional scope discovered

---

💡 Suggestions

• Use git grep "completed" in the backend directory to find all references before removing fields in the future
• Consider running the full test suite locally before creating the PR to catch issues like the test_models.py failure

Let me know if you need any clarification on these issues or help with the fixes!