feat(invitations): complete external ID bulk invite flow and redesign result dialog#8406
Open
LWS49 wants to merge 2 commits into
Open
feat(invitations): complete external ID bulk invite flow and redesign result dialog#8406LWS49 wants to merge 2 commits into
LWS49 wants to merge 2 commits into
Conversation
c0dcc05 to
e892fd9
Compare
d6e1808 to
b75a804
Compare
a037a06 to
023720d
Compare
9c8e986 to
007edda
Compare
…itations
Allows institutions to link Coursemology enrolments to their own student
records or LMS identifiers. The field is stored on CourseUser and
Course::UserInvitation and validated unique per course across both tables
via a cross-table concern and partial DB index - a pending invitation holds
its ext_id until confirmed, preventing collisions with direct enrolments.
Surfaces:
- Individual invite form: ext_id input field
- Bulk CSV invite: ext_id column in both template variants (with and without
Timeline column); set on new records only - existing pending invitations
and enrolled users are read-only in this flow
- Manage users table: inline editable ext_id column (always visible)
- Score summary export: ext_id column included when any student has one
- Statistics > Students table: ext_id column, sortable and searchable,
shown only when at least one student has a non-null ext_id
View-only tables suppress the ext_id column when no course members have
one set, consistent with how group manager, gamification, and video columns
are conditionally shown. Edit tables always show it.
Also changes error responses from the invitations controller from a single
concatenated string to an array of per-record strings, enabling the frontend
to render overflow counts without truncating meaningful error detail.
ed5418b to
0ef7442
Compare
… dialog - add ext_id to CourseUser and UserInvitation with unique-per-course index - accept ext_id in bulk CSV and individual invite form; upsert for existing records (enrolled users and pending invitations) - conflicts (duplicate email or ext_id) surface in Failed with reasons - redesign result dialog: replace updated chip with row tint and tooltip - show ext_id in manage users table, statistics students table, and score export
0ef7442 to
62635b4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes the external ID bulk invite flow started in the previous PR. The CSV upload now acts as a full upsert for external IDs on existing records: a nil ext_id is filled in, a non-nil ext_id that differs from the CSV value is overwritten (not rejected), and a blank CSV cell preserves the existing value. The
external_id_conflicterror condition is removed - the only remaining hard rejection for ext_id isexternal_id_taken(the CSV value is already held by a different course member). The invite result dialog is redesigned: six conditional flat sections replace the previous accordion layout, updated rows fold into the Existing Invitations and Existing Course Users sections with a bold ext_id and old-value tooltip, and the separate "External IDs Updated" section is removed. Invitations withis_retryable: false(permanent email delivery failures) are split out of Existing Invitations into a dedicated Failed section, highlighted in red, with a caption identifying exactly which rows could not be sent.Design decisions
external_id_taken(ID collision across different people), which is still a hard error. Flagging a staff member correcting a previous ext_id entry as a conflict imposes workflow cost with no safety gain beyond whatexternal_id_takenalready provides. Nil-to-value and non-nil-to-different-value are now treated symmetrically.freed-idtonew-idand Bob claimsfreed-idin the same batch, Alice's update must persist first so Bob's insert doesn't hit the unique constraint.is_retryable: falseinvitations routed to Failed - Invitations that permanently failed to send appeared under Existing Invitations on master, indistinguishable from normal pending invitations. Since those emails will never arrive, treating them as "already invited" misleads the admin. They are now highlighted in red in the Failed section and excluded from the "already in course" summary count. A red caption subtitle - "N rows highlighted in red could not be sent" - explicitly names the highlight so admins know which rows need action without hover interaction. TheHelpIcontooltip was replaced with the subtitle because the header count includes both failure types; a tooltip on that header would mislead staff into thinking it applied to all rows, not just the red subset.Design Considerations
Failed section: subtitle vs HelpIcon for red-highlighted rows
Decision: Add a
Typography variant="caption" color="error"subtitle below the "Failed (N)" header, shown only whenisRetryable === falserows exist. Remove theHelpIcontooltip from the section header.Why considered: After consolidating all failure types into one "Failed" section, the HelpIcon tooltip sat next to a count that included CSV-issue rows too - implying it applied to all of them. Non-technical staff would read "Some invitation emails failed to send - please try again" and not know whether to retry all rows or just the red ones.
For:
failed_to_sendsubset only, not the totalneedsAttentionCountupdatedSubtitlecaption pattern already established in the same componentAgainst:
Alternatives:
HelpIconwith updated tooltip text covering both row types conditionally - rejected because conditional tooltip text is harder to maintain and staff may not hover the iconRationale: I chose the subtitle over the HelpIcon because the HelpIcon was anchored to a count that included both failure types, making the tooltip misleading for CSV-issue rows. The subtitle count is scoped to the
failed_to_sendsubset only, making the connection between the red highlight and the meaning explicit without hover interaction. The action guidance ("could not be sent") is intentionally neutral - the invitations page has no current retry path for permanently failed rows, and I did not want to imply one exists.Service behavior: master vs this PR
Ext_id is checked at four points in the service pipeline.
@taken_external_idsis pre-loaded from bothCourseUserand unconfirmedCourse::UserInvitationext_ids, so a conflict with either type anchors the same rejection.@taken_external_ids; conflict -external_id_takenfailureenroll_new_userexternal_id_takenfailureexternal_id_takenfailureFailed section: case matrix
failed_to_sendrowsCross-type freed-ID ordering limitation
When an existing course user frees an ext_id and a brand-new user (no Coursemology account) claims it in the same batch, the new user is still rejected with
external_id_taken.invite_new_usersruns beforeadd_existing_usersin the pipeline, so the new invitation is evaluated before the existing course user releases the ID. The same-type freed-ID case (both are existing invitations, or both are course users) works correctly. The spec documents this behavior explicitly rather than asserting it should work - a two-pass fix would add complexity for a narrow edge case.Regression prevention
Service specs cover: nil-to-value upsert for invitations and course users; non-nil-to-different-free-value upsert with
previous_external_idcapture;external_id_takenon new invitations, new enrollments (existing user not yet in course), existing invitation updates, and existing course user updates; conflict anchored by course user ext_id vs invitation ext_id; freed-ID batch claim within the same record type; cross-type freed-ID ordering limitation; unconfirmed-email path sends invitation not direct enrollment;isRetryable: falsepropagated through jbuilder.Frontend specs cover:
InvitationResultSkippedTableupdated-row rendering (bold + badge + tooltip);InvitationResultDialog- failed invitations in Failed section not Existing Invitations,failedRowsSubtitleshown only forfailed_to_sendrows with correct count, subtitle absent when only CSV-issue duplicates present, mixed-failure count correctly split between header and caption;InvitationResultUsersTablereason text for all four failure reasons.Manual testing: happy path new batch; nil-to-value upsert with badge; non-nil-to-different upsert with tooltip;
external_id_takenin Failed section; freed-ID same-type batch; existing user no-change; failed-to-send rows in red with caption; CSV-issue rows without caption; mixed failure section showing correct split counts.Backward compatibility: existing invite flows unaffected. The only behavioral change for existing data is that re-inviting a user whose ext_id differs from the CSV value now updates the record instead of rejecting it.
Current Result Display
Happy Path
All Failure Reasons Shown