-
Notifications
You must be signed in to change notification settings - Fork 38
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.
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
Remove several registration fields that are not required anymore #494
base: master
Are you sure you want to change the base?
Conversation
…cted_ratings everywhere, since they have not been used in a long long time and are the only thing peak_classical_rating is used for
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.
I haven't finished a full review, but I thought I would send my first comments anyways. I'll want to do a deeper review and test this a bit, which don't have time for today. But first let's deal with changing the old migrations. What was the intent for those?
@@ -20,7 +20,7 @@ class Migration(migrations.Migration): | |||
migrations.AlterField( | |||
model_name='registration', | |||
name='already_in_slack_group', | |||
field=models.BooleanField(), |
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.
Altering old migrations has no effect on the live database, what was the intent with this change?
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.
the intent was simply to make the new migrations revertible. as far as i can tell, that's the only thing it does, and i didn't see a downside to it. i don't mind removing the changes to the old migrations from the pull request.
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.
I am pretty sure there are ways to make the new migrations invertible that don't include modifying old migrations. The problem with modifying old migrations is that our live database will not be in sync with this change, which means that new migrations may assume they can do things that they can't and it will succeed in development, but not on prod. Let's remove the changes to old migrations and if you want the new migrations to be revertible, we can work through that.
when ready, this will close #488
i removed "expected rating" at the same time. this hasn't been used in ages, and cannot even be used with the current team gen algorithm. frankly, i am not certain it ever made sense to use it.