[WIP] CLDT seat implementation by froggleston · Pull Request #2689 · carpentries/amy

froggleston · 2024-08-19T15:16:58Z

This PR is a WIP to implement CLDT seat allocation and tracking

pbanaszkiewicz · 2024-08-19T20:06:24Z

    def annotate_with_seat_usage(self):
+        cldt_tag = Tag.objects.get(name="CLDT")
+        # TTT/ITT included
+        ttt_tags = Tag.objects.exclude(name__in=["SWC", "DC", "LC", "WiSE", "CLDT"])


In here (L143, L145) use tag names:

cldt_tag_name = "CLDT" ttt_tag_names = ["TTT", "ITT"]

Then the lookups using ttt_tags or cldt_tag have to be changed:

from task__event__tags=[ttt_tags] to task__event__tags__name__in=ttt_tag_names

from task__event__tags=[cldt_tag] to task__event__tags__name__in=[cldt_tag_name]

pbanaszkiewicz · 2024-09-23T18:37:36Z

+    @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()


Since this property should mimick the query in MembershipManager.annotate_with_seat_usage, there's additional filtering missing:

cld_seats_utilized() should also filter by event tag CLDT

in MembershipManager.annotate_with_seat_usage, _cldt_seats_utilized should also filter by seat_public

in MembershipManager.annotate_with_seat_usage, _cldt_seats_remaining should filter by seat_public in the same way (currently it uses public=True)

both public_instructor_training_seats_utilized() and inhouse_instructor_training_seats_utilized() should also filter by TTT tags (TTT, ITT)

Generally speaking the model properties should come down to the same equations as in annotations.

Updated all calculations and set CLDT to public=False as these events are member based and not open to the public.

pbanaszkiewicz · 2024-09-23T18:58:36Z

        <td>{{ result.instructor_training_seats_remaining }}</td>
+        <td>{{ result._cldt_training_seats_total }}</td>
+        <td>{{ result._cldt_training_seats_utilized }}</td>
+        <td>{{ result._cldt_training_seats_remaining }}</td>


Django template is complaining about these names. Apparently they can't start with _, so my proposition is to use these names instead:

cldt_seats_total_annotation

cldt_seats_utilized_annotation

cldt_seats_remaining_annotation

Changes should be applied here and in MembershipManager.annotate_with_seat_usage.

I've changed the CLDT annotation variable names but not the others. We can do this after merge/rebase?

I think only CLDT annotations started with _, so only they needed to be updated with that prefix. Other annotations work just fine because they don't have name clash with model properties.

pbanaszkiewicz · 2024-09-23T19:00:04Z

        <th rowspan="2">Agreement</th>
        <th rowspan="2">Contribution</th>
        <th colspan="3" class="text-center">Instructor training seats (combined public and in-house)</th>
+        <th colspan="3" class="text-center">CLDT seats</th>


This introduces 3 new columns in the row that's following (lines 23-25 below). Can you duplicate those lines? This way the header displays correctly:

pbanaszkiewicz · 2024-09-23T19:02:40Z

+            {{ membership.instructor_training_seats_remaining }} / {{ membership.instructor_training_seats_total }}
+          </td>
+          <td {% 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 %}>
+            {{ membership._cldt_seats_remaining }} / {{ membership._cldt_seats_total }}


Django template also complains here, so I suggest applying these changes in the lines above:

_cldt_seats_remaining -> cldt_seats_remaining_annotation

_cldt_seats_total -> cldt_seats_total_annotation

pbanaszkiewicz · 2024-09-23T19:04:32Z

          <td>{{ membership.get_contribution_type_display }}</td>
          <td {% if membership.instructor_training_seats_remaining <= 0 and membership.instructor_training_seats_total > 0 or membership.instructor_training_seats_remaining < 0 and membership.instructor_training_seats_total == 0 %}class="table-danger"{% endif %}>
-            {{ membership.instructor_training_seats_remaining }}
+            {{ membership.instructor_training_seats_remaining }} / {{ membership.instructor_training_seats_total }}


This notation (remaining count / total count) is clever, but I'm wondering if users would know this is remaining / total, and not utilized / total.

I'll check this with the CT, but as these calculations have been in place for the other seat types, I wouldn't want to change it without consulting them.

pbanaszkiewicz · 2024-09-30T19:43:03Z

-        cldt_tag_name = "CLDT"
-        ttt_tag_names = ["TTT", "ITT"]
+        # cldt_tag_name = "CLDT"
+        # ttt_tag_names = ["TTT", "ITT"]


Can you remove this commented-out code?

pbanaszkiewicz · 2024-09-30T19:44:12Z

 STR_LONGEST = 255  # length of the longest strings
 STR_REG_KEY = 20  # length of Eventbrite registration key
+
+CLDT_TAG_NAME = ["CLDT"]


I'd keep this plural (nameS) since this is a list.

maneesha · 2026-03-26T18:24:32Z

@froggleston I think this is now obsolete with the new benefits model, correct?

froggleston · 2026-03-26T22:39:51Z

Hey @maneesha - yes, I believe we'll have another go at this once we have a follow-up about the general approach to tags vs proper lesson program entities/enums.

froggleston added 5 commits June 21, 2024 16:13

[WIP] First pass at adding CLDT seat functionality

b00cd6c

Merge develop

5eb218a

Refactor based on first review

a0ec5a7

Adjust memberships table to include CLDT

37303cd

Adjust organisation view template to include CLDT

bc7c4b3

froggleston changed the title ~~Cldt seats~~ [WIP] CLDT seat implementation Aug 19, 2024

pbanaszkiewicz reviewed Aug 19, 2024

View reviewed changes

Fix CLDT seat membership models, update templates, add CLDT migrations

4665d17

pbanaszkiewicz reviewed Sep 23, 2024

View reviewed changes

Update annotation names, add filters, update templates

31a6b92

pbanaszkiewicz reviewed Sep 30, 2024

View reviewed changes

pbanaszkiewicz added the priority: defer label Jan 14, 2025

Uh oh!

Conversation

froggleston commented Aug 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pbanaszkiewicz Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maneesha commented Mar 26, 2026

Uh oh!

froggleston commented Mar 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pbanaszkiewicz Sep 23, 2024 •

edited

Loading