Skip to content

Conversation

@yubimamiya
Copy link

  • Update add player code and User database to save information on each player's full name and class year in the User Database during CAS authentication
  • Change top player functions to return Full Name (Class Year) instead of netid for players so that this is reflected in leader board of TigerSpot UI
  • I was unable to test the full app due to local machine issues so please test with full app before deploying

Yubi Mamiya and others added 4 commits October 26, 2025 14:45
…ON of user information and update app.py to parse JSON user_info correctly for username
…layers and insert_player function to include display_name and year
…ctions to return display_name with year instead of username (with the same json name of username)
@linear
Copy link

linear bot commented Nov 4, 2025

@Ammaar-Alam
Copy link
Member

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +154 to +159
# YUBI, ASK: does users return the whole row of information including display name and year?

for user in users:
player_stats = {"username": user.username, "points": user.points}
# YUBI: replace username with display name and year
# Keep reference as username, but want to change this to name later
player_stats = {"username": user.display_name + " (" + user.year + ")", "points": user.points}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Guard against null display_name/year when formatting leaderboard names

In the total leaderboard, get_top_players now returns user.display_name + " (" + user.year + ")". Existing users rows created before this migration have NULL values in the new columns because insert_player skips updates for existing users, so concatenation raises TypeError: can only concatenate str (not "NoneType"). Any legacy account appearing in the leaderboard will crash the view until its record is manually updated. Consider backfilling or defaulting missing fields before concatenation.

Useful? React with 👍 / 👎.

Comment on lines 291 to 295
for user in users:
player_stats = {"username": user.username, "points": user.points}
# YUBI: update player_stats to show display_name concatenated with year
# I have replaced username with the name, but kept the json reference as username for now
player_stats = {"username": user.display_name + " (" + user.year + ")", "points": user.points}
daily_top_players.append(player_stats)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Daily leaderboard also concatenates potentially null display metadata

get_daily_top_players builds player_stats using user.display_name + " (" + user.year + ")". Older usersdaily entries created before this migration never populate these new columns—the migration adds nullable fields and insert_player_daily returns immediately when a record already exists—so the code will hit the same TypeError for any legacy daily record, breaking /leaderboard until data is cleaned. Use null-safe defaults or populate the fields when upgrading existing rows.

Useful? React with 👍 / 👎.

Copy link
Member

@Ammaar-Alam Ammaar-Alam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see codex comments

Copy link
Member

@joshuamotoaki joshuamotoaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The display_name doesn't appear to be working (it just gives me back my netid). I'd really make sure to run this locally, ask @joshuamotoaki if you need help.

Edge case that needs to be considered -- a lot of users are already in the database. These users need to be updated when they log in again to add the display_name + year. If a user is already signed in, if these don't exist in the session, you might need to force a re-auth (ideally, you don't need to force a sign out, but if this is what's necessary then it's necessary).

@Ammaar-Alam
Copy link
Member

CleanShot 2025-11-04 at 20 55 07@2x CleanShot 2025-11-04 at 20 55 27@2x

played around with it a little and was able to get it to atl populate names, can push if necessary but @yubimamiya or @joshuamotoaki should be able to takeover; codex found the problems as:

  • Identity logic broke (e.g., routes comparing netids now compared against “First Last (Year)” strings).
  • Existing users had NULL display_name/year; concatenations like user.display_name + " (" + user.year + ")" raised TypeError.
  • Session cache kept an initial netid “displayName”, preventing refresh.

