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.

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Implement new penalty rules #3349

wants to merge 4 commits into from

Conversation

Arashfa0301
Copy link
Contributor

@Arashfa0301 Arashfa0301 commented Apr 30, 2023

Implement new rules and consequences for penalties. The changes are as following:

  • If you have at least one mark, you must wait for 5 hours after the regular registration start to sign up for an event.
  • The duration of each new penalty is 10 days or after being banned from participating in 6 events where you have registration eligibility
  • Each new penalty is added to the end of the last active penalty. So if 5 days passes after the first penalty is given, and then a new penalty is given, the remaining time is then 15 days

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

@Arashfa0301 Arashfa0301 added help-wanted Pull requests that want help in the form of contributions in-progress backend priority:high Pull requests that have high priority, and should therefore be prioritized do-not-merge/WIP Pull requests that are "work in progress", and should not be merged review-needed Pull requests that need review labels Apr 30, 2023
@Arashfa0301 Arashfa0301 self-assigned this Apr 30, 2023
@ivarnakken ivarnakken added new-feature Pull requests that introduce or issues that suggest a new feature and removed backend priority:high Pull requests that have high priority, and should therefore be prioritized labels Apr 30, 2023
Copy link
Member

@norbye norbye left a 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/events/tests/test_penalties.py Outdated Show resolved Hide resolved
lego/.DS_Store Outdated Show resolved Hide resolved
lego/apps/users/tests/test_models.py Outdated Show resolved Hide resolved
@norbye norbye changed the title Implement new plenalty rules Implement new penalty rules May 1, 2023
@Arashfa0301 Arashfa0301 force-pushed the new-penalty-rules branch 2 times, most recently from 1df65ce to 7a487fa Compare May 1, 2023 18:19
@codecov
Copy link

codecov bot commented May 1, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (1362f10) 88.30% compared to head (7a487fa) 88.26%.
Report is 156 commits behind head on master.

❗ Current head 7a487fa differs from pull request most recent head d531414. Consider uploading reports for the commit d531414 to get more accurate results

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     
Files Coverage Δ
lego/apps/events/models.py 97.46% <100.00%> (+0.26%) ⬆️
lego/apps/events/tests/test_async_tasks.py 93.65% <ø> (-0.43%) ⬇️
lego/apps/events/tests/test_penalties.py 100.00% <100.00%> (ø)
lego/apps/users/action_handlers.py 57.50% <ø> (ø)
...s/users/migrations/0039_penalty_activation_time.py 100.00% <100.00%> (ø)
lego/apps/users/notifications.py 80.95% <ø> (ø)
lego/apps/users/serializers/penalties.py 100.00% <ø> (ø)
lego/apps/users/tests/test_models.py 100.00% <100.00%> (ø)
lego/apps/users/tests/test_users_api.py 96.81% <ø> (-0.02%) ⬇️
lego/apps/users/models.py 95.78% <94.73%> (+0.43%) ⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lego/.DS_Store Outdated Show resolved Hide resolved
lego/apps/events/models.py Outdated Show resolved Hide resolved
lego/apps/users/action_handlers.py Outdated Show resolved Hide resolved
lego/apps/users/migrations/0039_penalty_activation_time.py Outdated Show resolved Hide resolved
lego/apps/users/models.py Outdated Show resolved Hide resolved
lego/apps/users/migrations/0039_penalty_activation_date.py Outdated Show resolved Hide resolved
lego/apps/users/models.py Outdated Show resolved Hide resolved
lego/apps/users/models.py Outdated Show resolved Hide resolved
@Arashfa0301 Arashfa0301 force-pushed the new-penalty-rules branch 2 times, most recently from b50f868 to a036bfc Compare September 28, 2023 16:11
@Arashfa0301 Arashfa0301 removed help-wanted Pull requests that want help in the form of contributions in-progress do-not-merge/WIP Pull requests that are "work in progress", and should not be merged labels Sep 28, 2023
Copy link
Contributor

@falbru falbru left a 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

Copy link
Contributor

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
Copy link
Contributor

@falbru falbru Sep 30, 2023

Choose a reason for hiding this comment

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

Suggested change
.DS_Store
**/.DS_Store

Your current code only removes the .DS_Store in the root directory

Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

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

🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

right > wrong

Comment on lines +648 to +652
# Generated by Django 4.0.10 on 2023-05-01 16:33

from django.db import models


Copy link
Member

Choose a reason for hiding this comment

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

What's this?

Copy link
Member

@juniwbjerde juniwbjerde left a 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,
Copy link
Member

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()
Copy link
Member

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

Copy link
Contributor

@ollfkaih ollfkaih left a 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

right > wrong

@norbye norbye removed their request for review October 23, 2023 09:39
Copy link

linear bot commented Nov 23, 2023

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:

  • If you have one or more prikk you have to wait 5 hours after the ordinary registration start for an event before you can register.
  • The duration of a prikk is 10 days or 6 events where you have permission to register (påmeldingsrett). Each new prikk leads to an addition of the duration (i.e. 10 more days/ 6 events with a prikk)

i.e. the number of prikks you have does not really matter anymore, it only matters how long you have them.

pr: #3349

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature Pull requests that introduce or issues that suggest a new feature review-needed Pull requests that need review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants