Skip to content
This repository was archived by the owner on Jun 24, 2024. It is now read-only.

Moves RosterEntry update to background job #2233

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
3f9f889
Moves RosterEntry creation to background job
Aug 12, 2019
e0844ae
add pending specs for new job
Aug 13, 2019
9204d77
Add specs and squash bugs for the AddStudentsToRosterJob
Aug 14, 2019
8ff70e0
Merge branch 'master' into roster-add-active-job
Aug 15, 2019
83664cc
fix specs and job based on failing specs
Aug 16, 2019
8f03fa3
satisfy linter
Aug 16, 2019
1836324
Merge branch 'master' into roster-add-active-job
Aug 19, 2019
186d742
add hidden fields for action cable reference
Aug 19, 2019
07c1bb7
add a flash message to tell that update is in progress
Aug 19, 2019
c82937f
add counts to display total students and unlinked github accounts
Aug 19, 2019
c857342
Make linter happy
Aug 19, 2019
a29d7e1
Merge branch 'master' into roster-add-active-job
Aug 20, 2019
900e9e1
Merge branch 'master' into roster-add-active-job
Aug 22, 2019
23b6673
Merge branch 'master' into roster-add-active-job
Aug 23, 2019
bc82744
Merge branch 'master' into roster-add-active-job
Aug 29, 2019
b74a320
move progress bar to modal
Aug 29, 2019
f7a9dcc
move js code from channel js to roster_setup
Aug 30, 2019
234cec7
disable form while upload in progress
Aug 30, 2019
641a331
show number of entries added in message
Aug 30, 2019
6e60450
Merge branch 'master' into roster-add-active-job
Aug 30, 2019
0ea16a6
remove committed changes from a different stash
Aug 30, 2019
be2f56c
fix linter cries
Aug 30, 2019
62fe84d
fix outdated specs
Aug 30, 2019
e274ea1
Fix test with new message
stephaniegiang Aug 30, 2019
9f81fbb
Apply suggestions from code review
stephaniegiang Oct 17, 2019
060eb90
Merge branch 'roster-add-active-job' of https://github.com/education/…
shaunakpp Oct 17, 2019
2087b58
Merge pull request #2402 from shaunakpp/roster-add-active-job
stephaniegiang Oct 17, 2019
f4e0f82
Merge branch 'master' into roster-add-active-job
stephaniegiang Oct 22, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 109 additions & 0 deletions app/assets/javascripts/roster_setup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
(function(){
var PROGRESS_HALF_LIFE = 1000;
var setup_roster_update_cable,
progress_asymptotically,
display_message,
set_progress,
display_progress_bar,
initialize_progress;

var progress_complete = false;
setup_roster_update_cable = function(){
var roster_id = $("#current_roster_id").val();
var user_id = $("#current_user_id").val();
App.add_students_to_roster = App.cable.subscriptions.create({
channel: "AddStudentsToRosterChannel",
roster_id: roster_id,
user_id: user_id
},
{
connected: function() {
// Called when the subscription is ready for use on the server
},

disconnected: function() {
// Called when the subscription has been terminated by the server
},

received: function(data) {
// Called when there's incoming data on the websocket for this channel
if(data.status == "completed"){
progress_complete = true;
set_progress(100);
display_message(data.message);
}
}
});
};

progress_asymptotically = function() {
recursive_progress_asymptotically = function(recursive_callback, counter) {
var progress;
var remaining = 100/counter;
progress = 100 - (100/counter);
$(document)
.find(".roster-update-progress-bar")
.animate(
{ width: progress.toFixed() + "%" },
{ duration: PROGRESS_HALF_LIFE * counter }
);
setTimeout(function() {
if(!progress_complete){
recursive_callback(recursive_callback, counter + 1);
}
},
PROGRESS_HALF_LIFE * counter
);
};
recursive_progress_asymptotically(recursive_progress_asymptotically, 1);
};

display_message = function(message){
$(".roster-update-message").removeAttr("hidden");
$("#roster-progress").text(message);
};

set_progress = function(percent) {
$(".roster-update-progress-bar").stop(true, false);
if (percent === 0) {
$(".roster-update-progress-bar").css("width", 0);
} else if (percent) {
$(".roster-update-progress-bar").animate({width: percent + "%"});
}
};

toggle_roster_form = function(disable_fields){
var text_area = $("#entries-field");
var csv_button = $("#file-upload");
if(disable_fields){
text_area.attr("disabled", "disabled");
csv_button.addClass("disabled");
}else{
text_area.removeAttr("disabled");
csv_button.removeClass("disabled");
}
};

ready = (function(){
$("#add-students-roster-form").on("ajax:beforeSend", function(){
toggle_roster_form(true);
$('.roster-update-progress').removeAttr("hidden");
progress_asymptotically();
});

$("#add-students-roster-form").on("ajax:complete", function(){
toggle_roster_form(false);
$("#entries-field").val("");
});

$(document).on('closing', '[data-remodal-id=new-student-modal]', function (e) {
set_progress("0");
$('.roster-update-progress').attr("hidden", "hidden");
});

setup_roster_update_cable();
});

$(document).ready(ready);

}).call(this);
15 changes: 15 additions & 0 deletions app/channels/add_students_to_roster_channel.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# frozen_string_literal: true

