Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

E2115: Mentor Management #1957

Merged
merged 48 commits into from
May 13, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
3f3b9df
Fix Markdown formatting in design_document.md
AvlWx2014 Apr 12, 2021
399e0ee
Add DUTY_MENTOR constant
AvlWx2014 Apr 14, 2021
9d47e7d
Implement select_mentor and helper methods in new class MentorManagement
AvlWx2014 Apr 14, 2021
04854dd
Fix select mentor to return the selected mentor id or nil
AvlWx2014 Apr 15, 2021
cb4127b
Replace `first(1)` with `first` so unpacking works as expected
AvlWx2014 Apr 15, 2021
126249b
Merge branch 'beta' into e2115-choose-mentor
AvlWx2014 Apr 18, 2021
b3e6dae
Add trigger to invitation.rb
AvlWx2014 Apr 18, 2021
914441d
Implement update_mentor_state
AvlWx2014 Apr 18, 2021
9378d35
wip mentor added by id
Apr 19, 2021
af6e907
mentor views and instructor team adding trigger
Apr 21, 2021
049a16c
missed null check
molinamelendezj Apr 21, 2021
6e593fc
Add auto_assign_mentor column to assignments
AvlWx2014 Apr 22, 2021
9350eaa
Start adding mentor checkbox to assignment form
AvlWx2014 Apr 22, 2021
6cee6ac
rename file to match class name
AvlWx2014 Apr 22, 2021
712f5e8
rename the checkbox element
AvlWx2014 Apr 22, 2021
c85fa4f
Match jQuery selector to checkbox id
AvlWx2014 Apr 22, 2021
78e1157
auto_assign_mentor flag logic
molinamelendezj Apr 23, 2021
0fd80cd
added notify email, and feature toggle for mentor auto assign
molinamelendezj Apr 25, 2021
f34441f
added 5 tests for the MentorManagement methods
bahati2000 Apr 29, 2021
5917222
Add some more method-level comments
AvlWx2014 Apr 29, 2021
e72e4ae
Passing user_a_mentor test
AvlWx2014 Apr 29, 2021
0903436
Replace 'context' blocks with 'it' blocks throughout
AvlWx2014 Apr 29, 2021
c614020
Passing select_mentor test
AvlWx2014 Apr 29, 2021
e5f82fd
Remove notify_team_of_mentor_assignment test
AvlWx2014 Apr 29, 2021
26cabfb
Passing get_mentors_for_assignment test
AvlWx2014 Apr 29, 2021
b003d0f
Add 'it' block to last test so it actually gets picked up by RSpec
AvlWx2014 Apr 29, 2021
3e7238d
Should return empty array, rather than empty hash
AvlWx2014 Apr 29, 2021
5380b71
Passing zip_mentors_with_team_count tests
AvlWx2014 Apr 29, 2021
91c24d5
Update test to reflect impl change
AvlWx2014 Apr 29, 2021
e0ffb06
Update test to test for the right return result
AvlWx2014 Apr 29, 2021
8086994
Merge pull request #6 from bahati2000/e2115-choose-mentor
AvlWx2014 Apr 29, 2021
6262124
DRY up the test spec by utilizing let!
AvlWx2014 Apr 29, 2021
fa5125d
Update assignment factory to include new auto_assign_mentor flag
AvlWx2014 Apr 30, 2021
3254b69
Rename variables; Add flag to factory assignment;
AvlWx2014 Apr 30, 2021
6b7799d
Remove defunct test
AvlWx2014 Apr 30, 2021
6644181
Implement test for actually adding a mentor
AvlWx2014 Apr 30, 2021
f4c3cd5
Add tests for the other logical branches of update_mentor_state
AvlWx2014 Apr 30, 2021
7235ab1
Make the use of parens around final expectation method consistent
AvlWx2014 Apr 30, 2021
e1a1a51
Make use of parens consistent for and_return calls
AvlWx2014 Apr 30, 2021
cf567f7
Assert that Team#add_member is not called, rather than that notify is
AvlWx2014 Apr 30, 2021
b994fa6
Rename update_mentor_state to assign_mentor
AvlWx2014 Apr 30, 2021
a0de1b0
Rename get_mentors_for_assignment
AvlWx2014 Apr 30, 2021
717dba1
Add additional comments as discussed in 29 Apr mentor meeting
AvlWx2014 Apr 30, 2021
9e1bb2a
Commit change to schema.rb hoping to fix Travis CI build
AvlWx2014 Apr 30, 2021
7911544
Sync expertiza/beta with beta branch and resolve conflicts
AvlWx2014 Apr 30, 2021
57aed24
Attempt to remedy broken invitation_spec tests
AvlWx2014 Apr 30, 2021
e16233a
Combine dangling attr_accessor declaration with others
AvlWx2014 Apr 30, 2021
463a51a
Revert change as it seems to fail the CI build
AvlWx2014 Apr 30, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 10 additions & 3 deletions app/controllers/teams_users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,16 @@ def create
add_member_return = team.add_member(user, team.parent_id)
flash[:error] = "This team already has the maximum number of members." if add_member_return == false

