MFB-717: Phase 1 — Add ScreenCurrentBenefit model and show_in_has_benefits_step#1433
MFB-717: Phase 1 — Add ScreenCurrentBenefit model and show_in_has_benefits_step#1433
Conversation
…efits_step field - Add ScreenCurrentBenefit join table to screener/models.py - Add show_in_has_benefits_step BooleanField to Program model - Write migrations for both (0134, 0148) - Expose show_in_has_benefits_step in ProgramAdmin (list_filter + fields) - No changes to serializer, views, or has_benefit() — purely additive Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Program model + admin programs/models.py, programs/migrations/0134_program_show_in_has_benefits_step.py, programs/admin.py |
Added Program.show_in_has_benefits_step: BooleanField(default=False, help_text=...); migration to add the field; updated ProgramAdmin.list_filter and fields to include the new field. |
Screener CurrentBenefit model screener/models.py, screener/migrations/0148_add_currentbenefit.py |
Added CurrentBenefit model with screen (FK to screener.Screen, related_name="current_benefits") and program (FK to programs.Program); DB table set to screener_current_benefits and (screen, program) uniqueness constraint; migration added with cross-app dependency. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title accurately describes the main changes: adding a ScreenCurrentBenefit model and show_in_has_benefits_step field, matching the changeset scope. |
| Description check | ✅ Passed | The PR description includes all required sections: context, changes, testing, deployment, and reviewer notes. It thoroughly documents the additive Phase 1 work with clear limitations and future considerations. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
caton/mfb-716-migrate-has_-boolean-columns-on-screen-to-a
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 @coderabbitai help to get the list of available commands and usage tips.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
catonph
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
screener/migrations/0148_add_currentbenefit.py (1)
37-40: ConsiderUniqueConstraintfor future migrations.While
unique_togetheris still functional in Django 4.2, it's deprecated in favor of usingconstraintswithUniqueConstraint. Since this is Phase 1 and the model definition also usesunique_together, this is fine for now. Consider modernizing both the model and future migrations when convenient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@screener/migrations/0148_add_currentbenefit.py` around lines 37 - 40, The migration uses migrations.AlterUniqueTogether on the "currentbenefit" model with unique_together={("screen", "program")}; replace this with a UniqueConstraint-based approach for future migrations by adding a migrations.AddConstraint (or migrations.AlterModelOptions with constraints) that creates models.UniqueConstraint(fields=["screen", "program"], name="unique_currentbenefit_screen_program") and remove the AlterUniqueTogether entry; update the corresponding model's Meta to use constraints = [models.UniqueConstraint(...)] so code and migrations stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@screener/migrations/0148_add_currentbenefit.py`:
- Around line 37-40: The migration uses migrations.AlterUniqueTogether on the
"currentbenefit" model with unique_together={("screen", "program")}; replace
this with a UniqueConstraint-based approach for future migrations by adding a
migrations.AddConstraint (or migrations.AlterModelOptions with constraints) that
creates models.UniqueConstraint(fields=["screen", "program"],
name="unique_currentbenefit_screen_program") and remove the AlterUniqueTogether
entry; update the corresponding model's Meta to use constraints =
[models.UniqueConstraint(...)] so code and migrations stay in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: MyFriendBen/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f7cdc244-90ba-4e64-ae08-df03badc7c1b
📒 Files selected for processing (4)
programs/migrations/0134_program_show_in_has_benefits_step.pyprograms/models.pyscreener/migrations/0148_add_currentbenefit.pyscreener/models.py
🚧 Files skipped from review as they are similar to previous changes (2)
- programs/models.py
- screener/models.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@screener/migrations/0148_add_currentbenefit.py`:
- Around line 33-35: The migration sets options["db_table"] to
"screener_current_benefits" which conflicts with the rollout contract expecting
"screener_screen_current_benefits"; update the db_table value in the migration
(file/migration name 0148_add_currentbenefit.py) to
"screener_screen_current_benefits" and ensure any references in this migration
(e.g., the model's Meta options or CreateModel call) use that exact table name
so Phase 2/manual verification and runbooks align with the deployment notes.
🪄 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: Repository: MyFriendBen/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 44b58e76-3044-43e9-9f45-6f0bb9204213
📒 Files selected for processing (2)
programs/migrations/0134_program_show_in_has_benefits_step.pyscreener/migrations/0148_add_currentbenefit.py
| options={ | ||
| "db_table": "screener_current_benefits", | ||
| }, |
There was a problem hiding this comment.
db_table name appears inconsistent with rollout contract.
The migration creates screener_current_benefits, but the PR objective/deployment notes expect screener_screen_current_benefits. This can break Phase 2+/manual verification if other code or runbooks key off the expected table name.
Proposed fix
options={
- "db_table": "screener_current_benefits",
+ "db_table": "screener_screen_current_benefits",
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| options={ | |
| "db_table": "screener_current_benefits", | |
| }, | |
| options={ | |
| "db_table": "screener_screen_current_benefits", | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@screener/migrations/0148_add_currentbenefit.py` around lines 33 - 35, The
migration sets options["db_table"] to "screener_current_benefits" which
conflicts with the rollout contract expecting
"screener_screen_current_benefits"; update the db_table value in the migration
(file/migration name 0148_add_currentbenefit.py) to
"screener_screen_current_benefits" and ensure any references in this migration
(e.g., the model's Meta options or CreateModel call) use that exact table name
so Phase 2/manual verification and runbooks align with the deployment notes.
1 participant
Context & Motivation
Phase 1 of MFB-716 — purely additive, no behavior change.
The
Screenmodel currently has ~87has_*boolean columns to track which benefits a user already has. This phase lays the groundwork for migrating to aScreenCurrentBenefitjoin table by adding the new model and ashow_in_has_benefits_stepflag onProgram(replacing the staticcategory_benefitswhite label config). Nothing reads from them yet.Changes Made
ScreenCurrentBenefitjoin table model toscreener/models.py(screenFK +programFK, unique together,db_table = 'screener_screen_current_benefits')show_in_has_benefits_step = BooleanField(default=False)toProgrammodel (distinct fromshow_on_current_benefits)programs/0134,screener/0148show_in_has_benefits_stepinProgramAdmin(list_filter+fields) alongsideshow_on_current_benefitsTesting
0134_program_show_in_has_benefits_step,0148_add_screencurrentbenefitscreener_screen_current_benefitstable is created andshow_in_has_benefits_steptoggle appears on the Program admin pageDeployment
Notes for Reviewers
ScreenCurrentBenefitis not yet written to or read from — that comes in Phase 2 (dual-write) and Phase 3 (flip reads)show_in_has_benefits_stepwill replace thecategory_benefitswhite label config in Phase 4; programs will need to be flagged via the admin before that cutoverSummary by CodeRabbit
New Features
Admin Updates