feat: Implement max games limit

[faucomte97]

This change is 

[SKairinos]

Reviewed 14 of 18 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 15 of 18 files reviewed, 10 unresolved discussions (waiting on @faucomte97)

---

aimmo/models.py line 22 at r2 (raw file):

]

MAX_GAMES_LIMIT = 15

Let's add this as a setting.
from django.conf import settings
settings.MAX_GAMES_LIMIT

Code quote:

MAX_GAMES_LIMIT = 15

---

aimmo/models.py line 139 at r2 (raw file):

        }

    def create_avatar_for_user(self, user):

let's move this logic to the Avatar.objects.create method. Creation of a model should be the responsibility of that model's object manager. This you would simply say:
avatar = Avatar.objects.create(owner=user, game=game)

Furthemore, let's add the generate_auth_token helper as a default callable of the auth_token field. It's safer since it should be impossible to create an avatar without an auth token. It would like something like:
def generate_game_auth_token():
return generate_auth_token(16, 24)
auth_token = models.CharField(max_length=24, blank=True, default=generate_game_auth_token)
see: https://docs.djangoproject.com/en/4.2/ref/models/fields/#default

Code quote:

create_avatar_for_user

---

aimmo/models.py line 152 at r2 (raw file):

    def save(self, **kwargs):
        new_game = self.id is None

no need for variable, can write inline since it's only referenced once

Code quote:

new_game

---

aimmo/models.py line 154 at r2 (raw file):

        new_game = self.id is None

        if new_game:

relocate everything inside this if block in the Game.objects.create

Code quote:

if new_game:

---

aimmo/models.py line 155 at r2 (raw file):

        if new_game:
            self.auth_token = generate_auth_token(32, 48)

see above comment about adding this as a default callable.

Code quote:

self.auth_token = generate_auth_token(32, 48)

---

aimmo/models.py line 156 at r2 (raw file):

        if new_game:
            self.auth_token = generate_auth_token(32, 48)
            self.generator = "Main"

set as default value. only set it in the create method if we want to override the default based on some condition(s). but since we always want it to be "Main", it can just be a default.

Code quote:

            self.generator = "Main"

---