@teams_user = TeamsUser.last
undo_link("The team user \"#{user.name}\" has been successfully added to \"#{team.name}\".")
user = TeamsUser.last
undo_link("The team @teams_user \"#{user.name}\" has been successfully added to \"#{team.name}\".")

# E2115 Mentor Management
# Kick off the Mentor Management workflow
# Note: this is _not_ supported for CourseTeams which is why the other
# half of this if block does not include the same code
if add_member_return
MentorManagement.assign_mentor(assignment.id, team.id)
end
end
else # CourseTeam
course = Course.find(team.parent_id)
Expand All @@ -51,7 +59,6 @@ def create
else
add_member_return = team.add_member(user)
flash[:error] = "This team already has the maximum number of members." if add_member_return == false

@teams_user = TeamsUser.last
undo_link("The team user \"#{user.name}\" has been successfully added to \"#{team.name}\".")
end
Expand Down
8 changes: 7 additions & 1 deletion app/models/assignment_form.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@
require 'active_support/time_with_zone'

class AssignmentForm
attr_accessor :assignment, :assignment_questionnaires, :due_dates, :tag_prompt_deployments, :is_conference_assignment
attr_accessor :assignment,
:assignment_questionnaires,
:due_dates,
:tag_prompt_deployments,
:is_conference_assignment,
:auto_assign_mentor

attr_accessor :errors
johnbumgardner marked this conversation as resolved.
Show resolved Hide resolved

DEFAULT_MAX_TEAM_SIZE = 1
Expand Down
13 changes: 13 additions & 0 deletions app/models/invitation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,19 @@ def self.accept_invite(team_id, inviter_user_id, invited_user_id, assignment_id)
if can_add_member # The member was successfully added to the team (the team was not full)
Invitation.update_users_topic_after_invite_accept(inviter_user_id, invited_user_id, assignment_id)

# E2115 Mentor Management
# Kick off the Mentor Management workflow
# Since there are two places in the code base where members are added to
# teams we have to call the MentorManagement class in both places.
# Those places are here when a student accepts an invitation to join a
# team, and in teams_users_controller.rb. Ideally, both code paths would
# call the same method to perform this action and we could DRY this up.
# It is worth noting that while ultimately, both code paths do call Team#add_member
# adding this code there would risk a recursive loop since MentorManagement
# also calls Team#add_member to add a mentor to the team
new_team_id = TeamsUser.team_id(assignment_id, inviter_user_id)
MentorManagement.assign_mentor(assignment_id, new_team_id)

# invited_participant = Participant.where(user_id: invited_user_id, parent_id: assignment_id).first
# inviter_participant = Participant.where(user_id: inviter_user_id, parent_id: assignment_id).first
# inviter_assignment_team = AssignmentTeam.team(inviter_participant)
Expand Down
122 changes: 122 additions & 0 deletions app/models/mentor_management.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
class MentorManagement
# Select a mentor using the following algorithm
#
# 1) Find all assignment participants for the
# assignment with id [assignment_id] whose
# duty is the same as [Particpant#DUTY_MENTOR].
# 2) Count the number of teams those participants
# are a part of, acting as a proxy for the
# number of teams they mentor.
# 3) Return the mentor with the fewest number of
# teams they're currently mentoring.
#
# This method's runtime is O(n lg n) due to the call to
# Hash#sort_by. This assertion assumes that the
# database management system is capable of fetching the
# required rows at least as quickly.
#
# Implementation detail: Any tie between the top 2
# mentors is decided by the Hash#sort_by algorithm.
#
# @return The id of the mentor with the fewest teams
# they are assigned to. Returns `nil` if there are
# no participants with mentor duty for [assignment_id].
def self.select_mentor(assignment_id)
mentor_user_id, _ = self.zip_mentors_with_team_count(assignment_id).first
User.where(id: mentor_user_id).first
end

# = Mentor Management
# E2115: Handles calls when an assignment has the auto_assign_mentor flag enabled and triggered by the event when a new member joins an assignment team.
#
# This event happens when:
# 1.) An invited student user accepts and successfully added to a team from
# app/models/invitation.rb
# 2.) A student user is successfully added to the team manually from
# app/controllers/teams_users_controller.rb.
#
# This method will determine if a mentor needs to be assigned, if so,
# selects one, and adds the mentor to the team if:
# 1.) The assignment does not have a topic.
# 2.) If the team has reached >50% full capacity.
# 3.) If the team does not have a mentor.
def self.assign_mentor(assignment_id, team_id)
assignment = Assignment.find(assignment_id)
team = Team.find(team_id)

# RuboCop 'use guard clause instead of nested conditionals'
# return if assignments can't accept mentors
return unless assignment.auto_assign_mentor

# RuboCop 'use guard clause instead of nested conditionals'
# return if the assignment or team already have a topic
return if assignment.topics? || !team.topic.nil?

curr_team_size = Team.size(team_id)
max_team_members = Assignment.find(assignment_id).max_team_size

# RuboCop 'use guard clause instead of nested conditionals'
# return if the team size hasn't reached > 50% of capacity
return if curr_team_size * 2 <= max_team_members

# RuboCop 'use guard clause instead of nested conditionals'
# return if there's already a mentor in place
return if team.participants.any? { |participant| participant.duty == Participant::DUTY_MENTOR }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why arent you using the def self.user_a_mentor?(user) function here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point! user_a_mentor? checks whether there's a corresponding entry in Participant for a given User with the mentor duty. So we could equivalently do:

return if team.users.any? &:user_a_mentor?

An advantage I can think of for the way it exists currently is that we avoid a bunch of database calls.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this looks cleaner.


mentor_user = select_mentor(assignment_id)

# Add the mentor using team model class.
team_member_added = mentor_user.nil? ? false : team.add_member(mentor_user, assignment_id)

return unless team_member_added

notify_team_of_mentor_assignment(mentor_user, team)
end

def self.notify_team_of_mentor_assignment(mentor, team)
members = team.users
emails = members.map(&:email)
members_info = members.map{|mem| "#{mem.fullname} - #{mem.email}"}
mentor_info = "#{mentor.fullname} (#{mentor.email})"
message = "#{mentor_info} has been assigned as your mentor for assignment #{Assignment.find(team.parent_id).name} <br>Current members:<br> #{members_info.join('<br>')}"

Mailer.delayed_message(bcc: emails,
subject: "[Expertiza]: New Mentor Assignment",
body: message).deliver_now
end

# Returns true if [user] is a mentor, and false if not.
#
# [user] must be a User object.
#
# Checks the Participant relation to see if a row exists with
# user_id == user.id that also has 'mentor' in the duty attribute.
def self.user_a_mentor?(user)
Participant.exists?(user_id: user.id, duty: Participant::DUTY_MENTOR)
end

# Select all the participants who's duty in the participant
# table is [DUTY_MENTOR], and who are a participant of
# [assignment_id].
#
# @see participant.rb for the value of DUTY_MENTOR
def self.mentors_for_assignment(assignment_id)
Participant.where(parent_id: assignment_id, duty: Participant::DUTY_MENTOR)
end

# Produces a hash mapping mentor's user_ids to the aggregated
# number of teams they're part of, which acts as a proxy for
# the number of teams they're mentoring.
def self.zip_mentors_with_team_count(assignment_id)
mentor_ids = self.mentors_for_assignment(assignment_id).pluck(:user_id)

return [] if mentor_ids.empty?
johnbumgardner marked this conversation as resolved.
Show resolved Hide resolved

team_counts = {}
mentor_ids.each { |id| team_counts[id] = 0 }
team_counts.update(TeamsUser.where(user_id: mentor_ids).group(:user_id).count(:team_id))

team_counts.sort_by { |_, v| v }
end

end
7 changes: 7 additions & 0 deletions app/models/participant.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ class Participant < ActiveRecord::Base

PARTICIPANT_TYPES = %w[Course Assignment].freeze

# define a constant to hold the duty title Mentor
# this will be used in the duty column of the participant
# table to define participants who can mentor teams, topics, or assignments
# since the column's type is VARCHAR(255), other string constants should be
# defined here to add different duty titles
DUTY_MENTOR = "mentor"

def team
TeamsUser.find_by(user: user).try(:team)
end
Expand Down
11 changes: 10 additions & 1 deletion app/models/teams_user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,16 @@ class TeamsUser < ActiveRecord::Base
attr_accessible :user_id, :team_id

