Add mandatory color fields with validation to Division and Stage for visual scheduler #256
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| # Generated migration to add color fields to Division and Stage models | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ("competition", "0056_remove_ranking_tables_and_fields"), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name="division", | ||
| name="color", | ||
| field=models.CharField( | ||
| blank=True, | ||
| default="", | ||
| help_text="Color for division in the visual scheduler. Affects the left border of match cards and division headers.", | ||
| max_length=7, | ||
| verbose_name="Color", | ||
| ), | ||
| ), | ||
| migrations.AddField( | ||
| model_name="stage", | ||
| name="color", | ||
| field=models.CharField( | ||
| blank=True, | ||
| default="", | ||
| help_text="Background color for matches in the visual scheduler. Used to highlight matches of increased importance.", | ||
| max_length=7, | ||
| verbose_name="Background Color", | ||
| ), | ||
| ), | ||
| ] |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -775,6 +775,17 @@ class Division( | |
| ), | ||
| ) | ||
|
|
||
| color = models.CharField( | ||
| max_length=7, | ||
| blank=True, | ||
| default="", | ||
| verbose_name=_("Color"), | ||
| help_text=_( | ||
| "Color for division in the visual scheduler. " | ||
| "Affects the left border of match cards and division headers." | ||
| ), | ||
| ) | ||
|
|
||
| # This is an advanced feature, we would not wish to surface it under | ||
| # normal circumstances, but the theory is that we can use the report URL | ||
| # to construct the minimum data for a division. | ||
|
|
@@ -796,6 +807,25 @@ def _get_admin_namespace(self): | |
| def _get_url_args(self): | ||
| return (self.season.competition_id, self.season_id, self.pk) | ||
|
|
||
| def get_color(self): | ||
| """ | ||
| Get the division color, falling back to a default based on order. | ||
| """ | ||
| if self.color: | ||
| return self.color | ||
| # Default colors based on division order (matching CSS) | ||
| default_colors = { | ||
| 1: "#e74c3c", # Red | ||
| 2: "#3498db", # Blue | ||
| 3: "#2ecc71", # Green | ||
| 4: "#f39c12", # Orange | ||
| 5: "#9b59b6", # Purple | ||
| 6: "#1abc9c", # Teal | ||
| 7: "#e67e22", # Dark Orange | ||
| 8: "#34495e", # Dark Gray | ||
| } | ||
| return default_colors.get(self.order, "#6c757d") # Gray as ultimate fallback | ||
|
|
||
| @property | ||
| def matches(self): | ||
| return Match.objects.filter(stage__division=self).select_related( | ||
|
|
@@ -1116,6 +1146,17 @@ class Stage(AdminUrlMixin, OrderedSitemapNode): | |
| ), | ||
| ) | ||
|
|
||
| color = models.CharField( | ||
| max_length=7, | ||
| blank=True, | ||
| default="", | ||
| verbose_name=_("Background Color"), | ||
| help_text=_( | ||
| "Background color for matches in the visual scheduler. " | ||
| "Used to highlight matches of increased importance." | ||
| ), | ||
| ) | ||
|
|
||
| matches_needing_printing = ManyToManyField( | ||
| "Match", blank=True, related_name="to_be_printed" | ||
| ) | ||
|
|
@@ -1142,6 +1183,15 @@ def _get_url_args(self): | |
| def _get_url_names(self): | ||
| return super()._get_url_names() + ["build", "undo", "progress"] | ||
|
|
||
| def get_color(self): | ||
| """ | ||
| Get the stage background color, falling back to a light green default. | ||
| """ | ||
| if self.color: | ||
| return self.color | ||
| # Default to light green (matching current CSS default for scheduled items) | ||
|
||
| return "#e8f5e8" | ||
|
|
||
| def __str__(self): | ||
| return self.title | ||
|
|
||
|
|
||
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
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
|
Owner
There was a problem hiding this comment. @copilot rethink the testing based on the other comments in this review. There should also be some tests of the actual UI with the framework tests. The Division and Stage factories probably need an update too.
Contributor
Author
There was a problem hiding this comment. Done in commit c562ab9. Tests completely rethought:
All tests passing: 15 unit, 69 admin, 3 E2E. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,133 @@ | ||
| from test_plus import TestCase | ||
|
|
||
| from tournamentcontrol.competition.forms import DivisionForm, StageForm | ||
| from tournamentcontrol.competition.tests import factories | ||
|
|
||
|
|
||
| class DivisionColorTests(TestCase): | ||
| """Test cases for Division color field and get_color method.""" | ||
|
|
||
| @classmethod | ||
| def setUpTestData(cls): | ||
| cls.season = factories.SeasonFactory.create() | ||
|
|
||
| def test_division_color_field_default(self): | ||
| """Test that division color field defaults to empty string.""" | ||
| division = factories.DivisionFactory.create(season=self.season) | ||
| self.assertEqual(division.color, "") | ||
|
|
||
| def test_division_color_field_accepts_hex_value(self): | ||
| """Test that division color field accepts a hex color value.""" | ||
| division = factories.DivisionFactory.create( | ||
| season=self.season, color="#ff5733" | ||
| ) | ||
| self.assertEqual(division.color, "#ff5733") | ||
|
|
||
| def test_get_color_returns_custom_color(self): | ||
| """Test that get_color returns custom color when set.""" | ||
| division = factories.DivisionFactory.create( | ||
| season=self.season, color="#123456" | ||
| ) | ||
| self.assertEqual(division.get_color(), "#123456") | ||
|
|
||
| def test_get_color_returns_default_for_order_1(self): | ||
| """Test that get_color returns default red for division order 1.""" | ||
| division = factories.DivisionFactory.create(season=self.season, order=1) | ||
| self.assertEqual(division.get_color(), "#e74c3c") | ||
|
|
||
| def test_get_color_returns_default_for_order_2(self): | ||
| """Test that get_color returns default blue for division order 2.""" | ||
| division = factories.DivisionFactory.create(season=self.season, order=2) | ||
| self.assertEqual(division.get_color(), "#3498db") | ||
|
|
||
| def test_get_color_returns_default_for_order_3(self): | ||
| """Test that get_color returns default green for division order 3.""" | ||
| division = factories.DivisionFactory.create(season=self.season, order=3) | ||
| self.assertEqual(division.get_color(), "#2ecc71") | ||
|
|
||
| def test_get_color_returns_fallback_for_unknown_order(self): | ||
| """Test that get_color returns gray fallback for orders > 8.""" | ||
| division = factories.DivisionFactory.create(season=self.season, order=10) | ||
| self.assertEqual(division.get_color(), "#6c757d") | ||
|
|
||
| def test_division_form_includes_color_field(self): | ||
| """Test that DivisionForm includes the color field.""" | ||
| division = factories.DivisionFactory.create(season=self.season) | ||
| form = DivisionForm(instance=division) | ||
| self.assertIn("color", form.fields) | ||
|
|
||
| def test_division_form_color_widget_type(self): | ||
| """Test that DivisionForm uses color input widget.""" | ||
| division = factories.DivisionFactory.create(season=self.season) | ||
| form = DivisionForm(instance=division) | ||
| widget = form.fields["color"].widget | ||
| self.assertEqual(widget.input_type, "color") | ||
|
|
||
| def test_division_form_saves_color(self): | ||
| """Test that DivisionForm correctly saves color field.""" | ||
| division = factories.DivisionFactory.create(season=self.season, color="") | ||
| # Verify color is initially empty | ||
| self.assertEqual(division.color, "") | ||
|
|
||
| # Update the color | ||
| division.color = "#abcdef" | ||
| division.save() | ||
| division.refresh_from_db() | ||
|
|
||
| # Verify color was saved | ||
| self.assertEqual(division.color, "#abcdef") | ||
|
|
||
|
|
||
| class StageColorTests(TestCase): | ||
| """Test cases for Stage color field and get_color method.""" | ||
|
|
||
| @classmethod | ||
| def setUpTestData(cls): | ||
| cls.division = factories.DivisionFactory.create() | ||
|
|
||
| def test_stage_color_field_default(self): | ||
| """Test that stage color field defaults to empty string.""" | ||
| stage = factories.StageFactory.create(division=self.division) | ||
| self.assertEqual(stage.color, "") | ||
|
|
||
| def test_stage_color_field_accepts_hex_value(self): | ||
| """Test that stage color field accepts a hex color value.""" | ||
| stage = factories.StageFactory.create(division=self.division, color="#ff5733") | ||
| self.assertEqual(stage.color, "#ff5733") | ||
|
|
||
| def test_get_color_returns_custom_color(self): | ||
| """Test that get_color returns custom color when set.""" | ||
| stage = factories.StageFactory.create(division=self.division, color="#fedcba") | ||
| self.assertEqual(stage.get_color(), "#fedcba") | ||
|
|
||
| def test_get_color_returns_default_when_empty(self): | ||
| """Test that get_color returns default light green when no color is set.""" | ||
| stage = factories.StageFactory.create(division=self.division) | ||
| self.assertEqual(stage.get_color(), "#e8f5e8") | ||
|
|
||
| def test_stage_form_includes_color_field(self): | ||
| """Test that StageForm includes the color field.""" | ||
| stage = factories.StageFactory.create(division=self.division) | ||
| form = StageForm(instance=stage) | ||
| self.assertIn("color", form.fields) | ||
|
|
||
| def test_stage_form_color_widget_type(self): | ||
| """Test that StageForm uses color input widget.""" | ||
| stage = factories.StageFactory.create(division=self.division) | ||
| form = StageForm(instance=stage) | ||
| widget = form.fields["color"].widget | ||
| self.assertEqual(widget.input_type, "color") | ||
|
|
||
| def test_stage_form_saves_color(self): | ||
| """Test that StageForm correctly saves color field.""" | ||
| stage = factories.StageFactory.create(division=self.division, color="") | ||
| # Verify color is initially empty | ||
| self.assertEqual(stage.color, "") | ||
|
|
||
| # Update the color | ||
| stage.color = "#123abc" | ||
| stage.save() | ||
| stage.refresh_from_db() | ||
|
|
||
| # Verify color was saved | ||
| self.assertEqual(stage.color, "#123abc") |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot let's make the colour mandatory and use a data migration for the historical cases instead.
On creation of a division, the colour widget default value should be randomised.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit c562ab9. Division color is now mandatory with
generate_random_color()as the default. A data migration (0058) populates historical records using order-based colors. The color picker widget will show a random bright color on division creation.