Skip to content

show multiple talks badge#4557

Merged
patrick91 merged 1 commit intomainfrom
show-multiple-talks-badge
Feb 4, 2026
Merged

show multiple talks badge#4557
patrick91 merged 1 commit intomainfrom
show-multiple-talks-badge

Conversation

@patrick91
Copy link
Member

image

@vercel
Copy link

vercel bot commented Feb 4, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
pycon Ready Ready Preview Feb 4, 2026 6:14pm

@claude

This comment has been minimized.

@codecov
Copy link

codecov bot commented Feb 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.16%. Comparing base (ed679d2) to head (8a551b6).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4557   +/-   ##
=======================================
  Coverage   94.16%   94.16%           
=======================================
  Files         353      353           
  Lines       10346    10347    +1     
  Branches      761      761           
=======================================
+ Hits         9742     9743    +1     
  Misses        502      502           
  Partials      102      102           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude
Copy link
Contributor

claude bot commented Feb 4, 2026

Summary

This PR adds a visual badge to show when speakers have submitted multiple talks to the conference. The badge appears next to the speaker's name in the proposals recap view, displaying the count when a speaker has more than one submission.

Review Feedback

Architecture & Design ✅

Good:

  • Clean separation: query logic in adapter, presentation in template
  • Single responsibility: the count query is focused and simple
  • Appropriate abstraction level: uses existing .for_conference() and .non_cancelled() querysets

Minor consideration:

  • The query at reviews/adapters.py:181-188 loads counts for all speakers in the conference, not just those in the current review session items. This could include speakers not visible on the current page. While not a bug, it's slightly inefficient.

    Consider filtering to only speakers in the current items by adding .filter(speaker_id__in=speakers_ids) after .non_cancelled().

Testing & Coverage ✅

Strong test coverage:

  • ✅ Basic case: single submission per speaker
  • ✅ Multi-submission case: verifies correct counting for 3 submissions
  • ✅ Mixed case: tests both single and multiple submissions in same session
  • ✅ Tests verify both context structure and actual count values

Missing edge case:
Consider adding a test for cancelled submissions to verify they're correctly excluded from the count (since the query uses .non_cancelled()).

Error Handling ✅

Well handled:

  • Template uses |get_item:speaker_id|default:1 fallback for missing counts
  • Conditional check guards against edge cases
  • String conversion with stringformat:"i" ensures consistent key types

Performance ✅

Good:

  • Single aggregation query (no N+1 issue)
  • Uses values()/annotate()/values_list() for efficient grouping
  • Badge only shown when count > 1 (reduces DOM noise)

Already noted: Minor inefficiency loading all speaker counts vs. just current items.


Verdict

Approved with minor optimization suggestion. The implementation is solid, well-tested, and follows good patterns. The suggested filter optimization is optional but would be nice to have for large conferences with many speakers.

@patrick91 patrick91 marked this pull request as ready for review February 4, 2026 18:40
@patrick91 patrick91 merged commit 61861cd into main Feb 4, 2026
8 checks passed
@patrick91 patrick91 deleted the show-multiple-talks-badge branch February 4, 2026 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant