Fixed LTI duplicate roster error #2213
Conversation
WalkthroughWalkthroughThe changes introduce a mechanism for handling warnings during roster uploads in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CoursesController
participant Roster
User->>CoursesController: Upload roster
CoursesController->>Roster: Process roster data
Roster-->>CoursesController: Return warnings/errors
CoursesController->>CoursesController: Check for warnings
alt Warnings exist
CoursesController->>User: Display warnings
end
alt Errors exist
CoursesController->>User: Display errors
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/controllers/courses_controller.rb (4 hunks)
Additional comments not posted (1)
app/controllers/courses_controller.rb (1)
556-559: LGTM: Successfully implemented roster warning systemThe changes successfully implement a warning system for roster uploads, focusing on detecting and reporting duplicate email addresses. This aligns well with the PR objectives of allowing roster synchronization to proceed while alerting users to potential issues.
Key improvements:
- Added a mechanism to collect and store warnings during the roster processing.
- Implemented specific handling for duplicate email warnings.
- Added logic to display warnings to the user after roster upload.
The implementation is sound and doesn't introduce any apparent bugs or issues. However, there are opportunities for minor improvements in naming conventions and error/warning handling, as mentioned in the previous comments.
Overall, this change enhances the user experience by providing more informative feedback during the roster upload process.
Also applies to: 805-805, 953-957, 960-961
| unless @roster_warnings.nil? | ||
| w = @roster_warnings.keys.join('\n') | ||
| flash[:error] = w | ||
| end |
There was a problem hiding this comment.
Consider improving the warning display mechanism
The new warning system is a good addition, but there are a few points to consider:
- Using
flash[:error]for warnings might be confusing. Consider usingflash[:warning]instead to differentiate between errors and warnings. - Joining warning messages with '\n' might not render properly in HTML. Consider using
<br>for line breaks or formatting the warnings as an HTML list. - The success message is still displayed even when there are warnings. Consider adjusting the logic to provide a more nuanced feedback.
Here's a suggested improvement:
if @roster_warnings.present?
flash[:warning] = "Roster uploaded with warnings:"
flash[:warning] += "<ul>"
@roster_warnings.keys.each do |warning|
flash[:warning] += "<li>#{warning}</li>"
end
flash[:warning] += "</ul>"
flash[:partial_success] = "Roster updated, but with some warnings. Please review."
else
flash[:success] = "Successfully updated roster!"
end
flash[:html_safe] = trueThis change separates warnings from errors, formats the warnings as an HTML list, and provides a more accurate success message.
| @@ -798,6 +802,7 @@ def categorize_courses_for_listing(courses) | |||
| def write_cuds(cuds) | |||
| rowNum = 0 | |||
| rosterErrors = {} | |||
| rosterWarnings = {} | |||
There was a problem hiding this comment.
Improve warning handling and follow Ruby conventions
The implementation of the warning system for duplicate emails is a good addition. However, there are a few suggestions for improvement:
- The warning message is hardcoded. Consider making it more descriptive or customizable.
- The warning system currently only handles duplicate emails. It might be beneficial to design it to handle other types of warnings in the future.
- The
rosterWarningsvariable uses camelCase, which is not consistent with Ruby naming conventions.
Here's a suggested improvement:
roster_warnings = {}
# ... (other code)
duplicate_email_msg = "Warning: Duplicate email detected"
if !roster_warnings.key?(duplicate_email_msg)
roster_warnings[duplicate_email_msg] = []
end
roster_warnings[duplicate_email_msg].push(cud)
# ... (other code)
@roster_warnings = roster_warningsThis change uses snake_case for variable naming, makes the warning message slightly more descriptive, and sets up the structure to potentially handle different types of warnings in the future.
Also applies to: 953-957, 960-961
An error occurs when we have a roster with duplicate email addresses, so changed it to only a warning.
Description
Before: When we sync the roster to a LTI course that has duplicate users, an error occured.
After: When we sync the roster, the sync works, but a warning is produced.
Motivation and Context
The LTI Test Course has duplicate users, and the roster sync was failing.
How Has This Been Tested?
I created a new course in my development environment, and linked the LTI test course, and synced the roster.
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for lintingOther issues / help required
If unsure, feel free to submit first and we'll help you along.