CSC/ECE 517 Spring 2016/E1720. UI issues/fixes

This wiki page is for the description of changes made according to the specification of E1720 OSS assignment for Spring 2016.

Peer Review Information

For testing the changes made, the following credentials are recommended:

• Instructor Login: username: instructor6 password: password
• Student Login: username: student11 password: password
• Student Login: username: student1862 password: password

The above users are suggested for testing because there are a few users which would lead to exceptions upon login for unknown reasons completely unrelated to our work.

Introduction

Background

Expertiza is a web portal which can be used to manage assignments related to a course. It provides a platform to view assignments, manage teams, select topics and work improvement through anonymous peer reviews. For the instructor it provides complete control to create assignments, view reviews submitted and provide feedback. The instructors also have an option to publish the students work based on the rights provided by the student.

Problem Statement

The following were the tasks identified to accomplish through this project.

• Issue #702: Add another institution_id to course table and The Create Course page needs to be fixed to tell the creator to specify the institution (course/_course.html.erb).
• Issue #316: Remove "Actions" column on signup sheet in a completed assignment (sign_up_sheet/list.html.erb).
• Issue #295: Add a confirmation on deleting an assignment on the admin screen (tree_display/actions/_shared_actions.html.rb).
• Issue #256: Add a one-time notification at the top of the page which links to an article on the details of the change (it might make sense to put a javascript module in the site_controllers/index.html.rb and let other pages call this).

Files Modified in this Project

The following files were modified for this project.

• app/controllers/participants_controller.rb
• app/controllers/sign_up_sheet_controller.rb
• app/models/sign_up_topic.rb
• app/models/signed_up_team.rb
• app/models/team.rb
• app/models/waitlist.rb
• app/views/participants/view_publishing_rights.html.erb
• app/views/sign_up_sheet/list.html.erb
• app/views/sign_up_sheet/show_team.html.erb
• app/views/sign_up_sheet/view_publishing_rights.html.erb
• app/views/tree_display/actions/_assignments_actions.html.erb
• app/helpers/import_topics_helper.rb
• app/assets/javascripts/tree_display.jsx
• config/routes.rb
• spec/features/instructor_interface_spec.rb
• spec/features/team_creation_spec.rb

Solutions implemented and Delivered

WI-1:

For this work item the following files were modified: app/controllers/sign_up_sheet_controller.rb: In this file the method ad_info is defined. This method was defined using sql query to provide its results. To change this method in such a way that it uses active record associations changes needed to be made to the function as shown below. Additionally since the new implementation returns a list of hashes the way in which this result is accessed in show_team method was also changed to accommodate this change.

Original Code:

 def ad_info(assignment_id, topic_id)
   query = "select t.id as team_id,t.comments_for_advertisement,t.name,su.assignment_id, t.advertise_for_partner from teams t, signed_up_teams s,sign_up_topics su "+
       "where s.topic_id='"+topic_id.to_s+"' and s.team_id=t.id and s.topic_id = su.id;    "
   SignUpTopic.find_by_sql(query)
 end

Modified Code:

 def ad_info(assignment_id, topic_id)
   # List that contains individual result object
   @result_list = []
   # Get the results
   @results = SignedUpTeam.where("topic_id = ?", "#{topic_id}")
   # Iterate through the results of the query and get the required attributes
   @results.each do |result| 
     team = result.team
     topic = result.topic
     resultMap = {}
     resultMap[:team_id] = team.id
     resultMap[:comments_for_advertisement] = team.comments_for_advertisement
     resultMap[:name] = team.name
     resultMap[:assignment_id] = topic.assignment_id
     resultMap[:advertise_for_partner] = team.advertise_for_partner
     # Append to the list
     @result_list.append(resultMap)
   end
   @result_list
 end

Additionally changes were also required in the models signed_up_team.rb and team.rb. This was because these associations were required to fetch required data using active record associations. The following changes were made to the files:

app/models/signed_up_team.rb:

 class SignedUpTeam < ActiveRecord::Base
   belongs_to :topic, :class_name => 'SignUpTopic'
   belongs_to :team, :class_name => 'Team' #Added this association between SignedUpTeam and Team.

