Moves RosterEntry update to background job #2233

shaunakpp · 2019-08-12T23:19:48Z

What

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 create_entries method from model

TODO:

Add specs
Iterate on UIUX

UI: update with a few students(< 100)

UI: update with many students(> 100)

- 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

d12 · 2019-08-12T23:22:46Z

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.

shaunakpp · 2019-08-12T23:34:55Z

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.

shaunakpp · 2019-08-19T18:03:20Z

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:

Adding a flash message to describe that the update is in progress(already working on it)
Displaying information about the identifiers which were not added
Moving this to the modal itself?
If we move to a modal, display a message to teacher that the update is in progress, if the page refreshes and/or when the modal is not displayed.

femmebot · 2019-08-19T18:24:51Z

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.

shaunakpp · 2019-08-19T18:49:25Z

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.

Added this change:

stephaniegiang · 2019-08-20T19:08:00Z

@d12 This job is actually great especially with duplicate roster entries. With bigger classes, doing those duplicate checks will take a really long time.

stephaniegiang

Love this ❤️

But maybe we can get one more approval from @femmebot?

femmebot · 2019-09-03T15:43:17Z

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"

stephaniegiang · 2019-09-04T13:49:04Z

@femmebot Could you do a screenshot? Not sure if I'm seeing this

femmebot · 2019-09-04T13:54:32Z

@femmebot Could you do a screenshot? Not sure if I'm seeing this

ah, I was referring to the gif shown here

stephaniegiang · 2019-09-04T13:57:52Z

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?

femmebot · 2019-09-04T14:04:29Z

Wait … so the users in Unlinked GitHub accounts is not the same as unlinked user? Also, is the teacher expected to manually link all the students shortly after they add the roster?

stephaniegiang · 2019-09-04T14:07:38Z

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.

app/controllers/orgs/rosters_controller.rb

jeffrafter · 2019-09-03T14:09:59Z

app/controllers/orgs/rosters_controller.rb


- redirect_to roster_path(current_organization)
+ identifiers = params[:identifiers].split("\r\n").reject(&:blank?)


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.

app/jobs/add_students_to_roster_job.rb

shaunakpp · 2019-10-17T18:30:15Z

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]>

stephaniegiang · 2019-10-17T18:33:55Z

@shaunakpp accepted the changes. Let me know if you need more help 😄

…classroom into roster-add-active-job

shaunakpp · 2019-10-17T19:27:01Z

I've resolved the conflicts in #2402 as I cannot directly resolve conflicts in this branch. We can merge #2402 directly as it is updating this branch and does not affect master

stephaniegiang · 2019-10-17T19:31:58Z

@shaunakpp sounds like a good plan! Are you able to do this yourself?

shaunakpp · 2019-10-17T19:40:01Z

@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.

stephaniegiang · 2019-10-17T19:41:51Z

@shaunakpp Done 😄I'll do another round of testing on this PR before we merge

shaunakpp · 2019-10-17T19:42:31Z

@stephaniegiang thank you so much! ❤️

stephaniegiang · 2019-10-22T14:34:15Z

Looks really good 😄 Just tested it and ran smoothly

Moves RosterEntry creation to background job

3f9f889

- 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

Shaunak Pagnis added 2 commits August 12, 2019 18:28

add pending specs for new job

e0844ae

Add specs and squash bugs for the AddStudentsToRosterJob

9204d77

shaunakpp changed the title ~~Moves RosterEntry creation to background job~~ Moves RosterEntry update to background job Aug 15, 2019

Shaunak Pagnis added 5 commits August 15, 2019 16:12

Merge branch 'master' into roster-add-active-job

8ff70e0

fix specs and job based on failing specs

83664cc

satisfy linter

8f03fa3

Merge branch 'master' into roster-add-active-job

1836324

add hidden fields for action cable reference

186d742

shaunakpp marked this pull request as ready for review August 19, 2019 18:01

shaunakpp requested a review from a team as a code owner August 19, 2019 18:01

add a flash message to tell that update is in progress

07c1bb7

Shaunak Pagnis added 3 commits August 19, 2019 14:50

add counts to display total students and unlinked github accounts

c82937f

Make linter happy

c857342

Merge branch 'master' into roster-add-active-job

a29d7e1

shaunakpp mentioned this pull request Aug 20, 2019

Improve roster creation flow #2268

Open

7 tasks

