-
Notifications
You must be signed in to change notification settings - Fork 568
Moves RosterEntry update to background job #2233
base: master
Are you sure you want to change the base?
Conversation
- Setup AddStudentsToRosterChannel to send progress - channel is called from AddStudentsToRosterJob - progress via action cable is handled by add_students_to_roster.js - removes the old method from model
Curious how large of a CSV would need to be uploaded to see delays. RosterEntry's are very lightweight AR objects, so I'm surprised we're noticing any issues here. |
I agree, we don't see any noticeable delays with 15-20 objects, but I did notice delays with around 100 entries. Also, since everything is in a transaction if the teacher cancels, we do a rollback which takes significant time, especially with large number of students. In most cases(adding a few students), teachers wouldn't even notice the progress bar, but It can be useful if a large CSV is uploaded. |
cc: @femmebot I've added a progress bar for the update, but I'm not fully confident about how it looks, or it's placement. I'd love your inputs on how we could add it to this page. Personally, I think I could improve on:
|
One quick suggestion: In the tabs, could we add the total count of all students and unlinked students. It's one quick way teachers can confirm whether they have all students accounted for. |
@d12 This job is actually great especially with duplicate roster entries. With bigger classes, doing those duplicate checks will take a really long time. |
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.
Love this ❤️
But maybe we can get one more approval from @femmebot?
I think I'm confused by the existing UI 😂(perhaps not directly from this PR). If there are only 3 unlinked GitHub accounts in the Unlinked GitHub accounts, why do all the students in the All students say "unlinked user" |
@femmebot Could you do a screenshot? Not sure if I'm seeing this |
ah, I was referring to the gif shown here |
Ohhh 🤦♀ That is because the roster entries are not linked to a GitHub user yet. But we want to display all the roster entries. Maybe we can change this in the future? Maybe rename the tab or something? |
Wait … so the users in |
Yea they are not the same. Teachers could manually link all the students to the roster, but they usually don't. When a student accepts an assignment, they are presented with the screen to click their roster identifier. That is how most roster entries get linked to a GitHub account. |
|
||
redirect_to roster_path(current_organization) | ||
identifiers = params[:identifiers].split("\r\n").reject(&:blank?) |
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.
Non-blocking: this split
is the same as before but it feels odd that we are splitting on \r\n
I would have expected only \n
and then a trim on the identifiers.
Hi @jeffrafter it looks like I can't accept the change because of permissions, so @stephaniegiang can you help me out? |
Co-Authored-By: Jeff Rafter <[email protected]>
@shaunakpp accepted the changes. Let me know if you need more help 😄 |
…classroom into roster-add-active-job
@shaunakpp sounds like a good plan! Are you able to do this yourself? |
@stephaniegiang yup, conflicts are resolved and all specs pass, you'll just need to merge #2402 😅 |
Keep roster-add-active-job in sync with master.
@shaunakpp Done 😄I'll do another round of testing on this PR before we merge |
@stephaniegiang thank you so much! ❤️ |
Looks really good 😄 Just tested it and ran smoothly |
What
Fixes #2210
AddStudentsToRosterChannel
to send progressAddStudentsToRosterJob
add_students_to_roster.js
create_entries
method from modelTODO:
UI: update with a few students(< 100)
UI: update with many students(> 100)