app/models/team.rb

 class Team < ActiveRecord::Base
   has_many :teams_users, :dependent => :destroy
   has_many :users, :through => :teams_users
   has_many :join_team_requests
   has_one :team_node,:foreign_key => :node_object_id,:dependent => :destroy
   has_many :bids, :dependent => :destroy
   has_many :signed_up_teams #Added this association between Team and SignedUpTeam.

WI-2:

For this work item the file app/controllers/sign_up_sheet_controller.rb was modified. After performing a code-analysis it was concluded that the method add_default_microtask was not referenced from any other part of the project. Hence it was safely removed.

WI-3:

The first part of this task is it enable the view_publishing_rights view to all the assignments. When logged in as an instructor, (s)he can go to the list of assignments by navigating through Manage > Assignments. Here, only the assignments which have topics associated with them have the icon to go to the view_publishing_rights page. This icon should be available to all the assignments. This view of the assignments/courses is known as the tree_display. The part of the code that made sure that only the assignments with topics have this icon is in app/assets/javascripts/tree_display.jsx.

 if (this.props.is_available) {
   ...
   if (this. if (this.props.has_topic) {
     moreContent.push(
       
         <a title="View publishing rights" href={"/sign_up_sheet/view_publishing_rights?id="+(parseInt(this.props.id)/2).toString()}>
           <img src="/assets/tree_view/view-publish-rights-24.png" />
         </a>
       
     )
   }
 }

Here we see that the code that adds the icon to the content if two conditions are met. To get the icon for all the assignments, the change to made is pretty simple. Move the code outside of the two if conditions.

 if (this.props.is_available) {
   ...
   if (this. if (this.props.has_topic) {
     // Move it outside these ifs
   }
 }
 moreContent.push(
   
     <a title="View publishing rights" href={"/sign_up_sheet/view_publishing_rights?id="+(parseInt(this.props.id)/2).toString()}>
       <img src="/assets/tree_view/view-publish-rights-24.png" />
     </a>
   
 )

All the business logic that will be used to display content in the view_publishing_rights.html.erb is in the sign_up_sheet_controller.rb. Logically speaking, this should be in the participants_controller.rb, since we are taking about the publishing rights provided by the participants of the assignment. The next part of the tasks involves moving the view_publishing_rights method to the participants_controller.rb.

sign_up_sheet_controller.rb

 def view_publishing_rights
   load_add_signup_topics(params[:id])
 end
 
 def load_add_signup_topics(assignment_id)
   @id = assignment_id
   @sign_up_topics = SignUpTopic.where( ['assignment_id = ?', assignment_id])
   @slots_filled = SignUpTopic.find_slots_filled(assignment_id)
   @slots_waitlisted = SignUpTopic.find_slots_waitlisted(assignment_id)
 
   @assignment = Assignment.find(assignment_id)
   #ACS Removed the if condition (and corresponding else) which differentiate assignments as team and individual assignments
   # to treat all assignments as team assignments
   #Though called participants, @participants are actually records in signed_up_teams table, which
   #is a mapping table between teams and topics (waitlisted recored are also counted)
   @participants = SignedUpTeam.find_team_participants(assignment_id)
 end

view_publishing_rights then calls the load_add_signup_topics method. In that, a lot of attributes are set to be used in the view. But in reality, the view just made use of the @sign_up_topics and @assignment. But the load_add_signup_topics is used in multiple places within this controller, and to cater for the needs of all those views, a lot of extra attributes are set. In other words, the method is doing more than one thing. Also, the two relevant attributes to be used in the view_publishing_rights views are not set up ideally. Only those assignments that have topics can be shown in the view. This should be changed so that all assignments should show up, because a participant can be in any type of assignment, and this view is for viewing the publishing rights that the participant has provided. So, by using @sign_up_topics", the scope is limited to just the assignments with topics in them. So, apart from moving the method to the participant_controller.rb, the following code was added so that the participants of any type of assignment can be viewed.

Original code (what was in place and moved, but did not work as expected)

 def view_publishing_rights
   @sign_up_topics = SignUpTopic.where( ['assignment_id = ?', assignment_id])
   @assignment = Assignment.find(assignment_id)
 end

Modified code

 def view_publishing_rights
   # Get the assignment ID from the params
   assignment_id = params[:id]
 
   # Get the assignment object for the above ID and set the @assignment_name object for the view
   assignment = Assignment.find(assignment_id)
   @assignment_name = assignment.name
 
   # Initially set to false, will be true if the assignment has any topics
   @has_topics = false
 
   # Attribute that contains the list of the teams and their info related to this assignment
   @teams_info = []
 
   # Get all the teams that work on the assignment with ID assignment_id
   teams = Team.find_by_sql(["select * from teams where parent_id = ?", assignment_id])
 
   # For each of the teams, do
   teams.each do |team|
     team_info = {}
     # Set the team name
     team_info[:name] = team.name
     # List that hold the details of the users in the team
     users = []
     # For each of the users, do
     team.users.each do |team_user|       
       # Append the user info to the users list
       users.append(get_user_info(team_user, assignment))
     end
     # Append the users list to the team_info object
     team_info[:users] = users  
 
     # Get the signup topics for the assignment
     @has_topics = get_signup_topics_for_assignment(assignment_id, team_info, team.id)   
 
     # Choose only those teams that have signed up for topics
     team_without_topic = !SignedUpTeam.where(["team_id = ?", team.id]).any?
     if @has_topics && team_without_topic
       next
     end
     
     # Append the hashmap to the list of hashmaps
     @teams_info.append(team_info)
   end
   @teams_info = @teams_info.sort_by {|hashmap| [hashmap[:topic_id] ? 0 :1,hashmap[:topic_id] || 0 ] }
 
 end

The final part of this task was to move the corresponding view to the views/participants

views/sign_up_sheet/view_publishing_rights.html.erb

  <h1>Publishing rights for <%= @assignment.name %> assignment</h1>
  
  <% if flash[:notice] %>
    <div class="flash_note"><%= flash[:notice] %></div>
  <% end %>
  
  <% if @sign_up_topics.any? %>
    <table class="general">
      <tr>
        <th width="5%" rowspan="2">Topic #</th>
        <th width="40" rowspan="2">Topic name(s)</th>
        <th width="20%" rowspan="2">Team name</th>
        <th width="35%" colspan="4">Participant</th>
      </tr>
      <tr>
        <th>Name</th>
        <th>Fullname</th>
        <th>Publish Rights</th>
        <th>Verified</th>
      </tr>
  
      <% @sign_up_topics.each_with_index do |topic, i| %>
        <tr>
          <td><%= topic.topic_identifier %></td>
          <td><%= render :partial => 'topic_names', :locals => {:i => i, :topic => topic} %></td>
          <th colspan=<%= (@assignment.team_assignment? ? "5" : "4") %>> </th>
        </tr>
  
        <% if (@assignment.participants.size > 0)
             for signed_up_team in topic.signed_up_teams
               #ACS Find the user(s) signed up for a topic
               # removed code that handles team and individual assignments differently
               # An unknown bug causes teams to disappear. Don't crash if this happens
               team = Team.find(signed_up_team.team_id)
               users = []
               users = team.users if team
               showed_team = false
               for user in users
             %>
           <tr>
             <th> </th>
             <th> </th>
             <%   if !showed_team
           %>
         <td align="center" rowspan="<%= team.users.size %>"><b><%= team.name %></b></td>
       <% end %>
       <% showed_team = true %>
       <td><%=user.name%></td>
       <td><%=user.fullname%></td>
       <%
           permissionGranted = false
       hasSignature = false
       signatureValid = false
       @assignment.participants.each do |participant|
         if (user.id == participant.user.id)
           permissionGranted = participant.permission_granted?
         end
       end
     %>
   <td><%= (permissionGranted ? "granted" : "denied") %></td>
   <td>
     <% if (permissionGranted && hasSignature && signatureValid) %>
       <img src="/assets/Check-icon.png" title="verified with digitigal signature" alt="verified"/>
     <% else %>
       <img src="/assets/delete_icon.png" title="no digitigal signature" alt="unverified"/>
     <% end %>
      
   </td>
            </tr>
          <% end %>
        <% end %>
      <% end %>
    <% end %>
  </table>
  <% end %>
  

The above view had a lot of controller related logic in it. Since it is not the best of practices to have such logic in the view code, the changes shown above in the participants_controller.rb had to be made, so that the view code is just responsible for displaying the information rather than computing it as well.

views/participants/view_publishing_rights.html.erb

  <h1>Publishing rights for <%= @assignment_name %> assignment</h1>
  
  <% if flash[:notice] %>
      <div class="flash_note"><%= flash[:notice] %></div>
  <% end %>
  
  <table class="general">
      <tr>
          <% if @has_topics %>
              <th width="5%" rowspan="2">Topic #</th>
              <th width="40" rowspan="2">Topic name(s)</th>
          <% end %>
          <th width="20%" rowspan="2">Team name</th>
          <th width="35%" colspan="4">Participant</th>
      </tr>
      <tr>
          <th>Name</th>
          <th>Fullname</th>
          <th>Publish Rights</th>
          <th>Verified</th>
      </tr>
  
      <% @teams_info.each do |team_info| %>
          <% 
              users = team_info[:users]
              num_students = users.size 
          %>        
          <tr>
              <% if @has_topics %>
                  <td rowspan="<%= num_students %>"><%= team_info[:topic_id] %> </td>
                  <td rowspan="<%= num_students %>"><%= team_info[:topic_name] %></td>
              <% end %> 
              <td align="center" rowspan="<%= num_students %>">
                  <b><%= team_info[:name] %></b> 
              </td>
              
              <% users.each do |user| %>
                  <td> <%=user[:name]%> </td>
                  <td> <%=user[:fullname]%> </td>
                  <td> <%=user[:pub_rights]%> </td>
                  <td>
                      <% if user[:verfied] %>
                          <img src="/assets/Check-icon.png" title="verified with digitigal signature" alt="verified"/>
                      <% else %>
                          <img src="/assets/delete_icon.png" title="no digitigal signature" alt="unverified"/>
                      <% end %>
                  </td> </tr><tr>
              <% end %>
  
          </tr>
  
      <% end %>
  </table>
  

WI-4:

For this work-item the slotAvailable? method was removed from the file app/controllers/sign_up_sheet_controller.rb. This method is already implemented in the model app/models/sign_up_topic.rb.

WI-5:

For this work item the file app/controllers/sign_up_sheet_controller.rb was modified. After performing a code-analysis it was concluded that the method other_confirmed_topic_for_user was not referenced from any other part of the project. Hence it was safely removed.

WI-6:

For this work-item the files that were modified included app/helpers/import_topics_helper.rb. In this file one thing that has been changed is the variable used to represent the row array. This was done in order to remove any confusion. The array represents the individual columns of a particular row. Additionally since the category column is an optional column a check has been added to verify its existence before assigning it to the attributes hash. Original Code:

 def self.define_attributes(row)
   attributes = {}
   attributes["topic_identifier"] = row[0].strip
   attributes["topic_name"] = row[1].strip
   attributes["max_choosers"] = row[2]
   attributes["category"] = row[3].strip
   attributes
 end

Modified Code:

 def self.define_attributes(columns)
   attributes = {}
   attributes["topic_identifier"] = columns[0].strip
   attributes["topic_name"] = columns[1].strip
   attributes["max_choosers"] = columns[2].strip
   if columns.length > 3
     attributes["category"] = columns[3].strip 
   end
   attributes
 end

Another file that was modified for this work-item is app/models/sign_up_topic.rb. This was also modified to remove the ambiguity caused by the variable name row. This was changed to columns. Additionally the original check which verifies whether the number of columns in each row is 4 was modified so that it also works if the imported file has only 3 columns.

Original Code:

 def self.import(row,session,id = nil)
   if row.length != 4
     raise ArgumentError, "CSV File expects the format: Topic identifier, Topic name, Max choosers, Topic Category"
   end
   topic = SignUpTopic.where(topic_name: row[1], assignment_id: session[:assignment_id]).first
   if topic == nil
     attributes = ImportTopicsHelper::define_attributes(row)
     ImportTopicsHelper::create_new_sign_up_topic(attributes,session)
   else
     topic.max_choosers = row[2]
     topic.topic_identifier = row[0]
     #topic.assignment_id = session[:assignment_id]
     topic.save
   end
 end

Modified Code:

 def self.import(columns,session,id = nil)
   if columns.length < 3
     raise ArgumentError, "CSV File expects the format: Topic identifier, Topic name, Max choosers, Topic Category (optional)"
   end
   topic = SignUpTopic.where(topic_name: columns[1], assignment_id: session[:assignment_id]).first
   if topic == nil
     attributes = ImportTopicsHelper::define_attributes(columns)
     ImportTopicsHelper::create_new_sign_up_topic(attributes,session)
   else
     topic.max_choosers = columns[2]
     topic.topic_identifier = columns[0]
     #topic.assignment_id = session[:assignment_id]
     topic.save
   end
 end

WI-7:

We have added Rspec test cases to check functionality of import feature and view_publishing_rights view. These Rspec tests are present in "spec/features/instructor_interface_spec.rb". The tests use csv files that are present in "spec/features/assignment_topic_csvs/" folder. View Publishing Rights: This test case checks whether view_publishing_rights page has column headers "Topic name(s)" and "Topic #". Since an assignment created without a topic does not have topic name and topic id. The test case will fail if the page contains topic name and topic id.

 describe "View Publishing Rights" do
   it 'should display teams for assignment without topic' do
     login_as("instructor6")
     visit '/participants/view_publishing_rights?id=1'
     expect(page).to have_content('Team name')
     expect(page).should_not have_content('Topic name(s)')
     expect(page).should_not have_content('Topic #')
   end
 end

Import tests for assignment topics: There are 4 tests written under this category.They check the topics import feature. The file which is to be uploaded should contain 3 compulsory fields and the fourth field is optional. The tests perform the following tasks: Check the import pass when the import file has 3 columns.

 describe "Import tests for assignment topics" do
   it 'should be valid file with 3 columns' do
     login_as("instructor6")
     visit '/assignments/1/edit'
     click_link "Topics"
     click_link "Import topics"
     file_path=Rails.root+"spec/features/assignment_topic_csvs/3-col-valid_topics_import.csv"
     attach_file('file',file_path)
     click_button "Import"
     click_link "Topics"
     expect(page).to have_content('expertiza')
     expect(page).to have_content('mozilla')
   end
   

Check the import pass when the import file has 3 or 4 columns.

   it 'should be a valid file with 3 or more columns' do
     login_as("instructor6")
     visit '/assignments/1/edit'
     click_link "Topics"
     click_link "Import topics"
     file_path=Rails.root+"spec/features/assignment_topic_csvs/3or4-col-valid_topics_import.csv"
     attach_file('file',file_path)
     click_button "Import"
     click_link "Topics"
     expect(page).to have_content('capybara')
     expect(page).to have_content('cucumber')
   end

Check the import fail when the import file has 2 columns.

 it 'should be a invalid csv file' do
     login_as("instructor6")
     visit '/assignments/1/edit'
     click_link "Topics"
     click_link "Import topics"
     file_path=Rails.root+"spec/features/assignment_topic_csvs/invalid_topics_import.csv"
     attach_file('file',file_path)
     click_button "Import"
     click_link "Topics"
     expect(page).should_not have_content('airtable')
     expect(page).should_not have_content('devise')
   end

Check the import fail when the import file doesn't have valid data.

 it 'should be an random text file' do
     login_as("instructor6")
     visit '/assignments/1/edit'
     click_link "Topics"
     click_link "Import topics"
     file_path=Rails.root+"spec/features/assignment_topic_csvs/random.txt"
     attach_file('file',file_path)
     click_button "Import"
     click_link "Topics"
     expect(page).should_not have_content('this is a random file which should fail')
   end
 end

Testing from the UI

UI Testing

Since majority of the tasks for this assignment was code refactoring, only a few of these can be seen and tested through the UI. Follow the instructions below to check the:

• view_publishing_rights

1. Login as a instructor (better to log in as an instructor that has assignments in the tree_desiplay view. For eg. instructor6)
2. Navigate 'Manage > Assignments'
3. Against each assignment in the table, an icon for 'view_publishing_rights' can be seen
4. Click on the 'view_publishing_rights' icon against any assignment
5. If the assignment has topics (eg. Wikipedia contribution), the table in the view will have 'Topic Name' and 'Topic #' displayed.
6. If the assignment does not have topics, the table will not have the above two columns

• Topics import feature for an assignment

1. In the tree_display view of assignments, click on the edit icon. (Or while creating a new assignment)
2. Click on 'Topics' tab
3. Click on 'Import topics', towards the bottom of the page
4. Select a valid CSV file. The first, second and third columns should be the topic identifier, topic name and number of slots available, respectively. Note that the topic identifier should be more than 10 characters long, else import will fail. The CSV can have an optional 4th column for 'category' but this is displayed in the UI
5. In case of invalid CSV import, an error message will be shown.