Skip to content

Commit

Permalink
[WIP] First pass at adding CLDT seat functionality
Browse files Browse the repository at this point in the history
  • Loading branch information
froggleston committed Jun 21, 2024
1 parent ca733cd commit b00cd6c
Show file tree
Hide file tree
Showing 10 changed files with 341 additions and 13 deletions.
6 changes: 6 additions & 0 deletions amy/api/v2/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,9 @@ class MembershipSerializer(serializers.ModelSerializer):
inhouse_instructor_training_seats_total = serializers.IntegerField()
inhouse_instructor_training_seats_utilized = serializers.IntegerField()
inhouse_instructor_training_seats_remaining = serializers.IntegerField()
cldt_seats_total = serializers.IntegerField()
cldt_seats_utilized = serializers.IntegerField()
cldt_seats_remaining = serializers.IntegerField()

class Meta:
model = Membership
Expand Down Expand Up @@ -206,6 +209,9 @@ class Meta:
"inhouse_instructor_training_seats_total",
"inhouse_instructor_training_seats_utilized",
"inhouse_instructor_training_seats_remaining",
"cldt_seats_total",
"cldt_seats_utilized",
"cldt_seats_remaining",
)


Expand Down
10 changes: 10 additions & 0 deletions amy/extrequests/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def member_code_valid_training(
if (
membership.public_instructor_training_seats_remaining
+ membership.inhouse_instructor_training_seats_remaining
+ membership.cldt_seats_remaining

This comment has been minimized.

Copy link
@pbanaszkiewicz

pbanaszkiewicz Jun 25, 2024

Contributor

I'm not sure if this part should be here. I can imagine that training seats belong either to instructor training or to CLDT, so there should be two separate checks for them:

  1. if cumulative number of remaining training seats is <= 0 (AND the validation is for a seat for instructor training)
  2. if number of remaining CLDT seats is <= 0 (AND the validation is for a CLDT training).

I wrote additional conditions for the seat "type" in the parentheses because currently we don't distinguish between validation for either of the "type" (CLDT or instructor training). This function member_code_valid_training is used in:

  1. https://test-amy2.carpentries.org/forms/request_training/
  2. https://test-amy2.carpentries.org/requests/training_request/88/edit/ (example)

...so we should know if those forms are going to be used for applying for CLDT training.

<= 0
):
raise MemberCodeValidationError("Membership has no training seats remaining.")
Expand Down Expand Up @@ -152,6 +153,15 @@ def get_membership_warnings_after_match(
"it's been allowed.",
)

cldt_remaining = (
membership.cldt_seats_remaining
)
if cldt_remaining <= 0:
warnings.append(
f'Membership "{membership}" is using more CLDT seats than '
"it's been allowed.",
)

# check if membership is active
if not (membership.agreement_start <= date.today() <= membership.agreement_end):
warnings.append(
Expand Down
10 changes: 10 additions & 0 deletions amy/fiscal/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,13 +120,17 @@ class Meta:
"additional_public_instructor_training_seats",
"inhouse_instructor_training_seats",
"additional_inhouse_instructor_training_seats",
"cldt_seats",
"additional_cldt_seats",
"emergency_contact",
"workshops_without_admin_fee_rolled_over",
"workshops_without_admin_fee_rolled_from_previous",
"public_instructor_training_seats_rolled_over",
"public_instructor_training_seats_rolled_from_previous",
"inhouse_instructor_training_seats_rolled_over",
"inhouse_instructor_training_seats_rolled_from_previous",
"cldt_seats_rolled_over",
"cldt_seats_rolled_from_previous",
]

def __init__(
Expand All @@ -145,10 +149,12 @@ def __init__(
del self.fields["workshops_without_admin_fee_rolled_over"]
del self.fields["public_instructor_training_seats_rolled_over"]
del self.fields["inhouse_instructor_training_seats_rolled_over"]
del self.fields["cldt_seats_rolled_over"]
if not show_rolled_from_previous:
del self.fields["workshops_without_admin_fee_rolled_from_previous"]
del self.fields["public_instructor_training_seats_rolled_from_previous"]
del self.fields["inhouse_instructor_training_seats_rolled_from_previous"]
del self.fields["cldt_seats_rolled_from_previous"]

# set up a layout object for the helper
self.helper.layout = self.helper.build_default_layout(self)
Expand Down Expand Up @@ -288,6 +294,9 @@ class Meta(MembershipCreateForm.Meta):
"inhouse_instructor_training_seats",
"additional_inhouse_instructor_training_seats",
"inhouse_instructor_training_seats_rolled_from_previous",
"cldt_seats",
"additional_cldt_seats",
"cldt_seats_rolled_from_previous",
"emergency_contact",
"comment",
]
Expand All @@ -303,6 +312,7 @@ def __init__(self, *args, **kwargs):
"workshops_without_admin_fee_rolled_from_previous",
"public_instructor_training_seats_rolled_from_previous",
"inhouse_instructor_training_seats_rolled_from_previous",
"cldt_seats_rolled_from_previous",
]
for field in fields:
self[field].field.min_value = 0
Expand Down
13 changes: 13 additions & 0 deletions amy/fiscal/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,10 @@ def form_valid(self, form):
"inhouse_instructor_training_seats_rolled_over",
"inhouse_instructor_training_seats_rolled_from_previous",
),
(
"cldt_seats_rolled_over",
"cldt_seats_rolled_from_previous",
),
)
save_rolled_to = False
try:
Expand Down Expand Up @@ -584,6 +588,9 @@ def get_initial(self) -> Dict[str, Any]:
"inhouse_instructor_training_seats": self.membership.inhouse_instructor_training_seats, # noqa
"additional_inhouse_instructor_training_seats": self.membership.additional_inhouse_instructor_training_seats, # noqa
"inhouse_instructor_training_seats_rolled_from_previous": 0,
"cldt_seats": self.membership.inhouse_instructor_training_seats, # noqa
"additional_cldt_seats": self.membership.additional_inhouse_instructor_training_seats, # noqa

