-
Notifications
You must be signed in to change notification settings - Fork 214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed LTI duplicate roster error #2213
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -553,6 +553,10 @@ def upload_roster | |
begin | ||
save_uploaded_roster | ||
flash[:success] = "Successfully updated roster!" | ||
unless @roster_warnings.nil? | ||
w = @roster_warnings.keys.join('\n') | ||
flash[:error] = w | ||
end | ||
redirect_to(action: "users") && return | ||
rescue StandardError => e | ||
if e != "Roster validation error" | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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_warnings This 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 |
||
rowCUDs = [] | ||
duplicates = Set.new | ||
|
||
|
@@ -863,7 +868,7 @@ def write_cuds(cuds) | |
new_cud.delete(:year) | ||
|
||
# Build cud | ||
if !user.nil? | ||
if !user.nil? && !existing | ||
cud = @course.course_user_data.new | ||
cud.user = user | ||
params = ActionController::Parameters.new( | ||
|
@@ -945,13 +950,15 @@ def write_cuds(cuds) | |
rowCUDs.each do |cud| | ||
next unless duplicates.include?(cud[:email]) | ||
|
||
msg = "Validation failed: Duplicate email #{cud[:email]}" | ||
if !rosterErrors.key?(msg) | ||
rosterErrors[msg] = [] | ||
msg = "Warning : Duplicate email #{cud[:email]}" | ||
if !rosterWarnings.key?(msg) | ||
rosterWarnings[msg] = [] | ||
end | ||
rosterErrors[msg].push(cud) | ||
rosterWarnings[msg].push(cud) | ||
end | ||
|
||
@roster_warnings = rosterWarnings | ||
|
||
return if rosterErrors.empty? | ||
|
||
@roster_error = rosterErrors | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider improving the warning display mechanism
The new warning system is a good addition, but there are a few points to consider:
flash[:error]
for warnings might be confusing. Consider usingflash[:warning]
instead to differentiate between errors and warnings.<br>
for line breaks or formatting the warnings as an HTML list.Here's a suggested improvement:
This change separates warnings from errors, formats the warnings as an HTML list, and provides a more accurate success message.