Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Decouple Difficulty and Version Fields #321

Closed
wants to merge 6 commits into from
Closed

Decouple Difficulty and Version Fields #321

wants to merge 6 commits into from

Conversation

Incompleteusern
Copy link
Contributor

Fixes #317.

Implements version codes as

DIFFICULTY_CHOICES = (
    ("B", "Easy/Bet"),
    ("D", "Medium/Dalet"),
    ("Z", "Hard/Zayin"),
)

VERSION_CHOICES = (
    ("V", "V (Secret)"),
    ("W", "W"),
    ("X", "X"),
    ("Y", "Y"),
)

difficulty = models.CharField(
    max_length=2,
    choices=DIFFICULTY_CHOICES,
    help_text="The difficulty code for the handout, like B",
)
version = models.CharField(
    max_length=2,
    choices=VERSION_CHOICES,
    help_text="The version code for the handout, like W",
)

Benefits of this include being less prone to typos and less redundant information stored as the group subject.

Other things this implies

  1. Migrate the database. Probably make a backup before that? See the custom code in the migration.
  2. There's likely some latent bugs here. Will fix as notified. Free 500 diamond I suppose.
  3. venueQ needs to be updated for this
  4. fixtures should be re-dumped to conform to this format. Maybe deprecate all.json, though not sure anymore.

Incompleteusern
Incompleteusern commented Oct 26, 2023
View reviewed changes
core/templates/core/unit_list.html
@@ -19,7 +19,8 @@
<li>The second letter repeats the olympiad category.</li>
<li>
The third letter is a version identifier (either W, X, or Y).
Many topics have multiple versions so they can be repeated in different years.
Many topics have multiple versions so they can be repeated in different years,
but you can do the same topic multiple times in one year as well.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this comment since I misinterpreted it the first time.

Incompleteusern
Incompleteusern commented Oct 26, 2023
View reviewed changes
rpg/tests.py
@@ -115,7 +115,7 @@ def test_level_up(self):
alice = self.get_alice()
self.login(alice)
bonus = BonusLevelFactory.create(group__name="Level 40 Quest", level=40)
bonus_unit = UnitFactory.create(group=bonus.group, code="DKU")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursed U version code?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WHAT IS THIS

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's "Level 40 Quest"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It refers to a secret unit level.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i prefer meow :3

vEnhance
vEnhance reviewed Oct 27, 2023
View reviewed changes
core/migrations/0058_alter_unit_unique_together_unit_difficulty_and_more.py Outdated


def set_difficulty_and_version(apps, schema_editor):
# We can't import the Person model directly as it may be a newer
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to be a copy paste error?

core/migrations/0058_alter_unit_unique_together_unit_difficulty_and_more.py
),
preserve_default=False,
),
migrations.RunPython(set_difficulty_and_version),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't RunPython need a reverse operation argument or something like that? at least i feel like i've had to specified noop a bunch before in the past

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that its optional? Stackoverflow had that at some point and it seems to migrate properly.

@vEnhance
Copy link
Owner

although this is probably the way the code should have been written, i'm a bit hesitant to merge this at a time of year where i expect to be pretty busy. this is a pretty big migration, a lot of code changes (i feel like i'd need to test this a lot before i feel like it's safe to merge) and it would require venueQ to be updated as well

so i'll leave this open for now, but it'll be a while before i get to actually processing it

@vEnhance
Copy link
Owner

okay i've thought about this more and i've decided that fixing this is probably not worth it, due to all the changes that would be needed in many places (in particular, i'm sure we won't catch everything, even with the unit tests). "there's likely some latent bugs here" is a worse outcome to me than a small amount of extra redundancy in the database.

@vEnhance vEnhance closed this May 17, 2024
@Incompleteusern
Copy link
Contributor Author

Yeah that makes sense I suppose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vEnhance vEnhance vEnhance left review comments

@jatloe jatloe jatloe left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Split Unit code into properties
3 participants
@Incompleteusern @vEnhance @jatloe