Add an admin-only mass delete student progress UI #69979
Conversation
| end | ||
| end | ||
|
|
||
| # Path to the delete script |
There was a problem hiding this comment.
I wanted to leverage the existing script - I will be honest, I relied on ChatGPT for this part and then just tested it afterward.
|
|
||
| // Parse header row | ||
| const headers = lines[0] | ||
| .split(',') |
There was a problem hiding this comment.
CSV files can sometimes use other characters as separators (I think I've seen semicolons and possibly tabs).
I think this works as a hack, however could you test to see if you could use papaparse, which already looks installed and if that doesn't work or gets too hairy just add a comment about the specific CSV format we need.
There was a problem hiding this comment.
Oh, great catch! I will make that change to use papaparse
|
|
||
| // Determine if conversion is needed | ||
| if (hasUsernameColumn(parsedData)) { | ||
| setHasUsernames(true); |
There was a problem hiding this comment.
Even auto-checking whether it has usernames or not. Beautiful!
| end | ||
|
|
||
| # POST /admin/convert_usernames_to_ids | ||
| def convert_usernames_to_ids |
There was a problem hiding this comment.
Do you know if there are any limits for parameter size or rails endpoints? I know I've seen some massive CSVs, I wonder if those will fail.
Again, probably not something to worry about, but we could put a note on the page.
There was a problem hiding this comment.
Yeah... I also thought about this... I did some testing with 100 students, but only for converting usernames_to_ids.
Let me look into the best way to test this...
There was a problem hiding this comment.
Okay Dokie.... I looked into this the TINIEST bit but ultimately put it as a follow up.
|
|
||
| const openingDirections = ( | ||
| <div> | ||
| <h1>Mass Delete Student Progress</h1> |
There was a problem hiding this comment.
What do you think about adding a link to our manual instructions so that they are easy to find from here?
There was a problem hiding this comment.
Like it! Added.
|
|
||
| if error_message | ||
| temp_input.unlink | ||
| render json: {success: false, error: error_message}, status: :bad_request |
There was a problem hiding this comment.
optional nit: This fails if we fail on any student. Do we want to return a partial success with the error message filled out? e.g. successfully converted X records, failed on 2 students: studentX, studentY
There was a problem hiding this comment.
I think we absolutely do want to do this. I think this area in general is "up next" for improvement.
| end | ||
|
|
||
| # POST /admin/convert_usernames_to_ids | ||
| def convert_usernames_to_ids |
There was a problem hiding this comment.
Unfortunately I think these BE endpoints might deserve some tests. I'm fine merging now and adding later if you would prefer though.
There was a problem hiding this comment.
Yep, makes sense... I will add it to the follow-up sections of this PR.
| end | ||
|
|
||
| # POST /admin/delete_user_progress | ||
| def delete_user_progress |
There was a problem hiding this comment.
Just to confirm, this only works if the teacher specifies the unit_name and does not copy the delete_user_progress_all_units script?
There was a problem hiding this comment.
Correct, currently the uploaded csv requires a username. I asked Andrew what the more common request was and he said "delete by unit" so I started there.
There was a problem hiding this comment.
Could we put a note on the page that all unit deletion still needs the manual steps.
| setCsvData(parsedData); | ||
|
|
||
| // Determine if conversion is needed | ||
| if (hasUsernameColumn(parsedData)) { |
There was a problem hiding this comment.
| if (hasUsernameColumn(parsedData)) { | |
| if (hasUsernameColumn(parsedData) && !hasUserIds) { |
nit: what if a csv has userIds and usernames? Conversion should work fine in that case, but we wouldn't necessary need to convert before.
There was a problem hiding this comment.
Correct... however, I don't see a scenario where we have userIds already as they are so opaque to users.
|
|
||
| const deleteProgress = async (isDryRun = true) => { | ||
| try { | ||
| const response = await fetch('/admin/delete_user_progress', { |
There was a problem hiding this comment.
nit: Similar to isConverting, we may want a loading state/to disable the button so that we can't call this deleteProgress method multiple times before it completes.
There was a problem hiding this comment.
100% that is part of my follow ups too.
Initial start to an admin page for deleting user progress.
Screen.Recording.2025-12-11.at.10.56.34.AM.mov
Links
Testing story
Follow ups