def name(ip_address = nil)
self.user.name(ip_address)
name = self.user.name(ip_address)

# E2115 Mentor Management
# Indicate that someone is a Mentor in the UI. The view code is
# often hard to follow, and this is the best place we could find
# for this to go.
if MentorManagement.user_a_mentor?(self.user)
name += " (Mentor)"
end
name
end

def delete
Expand Down
31 changes: 30 additions & 1 deletion app/views/assignments/edit/_general.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,19 @@ function microtaskChanged() {
</td>
</tr>

<!-- E2115 Mentor Management -->
<tr>
<td style='padding:5px' id='assignment_auto_assign_mentor_field'>
<input name="assignment_form[assignment][auto_assign_mentor]" type="hidden" value="false"/>
<%= check_box_tag(
'assignment_form[assignment][auto_assign_mentor]',
value='true',
checked=@assignment_form.assignment.auto_assign_mentor
) %>
<%= label_tag('auto_assign_mentor_label', 'Auto assign mentors when team hits > 50% capacity?') %>
</td>
</tr>

<!--Quiz fields-->
<tr>
<td style='padding:5px' id='assignment_require_quiz_field'>
Expand Down Expand Up @@ -263,9 +276,17 @@ function microtaskChanged() {
function topicsChanged() {
var msg = 'Warning! Un-checking this box will remove all topics that have been created.';
var checkbox = jQuery('#assignment_has_topics');

// E2115 - bind enabled state of mentor checkbox to checked property
// of topics checkbox
var mentorCheckbox = jQuery("#auto_assign_mentor_checkbox");
mentorCheckbox.prop("disabled", checkbox.is(':checked'));

if (checkbox.is(':checked')) {
// If this box is checked, display the 'Topics' tab
jQuery("#topics_tab_header").show();
// E2115 - if 'has topics' is checked, uncheck mentor
mentorCheckbox.prop("checked", false);
} else {
if (confirm(msg)) {
// If this box is un-checked, remove all topics and reload page
Expand All @@ -292,14 +313,19 @@ function microtaskChanged() {
var checkbox = jQuery('#team_assignment');
var team_count_field = jQuery('#assignment_team_count_field');
var teammate_reviews_field = jQuery('#assignment_show_teammate_reviews');
var team_formation_due_date_checkbox = jQuery('#team_formation_due_date_checkbox')
var team_formation_due_date_checkbox = jQuery('#team_formation_due_date_checkbox');
var autoAssignMentorCheckbox = jQuery('#auto_assign_mentor_checkbox');

jQuery("#questionnaire_table_TeammateReviewQuestionnaire").remove()
if (checkbox.is(':checked')) {
team_count_field.removeAttr('hidden');
team_formation_due_date_checkbox.removeAttr('hidden')
jQuery('#assignment_form_assignment_max_team_size').val('2');
teammate_reviews_field.removeAttr('hidden');
// E2115 hide auto assign mentor checkbox when an
// an assignment does not have teams
// what would the mentor be assigned to if there are no teams?
autoAssignMentorCheckbox.hide();
addDueDateTableElement(<%= @due_date_nameurl_not_empty==nil ? false:@due_date_nameurl_not_empty %>,'team_formation', 0,<%= due_date(@assignment_form.assignment, 'team_formation').to_json.html_safe %>);
<% assignment_questionnaire = @assignment_form.assignment_questionnaire('TeammateReviewQuestionnaire', nil, nil) %>
<% questionnaire = @assignment_form.questionnaire(assignment_questionnaire, 'TeammateReviewQuestionnaire') %>
Expand All @@ -316,6 +342,9 @@ function microtaskChanged() {
team_count_field.attr('hidden', true);
team_formation_due_date_checkbox.attr('hidden', true)
teammate_reviews_field.attr('hidden', true);
// E2115 show auto assign mentor checkbox when an
// an assignment has teams
autoAssignMentorCheckbox.show();
document.getElementById('assignment_form_assignment_max_team_size').value = '1';
removeDueDateTableElement('team_formation', 0);

Expand Down
3 changes: 2 additions & 1 deletion app/views/student_teams/view.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
<tr style="border: 1px outset #000000; padding: 10px 20px" >
<th class="head">Username</th>
<th class="head">Full name</th>
<!--<th class="head">Team-member role (duty)</th>-->
<th class="head">Team-member role (duty)</th>
<th class="head">Email address</th>
<% if @teammate_review_allowed %>
<th class="head">Review action</th>
Expand All @@ -31,6 +31,7 @@
<tr>
<td><%= member.user.name(session[:ip]) %></td>
<td><%= member.user.fullname(session[:ip]) %></td>
<td><%= member.duty.nil? ? "" : member.duty.capitalize %></td>
<td><%= member.user.email(session[:ip]) %></td>

<!--if you can review a teammate, display Review hyperlink; if the review has been done, display View and Edit Hyperlinks-->
Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20210422185445_add_auto_assign_mentor_column.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddAutoAssignMentorColumn < ActiveRecord::Migration
def change
add_column :assignments, :auto_assign_mentor, :boolean, :default => false
end
end
3 changes: 2 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 20201125202200) do
ActiveRecord::Schema.define(version: 20210422185445) do

create_table "account_requests", force: :cascade do |t|
t.string "name", limit: 255
Expand Down Expand Up @@ -130,6 +130,7 @@
t.boolean "vary_by_round", default: false
t.boolean "reviewer_is_team"
t.boolean "is_conference_assignment", default: false
t.boolean "auto_assign_mentor", default: false
end

add_index "assignments", ["course_id"], name: "fk_assignments_courses", using: :btree
Expand Down
16 changes: 3 additions & 13 deletions design_document.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,7 @@ This document provides the common design features to be followed while developin

Icons are available in 4 sizes : 16, 24, 32, 48. It is possibility that we might not have all sizes for all icons. Edit the number as per size chose, everything else will remain same.
johnbumgardner marked this conversation as resolved.
Show resolved Hide resolved

**Sr. No.**
**Element Name**
**Image**
**Guide**
**Sr. No.** | **Element Name** | **Image** | **Guide**
|---|---|---|---|
| 1 | Add assignment | ![Add Assignment](app/assets/images/tree_view/image4.png) | To add 'add assignment' icon, use path **```/assets/tree_view/add-assignment-24.png```** |
| 2 | Add Teaching assistant | ![Add TA](app/assets/images/tree_view/add-ta-24.png) | To add 'add TA' icon, use path **```/assets/tree_view/add-ta-24.png```** |
Expand Down Expand Up @@ -40,11 +37,7 @@ Icons are available in 4 sizes : 16, 24, 32, 48. It is possibility that we might

## Buttons :

**Sr. No.**
**Element Name**
**Image**
**Guide**
**Class**
**Sr. No.** | **Element Name** | **Image** | **Guide** | **Class**
|---|---|---|---|---|
| 1 | Button - Default style | *to be added* | Default button | ```btn btn-default btn-md``` |
| 2 | Button - Success style | *to be added* | For accepting. | ```btn btn-success btn-md``` |
Expand All @@ -63,10 +56,7 @@ The class to be used in a table tag is ```table table-striped```.

## Notifications :

**Sr. No.**
**Element Name**
**Image**
**Guide**
**Sr. No.** | **Element Name** | **Image** | **Guide**
|---|---|---|---|
| 1 | Success | *to be added* | For notification, add class as ```flash_note alert alert-success``` |
| 2 | Error | *to be added* | For notification, add class as ```flash_note alert alert-danger``` |
Expand Down
1 change: 1 addition & 0 deletions spec/factories/factories.rb
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@
is_calibrated false
has_badge false
allow_selecting_additional_reviews_after_1st_round false
auto_assign_mentor false
end

factory :assignment_team, class: AssignmentTeam do
Expand Down
2 changes: 2 additions & 0 deletions spec/models/invitation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
allow(Invitation).to receive(:remove_users_sent_invites_for_assignment).with(user3.id, assignment.id).and_return(true)
allow(TeamsUser).to receive(:add_member_to_invited_team).with(user2.id, user3.id, assignment.id).and_return(true)
allow(Invitation).to receive(:update_users_topic_after_invite_accept).with(user2.id, user3.id, assignment.id).and_return(true)
allow(MentorManagement).to receive(:assign_mentor)
expect(Invitation.accept_invite(team_id, user2.id, user3.id, assignment.id)).to eq(true)
end
end
Expand All @@ -49,6 +50,7 @@
allow(Invitation).to receive(:remove_users_sent_invites_for_assignment).with(user3.id, assignment.id).and_return(true)
allow(TeamsUser).to receive(:add_member_to_invited_team).with(user2.id, user3.id, assignment.id).and_return(true)
allow(Invitation).to receive(:update_users_topic_after_invite_accept).with(user2.id, user3.id, assignment.id).and_return(true)
allow(MentorManagement).to receive(:assign_mentor)
expect(Invitation.accept_invite(team_id, user2.id, user3.id, assignment.id)).to eq(true)
end
end
Expand Down