JMoran1 / DAESDgroup

2 stars 0 forks source link

Jacob/student join club #107

Closed JMoran1 closed 1 year ago

JMoran1 commented 1 year ago

@saxbophone had to remove your constraints as I was not able to create a club rep, not sure if you want to try edit these constraints or just leave them as being removed

Closes #106

saxbophone commented 1 year ago

@saxbophone had to remove your constraints as I was not able to create a club rep, not sure if you want to try edit these constraints or just leave them as being removed

Hmmm, that's odd and unexpected, I will take a look at the code in the add club rep view, as if I'm not mistaken, it's meant to be the case that club reps are required to have a club. My understanding of @TylerIves94 's request from last night to alter the constraint was to only remove it for student, not club rep (unless I misunderstood?).

JMoran1 commented 1 year ago

Hmmm, that's odd and unexpected, I will take a look at the code in the add club rep view, as if I'm not mistaken, it's meant to be the case that club reps are required to have a club. My understanding of @TylerIves94 's request from last night to alter the constraint was to only remove it for student, not club rep (unless I misunderstood?).

Not wrong at all, the Club rep should be linked to a club but for some reason when the CM created the club rep via the link and assigned a club it would error supposedly due to the constraints. Also tried making a CR via shell and got the same issue

saxbophone commented 1 year ago

@saxbophone had to remove your constraints as I was not able to create a club rep, not sure if you want to try edit these constraints or just leave them as being removed

Hmmm, that's odd and unexpected, I will take a look at the code in the add club rep view, as if I'm not mistaken, it's meant to be the case that club reps are required to have a club. My understanding of @TylerIves94 's request from last night to alter the constraint was to only remove it for student, not club rep (unless I misunderstood?).

I've found the source of the issue @JMoran1 —it's my fault. Having a constraint is not the problem, but I rewrote the constraint slightly mistakenly last night. The second condition should have been flipped!

Before:

    class Meta:
        constraints = (
            models.CheckConstraint(
                # Logically: (Role EQUALS Club Rep) IMPLIES (Club EXISTS)
                # AKA: (NOT CLUB_REP) OR CLUB EXISTS
                check= ~Q(role=Role.CLUB_REP) | Q(club__isnull=True),
                name='club_reps_have_club',
                violation_error_message='Club Rep must have a Club!'
            ),
        )

After:

    class Meta:
        constraints = (
            models.CheckConstraint(
                # Logically: (Role EQUALS Club Rep) IMPLIES (Club EXISTS)
                # AKA: (NOT CLUB_REP) OR CLUB EXISTS
                check= ~Q(role=Role.CLUB_REP) | Q(club__isnull=False),
                name='club_reps_have_club',
                violation_error_message='Club Rep must have a Club!'
            ),
        )

I will send you a PR now to undo the migration removal and correct my mistake!

saxbophone commented 1 year ago

@JMoran1 I've sent you a PR into your branch for fixing my mess-up, sorry about that!

This time, I've actually tested my work and it now works :)

JMoran1 commented 1 year ago

@JMoran1 I've sent you a PR into your branch for fixing my mess-up, sorry about that!

This time, I've actually tested my work and it now works :)

Thank you :)

saxbophone commented 1 year ago

I have functionally tested the code and functionally it works fine, nice work!

JMoran1 commented 1 year ago

I have functionally tested the code and functionally it works fine, nice work!

Great! Made all of the changes requested so should be good to go