CSC/ECE 517 Fall 2017/E1788 OSS project Maroon Heatmap fixes
About Expertiza
Expertiza is a Ruby on Rails based Open Source project. It is a collaboration tool which lets users with different roles (student, instructor, teaching assistant) to collaborate on a course in an institution. A collaboration could be for an assignment where students teams up for an assignment and instructors grades them on the basis of their submission. Students could review other's works and give feedbacks as well.
Github link
Wiki link
Problem Statement
Background
Heatgrid is the view which summarizes all the reviews given for the work of a participant (reviews, author feedbacks and meta reviews) on a singular web page so that an instructor can go through all the feedback given to a student and decide their grade. The columns are sortable by their score, criterion, average score for a criterion or a metric. (number of comments with more than 10 words)
What’s wrong with it?
app/views/grades/view_team.html.erb is a fairly complex for a view. It uses the concept of view models to generate the required tables. When multiple rounds of reviews are displayed on the heatgrid, a bug prevents the second round of the reviews from being sorted by their criteria, average score, or the metric mentioned above.
Problems
Problem 1
Tables which shows each round's report were not getting sorted except the first.
- Find out what’s preventing the reviews in the second round from being sorted by a criterion, average score or the metric even though same code is used for first and second round of the reviews.
Problem 2
- Come up with a design that can be used for all the rounds of reviews and implement it.
Problem 3
Previously, TA was also seeing instructor's course list in their home page.
- TAs should only be able to view the heatgrid of students for the assignments in courses for which they’re TAs for.
- Nor should a TA’s homepage list any courses (s)he is not a TA for.
- Improve the Access Control and allow the TAs of that particular course to view the heatgrid for the participants of that particular course.
Issue links (external)
Design Decision
We found a discrepancy in the unique identifier property of HTML element which was causing a bug and other was related to adding a condition for populating items in the list to be returned.
We identified the bug and found that there were no major changes required in terms of back-end services or UI.
We did not try to modify any existing variable names and did not either try to refactor any existing code as it was beyond the scope of this task.
Files modified
For Problem 1 & Problem 2.
File 1.
- app/assets/javascripts/view_team_in_grades.js
File 2.
- app/views/grades/view_team.html.erb
For Problem 3.
File 1. app/controllers/tree_display_controller.rb
Issue and Solution
Solution for Problem 1
For all the tables on the page which had each round's data, HTML component table was given same id which was only allowing only first table to have the sorting properties.
Solution for Problem 2
We assigned a unique id to all tables based on the round number and included a class "scoresTable" for each table and initialised sorting features based on class.
<!--
scoreTable was previously assigned as id for all the table which required to be sortable
change was made to keep it unique by adding round number at the end
also scoreTable was added as class to each of the table to make it sortable
remaining changes could be found in app/assets/javascripts/view_team_in_grades.js
fix comment end
-->
<table id="scoresTable<%= vm.round.to_s %>" class="scoresTable tbl_heat tablesorter">
<!-- display the header row of the heatgrid table. this involves iterating through reviewers.-->
//
// scoreTable was assigned to as classes for the table which required to be sortable
// tablesorter is initialised on all the elements having class scoreTable
//
$(".scoresTable").tablesorter();
Problem 3
We found that for a teaching assistant, if he/she is not a TA of a course, private field of tmp_object (already existing name) was assigned false value. So, by adding only true values in the resource object returned for the TA, ensured that only courses in which he/she is TA of will be sent back to the view.
We did not change the variable name as it was out of scope of this change request.
# getting result nodes for child
#
# courses_assignments_obj method makes a call to update_in_ta_course_listing which
# separates out courses based on if he/she is the TA for the course passed
# by marking private to be true in that case
#
# this also ensures that instructors (who are not ta) would have update_in_ta_course_listing
# not changing the private value if he/she is not TA which was set to true for all courses before filtering
# in update_tmp_obj in courses_assignments_obj
#
# below objects/variable names were part of the project as before and
# refactoring could have affected other functionalities too, so it was avoided in this fix
#
# fix comment end
#
def res_node_for_child(tmp_res)
res = {}
tmp_res.keys.each do |node_type|
res[node_type] = []
tmp_res[node_type].each do |node|
tmp_object = {
"nodeinfo" => node,
"name" => node.get_name,
"type" => node.type
}
if node_type == 'Courses' || node_type == "Assignments"
courses_assignments_obj(node_type, tmp_object, node)
end
# below logic makes sure that only courses/assignments for the instructor/ta
# which were marked private = true
# based on the filtering criteria of ta seeing courses he/she is ta of &
# instructor seeing all courses he/she is instructor of,
# is added to the list which is returned
res[node_type] << tmp_object if tmp_object['private']
end
end
res
end
Test Plan
Automated Test
We wrote tests to verify that the courses returned are only those which are specified in ta-mapping table.
For that we considered 7 scenarios
- When there is a mapping between the user and course but user is not a TA
- When there is a mapping between TA and course - in this case the course mapped to TA should be returned
- When there is no mapping - in this case, no course should be returned.
- When TA is also a student of another course
- When TA is also a student of the same course he is TA of
- When there is mapping between TA and course - in this case TA should receive assignments which are for the mapped course
- When there is mapping between TA and course and no assignments are linked to the course
Below is the test written:
describe "POST #children_node_ng for TA" do
before(:each) do
@treefolder = TreeFolder.new
@treefolder.parent_id = nil
@treefolder.name = "Courses"
@treefolder.child_type = "CourseNode"
@treefolder.save!
@treefolder = TreeFolder.new
@treefolder.parent_id = nil
@treefolder.name = "Assignments"
@treefolder.child_type = "AssignmentNode"
@treefolder.save!
@foldernode = FolderNode.new
@foldernode.parent_id = nil
@foldernode.type = "FolderNode"
@foldernode.node_object_id = 1
@foldernode.save!
@foldernode = FolderNode.new
@foldernode.parent_id = nil
@foldernode.type = "FolderNode"
@foldernode.node_object_id = 2
@foldernode.save!
# create a new course
@course1 = create(:course)
# make sure the course is not private
@course1.private = false
@course1.save
@assignment_node1 = create(:assignment_node)
create(:course_node)
# make a teaching assistant
user_ta = build(:teaching_assistant)
@role = user_ta.role
@ta = User.new(user_ta.attributes)
@ta.save!
end
it "returns empty array if the logged in user is not ta" do
# make a student for testing edge case
user_student = build(:student)
student = User.new(user_student.attributes)
student.save!
# create ta-course mapping for the student
ta_mapping = TaMapping.new
ta_mapping.ta_id = User.where(role_id: student.role_id).first.id
ta_mapping.course_id = Course.find(@course1.id).id
ta_mapping.save!
params = FolderNode.all
post :children_node_ng, {reactParams: {child_nodes: params.to_json, nodeType: "FolderNode"}}, user: student
# service should not return anything as the user is student
output = JSON.parse(response.body)['Courses']
expect(output.length).to eq 0
end
it "returns a list of course objects ta is supposed to ta in as json" do
# create ta-course mapping
ta_mapping = TaMapping.new
ta_mapping.ta_id = User.where(role_id: @ta.role_id).first.id
ta_mapping.course_id = Course.find(@course1.id).id
ta_mapping.save!
# make sure it's the current user
stub_current_user(@ta, @role.name, @role)
params = FolderNode.all
post :children_node_ng, {reactParams: {child_nodes: params.to_json, nodeType: "FolderNode"}}, user: @ta
# service should return the course as per the ta-course mapping
output = JSON.parse(response.body)['Courses']
expect(output.length).to eq 1
expect(output[0]['name']).to eq @course1.name
end
it "returns an empty list when there are no mapping between ta and course" do
params = FolderNode.all
# do not create any ta-course mapping
# make sure it's the current user
stub_current_user(@ta, @role.name, @role)
post :children_node_ng, {reactParams: {child_nodes: params.to_json, nodeType: "FolderNode"}}, user: @ta
# service should not return anything as there is no mapping
output = JSON.parse(response.body)['Courses']
expect(output.length).to eq 0
end
it "returns only the course he is ta of when ta is a student of another course at the same time" do
params = FolderNode.all
# create a new course
@course2 = create(:course)
# make sure the course is not private
@course2.private = false
@course2.save
# make ta student of that course
# create assignment against course_2
@assignment1 = create(:assignment_mapping)
@assignment1.course_id = @course2.id
@assignment1.save
# make ta participant of that assigment
@participant1 = create(:participant)
@participant1.parent_id = @assignment1.id
@participant1.user_id = @ta.id
@participant1.save
# create a ta mapping for the other existing course (other than in which he is ta of)
ta_mapping = TaMapping.new
ta_mapping.ta_id = User.where(role_id: @ta.role_id).first.id
ta_mapping.course_id = Course.find(@course1.id).id
ta_mapping.save!
# make sure it's the current user
stub_current_user(@ta, @role.name, @role)
post :children_node_ng, {reactParams: {child_nodes: params.to_json, nodeType: "FolderNode"}}, user: @ta
output = JSON.parse(response.body)['Courses']
# service should return 1 course and should be course 1 not course 2
expect(output.length).to eq 1
expect(output[0]['name']).to eq @course1.name
expect(output[0]['name']).not_to eq @course2.name
end
it "returns only the course he is ta of when ta is a student of same course at the same time" do
params = FolderNode.all
# make ta student of the existing course he is ta of
# create assignment against course_1
@assignment1 = create(:assignment_mapping)
@assignment1.course_id = @course1.id
@assignment1.save
# make ta participant of that assigment
@participant1 = create(:participant)
@participant1.parent_id = @assignment1.id
@participant1.user_id = @ta.id
@participant1.save
# create a ta mapping for the same course he is ta of
ta_mapping = TaMapping.new
ta_mapping.ta_id = User.where(role_id: @ta.role_id).first.id
ta_mapping.course_id = Course.find(@course1.id).id
ta_mapping.save!
# make sure it's the current user
stub_current_user(@ta, @role.name, @role)
post :children_node_ng, {reactParams: {child_nodes: params.to_json, nodeType: "FolderNode"}}, user: @ta
# service should return 1 course
output = JSON.parse(response.body)['Courses']
expect(output.length).to eq 1
expect(output[0]['name']).to eq @course1.name
end
it "returns only the assignments which belongs to the course he is ta of" do
params = FolderNode.all
# create assignment against course_1
# this is 2nd assignment added to course_1, other being in "before" method
@assignment2 = create(:assignment_mapping)
@assignment2.course_id = @course1.id
@assignment2.save!
@assignment_node2 = create(:assignment_node)
@assignment_node2.node_object_id = @assignment2.id
@assignment_node2.save!
# create ta-course mapping
ta_mapping = TaMapping.new
ta_mapping.ta_id = User.where(role_id: @ta.role_id).first.id
ta_mapping.course_id = Course.find(@course1.id).id
ta_mapping.save!
# make sure it's the current user
stub_current_user(@ta, @role.name, @role)
post :children_node_ng, {reactParams: {child_nodes: params.to_json, nodeType: "FolderNode"}}, user: @ta
# service should return 2 assignments as those are mapped
# the sequence of assignments could be random so just checking for array size
output = JSON.parse(response.body)['Assignments']
expect(output.length).to eq 2
end
it "returns empty assignments list if none of the assignments belong to course he is ta of" do
params = FolderNode.all
# delete the assignment node
Node.delete(@assignment_node1.id)
# create ta-course mapping
ta_mapping = TaMapping.new
ta_mapping.ta_id = User.where(role_id: @ta.role_id).first.id
ta_mapping.course_id = Course.find(@course1.id).id
ta_mapping.save!
# make sure it's the current user
stub_current_user(@ta, @role.name, @role)
post :children_node_ng, {reactParams: {child_nodes: params.to_json, nodeType: "FolderNode"}}, user: @ta
# service should not return anything as there is no mapping
output = JSON.parse(response.body)['Assignments']
expect(output.length).to eq 0
end
end
Code coverage
Based on the scenarios we added, code coverage increased by 0.2%.
More report could be found from below link:
Testing in UI
Problem 1 and Problem 2
Steps
1. Login as instructor/TA (who has the privilege to view summary of reviews for all rounds) 2. Choose an assignment and go to summary page
You would see a page similar to below with sorting enabled on specific columns on the right side of name.
Screenshots
Below are the screenshots displaying the fix :
Round 1
Round 2
Author Feedback
Problem 2
Steps
1. Login as a TA
Screenshots
You would be directed to the hop page displaying all courses a TA has privilege to view.
Below is the db result which matches with the results displayed on the screen
Visual Demo of the fix
A video explaining the fix can be found at below location: [1]
Other links
Pull request link :