Skip to content

Fix/lti link issue from csv import#1110

Merged
catinhere merged 3 commits into
masterfrom
fix/lti-link-issue-from-csv-import
May 25, 2026
Merged

Fix/lti link issue from csv import#1110
catinhere merged 3 commits into
masterfrom
fix/lti-link-issue-from-csv-import

Conversation

@catinhere
Copy link
Copy Markdown
Contributor

Users imported via CSV classlist (CAS/SAML type) were created without global_unique_identifier populated. When these users later attempted to log in via LTI, two things could happen:

  1. LTIUser could not match the existing user by global_unique_identifier (it was NULL)
  2. If the LTI consumer provided a student_number, attempting to create a new user record would fail with a database integrity error due to the unique constraint on student_number

This resulted in an internal server error on LTI login.

Changes
classlist.py — Root cause fix. When importing users via CSV with CAS/SAML type, set global_unique_identifier to the CAS/SAML identifier at creation time. Prevents the problem for all future imports.

lti_models/lti_user.py — Fallback for existing affected users. During LTI login, if a user cannot be matched by global_unique_identifier, attempt to match by
student_number. If found, link the LTI user to the existing record and backfill global_unique_identifier.

tests/test_models.py — Adds a test case covering the fallback: a SAML user with a student_number but no global_unique_identifier is correctly linked when logging in via LTI, no duplicate user is created, and global_unique_identifier is backfilled.

Notes

  • Existing users affected by this issue (created before this fix) will be automatically remediated on their next LTI login via the student_number fallback.
  • Grades for any users with duplicate records can be recalculated after merging via flask grades generate --course-id .

catinhere added 2 commits May 25, 2026 12:59
…king student number

Users created via csv import did not populate the global_unique_identifier column in user table. If user logs in via LTI, lti_user unable to link to existing record in user table. It's also unable to create a new user record due to duplicate student number.
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 25, 2026

Greptile Summary

This PR fixes a two-part bug where users imported via CSV with CAS/SAML type were created without global_unique_identifier populated, causing LTI login failures (match miss + duplicate-user integrity error) for those accounts.

  • classlist.py: Sets global_unique_identifier = username at creation time for new CAS/SAML users, and backfills it for existing users (matched by username) who are missing it on re-import.
  • lti_models/lti_user.py: Adds a student_number-based fallback in generate_or_link_user_account so already-affected users can log in via LTI; only backfills global_unique_identifier when the matched user's field is None, preventing accidental overwrite of a legitimate existing identifier.
  • tests/test_models.py: Adds two test cases covering the new fallback path — one verifying the backfill when the identifier is absent, and one verifying the existing identifier is preserved when it's already set.

Confidence Score: 5/5

Safe to merge; the fallback logic is guarded correctly and the root-cause fix prevents recurrence for all new CSV imports.

The root-cause fix (setting global_unique_identifier at import time) is straightforward and correct. The LTI fallback guards the backfill with an explicit is None check, addressing the concern raised in the prior review thread. Two new tests verify the primary happy path and the no-overwrite edge case. The one known gap (student_number-matched path in the CSV import not backfilling the identifier) was flagged in a previous review comment and does not introduce new breakage.

The student_number-match branch in classlist.py (around the continue at line 240) still skips the backfill for existing users matched by student number rather than by username; this was noted in a prior review comment.

Important Files Changed

Filename Overview
compair/models/lti_models/lti_user.py Adds student_number fallback in generate_or_link_user_account; correctly guards backfill with is None check to avoid overwriting existing identifiers.
compair/api/classlist.py Backfills global_unique_identifier for new and username-matched CAS/SAML users; student_number-matched path (line 240 continue) still skips the backfill — already flagged in a prior review comment.
compair/tests/test_models.py Adds two well-structured test cases covering the backfill and no-overwrite paths; test assertions correctly validate user count, link, and identifier value.

Sequence Diagram

sequenceDiagram
    participant LTI as LTI Consumer
    participant App as ComPAIR (lti_user.py)
    participant DB as Database

    LTI->>App: Login (global_unique_identifier, student_number)
    App->>DB: Lookup User by global_unique_identifier
    DB-->>App: None (user imported via CSV, no GUID set)

    alt student_number provided
        App->>DB: Lookup User by student_number
        DB-->>App: existing_user (matched)
        alt existing_user.global_unique_identifier is None
            App->>DB: Backfill global_unique_identifier
        end
        App->>DB: Link LTIUser to existing_user
    else no student_number or no match
        App->>DB: Create new User
        App->>DB: Link LTIUser to new User
    end

    App-->>LTI: Login successful
Loading

Reviews (2): Last reviewed commit: "Guard global_unique_identifier overwrite..." | Re-trigger Greptile

Comment thread compair/models/lti_models/lti_user.py
@catinhere
Copy link
Copy Markdown
Contributor Author

@greptileai

@catinhere catinhere merged commit f59e5bd into master May 25, 2026
12 checks passed
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