[Bugfix:RainbowGrades] Handle null bucket data#12664
Conversation
automateprojectmangement
Bot
moved this from Seeking Reviewer
to In Review
in Submitty Development
There was a problem hiding this comment.
Pull request overview
Fixes a crash on the Rainbow Grades customization page when older gui_customization.json files contain legacy bucket types and/or ids: null, by normalizing those fields to empty arrays instead of iterating/filtering null.
Changes:
- Normalize missing/invalid bucket gradeable data before
array_filter()usage inbuildCustomization(). - Normalize
json_bucket->idsbefore iterating in multiple code paths (percent overrides, bucket reordering, per-gradeable curves). - Add a regression unit test covering legacy bucket types and
ids: null.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| site/app/models/RainbowCustomization.php | Adds normalization helpers and replaces unsafe direct access/iteration that could crash on legacy JSON data. |
| site/tests/app/models/RainbowCustomizationTester.php | Adds a regression test ensuring legacy null buckets/ids don’t crash and are treated as empty. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
not-so-shubh
changed the title
|
Hey @not-so-shubh , thank you for your contribution! Please remember use our PR description template: Once this pull request adheres to all Submitty conventions, we will proceed with a full review. |
|
@hyeenaa Updated the PR description to follow the Submitty template. Thanks! |
|
I've tested it thoroughly and have reached the conclusion that the instructions presented to re-create the error are not sufficient to to show malfunction. If you choose to continue working on this issue, include screenshots in demonstrating the error and re-creation steps. Once that is fulfilled feel free to re-open the PR and tag me for re-review. |
|
@hyeenaa I’ve updated the PR with clearer reproduction steps and added before/after screenshots demonstrating the issue and fix. Please feel free to re-review! |
|
@not-so-shubh Thank you for providing more context, I was able to re-create the issue on the main branch and so far, your PR fixes the identified problems. I'm going to follow through with my review. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12664 +/- ##
============================================
+ Coverage 21.62% 21.66% +0.04%
- Complexity 9824 9839 +15
============================================
Files 268 268
Lines 36816 36847 +31
Branches 490 490
============================================
+ Hits 7960 7983 +23
- Misses 28370 28378 +8
Partials 486 486
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
automateprojectmangement
Bot
moved this from Seeking Reviewer
to In Review
in Submitty Development
hyeenaa
left a comment
hyeenaa
left a comment
There was a problem hiding this comment.
In my testing, this PR solves the initial problem that was causing the breakage, which was having a capital "Tests" in the type category in the rainbow grades buckets. The page no longer crashes when case isn't matched. Instead, it changes the bucket type to be force-capitalized when rendering the items. This is a good approach, my only worry is that it will not tell the user when a null bucket is present. I would suggest adding an error message when a null value is in a bucket is detected before reverting, just to nudge the user.
github-project-automation
Bot
moved this from In Review
to Work in Progress
in Submitty Development
|
Pushed an update that keeps the current normalization behavior, so malformed legacy bucket data still loads safely instead of crashing. I also added a non-blocking warning on the customization page when malformed legacy bucket data is detected (for example, a null |
github-actions
Bot
added
the
Abandoned PR - Needs New Owner
automateprojectmangement
Bot
moved this from Work in Progress
to In Review
in Submitty Development
|
can you resolve your merge conflicts with the main branch? after then your change should be approved fully. |
|
@hyeenaa I’ve resolved the merge conflicts with main and pushed the updated branch. |
…hubh/Submitty into not-so-shubh-fix-rainbow-null-bucket
…hubh/Submitty into not-so-shubh-fix-rainbow-null-bucket
37afe49 to
cbcabc2
Compare
dandrecollins07-ctrl
left a comment
dandrecollins07-ctrl
left a comment
There was a problem hiding this comment.
Tested using a gui_customization.json containing a legacy bucket with "ids": null. The page now loads successfully and displays the warning message instead of generating a PHP warning. Also resolved the merge conflict in RainbowCustomization.php and verified the fix after updating the branch with the latest main.
The remaining PHPUnit failure appears unrelated to this change.
Approving.
github-actions
Bot
removed
the
Abandoned PR - Needs New Owner
mattLif
left a comment
mattLif
left a comment
There was a problem hiding this comment.
Code and changes look good. Was able to reproduce error on main, and PR branch fixes error. Only thing I notices is that I got 3 warning messages instead of 1, but that seems to only be an issue on my machine. I approve these changes.
Why is this Change Important & Necessary?
Fixes #12544.
The Rainbow Grades customization page crashes for some older courses when legacy
gui_customization.jsondata contains:"ids": nullinstead of an arrayThe backend assumes these values are arrays and directly calls
array_filter()or iterates usingforeach, resulting in a PHP 8TypeError.What is the New Behavior?
json_bucket->idsis normalized before iterationMalformed or legacy data is now treated as an empty bucket instead of causing a crash. Existing valid behavior is unchanged.
Before (bug)
Legacy customization data with
"ids": nullcauses a crash on the customization page:After (fixed)
The same input no longer crashes and the page loads successfully:
What steps should a reviewer take to reproduce or test the bug or new feature?
rainbow_grades/gui_customization.jsonin a course directory{ "gradeables": [ { "type": "legacy-bucket", "count": 1, "remove_lowest": 0, "percent": 0.25, "ids": null } ] }Visit:
/courses/<term>/<course>/reports/rainbow_grades_customizationBefore this patch:
array_filter(): Argument #1 ($array) must be of type array, null givenAfter this patch:
Automated Testing & Documentation
Added regression test:
tests/app/models/RainbowCustomizationTester.phpRan:
php vendor/bin/phpunit tests/app/models/RainbowCustomizationTester.phpphp vendor/bin/phpunit tests/app/controllers/admin/ReportControllerTester.phpNo documentation changes required
Other information