CSC/ECE 517 Spring 2022 - E2214: Refactor teams controller

This wiki page is for the description of changes made under E2214 OSS assignment for Spring 2022, CSC/ECE 517.

Peer Review Information

For users intending to view the deployed Expertiza associated with this assignment, the credentials are below:

Instructor Login: username -> instructor6, password -> password

Expertiza Background

Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NCSU. It’s an open-source project developed on the Ruby on Rails platform and its code is available on Github. It allows students to review each other’s work and improve their work upon this feedback.

Description of the current project

The following is an Expertiza based OSS project which deals primarily with the models, views, and controllers of Teams. Core to any online learning platform is the ability of students and instructors to create and delete teams. Teams_Controller primarily handles the creation, deletion, and other behaviors of course teams and assignment teams. Most of its methods are called from the instructor’s point of view in expertiza. This controller has some problems that violate some essential Rails design principles. There are a few methods in this controller that should have been in model classes. Some methods share code, which creates code repetition. Our project primarily focuses on refactoring some complex methods that involve high branching, removing redundant and dead code, modularising common functionalities, and condensing the existing bad code into a few lines to achieve the same functionality. The goal of this project is to attempt to make this part of the application easier to read and maintain.

Files modified in current project

Controller, model and views were modified for this project namely:
1. Teams Controller: teams_controller
2. Teams Model: team.rb
3. Teams Views

• list.html.erb
• new.html.erb
• _team.html.erb

4. (Generic)Tree Display Views

• _page_footer.html.erb
• _entry_html.erb
• _folder.html.erb
• _listing.html.erb

Teams Controller

The controller contains all the methods to perform CRUD operations on the teams when seen from the instructor's point of view in the UI.

List of changes

We worked on the following work items(WIs)
WI1 : Move allowed types list constant in list method to model as it is a best practice to keep all the constants at one place.
WI2 : Refactor delete method. This method deletes the team and erases the records from teams_users and signed_up_teams table.
WI3 : Refactor inherit and bequeath_all methods. Both the methods contain duplicated code with nested branching statements.
WI4 : Introduce new methods like init_team_type, get_parent_by_id, get_parent_from_child which commonly operate on fetching team type to render UI elements accordingly.
WI5 : Fix for a flaky test case on creating teams with the random members method.

Solutions Implemented and Delivered

1. Refactoring in the list method. The list method contains a list of constants to check if the team_type from the session variables is a valid one or not.

Existing code snippet in teams_controller.rb:

allowed_types = %w[Assignment Course]
session[:team_type] = params[:type] if params[:type] && allowed_types.include?(params[:type])

We have moved the constant allowed_types to the teams model so that it can be used wherever needed in the teams_controller.rb.
Code snippet after refactoring in team.rb:

# Allowed types of teams -- ASSIGNMENT teams or COURSE teams
def self.allowed_types
  # non-interpolated array of single-quoted strings
  %w[Assignment Course]
end

Code snippet after refactoring showing the usage in teams_controller.rb:

session[:team_type] = params[:type] if params[:type] && Team.allowed_types.include?(params[:type])

2. Refactor delete method. This method contains an if block in which the condition is never satisfied. It is considered a dead code. Related piazza post: https://piazza.com/class/kxwmkrv8djq573?cid=210

So we have simply removed that if block. Below is the removed code snippet.

if @signed_up_team == 1 && !@signUps.first.is_waitlisted # this team hold a topic
   # if there is another team in waitlist, make this team hold this topic
   topic_id = @signed_up_team.first.topic_id
   next_wait_listed_team = SignedUpTeam.where(topic_id: topic_id, is_waitlisted: true).first
   # if slot exist, then confirm the topic for this team and delete all waitlists for this team
   SignUpTopic.assign_to_first_waiting_team(next_wait_listed_team) if next_wait_listed_team
end

And also there is a statement at line 89 which we replaced with

@team.destroy

instead of

@team.destroy if @team

because we already have a nil check for this variable in line 74, so we can remove the redundant ones.

3. Refactor inherit and bequeath_all methods. Both methods contain a piece of code in common. They also have high branching in them which makes the code readability much more difficult. Existing Code Snippet:

# Copies existing teams from a course down to an assignment
# The team and team members are all copied.
def inherit
 assignment = Assignment.find(params[:id])
 if assignment.course_id
   course = Course.find(assignment.course_id)
   teams = course.get_teams
   if teams.empty?
     flash[:note] = 'No teams were found when trying to inherit.'
   else
     teams.each do |team|
       team.copy(assignment.id)
     end
   end
 else
   flash[:error] = 'No course was found for this assignment.'
 end
 redirect_to controller: 'teams', action: 'list', id: assignment.id