This comment has been minimized.

Copy link
@pbanaszkiewicz

pbanaszkiewicz Jun 25, 2024

Contributor

Possibly in both lines L591 and L592 should be a different value, e.g. "cldt_seats": self.membership.cldt_seats.

"cldt_seats_rolled_from_previous": 0,
"emergency_contact": self.membership.emergency_contact,
}

Expand All @@ -599,6 +606,9 @@ def get_form_kwargs(self) -> Dict[str, Any]:
"inhouse_instructor_training_seats_rolled_from_previous": max(
self.membership.inhouse_instructor_training_seats_remaining, 0
),
"cldt_seats_rolled_from_previous": max(
self.membership.cldt_seats_remaining, 0
),
}
return {
"max_values": max_values,
Expand All @@ -623,6 +633,9 @@ def form_valid(self, form):
self.membership.inhouse_instructor_training_seats_rolled_over = (
form.instance.inhouse_instructor_training_seats_rolled_from_previous
)
self.membership.cldt_seats_rolled_over = (
form.instance.cldt_seats_rolled_from_previous
)
self.membership.rolled_to_membership = self.object
self.membership.save()

Expand Down
5 changes: 3 additions & 2 deletions amy/reports/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,13 @@ def instructor_issues(request):
airport__isnull=True
)

# Everyone who's been in instructor training but doesn't yet have a badge.
# Everyone who's been in instructor training or CLDT but doesn't yet have a badge.
learner = Role.objects.get(name="learner")
ttt = Tag.objects.get(name="TTT")
cldt = Tag.objects.get(name="CLDT")
stalled = Tag.objects.get(name="stalled")
trainees = (
Task.objects.filter(event__tags__in=[ttt], role=learner)
Task.objects.filter(event__tags__in=[ttt,cldt], role=learner)

This comment has been minimized.

Copy link
@pbanaszkiewicz

pbanaszkiewicz Jun 25, 2024

Contributor

Styling issue, please install pre-commit (pre-commit install). You can also run just black on this file (black amy/reports/views.py).

.exclude(person__badges__in=instructor_badges)
.order_by("person__family", "person__personal", "event__start")
.select_related("person", "event")
Expand Down
24 changes: 24 additions & 0 deletions amy/templates/fiscal/membership.html
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,30 @@
</p>
</td>
</tr>
<tr>
<th rowspan="3" width="30%">CLDT seats</th>
<th>Allowed:</th>
<td>
{{ membership.cldt_seats_total }}
<p class="small">
Includes {{ membership.additional_cldt_seats }} additional seats.<br>
Includes {{ membership.cldt_seats_rolled_from_previous|default_if_none:0 }} seats rolled from previous membership.
</p>
</td>
</tr>
<tr>
<th>Utilized:</th>
<td>{{ membership.cldt_seats_utilized }}</td>
</tr>
<tr {% if membership.cldt_seats_remaining <= 0 and membership.cldt_seats_total > 0 or membership.cldt_seats_remaining < 0 and membership.cldt_seats_total == 0 %}class="table-danger"{% endif %}>
<th>Remaining:</th>
<td>
{{ membership.cldt_seats_remaining }}
<p class="small">
{{ membership.cldt_seats_rolled_over|default:"None" }} were rolled over to following membership.
</p>
</td>
</tr>
<tr>
<th>Instructor training seats:</th>
<td colspan="2">
Expand Down
1 change: 1 addition & 0 deletions amy/workshops/management/commands/fake_database.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ def fake_tags(self):
("LSO", 170, "Lesson Specific Onboarding"),
("hackathon", 180, "Event is a hackathon"),
("WiSE", 190, "Women in Science and Engineering"),
("CLDT", 200, "Collaborative Lesson Development Training"),
]