Shaunak Pagnis added 8 commits August 22, 2019 11:52

Merge branch 'master' into roster-add-active-job

900e9e1

Merge branch 'master' into roster-add-active-job

23b6673

Merge branch 'master' into roster-add-active-job

bc82744

move progress bar to modal

b74a320

move js code from channel js to roster_setup

f7a9dcc

disable form while upload in progress

234cec7

show number of entries added in message

641a331

Merge branch 'master' into roster-add-active-job

6e60450

stephaniegiang added the student roster 👨‍🎓 👩‍🎓 label Aug 30, 2019

stephaniegiang approved these changes Sep 3, 2019

View reviewed changes

shaunakpp mentioned this pull request Oct 1, 2019

Keep roster-add-active-job in sync with master. #2402

Merged

jeffrafter reviewed Oct 17, 2019

View reviewed changes

Apply suggestions from code review

9f81fbb

Co-Authored-By: Jeff Rafter <[email protected]>

Merge branch 'roster-add-active-job' of https://github.com/education/…

060eb90

…classroom into roster-add-active-job

Merge pull request #2402 from shaunakpp/roster-add-active-job

2087b58

Keep roster-add-active-job in sync with master.

Merge branch 'master' into roster-add-active-job

f4e0f82

jeffrafter mentioned this pull request Dec 3, 2019

Move slow LMS actions to background jobs #2491

Open

femmebot mentioned this pull request Jan 2, 2020

[Tracking] IA redesign #2494

Open

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moves RosterEntry update to background job #2233

Moves RosterEntry update to background job #2233

shaunakpp commented Aug 12, 2019 •

edited

Loading

d12 commented Aug 12, 2019

shaunakpp commented Aug 12, 2019

shaunakpp commented Aug 19, 2019 •

edited

Loading

femmebot commented Aug 19, 2019

shaunakpp commented Aug 19, 2019 •

edited

Loading

stephaniegiang commented Aug 20, 2019

stephaniegiang left a comment

femmebot commented Sep 3, 2019

stephaniegiang commented Sep 4, 2019

femmebot commented Sep 4, 2019

stephaniegiang commented Sep 4, 2019

femmebot commented Sep 4, 2019

stephaniegiang commented Sep 4, 2019

jeffrafter Sep 3, 2019

shaunakpp commented Oct 17, 2019

stephaniegiang commented Oct 17, 2019

shaunakpp commented Oct 17, 2019 •

edited

Loading

stephaniegiang commented Oct 17, 2019

shaunakpp commented Oct 17, 2019

stephaniegiang commented Oct 17, 2019

shaunakpp commented Oct 17, 2019

stephaniegiang commented Oct 22, 2019


		redirect_to roster_path(current_organization)
		identifiers = params[:identifiers].split("\r\n").reject(&:blank?)

Moves RosterEntry update to background job #2233

Are you sure you want to change the base?

Moves RosterEntry update to background job #2233

Conversation

shaunakpp commented Aug 12, 2019 • edited Loading

What

d12 commented Aug 12, 2019

shaunakpp commented Aug 12, 2019

shaunakpp commented Aug 19, 2019 • edited Loading

femmebot commented Aug 19, 2019

shaunakpp commented Aug 19, 2019 • edited Loading

stephaniegiang commented Aug 20, 2019

stephaniegiang left a comment

Choose a reason for hiding this comment

femmebot commented Sep 3, 2019

stephaniegiang commented Sep 4, 2019

femmebot commented Sep 4, 2019

stephaniegiang commented Sep 4, 2019

femmebot commented Sep 4, 2019

stephaniegiang commented Sep 4, 2019

jeffrafter Sep 3, 2019

Choose a reason for hiding this comment

shaunakpp commented Oct 17, 2019

stephaniegiang commented Oct 17, 2019

shaunakpp commented Oct 17, 2019 • edited Loading

stephaniegiang commented Oct 17, 2019

shaunakpp commented Oct 17, 2019

stephaniegiang commented Oct 17, 2019

shaunakpp commented Oct 17, 2019

stephaniegiang commented Oct 22, 2019

shaunakpp commented Aug 12, 2019 •

edited

Loading

shaunakpp commented Aug 19, 2019 •

edited

Loading

shaunakpp commented Aug 19, 2019 •

edited

Loading

shaunakpp commented Oct 17, 2019 •

edited

Loading