aimmo/models.py line 179 at r2 (raw file):

        def update(self, **kwargs) -> int:
            if (
                kwargs.get("status", Game.STOPPED) == Game.RUNNING

no need for a default value. none is an acceptable return value

Code quote:

Game.STOPPED

---

aimmo/models.py line 180 at r2 (raw file):

            if (
                kwargs.get("status", Game.STOPPED) == Game.RUNNING
                and Game.objects.filter(status=Game.RUNNING).count() + self.count() > MAX_GAMES_LIMIT

rethinking this, this doesn't seem like the correct check because we're not accounting for the fact there may be duplicate values between queryset 1 and 2. Therefore, we should first get the union of the 2 queryset to count only the distinct values.
see: https://docs.djangoproject.com/en/4.2/ref/models/querysets/#union
we should create a test case to confirm

Code quote:

 Game.objects.filter(status=Game.RUNNING).count() + self.count()

---

aimmo/models.py line 190 at r2 (raw file):

@receiver(models.signals.pre_save, sender=Game)
def check_game_limit(sender: t.Type[Game], instance: Game, **kwargs):
    if instance.status == Game.RUNNING and sender.objects.filter(status=Game.RUNNING).count() >= MAX_GAMES_LIMIT:

to make the query more complete, let's also exclude the current instance from the queryset if the instance already exists (id is not None).
if instance.status == Game.RUNNING:
queryset = sender.objects.filter(status=Game.RUNNING)
if instance.id is not None:
queryset = queryset.exclude(id=instance.id)
if queryset.count() >= ....

Code quote:

 if instance.status == Game.RUNNING and sender.objects.filter(status=Game.RUNNING).count() 

---

aimmo/views.py line 51 at r2 (raw file):

        avatar = game.avatar_set.get(owner=request.user)
    except Avatar.DoesNotExist:
        avatar = game.create_avatar_for_user(request.user)

Avatar.objetcs.create(game=game, owner=request.user)

Code quote:

 game.create_avatar_for_user(request.user)

[faucomte97]

Reviewable status: 15 of 18 files reviewed, 10 unresolved discussions (waiting on @faucomte97 and @SKairinos)

---

aimmo/models.py line 22 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

Let's add this as a setting.
from django.conf import settings
settings.MAX_GAMES_LIMIT

Leaving it in here for now cos moving it to settings causes some problems with the mocking in the tests... Can revisit later

---

aimmo/models.py line 139 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

let's move this logic to the Avatar.objects.create method. Creation of a model should be the responsibility of that model's object manager. This you would simply say:
avatar = Avatar.objects.create(owner=user, game=game)

Furthemore, let's add the generate_auth_token helper as a default callable of the auth_token field. It's safer since it should be impossible to create an avatar without an auth token. It would like something like:
def generate_game_auth_token():
return generate_auth_token(16, 24)
auth_token = models.CharField(max_length=24, blank=True, default=generate_game_auth_token)
see: https://docs.djangoproject.com/en/4.2/ref/models/fields/#default

Left logic in save() in the end.

---

aimmo/models.py line 152 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

no need for variable, can write inline since it's only referenced once

Done.

---

aimmo/models.py line 154 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

relocate everything inside this if block in the Game.objects.create

Done.

---

aimmo/models.py line 155 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

see above comment about adding this as a default callable.

Done.

---

aimmo/models.py line 156 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

set as default value. only set it in the create method if we want to override the default based on some condition(s). but since we always want it to be "Main", it can just be a default.

Already a default

---

aimmo/models.py line 179 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

no need for a default value. none is an acceptable return value

Done.

---

aimmo/models.py line 180 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

rethinking this, this doesn't seem like the correct check because we're not accounting for the fact there may be duplicate values between queryset 1 and 2. Therefore, we should first get the union of the 2 queryset to count only the distinct values.
see: https://docs.djangoproject.com/en/4.2/ref/models/querysets/#union
we should create a test case to confirm

Done.

---

aimmo/models.py line 190 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

to make the query more complete, let's also exclude the current instance from the queryset if the instance already exists (id is not None).
if instance.status == Game.RUNNING:
queryset = sender.objects.filter(status=Game.RUNNING)
if instance.id is not None:
queryset = queryset.exclude(id=instance.id)
if queryset.count() >= ....

I don't follow what you're saying here - why would we want to do that?

---

aimmo/views.py line 51 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

Avatar.objetcs.create(game=game, owner=request.user)

Done.

[SKairinos]

Reviewed 1 of 18 files at r1, 5 of 6 files at r3, all commit messages.
Reviewable status: 20 of 21 files reviewed, 2 unresolved discussions (waiting on @faucomte97)

---

aimmo/models.py line 155 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Done.

default missing on model field

---

aimmo/models.py line 190 at r2 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

I don't follow what you're saying here - why would we want to do that?

Basically, I'm saying that instance MAY ALREADY BE RUNNNING and included in the queryset. Therefore, the instance should be excluded from the queryset and not raise an exception. This scenario could occur when updating another field on a game instance that is already running.

[faucomte97]

Reviewable status: 19 of 21 files reviewed, 2 unresolved discussions (waiting on @faucomte97 and @SKairinos)

---

aimmo/models.py line 155 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

default missing on model field

Done.

---

aimmo/models.py line 190 at r2 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

Basically, I'm saying that instance MAY ALREADY BE RUNNNING and included in the queryset. Therefore, the instance should be excluded from the queryset and not raise an exception. This scenario could occur when updating another field on a game instance that is already running.

Done. Had to rebuild the queryset altogether because just calling exclude() on an already created queryset doesn't seem to work.

[SKairinos]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faucomte97)

[SKairinos]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @faucomte97)

[faucomte97]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @faucomte97)

[codecov]

Codecov Report

Merging #1797 (cae7ef7) into master (0652815) will increase coverage by 44.19%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #1797       +/-   ##
===========================================
+ Coverage   44.01%   88.20%   +44.19%     
===========================================
  Files         177       38      -139     
  Lines        3658     1060     -2598     
  Branches      155      109       -46     
===========================================
- Hits         1610      935      -675     
+ Misses       2043      102     -1941     
- Partials        5       23       +18     

see 181 files with indirect coverage changes