self.stdout.write("Generating {} fake tags...".format(len(tags)))
Expand Down
116 changes: 110 additions & 6 deletions amy/workshops/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,9 @@ class Meta:

class MembershipManager(models.Manager):
def annotate_with_seat_usage(self):
cldt_tag = Tag.objects.get(name="CLDT")
no_cldt_tags = Tag.objects.filter(name__in=["SWC", "DC", "LC", "TTT", "ITT", "WiSE"])

return self.get_queryset().annotate(
instructor_training_seats_total=(
# Public
Expand All @@ -152,7 +155,10 @@ def annotate_with_seat_usage(self):
+ Coalesce("inhouse_instructor_training_seats_rolled_from_previous", 0)
),
instructor_training_seats_utilized=(
Count("task", filter=Q(task__role__name="learner"))
Count("task", filter=Q(
task__role__name="learner",
task__event__tags=[no_cldt_tags]
))

This comment has been minimized.

Copy link
@pbanaszkiewicz

pbanaszkiewicz Jun 25, 2024

Contributor

You should carefully check if this calculation is correct. Sometimes what we see is that for example event with both tags "SWC" and "TTT" could be counted here twice. This is why I was advocating for using .exclude instead of .filter in L143.

This comment has been minimized.

Copy link
@froggleston

froggleston Aug 15, 2024

Author Contributor

Would something like ttt_tags = Tag.objects.exclude(name__in=["SWC", "DC", "LC", "WiSE", "CLDT"]) work instead?

This comment has been minimized.

Copy link
@maneesha

maneesha Aug 15, 2024

Contributor

We should probably have other checks in here anyways. For example, something tagged "TTT" should never also be tagged SWC/DC/LC.

),
instructor_training_seats_remaining=(
# Public
Expand All @@ -161,7 +167,11 @@ def annotate_with_seat_usage(self):
# Coalesce returns first non-NULL value
+ Coalesce("public_instructor_training_seats_rolled_from_previous", 0)
- Count(
"task", filter=Q(task__role__name="learner", task__seat_public=True)
"task", filter=Q(
task__role__name="learner",
task__seat_public=True,
task__event__tags=[no_cldt_tags]
)
)
- Coalesce("public_instructor_training_seats_rolled_over", 0)
# Inhouse
Expand All @@ -170,10 +180,42 @@ def annotate_with_seat_usage(self):
+ Coalesce("inhouse_instructor_training_seats_rolled_from_previous", 0)
- Count(
"task",
filter=Q(task__role__name="learner", task__seat_public=False),
filter=Q(
task__role__name="learner",
task__seat_public=False,
task__event__tags=[no_cldt_tags]
),
)
- Coalesce("inhouse_instructor_training_seats_rolled_over", 0)
),

# CLDT
cldt_seats_total=(
F("cldt_seats")
+ F("additional_cldt_seats")
# Coalesce returns first non-NULL value
+ Coalesce("cldt_seats_rolled_from_previous", 0)
),
cldt_seats_utilized=(
Count("task", filter=Q(
task__role__name="learner",
task__event__tags=[cldt_tag]
))
),
cldt_seats_remaining=(
F("cldt_seats")
+ F("additional_cldt_seats")
# Coalesce returns first non-NULL value
+ Coalesce("cldt_seats_rolled_from_previous", 0)
- Count(
"task", filter=Q(
task__role__name="learner",
task__seat_public=True,
task__event__tags=[cldt_tag]
)
)
- Coalesce("cldt_seats_rolled_over", 0)
),
)


Expand Down Expand Up @@ -290,6 +332,34 @@ class Membership(models.Model):
blank=True,
help_text="In-house instructor training seats rolled over into next membership.", # noqa
)

