Migrate attendance viewsets to serializer-derived values pattern

[rtibbles]

❌ This issue is not open for contribution. Visit Contributing guidelines to learn about the contributing process and how to find suitable issues.

Overview

Migrate AttendanceSessionViewSet and AttendanceRecordViewSet from explicit values tuples to the serializer-derived pattern as part of the BaseValuesViewset refactor (#14036). The attendance app postdates #14036's original scoping, so it's missing from the affected-viewsets table and sub-issues. As the only issue touching this app, it also moves the code into kolibri/core/attendance/viewsets/ and deletes the old modules, per the File Organization convention.

Complexity: Medium
Target branch: develop

Context

Both viewsets live in kolibri/core/attendance/api.py, with AttendanceRecordSerializer and AttendanceSessionSerializer in serializers.py:

• AttendanceSessionViewSet(ValuesViewset) has serializer_class = AttendanceSessionSerializer plus an explicit values tuple including the present_count/total_count annotations and the date_created/date_modified model fields — none of which are serializer fields. Conversely, the serializer's nested attendance_records (write path) is excluded from values.
• AttendanceRecordViewSet(ReadOnlyValuesViewset) has no serializer_class at all — reads are driven purely by its values tuple, with user_name/user_username annotations.
• Both viewsets have custom actions (recent, bulk_update) that call queryset.values(*self.values) directly and build their own Response, bypassing the standard serialize() path.

The Change

Make the serializers the source of truth for the read path:

• Extend AttendanceSessionSerializer with the read-path fields currently only in values (present_count, total_count, date_created, date_modified), and mark the nested attendance_records write-only so the read output is unchanged.
• Define a read serializer for AttendanceRecordViewSet covering its current output (id, user, present, attendance_session, user_name, user_username).
• Remove the explicit values tuples, and rework the recent and bulk_update actions to serialize via the standard path rather than queryset.values(*self.values).

See #14036 for the overall pattern and FacilityUserViewSet for a concrete example (currently in kolibri/core/auth/api.py; moving to kolibri/core/auth/viewsets/facility_user.py in #14817).

Then relocate: migration commits first, in place; then a pure-move commit creating kolibri/core/attendance/viewsets/ and moving both viewsets with their serializers inline (plus filters and permission classes). Update imports (api_urls.py, tests) and delete api.py and serializers.py.

Moving code breaks GitHub blame (local git blame -C -C still works), so digest the history the move obscures:

1. Run blame over the code being moved; flag hunks whose purpose isn't evident from the code or tests (odd guards, magic orderings, special cases).
2. Chase each flagged hunk to its originating PR/issue; where the rationale is non-obvious and load-bearing, carry it forward as a code comment citing the reference.
3. Where the rationale turns out to be obsolete, remove the code as part of the migration commits rather than annotating it.

Out of Scope

• Changes to the API response shape — the API contract must not change
• Modifications to existing tests — if tests need to change, the migration is wrong
• Write-path behavior changes — create/update/bulk_update semantics stay identical; only bulk_update's response serialization is reworked

Acceptance Criteria

• Explicit values tuples removed from both viewsets
• AttendanceSessionSerializer carries all read-path fields, with attendance_records write-only; AttendanceRecordViewSet has a read serializer
• recent and bulk_update serialize via the standard path, with unchanged response output
• Both viewsets, their serializers (inline), filters, and permission classes live in kolibri/core/attendance/viewsets/; api.py and serializers.py are deleted
• Migration commits precede the pure-move commit, which is separate from import updates
• Existing API tests pass unchanged
• Benchmark comparison passes for both viewsets (data hash match + no performance regression)
• Comments citing the originating PR/issue added where blame digestion found non-obvious, load-bearing rationale

Testing

• Existing coverage is good (40 API tests in kolibri/core/attendance/test/test_api.py); they must pass before and after without modification.
• Run the benchmark for both viewsets, baseline with the old dotted path, comparison with the new one:

  python integration_testing/scripts/viewset_serialization_benchmark.py \
      kolibri.core.attendance.api.AttendanceSessionViewSet \
      --inherit-kolibri-home -o attendance_session_baseline.json
  
  # ... migrate and move ...
  
  python integration_testing/scripts/viewset_serialization_benchmark.py \
      kolibri.core.attendance.viewsets.<module>.AttendanceSessionViewSet \
      --inherit-kolibri-home --compare attendance_session_baseline.json

  Likewise for AttendanceRecordViewSet. Both comparisons must pass; include the before/after output in the PR description.
• Benchmark data setup: populate users via kolibri manage generateuserdata, then create attendance data in kolibri manage shell:

  from django.utils import timezone
  
  from kolibri.core.attendance.models import AttendanceRecord
  from kolibri.core.attendance.models import AttendanceSession
  from kolibri.core.auth.models import Classroom
  from kolibri.core.auth.models import FacilityUser
  
  classroom = Classroom.objects.first()
  coach = FacilityUser.objects.first()
  learners = list(FacilityUser.objects.filter(memberships__collection=classroom))
  for i in range(20):
      session = AttendanceSession.objects.create(
          collection=classroom,
          created_by=coach,
          session_start_datetime=timezone.now(),
      )
      for user in learners:
          AttendanceRecord.objects.create(
              attendance_session=session,
              user=user,
              present=bool(i % 2),
          )

AI usage

Drafted with AI assistance (Claude Code) from an interactive design discussion, section by section, with review and final edits by @rtibbles. Viewset internals, serializer contents, custom-action serialization paths, and test coverage were verified against the codebase during drafting.

[rtibblesbot]

Watching for resolution on behalf of #14302.

[rtibblesbot]

NEEDS_HELP

Tasks 0–5 are implemented and committed. Task 6 (benchmark comparison) is partially complete — the session viewset passes but the record viewset fails the 5% timing threshold.

Session viewset benchmark: PASS

Time (mean)    8.542 ms  →  7.058 ms    -17.4%   PASS
Memory (mean)  223.0 KB  →  222.7 KB     -0.2%   PASS
DB Queries          1   →       1          +0    PASS
Data output                             match    PASS
OVERALL VERDICT: PASS

Record viewset benchmark: FAIL

Time (mean)   25.041 ms  →  27.068 ms    +8.1%   FAIL (> 5.0%)
Memory (mean)  2165.8 KB →  2246.2 KB    +3.7%   PASS
DB Queries          1   →       1          +0    PASS
Data output                              match    PASS
OVERALL VERDICT: FAIL

Root cause

The two baselines are not comparing equivalent patterns:

• Session baseline was captured after the serializer migration (Task 2, has_explicit_values: false) but before the code move. The comparison is serializer-derived → serializer-derived (same pattern, different module) → PASS
• Record baseline was captured before the serializer migration (Task 3, has_explicit_values: true). The comparison is explicit-values → serializer-derived (different patterns) → FAIL

The 8.1% overhead is not from the code move — it's inherent to the explicit-values→serializer-derived migration for the record viewset. The cause is architectural: the old explicit-values path with an empty field_map uses _serialize_flat → list(queryset.values(...)) directly (zero per-row overhead). The new serializer-derived path always has a non-trivial field_map (due to user_name/user_username renames from source= traversal), so map_row builds a new dict per row via dict comprehension — 2395 new dicts for this dataset.

This overhead cannot be eliminated within the serializer-derived approach without changes to the ValuesViewset infrastructure (e.g., optimizing the noop check to allow trivial annotation fields). Making user_name and user_username annotation-based (rather than source= traversal) would still call CharField.to_representation per record, keeping is_noop() False.

Question

Is the 8.1% timing regression acceptable for the record viewset given that:

1. Data output is identical (hash match)
2. Query count is unchanged (1 query)
3. The regression comes from the migration pattern itself, not the code move
4. In absolute terms it's ~2ms per batch of 2395 records

If acceptable, should we use --time-threshold 10 for the record comparison, or document the regression explicitly?

Alternatively, if the baseline should have been captured post-Task 3 (from api.py after serializer migration but before move — now deleted), should we re-run the record baseline using the new viewset path to get a same-pattern comparison?

[rtibbles]

Could we change how the core ValuesViewset handles simple renames? Instead of doing them in Python - could they instead be done as annotations with F statements in the Django query, and then they get returned in values?

[rtibblesbot]

Implemented in cff706c405 (part of PR #14844).

In _serialize_flat, before calling .values(), serializer-derived viewsets now pre-annotate plain source→target renames as SQL F() aliases. The queryset returns rows already keyed by the target names, so _field_map.is_noop() becomes true and the per-row map_row() dict comprehension is skipped entirely.

Two correctness guards are applied: sources shared by more than one rename entry are excluded (to avoid the losing entry reading a key that no longer exists in the values tuple), and only the rename-recipient SourceFieldEntry is rewritten to a trivial passthrough — not passthrough entries that happen to share the same source column.

Result for the AttendanceRecord viewset (2395 records): +8.1% timing regression against the explicit-values baseline became −29.0% improvement. PR description has the full benchmark table.

[rtibblesbot]

The bot has been waiting for a response for 2 hour(s). It will resume automatically when someone replies.