class AddStudentsToRosterChannel < ApplicationCable::Channel
def self.channel(user_id:, roster_id:)
"#{channel_name}_#{user_id}_#{roster_id}"
end

def subscribed
stream_from self.class.channel(roster_id: params[:roster_id], user_id: current_user.id)
end

def unsubscribed
# Any cleanup needed when channel is unsubscribed
end
end
43 changes: 14 additions & 29 deletions app/controllers/orgs/rosters_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class RostersController < Orgs::Controller
depends_on :google_classroom

# rubocop:disable AbcSize
# rubocop:disable MethodLength
def show
@google_course_name = current_organization_google_course_name

Expand All @@ -34,9 +35,13 @@ def show
.order(:id)
.page(params[:unlinked_users_page])

# Used for displaying all roster entries in tabs
@roster_entries_count = current_roster.roster_entries.count

download_roster if params.dig("format")
end
# rubocop:enable AbcSize
# rubocop:enable MethodLength

def new
@roster = Roster.new
Expand Down Expand Up @@ -129,38 +134,19 @@ def edit_entry
redirect_to roster_path(current_organization, params: { roster_entries_page: params[:roster_entries_page] })
end

# rubocop:disable Metrics/MethodLength
# rubocop:disable Metrics/AbcSize
def add_students
if params[:lms_user_ids].is_a? String
params[:lms_user_ids] = params[:lms_user_ids].split
end
identifiers = params[:identifiers].split("\r\n").reject(&:blank?)
lms_ids = params[:lms_user_ids] || []

begin
entries = RosterEntry.create_entries(
identifiers: identifiers,
roster: current_roster,
lms_user_ids: lms_ids
)

if entries.empty?
flash[:warning] = "No students created."
elsif entries.length == identifiers.length
flash[:success] = "Students created."
imported_students_lms_statsd(lms_user_ids: params[:lms_user_ids])
else
flash[:success] = "Students created. Some duplicates have been omitted."
imported_students_lms_statsd(lms_user_ids: params[:lms_user_ids])
end
rescue RosterEntry::IdentifierCreationError
flash[:error] = "An error has occurred. Please try again."
end
params[:lms_user_ids].split! if params[:lms_user_ids].is_a? String
lms_user_ids = Array.wrap(params[:lms_user_ids])

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

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.

job_info = AddStudentsToRosterJob.perform_later(identifiers, current_roster, current_user, lms_user_ids)
render json: { job_info: job_info }
end
# rubocop:enable Metrics/AbcSize