end




def bequeath_all
 if session[:team_type] == 'Course'
   flash[:error] = 'Invalid team type for bequeathal'
   redirect_to controller: 'teams', action: 'list', id: params[:id]
   return
 end
 assignment = Assignment.find(params[:id])
 if assignment.course_id
   course = Course.find(assignment.course_id)
   if course.course_teams.any?
     flash[:error] = 'The course already has associated teams'
     redirect_to controller: 'teams', action: 'list', id: assignment.id
     return
   end
   teams = assignment.teams
   teams.each do |team|
     team.copy(course.id)
   end
   flash[:note] = teams.length.to_s + ' teams were successfully copied to "' + course.name + '"'
 else
   flash[:error] = 'No course was found for this assignment.'
 end
 redirect_to controller: 'teams', action: 'list', id: assignment.id
end

GradesHelper

This is a helper class which contains methods for constructing a table(construct_table) and to check whether an assignment has a team and metareveiw(has_team_and_metareview)

List of changes

We worked on the following work items(WIs)
WI1 : Move allowed types list constant to model as it is a best practice to keep all the constants at one place.
WI2 : Refactor delete method. This method deletes the team and erases the records from teams_users and signed_up_teams table. It contains a code block which is not being executed at all (Dead Code). We have simply removed the block
WI3 : Refactor the code as per the Ruby style guidelines and incorporate the good practices
WI4 : Test the conflict_notification method to test the changes made.
WI5 : Move the repeated code in view and view_my_scores methods to a separate method retrieve_questions

Solutions Implemented and Delivered

• Refactoring calculate_all_penalties method

This is used to calculate various penalty values for each assignment if penalty is applicable.

The following changes were made:

1. This method was very complex, performing too many functions within a single method and had to be broken into 3 smaller methods each having a more well defined function. 2. The following 3 methods were created after splitting the first method

  i.  calculate_all_penalties

  ii. calculate_penatly_attributes

  iii. assign_all_penalties

3. Changes were also made to make the code follow ruby style.The language was made more ruby friendly. 4. Finally some redundant code was commented out as it was non-functional.

Refactoring into smaller more specific methods:

Removal of non-functional code :

Change of language to make it more Ruby friendly:

• Move the redundant piece of code from conflict_notification & edit methods to a new method list_questions

The conflict_notification method is used to help the instructors decide if one of the reviews are unfair or inaccurate. This was again split into 2 methods with some part of the code which is repeated in another method refactored into a new method.

Refactored #Created a method which was a duplicate in conflict_notification and edit methods

edit method:

This method is used to edit the questionnaires. This method again has code which is repeated in the conflict_notification method and thus the repeated section was split into a new method.

New method: Refactored #Created a method which was a duplicate in conflict_notification and edit methods

Similar refactoring was performed to obtain the retrieve_questions method:

This is the new method created after the above refactoring:

Testing Details

RSpec

There were no existing test cases for the GradesController. We have added a new spec file 'grades_spec.rb' which covers testing scenario for the newly added method. The specs were run on the previous and current files and they return the same results implying that the refactored code does not break anything. As the model was not changed, no test cases were added for the model.

UI Testing

Following steps needs to be performed to test this code from UI:
1. Login as instructor. Create a course and an assignment under that course.
2. Keep the has team checkbox checked while creating the assignment. Add a grading rubric to it. Add at least two students as participants to the assignment.
3. Create topics for the assignment.
4. Sign in as one of the students who were added to the assignment.
5. Go to the assignment and sign up for a topic.
6. Submit student's work by clicking 'Your work' under that assignment.
7. Sign in as a different student which is participant of the assignment.
8. Go to Assignments--><assignment name>-->Others' work (If the link is disabled, login as instructor and change the due date of the assignment to current time).
9. Give reviews on first student's work.
10. Login as instructor or first student to look at the review grades.

Scope for future improvement

1. The construct_table method in GradesHelper is not used anywhere. It has no reference in the project. So we feel it can be safely removed.
2. The has_team_and_metareview? method in GradesHelper can be broken down into separate methods, one each for team and metareview. This will provide improved flexibility. It needs some analysis though, as both the entities(team & metareview) are currently checked in conjuction from all the views they are referenced from.