-
Notifications
You must be signed in to change notification settings - Fork 20
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
Implement new penalty rules #3349
base: master
Are you sure you want to change the base?
Conversation
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 don't really have enough insight into how this works to be able to give a thorough review without spending too much time on it right now, but I've skimmed through the majority on it and left a few comments(:
lego/apps/users/migrations/0040_rename_activation_date_penalty_activation_time.py
Outdated
Show resolved
Hide resolved
1df65ce
to
7a487fa
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #3349 +/- ##
==========================================
- Coverage 88.30% 88.26% -0.04%
==========================================
Files 661 662 +1
Lines 20968 20786 -182
==========================================
- Hits 18515 18347 -168
+ Misses 2453 2439 -14
☔ View full report in Codecov by Sentry. |
a036bfc
to
d531414
Compare
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.
Good job!
I tried to migrate on my local backend. However, I got this error message. Do you have any idea what caused this issue?
Traceback (most recent call last):
File "/home/falk/projs/webkom/lego/manage.py", line 10, in <module>
execute_from_command_line(sys.argv)
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/core/management/__init__.py", line 446, in execute_from_command_line
utility.execute()
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/core/management/__init__.py", line 440, in execute
self.fetch_command(subcommand).run_from_argv(self.argv)
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/core/management/base.py", line 414, in run_from_argv
self.execute(*args, **cmd_options)
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/core/management/base.py", line 460, in execute
output = self.handle(*args, **options)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/core/management/base.py", line 98, in wrapped
res = handle_func(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/core/management/commands/migrate.py", line 290, in handle
post_migrate_state = executor.migrate(
^^^^^^^^^^^^^^^^^
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/migrations/executor.py", line 131, in migrate
state = self._migrate_all_forwards(
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/migrations/executor.py", line 163, in _migrate_all_forwards
state = self.apply_migration(
^^^^^^^^^^^^^^^^^^^^^
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/migrations/executor.py", line 248, in apply_migration
state = migration.apply(state, schema_editor)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/migrations/migration.py", line 131, in apply
operation.database_forwards(
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/migrations/operations/fields.py", line 108, in database_forwards
schema_editor.add_field(
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/backends/base/schema.py", line 641, in add_field
self.execute(sql, params)
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/backends/base/schema.py", line 192, in execute
cursor.execute(sql, params)
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/backends/utils.py", line 102, in execute
return super().execute(sql, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/backends/utils.py", line 67, in execute
return self._execute_with_wrappers(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/backends/utils.py", line 80, in _execute_with_wrappers
return executor(sql, params, many, context)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/backends/utils.py", line 84, in _execute
with self.db.wrap_database_errors:
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/utils.py", line 91, in __exit__
raise dj_exc_value.with_traceback(traceback) from exc_value
File "/home/falk/projs/webkom/lego/venv/lib/python3.11/site-packages/django/db/backends/utils.py", line 89, in _execute
return self.cursor.execute(sql, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
django.db.utils.IntegrityError: column "penalty_group_id" contains null values
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.
Remove this .DS_STORE file
@@ -74,3 +74,6 @@ google-credentials.json | |||
|
|||
# vscode | |||
.vscode/ | |||
|
|||
# mac | |||
.DS_Store |
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.
.DS_Store | |
**/.DS_Store |
Your current code only removes the .DS_Store in the root directory
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.
Remove this.
|
||
pools = Pool.objects.filter(permission_groups__in=self.all_groups).distinct() | ||
|
||
# TODO: i believe wrong. |
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.
🤔
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.
right > wrong
# Generated by Django 4.0.10 on 2023-05-01 16:33 | ||
|
||
from django.db import models | ||
|
||
|
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.
What's this?
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.
Just a bit of nitpick, looks good otherwise!
PenaltyGroup.objects.create( | ||
user=user, | ||
reason="Dette er en sammensetting av forrige prikkene", | ||
source_event=dummy_event, |
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 think that the source event here should be one of the events from the earlier penalties, and that the reason should include a list of the reasons that the user got the original penalties. Something like: "Dette er en sammensettning av tidligere prikker pga. migrering til nytt prikksystem. Prikkene som inngår i denne prikken er: 'kom for sent' (Julebord), 'slo til bedrep' (bedpress ned Warehouse), ..."
.filter( | ||
penalty_group__user=self, | ||
) | ||
.count() |
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.
This does not really make that much sense anymore, as a user never has any more than one penalty, but I suppose it doesn't hurt
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.
LGTM
|
||
pools = Pool.objects.filter(permission_groups__in=self.all_groups).distinct() | ||
|
||
# TODO: i believe wrong. |
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.
right > wrong
ABA-92 Implement new rules for prikk
After a meeting between all the committees responsible for events a conclusion to alter the rules for prikker was reached. This has to be implemented. The new rules are:
i.e. the number of prikks you have does not really matter anymore, it only matters how long you have them. pr: #3349 |
Implement new rules and consequences for penalties. The changes are as following:
The biggest change is the addition of PenaltyGroup for the purpose of separating penalty consequence calculation logic and the the information used in front-end.
I have deleted some tests that believe are not needed for our new implementation and i have also made changes to events app that i deemed necessary. I would appreciate reviewing specially these changes as it might be possible that i made some mistakes.
Resolves ABA-92