so to get it working i had to:

  1. Keep identity separate from presentation
  • user_database.py, daily_user_database.py
    • get_top_players() / get_daily_top_players() / get_top_player():
      • Now return both:
        • username = netid (for logic/identity)
        • display_name = “Full Name (Year)” if available, else falls back to display_name or username
      • Avoids every crash and preserves identity semantics.
  1. Upsert names on login for existing users (no more NULLs)
  • user_database.py: insert_player(username, display_name, year)
  • daily_user_database.py: insert_player_daily(username, display_name, year)
    • Both now upsert: if user exists, update display_name/year.
  • app.py:
    • On /menu (existing) and now also /leaderboard and /totalboard, call both upserts so landing there first still updates names.
  1. Render the right field in UI
  • templates/leaderboard.html and templates/totalboard.html: use player['display_name'] for the name column.
  • templates/menu.html: greet with {{display_name or username}}.
  1. Fix CAS parsing (root cause for your account)
  • src/CAS/auth.py:
    • Case‑insensitive attribute map.
    • Name selection priority:
      • pudisplayname → displayname → givenname + sn
      • Ignore cn (it equals netid in your data).
      • Final fallback is AD lookup, then netid (not needed for you once CAS fields are read correctly).
    • Session refresh: if cached “display name” looks like a netid, drop cache to re-validate.
    • Debug logs (enabled with DEBUG_CAS=1) to print attribute keys and sample values — you confirmed pudisplayname, displayname, givenname, sn exist in your
      CAS response.
  1. Optional AD fallback (not needed once CAS parsing is correct)
  • Added AD API fallback to pull displayname from Princeton Active Directory if CAS lacks a name.
  • Supports either a direct Bearer token (AD_API_TOKEN) or OAuth Client Credentials (AD_CONSUMER_KEY/SECRET).
  • This was to future‑proof and handle accounts where CAS doesn’t release names; for your case, CAS provides them.

TLDR ish
Summary

  • Display names didn’t show because:
    • Identity/presentation were conflated (UI replaced username with a display string).
    • Existing users had NULL display_name/year, causing string concat crashes.
    • Session cached an initial netid “displayName”, so updates didn’t refresh.
    • CAS parsing looked for displayName (camelCase) while CAS returns pudisplayname/displayname/givenname/sn (lowercase) for this app.

Changes Needed

  • Keep identity vs. presentation separate
    • username stays netid.
    • Add/return a distinct display_name for UI.
    • Files:
      • src/Databases/user_database.py
        • get_top_players(), get_top_player() return:
          • username = netid
          • display_name = “Full Name (Year)” when available; fallback to display_name or username.
      • src/Databases/daily_user_database.py
        • get_daily_top_players() same shape and fallback.
  • Upsert names on login (avoid NULLs for existing users)
    • Files:
      • src/Databases/user_database.py:insert_player(username, display_name, year) → upsert (update if exists).
      • src/Databases/daily_user_database.py:insert_player_daily(username, display_name, year) → upsert.
      • app.py:
        • After auth.authenticate() on /menu, and also on /leaderboard and /totalboard, call both upserts.
  • Render the right field in UI
    • Files:
      • templates/leaderboard.html:57 → player['display_name']
      • templates/totalboard.html:57 → player['display_name']
      • templates/menu.html greeting → {{ display_name or username }}
  • Fix CAS parsing (root cause)
    • File: src/CAS/auth.py:
      • Build a case‑insensitive map of CAS attributes.
      • Name selection priority:
        • pudisplayname → displayname → givenname + sn (ignore cn because it equals netid here).
        • Final fallback is AD lookup (optional), then netid.
      • Force session refresh if cached display looks like a netid.
      • With DEBUG_CAS=1, logs print keys; we confirmed pudisplayname, displayname, givenname, sn exist for this app.

Why This Works

  • Preserves invariants: logic compares netids; UI shows human names.
  • Eliminates NULL concat failures via upsert and safe fallbacks.
  • Parses the actual CAS fields this application receives (lowercase keys).
  • Optional AD fallback covers accounts without CAS names (not required here once CAS parsing is corrected).

Test Plan

  • DB: verify upsert after login
    • SELECT username, display_name, year FROM users WHERE username='<your_netid>';
    • Expect display_name='First Last', year='2027'.
  • UI:
    • /leaderboard and /totalboard should show “First Last (2027)” rows.
  • Session:
    • If you see netid, hit /logoutcas and log back in; session drops stale display and re-parses CAS.

@Ammaar-Alam Ammaar-Alam force-pushed the main branch 4 times, most recently from b1cf4b9 to 76b4ad8 Compare November 12, 2025 19:10
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.

4 participants