Pass cuds through session storage to fix issue #2284
Conversation
📝 WalkthroughWalkthroughThe changes rework how missing student IDs are handled during submission creation. The previous approach, which passed missing IDs as URL parameters, has been replaced with a session storage mechanism. A new JavaScript function in the missing submissions view collects the IDs and stores them, while the submission creation view now retrieves these stored IDs during the page load, adjusts the form elements accordingly, and updates the URL with the retrieved IDs. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as Missing Submissions Page
participant JS as JavaScript
participant S as sessionStorage
participant N as New Submission Page
U->>M: Click "Fill In Empty Submissions"
M->>JS: Trigger submitMissingIds()
JS->>S: Store missing student IDs (JSON)
JS->>M: Redirect to submission creation page
U->>N: Load New Submission Page
N->>JS: DOMContentLoaded event triggers
JS->>S: Retrieve stored IDs
JS-->>N: Update input field, toggle display, modify URL with IDs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 0
🧹 Nitpick comments (3)
app/views/submissions/missing.html.erb (1)
18-25: Well-implemented session storage solutionThe
submitMissingIdsfunction effectively collects all missing student IDs, stores them in session storage as JSON, and redirects to the submission creation page. This is a good approach to handle potentially large data sets that would exceed URL parameter or cookie size limitations.A few suggestions for improvement:
- Consider adding error handling in case the JSON stringification fails
- Add a user feedback mechanism (like a loading spinner) during the redirect process, especially for large datasets
function submitMissingIds() { var ids = [<%= @missing.map { |u| u[:id] }.join(',') %>]; var url = '<%= new_course_assessment_submission_path(@course, @assessment) %>'; + try { sessionStorage.setItem('course_user_datum_ids', JSON.stringify(ids)); + // Optional: Show loading indicator for better UX + // document.getElementById('loading-indicator').style.display = 'block'; window.location.href = url; + } catch (e) { + alert('Failed to process student IDs. Please try with fewer students or contact support.'); + console.error(e); + } }app/views/submissions/new.html.erb (2)
19-37: Well-structured session storage retrieval mechanismThis code effectively retrieves the IDs stored in session storage, updates the form, and modifies the UI accordingly. The approach is sound and addresses the PR objectives.
A few observations:
- Line 35 updates
window.location.hrefwhich causes a page reload after the DOM has already been manipulated. This could lead to a flash of content where users briefly see the form before redirect.- The code does not handle potential JSON parsing errors.
- The code does not validate that the retrieved IDs are valid before using them.
document.addEventListener('DOMContentLoaded', function() { var storedIds = sessionStorage.getItem('course_user_datum_ids'); if (storedIds) { sessionStorage.removeItem('course_user_datum_ids'); + try { var ids = JSON.parse(storedIds); + if (!Array.isArray(ids) || ids.length === 0) { + console.error('Retrieved IDs are not in expected format:', ids); + return; + } document.querySelector('input[name="submission[course_user_datum_id]"]').value = ids.join(','); var studentSelectArea = document.getElementById('student_select_area'); if (studentSelectArea) { studentSelectArea.style.display = 'none'; var storedIdsArea = document.getElementById('stored_ids_area'); if (storedIdsArea) { storedIdsArea.style.display = 'block'; + // Optionally: Show a count of selected students + storedIdsArea.textContent = ids.length + ' students selected'; } } - window.location.href = window.location.pathname + '?course_user_datum_id=' + ids.join(','); + // Use history.replaceState instead of location.href to avoid page reload + if (!window.location.search.includes('course_user_datum_id=')) { + var newUrl = window.location.pathname + '?course_user_datum_id=' + ids.join(','); + history.replaceState(null, '', newUrl); + } + } catch (e) { + console.error('Error processing stored IDs:', e); + } } });
40-54: Clear UI structure for showing student selection or stored IDsThe UI structure is well-organized with appropriate conditional rendering. The added
stored_ids_areadiv provides a designated place to show information about the selected students from session storage.Consider enhancing this section to:
- Show the number of selected students in the stored_ids_area
- Allow users to clear the selection and return to individual student selection
<% if params[:course_user_datum_id].nil? %> <div id="student_select_area"> <div class="input-field"> <input type="text" size="3" id="student_autocomplete" class="autocomplete" autocomplete="off"> <label for="student_autocomplete">Start typing student name or email</label> </div> <%= f.hidden_field(:course_user_datum_id) %> </div> <div id="stored_ids_area" style="display: none;"> + <div>Multiple students selected (<span id="student-count">0</span>)</div> + <a href="#" onclick="clearSelection(); return false;" class="btn-small">Clear Selection</a> </div> <% else %> <%= @cuds.collect(&:email).join(", ") %> <input type='hidden' name='submission[course_user_datum_id]' value='<%= @cuds.collect(&:id).join(',') %>'> <% end %>Add this JavaScript function to handle clearing the selection:
function clearSelection() { document.querySelector('input[name="submission[course_user_datum_id]"]').value = ''; document.getElementById('student_select_area').style.display = 'block'; document.getElementById('stored_ids_area').style.display = 'none'; history.replaceState(null, '', window.location.pathname); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/views/submissions/missing.html.erb(1 hunks)app/views/submissions/new.html.erb(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: test
🔇 Additional comments (2)
app/views/submissions/missing.html.erb (1)
14-16: Good change to prevent URL parameter overflowsThis change replaces a direct URL with many parameters to an onclick handler that will use session storage instead. This addresses the cookie overflow issue mentioned in PR #2274 by avoiding the URL parameter size limitations when dealing with a large number of student IDs.
app/views/submissions/new.html.erb (1)
15-17: Improved condition check with blank? instead of nil?Good change from
nil?toblank?which is more robust as it will handle empty strings and other falsy values in addition to nil.
KesterTan
requested review from
a team and
coder6583
and removed request for
a team
2 participants
Description
Passing cuds through session storage prevents the cookie that the cuds were previously passed through from overflowing.
Motivation and Context
Resolves issue #2274
How Has This Been Tested?
Tested locally by creating missing submissions for >400 students.
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.