# rubocop:disable Metrics/MethodLength
# rubocop:disable Metrics/AbcSize
def download_roster
grouping = current_organization.groupings.find(params[:grouping]) if params[:grouping]

Expand All @@ -177,7 +163,6 @@ def download_roster
end
end
end

# rubocop:enable Metrics/MethodLength
# rubocop:enable Metrics/AbcSize

Expand All @@ -194,7 +179,7 @@ def create_statsd(lms_user_ids:)
end

def imported_students_lms_statsd(lms_user_ids:)
return if lms_user_ids.nil?
return if lms_user_ids.blank?
GitHubClassroom.statsd.increment("roster_entries.lms_imported", by: lms_user_ids.length)
end

Expand Down
58 changes: 58 additions & 0 deletions app/jobs/add_students_to_roster_job.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# frozen_string_literal: true

class AddStudentsToRosterJob < ApplicationJob
queue_as :roster

ROSTER_UPDATE_SUCCESSFUL = "Roster successfully updated."
ROSTER_UPDATE_FAILED = "Could not add any students to roster, please try again."
ROSTER_UPDATE_PARTIAL_SUCCESS = "Could not add following students:"

# Takes an array of identifiers and creates a
# roster entry for each. Omits duplicates, and
#
# rubocop:disable AbcSize
# rubocop:disable MethodLength
def perform(identifiers, roster, user, lms_user_ids = [])
channel = AddStudentsToRosterChannel.channel(roster_id: roster.id, user_id: user.id)

identifiers = add_suffix_to_duplicates!(identifiers, roster)
invalid_roster_entries =
identifiers.zip(lms_user_ids).map do |identifier, lms_user_id|
roster_entry = RosterEntry.create(identifier: identifier, roster: roster, lms_user_id: lms_user_id)
roster_entry.identifier if roster_entry.errors.include?(:identifier)
end.compact!

message = build_message(invalid_roster_entries, identifiers)
entries_created = identifiers.count - invalid_roster_entries.count
if lms_user_ids.present? && entries_created.positive?
GitHubClassroom.statsd.increment("roster_entries.lms_imported", by: entries_created)
end
ActionCable.server.broadcast(channel, message: message, status: "completed")
end
# rubocop:enable AbcSize
# rubocop:enable MethodLength

def add_suffix_to_duplicates!(identifiers, roster)
existing_roster_entries = RosterEntry.where(roster: roster).pluck(:identifier)
RosterEntry.add_suffix_to_duplicates(
identifiers: identifiers,
existing_roster_entries: existing_roster_entries
)
end

# rubocop:disable MethodLength
def build_message(invalid_roster_entries, identifiers)
if invalid_roster_entries.empty?
ROSTER_UPDATE_SUCCESSFUL + " #{identifiers.count} roster #{'entries'.pluralize(identifiers.count)} were added."
elsif invalid_roster_entries.size == identifiers.size
ROSTER_UPDATE_FAILED
else
formatted_students =
invalid_roster_entries.map do |invalid_roster_entry|
"#{invalid_roster_entry} \n"
end.join("")
"#{ROSTER_UPDATE_PARTIAL_SUCCESS} \n#{formatted_students}"
end
end
# rubocop:enable MethodLength
end
31 changes: 0 additions & 31 deletions app/models/roster_entry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,35 +112,4 @@ def self.students_not_on_team(group_assignment)
.uniq
where(user_id: nil).or(where.not(user_id: students_on_team))
end

# Takes an array of identifiers and creates a
# roster entry for each. Omits duplicates, and
# raises IdentifierCreationError if there is an
# error.
#
# Returns the created entries.

# rubocop:disable Metrics/MethodLength
def self.create_entries(identifiers:, roster:, lms_user_ids: [])
created_entries = []
RosterEntry.transaction do
identifiers = add_suffix_to_duplicates(
identifiers: identifiers,
existing_roster_entries: RosterEntry.where(roster: roster).pluck(:identifier)
)