#CLDT
cldt_seats = models.PositiveIntegerField(
null=False,
blank=False,
default=0,
verbose_name="Collaborative Lesson Development Training seats",
help_text="Number of CLDT seats",
)
additional_cldt_seats = models.PositiveIntegerField(
null=False,
blank=False,
default=0,
verbose_name="Additional CLDT seats",
help_text="Use this field if you want to grant more CLDT seats than "
"the agreement provides for.",
)
cldt_seats_rolled_from_previous = models.PositiveIntegerField(
null=True,
blank=True,
help_text="CLDT seats rolled over from previous membership.",
)
cldt_seats_rolled_over = models.PositiveIntegerField(
null=True,
blank=True,
help_text="CLDT seats rolled over into next membership.",
)

organizations = models.ManyToManyField(
Organization,
blank=False,
Expand Down Expand Up @@ -322,7 +392,7 @@ class Membership(models.Model):
max_length=20,
choices=PUBLIC_STATUS_CHOICES,
default=PUBLIC_STATUS_CHOICES[1][0],
verbose_name="Can this membership be publicized on The carpentries websites?",
verbose_name="Can this membership be publicized on The Carpentries websites?",
help_text="Public memberships may be listed on any of The Carpentries "
"websites.",
)
Expand Down Expand Up @@ -559,6 +629,30 @@ def inhouse_instructor_training_seats_remaining(self) -> int:
c = self.inhouse_instructor_training_seats_rolled_over or 0
return a - b - c

@property
def cldt_seats_total(self) -> int:
"""Calculate combined CLDT seats total.
Unlike workshops w/o admin fee, CLDT seats have two numbers
combined to calculate total of allowed seats in CLDT events.
"""
a = self.cldt_seats
b = self.additional_cldt_seats
c = self.cldt_seats_rolled_from_previous or 0
return a + b + c

@cached_property
def cldt_seats_utilized(self) -> int:
"""Count number of learner tasks that point to this membership."""
return self.task_set.filter(role__name="learner", seat_public=False).count()

This comment has been minimized.

Copy link
@pbanaszkiewicz

pbanaszkiewicz Jun 25, 2024

Contributor

Possibly missing the event__tag filter here. Possibly similar change required for calculating utilized instructor training seats (but for non-CLDT tags).


@property
def cldt_seats_remaining(self) -> int:
"""Count remaining seats for CLDT."""
a = self.cldt_seats_total
b = self.cldt_seats_utilized
c = self.cldt_seats_rolled_over or 0
return a - b - c

# ------------------------------------------------------------

Expand Down Expand Up @@ -1075,11 +1169,11 @@ def archive(self) -> None:

class TagQuerySet(QuerySet):
def main_tags(self):
names = ["SWC", "DC", "LC", "TTT", "ITT", "WiSE"]
names = ["SWC", "DC", "LC", "TTT", "ITT", "WiSE", "CLDT"]
return self.filter(name__in=names)

def carpentries(self):
return self.filter(name__in=["SWC", "DC", "LC"])
return self.filter(name__in=["SWC", "DC", "LC", "CLDT"])

def strings(self):
return self.values_list("name", flat=True)
Expand Down Expand Up @@ -1481,6 +1575,16 @@ class Event(AssignmentMixin, RQJobsMixin, models.Model):
"this event's member sites can also take part in this event.",
)

open_CLDT_applications = models.BooleanField(
null=False,
blank=True,
default=False,
verbose_name="CLDT Open applications",
help_text="If this event is <b>CLDT</b>, you can mark it as 'open "
"applications' which means that people not associated with "
"this event's member sites can also take part in this event.",
)

This comment has been minimized.

Copy link
@pbanaszkiewicz

pbanaszkiewicz Jun 25, 2024

Contributor

Is it possible to have both: open TTT applications and open CLDT application on the same event?

This comment has been minimized.

Copy link
@froggleston

froggleston Aug 15, 2024

Author Contributor

No, I don't believe so (or at least it shouldn't be allowed to arrive at that situation)

This comment has been minimized.

Copy link
@maneesha

maneesha Aug 15, 2024

Contributor

Agreed - CLDT and TTT will always be different events. We should actually prevent CLDT seats from being used at TTT event (and the other way around).


# taught curriculum information
curricula = models.ManyToManyField(
"Curriculum",
Expand Down
Loading

0 comments on commit b00cd6c

Please sign in to comment.