identifiers.zip(lms_user_ids).each do |identifier, lms_user_id|
roster_entry = RosterEntry.create(identifier: identifier, roster: roster, lms_user_id: lms_user_id)

if !roster_entry.persisted?
raise IdentifierCreationError unless roster_entry.errors.include?(:identifier)
else
created_entries << roster_entry
end
end
end

created_entries
end
# rubocop:enable Metrics/MethodLength, Metrics/AbcSize
end
27 changes: 24 additions & 3 deletions app/views/orgs/rosters/_new_student_modal.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,27 @@
<button data-remodal-action="close" class="remodal-close"><%= octicon 'x' %></button>
<h2 class="remodal-header text-left">Add students to your Roster</h2>

<%= hidden_field_tag 'user_id', current_user.id, id: "current_user_id" %>
<%= hidden_field_tag 'roster_id', roster.id, id: "current_roster_id" %>

<div class="flex-justify-between roster-update-progress" hidden>
<div class="Box Box--blue">
<div class="flash flash-full roster-update-message" hidden>
<button class="flash-close js-flash-close"><%= octicon "x" %></button>
<p id="roster-progress"></p>
</div>
<div class="Box-body">
<div class="Subhead border-bottom-0">
<h4 class="Subhead-heading Subhead-heading--blue">Adding students to Roster</h4>
</div>
<span class="Progress Progress">
<span class="bg-green roster-update-progress-bar" style="width: 0%;"></span>
</span>
</div>
</div>
</div>


<div class="my-3">
<h2 class="h4 mb-2">Import students from your institution</h2>
<p>GitHub Classroom is able to automatically import your roster from your institution. If you
Expand All @@ -18,7 +39,7 @@
<div class="my-3">
<h2 class="h4 mb-2">Manually add students</h2>
<div>
<%= form_tag add_students_roster_path(current_organization), method: :patch do |f| %>
<%= form_tag add_students_roster_path(current_organization), method: :patch, remote: true, id: "add-students-roster-form" do |f| %>
<div class="Box">
<dl class="Box-body <%= view.form_class_for(:roster_entries) %> mb-0">
<dt class="text-gray mb-1">Enter your list of students' identifiers, <strong>one per line</strong>.</dt>
Expand All @@ -32,8 +53,8 @@
</div>
</div>

<div class="d-flex flex-items-center border-top pt-5">
<%= submit_tag 'Add roster entries', class: 'btn btn-primary mr-3' %>
<div class="d-flex flex-items-center border-top pt-5">
<%= submit_tag 'Add roster entries', class: 'btn btn-primary mr-3', data: { "disable-with": "Updating..." } %>
</div>
<% end %>
</div>
Expand Down
4 changes: 2 additions & 2 deletions app/views/orgs/rosters/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@
<div class="site-content-body tabnav-body clearfix">
<div class="tabnav">
<nav class="tabnav-tabs">
<span id='students-tab' onclick="selectTab(this)" class="tabnav-tab selected tabnav-link" aria-current="page">All students</span>
<span id='unlinked-tab' onclick="selectTab(this)" class="tabnav-tab tabnav-link">Unlinked GitHub accounts</span>
<span id='students-tab' onclick="selectTab(this)" class="tabnav-tab selected tabnav-link" aria-current="page">All students <span class="Counter"><%= @roster_entries_count %></span></span>
<span id='unlinked-tab' onclick="selectTab(this)" class="tabnav-tab tabnav-link">Unlinked GitHub accounts <span class="Counter"><%= @unlinked_user_ids.count %></span>
</nav>
</div>
<span id='students-span'>
Expand Down
1 change: 1 addition & 0 deletions config/sidekiq.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,4 @@
- [boom, 3]
- [create_repository, 3]
- [porter_status, 3]
- [roster, 3]
Loading