E1728. Remove useless partials from grades view and move view logic to grades helper.rb: Difference between revisions
Line 299: | Line 299: | ||
===Create test file named grades_helper_spec.rb in spec/helpers=== | ===Create test file named grades_helper_spec.rb in spec/helpers=== | ||
A new test file was created and added to the project: spec/helpers/grades_helper_spec.rb | A new test file was created and added to the project: spec/helpers/grades_helper_spec.rb. Prior to this project, none of the existing methods in grades_helper.rb were tested using unit tests. The following unit tests were written and used to exercise the methods. An examination of the specific test cases is given below in the Test Plan section. The final two methods in this file are functional tests used to ensure all discrete functions perform normally when combined together. | ||
<pre> | <pre> |
Latest revision as of 00:23, 1 April 2017
Introduction
This wiki provides details on the refactoring tasks that were undertaken as part of the continuous improvement to the Expertiza project
Background
Expertiza is a web application where students can submit and peer-review learning objects (articles, code, web sites, etc). The Expertiza project is supported by the National Science Foundation.
The application provides a complete system through which students and instructors collaborate on the learning objects as well as submit, review and grade assignments for the courses.
Motivation
By participating in the overall refactoring effort as part of the continuous improvement of Expertiza, students get an opportunity to work on a open source software project. This helps them gain exposure on the technologies used in the project as well as much needed experience in collaborating with peers as part of the software development process.
Tasks
The tasks involved as part of this refactoring effort were geared towards cleaning up the grade view logic.
Initial Task List
The initial set of tasks were:
- For files: _scores_author_feedback.html.erb, _scores_metareview.html.erb, _scores_submitted_work.html.erb
- Move javascript code to assets
- For files: _scores_header.html.erb
- Move logical code (such as L43-96) to helper file and assign self-explanatory method name
- For files: _participant.html.erb, view_team.html.erb
- Move javascript code to assets
- Move logical code to helper file and assign self-explanatory method name
- Such as L8-22 in _participant.html.erb
- Create test file named grades_helper_spec.rb in spec/helpers
- Write test cases for all methods in grades_helper.rb
Revised Task List
On working with files _scores_author_feedback.html.erb, _scores_metareview.html.erb, _scores_submitted_work.html.erb, _scores_header.html.erb, the team came to find out that these files were not being used anymore in the application and the overall tasks were updated to the following:
- Remove useless partials from grades view, such as _scores_author_feedback.html.erb, _scores_metareview.html.erb, _scores_submitted_work.html.erb, _scores_header.html.erb, etc.
- For files _participant.html.erb, view_team.html.erb to grades_helper.rb
- Move javascript code to assets
- Move logical code to helper file and assign self-explanatory method name such as L8-22 in _participant.html.erb
- Create test file named grades_helper_spec.rb in spec/helpers
- Write test cases for all methods in grades_helper.rb by using factories
Refactoring Tasks
Remove useless partials from grades view, such as _scores_author_feedback.html.erb, _scores_metareview.html.erb, _scores_submitted_work.html.erb, _scores_header.html.erb, etc.
The named files were removed from the project and the deletions were committed to the repository
For files _participant.html.erb, view_team.html.erb to grades_helper.rb
Move javascript code to assets
A new file was created and added to the project: app/assets/javascripts/grades/view_team.js
For the view_team.html.erb file, the javascript code was moved to view_team.js
$=jQuery; $(function () { $("[data-toggle='tooltip']").tooltip(); $("#scoresTable").tablesorter(); }); var lesser = false; // Function to sort the columns based on the total review score function col_sort(m) { lesser = !lesser // Swaps two columns of the table jQuery.moveColumn = function (table, from, to) { var rows = jQuery('tr', table); var hidden_child_row = table.find('tr.tablesorter-childRow'); hidden_child_row.each(function () { inner_table = jQuery(this).find('table.tbl_questlist') hidden_table = inner_table.eq(0).find('tr') hidden_table.eq(from - 1).detach().insertBefore(hidden_table.eq(to - 1)); if (from - to > 1) { hidden_table.eq(to - 1).detach().insertAfter((hidden_table.eq(from - 2))); } }); var cols; rows.each(function () { cols = jQuery(this).children('th, td'); cols.eq(from).detach().insertBefore(cols.eq(to)); if (from - to > 1) { cols.eq(to).detach().insertAfter((cols.eq(from - 1))); } }); } // Gets all the table with the class "tbl_heat" var tables = $("table.tbl_heat"); // Get all the rows with the class accordion-toggle var tbr = tables.eq(m).find('tr.accordion-toggle'); // Get the cells from the last row of the table var columns = tbr.eq(tbr.length - 1).find('td'); // Init an array to hold the review total var sum_array = []; // iterate through the rows and calculate the total of each review for (var l = 2; l < columns.length - 2; l++) { var total = 0; for (var n = 0; n < tbr.length; n++) { var row_slice = tbr.eq(n).find('td'); if (parseInt(row_slice[l].innerHTML) > 0) { total = total + parseInt(row_slice[l].innerHTML) } } sum_array.push(total) } // The sorting algorithm for (var i = 3; i < columns.length - 2; i++) { var j = i; while (j > 2 && compare(sum_array[j - 2], sum_array[j - 3], lesser)) { var tmp tmp = sum_array[j - 3] sum_array[j - 3] = sum_array[j - 2] sum_array[j - 2] = tmp jQuery.moveColumn($("table.tbl_heat").eq(m), j, j - 1); // This part is repeated since the table is updated tables = $("table.tbl_heat") tbr = tables.eq(m).find('tr.accordion-toggle'); columns = tbr.eq(tbr.length - 1).find('td') j = j - 1; } } } // Function to return boolean based on lesser or greater operator function compare(a, b, less) { if (less) { return a < b } else { return a > b } }
Move logical code to helper file and assign self-explanatory method name such as L8-22 in _participant.html.erb
The following lines of code were removed
<% participant = pscore[:participant] if pscore[:review] @rscore_review=Rscore.new(pscore,:review) end if pscore[:metareview] @rscore_metareview=Rscore.new(pscore,:metareview) end if pscore[:feedback] @rscore_feedback=Rscore.new(pscore,:feedback) end if pscore[:teammate] @rscore_teammate=Rscore.new(pscore,:teammate) end %>
and converted to properties in the GradesHelper module defined in the app/helpers/grades_helper.rb file, in order to separate Controller logic from View.
def participant @participant = Participant.find(params[:id]) end def rscore_review @participant = Participant.find(params[:id]) @pscore = @participant.scores(@questions) if @pscore[:review] @rscore_review=Rscore.new(@pscore,:review) end @rscore_review end def rscore_metareview @participant = Participant.find(params[:id]) @pscore = @participant.scores(@questions) if @pscore[:metareview] @rscore_metareview=Rscore.new(@pscore,:metareview) end @rscore_metareview end def rscore_feedback @participant = Participant.find(params[:id]) @pscore = @participant.scores(@questions) if @pscore[:feedback] @rscore_feedback=Rscore.new(@pscore,:feedback) end @rscore_feedback end def rscore_teammate @participant = Participant.find(params[:id]) @pscore = @participant.scores(@questions) if @pscore[:teammate] @rscore_teammate=Rscore.new(@pscore,:teammate) end @rscore_teammate end def p_total_score @participant = Participant.find(params[:id]) @pscore = @participant.scores(@questions) if @participant.grade @total_score = participant.grade else @total_score = @pscore[:total_score] end @total_score end def p_title @participant = Participant.find(params[:id]) if @participant.grade @title = "A score in blue indicates that the value was overwritten by the instructor or teaching assistant." else @title = nil end @title end
The following lines of code in the _participant.html.erb file were also replaced to use the above properties created
Original | Replacement |
---|---|
<% if !@rscore_review.nil? && @rscore_review.my_avg %> |
<% if !rscore_review.nil? && rscore_review.my_avg %> |
<%= @rscore_review.my_avg.is_a?(Float)? sprintf("%.2f",@rscore_review.my_avg): @rscore_review.my_avg %>%<BR/> |
<%= rscore_review.my_avg.is_a?(Float)? sprintf("%.2f",rscore_review.my_avg): rscore_review.my_avg %>%<BR/> |
<%= @rscore_review.my_min.is_a?(Float)? sprintf("%.0f",@rscore_review.my_min): @rscore_review.my_min %>% - <%= @rscore_review.my_max.is_a?(Float)? sprintf("%.0f",@rscore_review.my_max): @rscore_review.my_max %>% |
<%= rscore_review.my_min.is_a?(Float)? sprintf("%.0f",rscore_review.my_min): rscore_review.my_min %>% - <%= rscore_review.my_max.is_a?(Float)? sprintf("%.0f",rscore_review.my_max): rscore_review.my_max %>% |
<% if @rscore_metareview && @rscore_metareview.my_avg %> <TD ALIGN="CENTER" VALIGN="TOP"><%= @rscore_metareview.my_avg.is_a?(Float)? sprintf("%.2f",@rscore_metareview.my_avg):@rscore_metareview.my_avg %>% |
<% if rscore_metareview && rscore_metareview.my_avg %> <TD ALIGN="CENTER" VALIGN="TOP"><%= rscore_metareview.my_avg.is_a?(Float)? sprintf("%.2f",rscore_metareview.my_avg):rscore_metareview.my_avg %>% |
<TD ALIGN="CENTER" VALIGN="TOP"><%= @rscore_metareview.my_min.is_a?(Float)? sprintf("%.0f",@rscore_metareview.my_min):@rscore_metareview.my_min %>% - <%= @rscore_metareview.my_max.is_a?(Float)?sprintf("%.0f",@rscore_metareview.my_max):@rscore_metareview.my_max %>% |
<TD ALIGN="CENTER" VALIGN="TOP"><%= rscore_metareview.my_min.is_a?(Float)? sprintf("%.0f",rscore_metareview.my_min):rscore_metareview.my_min %>% - <%= rscore_metareview.my_max.is_a?(Float)?sprintf("%.0f",rscore_metareview.my_max):rscore_metareview.my_max %>% |
<% if !@rscore_feedback.nil? && @rscore_feedback.my_avg %> <TD ALIGN="CENTER" VALIGN="TOP"><%= @rscore_feedback.my_avg.is_a?(Float)? sprintf("%.2f",@rscore_feedback.my_avg): @rscore_feedback.my_avg %>% |
<% if !rscore_feedback.nil? && rscore_feedback.my_avg %> <TD ALIGN="CENTER" VALIGN="TOP"><%= rscore_feedback.my_avg.is_a?(Float)? sprintf("%.2f",rscore_feedback.my_avg): rscore_feedback.my_avg %>% |
<TD ALIGN="CENTER" VALIGN="TOP"><%= @rscore_feedback.my_min.is_a?(Float)? sprintf("%.0f",@rscore_feedback.my_min): @rscore_feedback.my_min %>% - <%= @rscore_feedback.my_max.is_a?(Float)? sprintf("%.0f",@rscore_feedback.my_max):@rscore_feedback.my_max %>% |
<TD ALIGN="CENTER" VALIGN="TOP"><%= rscore_feedback.my_min.is_a?(Float)? sprintf("%.0f",rscore_feedback.my_min): rscore_feedback.my_min %>% - <%= rscore_feedback.my_max.is_a?(Float)? sprintf("%.0f",rscore_feedback.my_max):rscore_feedback.my_max %>% |
<% if @rscore_teammate and @rscore_teammate.my_avg %> |
<% if rscore_teammate and rscore_teammate.my_avg %> |
<% range = @rscore_teammate.my_min.is_a?(Float) ? sprintf("%.0f",@rscore_teammate.my_min) : @rscore_teammate.my_min.to_s + '%' + ' - ' %> <% range += @rscore_teammate.my_max.is_a?(Float) ? sprintf("%.0f",@rscore_teammate.my_max) : @rscore_teammate.my_max.to_s + '%' %> |
<% range = rscore_teammate.my_min.is_a?(Float) ? sprintf("%.0f",rscore_teammate.my_min) : rscore_teammate.my_min.to_s + '%' + ' - ' %> <% range += rscore_teammate.my_max.is_a?(Float) ? sprintf("%.0f",rscore_teammate.my_max) : rscore_teammate.my_max.to_s + '%' %> |
<% if participant.grade %> <% total_score = participant.grade %> <% title = "A score in blue indicates that the value was overwritten by the instructor or teaching assistant." %> <% else %> <% total_score = pscore[:total_score] %> <% title = nil %> <% end %> <div <% if title %>title="<%=title%>" style="color:#0033FF"<% end %>><%= total_score==(-1)? "N/A": sprintf("%.2f",total_score) %><%= score_postfix %></div> |
<div <% if p_title %>title="<%=p_title%>" style="color:#0033FF"<% end %>><%= p_total_score==(-1)? "N/A": sprintf("%.2f",p_total_score) %><%= score_postfix %></div> |
<% if !@rscore_review.nil? && @rscore_review.my_avg %> <%= @rscore_review.my_avg.is_a?(Float)?sprintf("%.2f",@rscore_review.my_avg):@rscore_review.my_avg %><%= score_postfix %> |
<% if !rscore_review.nil? && rscore_review.my_avg %> <%= rscore_review.my_avg.is_a?(Float)?sprintf("%.2f",rscore_review.my_avg):rscore_review.my_avg %><%= score_postfix %> |
Create test file named grades_helper_spec.rb in spec/helpers
A new test file was created and added to the project: spec/helpers/grades_helper_spec.rb. Prior to this project, none of the existing methods in grades_helper.rb were tested using unit tests. The following unit tests were written and used to exercise the methods. An examination of the specific test cases is given below in the Test Plan section. The final two methods in this file are functional tests used to ensure all discrete functions perform normally when combined together.
require 'rails_helper' require 'selenium-webdriver' describe GradesHelper, type: :helper do before(:each) do @assignment = create(:assignment, max_team_size: 1) @assignment2 = create(:assignment, name: 'whatever', max_team_size: 3) @deadline_type = create(:deadline_type, id: 5, name: 'metareview') @deadline_right = create(:deadline_right) @new_participant = create(:participant, assignment: @assignment) @participant = create(:participant, assignment: @assignment2) @assignment_team = create(:assignment_team, assignment: @assignment) @questionnaire = create(:questionnaire) @metareview_questionnaire = create(:metareview_questionnaire) @author_feedback_questionnaire = create(:author_feedback_questionnaire) @teammate_review_questionnaire = create(:teammate_review_questionnaire) create(:assignment_questionnaire, user_id: @new_participant.id, questionnaire: @questionnaire) create(:assignment_questionnaire, user_id: @new_participant.id, questionnaire: @metareview_questionnaire) create(:assignment_questionnaire, user_id: @new_participant.id, questionnaire: @author_feedback_questionnaire) create(:assignment_questionnaire, user_id: @new_participant.id, questionnaire: @teammate_review_questionnaire) @questions = {} @questions[@questionnaire.symbol] = @questionnaire.questions @questions[@metareview_questionnaire.symbol] = @metareview_questionnaire.questions @questions[@author_feedback_questionnaire.symbol] = @author_feedback_questionnaire.questions @questions[@teammate_review_questionnaire.symbol] = @teammate_review_questionnaire.questions create( :assignment_due_date, assignment: @assignment2, deadline_type: @deadline_type, submission_allowed_id: @deadline_right.id, review_allowed_id: @deadline_right.id, review_of_review_allowed_id: @deadline_right.id, due_at: '2015-12-30 23:30:12' ) end describe 'get_accordion_title' do it 'should render is_first:true if last_topic is nil' do get_accordion_title(nil, 'last question') expect(response).to render_template(partial: 'response/_accordion', locals: {title: 'last question', is_first: true}) end it 'should render is_first:false if last_topic is not equal to next_topic' do get_accordion_title('last question', 'next question') expect(response).to render_template(partial: 'response/_accordion', locals: {title: 'next question', is_first: false}) end it 'should render nothing if last_topic is equal to next_topic' do get_accordion_title('question', 'question') expect(response).to render_template(nil) end end describe 'has_team_and_metareview?' do it 'should correctly identify the assignment from an assignment id' do params[:id] = @assignment.id result = Assignment.find(params[:id]) expect(result).to eq(@assignment) end it 'should correctly identify the assignment from a participant id' do params[:id] = @new_participant.id result = Participant.find(params[:id]).parent_id expect(result).to eq(@assignment.id) end it 'should return 0 for an assignment without a team or a metareview after a view action' do params[:action] = 'view' params[:id] = @assignment.id result = has_team_and_metareview? expect(result).to be == {has_team: false, has_metareview: false, true_num: 0} end it 'should return 1 for an assignment with a team but no metareview after a view action' do @assignment.max_team_size = 2 @assignment.save params[:action] = 'view' params[:id] = @assignment.id result = has_team_and_metareview? expect(result).to be == {has_team: true, has_metareview: false, true_num: 1} end it 'should return 1 for an assignment without a team but with a metareview after a view action' do @assignment2.max_team_size = 1 @assignment2.save params[:action] = 'view' params[:id] = @assignment2.id result = has_team_and_metareview? expect(result).to be == {has_team: false, has_metareview: true, true_num: 1} end it 'should return 2 for an assignment with a team and with a metareview after a view action' do params[:action] = 'view' params[:id] = @assignment2.id result = has_team_and_metareview? expect(result).to be == {has_team: true, has_metareview: true, true_num: 2} end end describe 'participant' do it 'should return the correct particpant' do new_participant = create(:participant) params[:id] = new_participant.id result = participant expect(result).to eq(new_participant) end end describe 'rscore_X' do it 'should return records of each review type if available' do params[:id] = @new_participant.id expect(rscore_review).to_not be_nil expect(rscore_metareview).to_not be_nil expect(rscore_feedback).to_not eq(nil) expect(rscore_teammate).to_not eq(nil) end it 'should return nil if review types are not available' do params[:id] = @participant.id expect(rscore_review).to be_nil expect(rscore_metareview).to be_nil expect(rscore_feedback).to be_nil expect(rscore_teammate).to be_nil end end describe 'p_total_score' do it 'should return the grade if available' do graded_participant = create(:participant, grade: 90) create(:assignment_questionnaire, user_id: graded_participant.id, questionnaire: @questionnaire) @questions[@questionnaire.symbol] = @questionnaire.questions params[:id] = graded_participant.id expect(p_total_score).to eq(90) end it 'should return :total_score if no grade is available' do create(:assignment_questionnaire, user_id: @new_participant.id, questionnaire: @questionnaire) @questions[@questionnaire.symbol] = @questionnaire.questions params[:id] = @new_participant.id expect(p_total_score).to eq(0) end end describe 'p_title' do it 'should return a title when the participant has a grade' do params[:id] = @new_participant.id @new_participant.grade = 90 @new_participant.save expect(p_title).to eq('A score in blue indicates that the value was overwritten by the instructor or teaching assistant.') end it 'should return nil when the participant has no grade' do params[:id] = @new_participant.id expect(p_title).to eq(nil) end end describe 'get_css_style_for_X_reputation' do hamer_input = [-0.1, 0, 0.5, 1, 1.5, 2, 2.1] lauw_input = [-0.1, 0, 0.2, 0.4, 0.6, 0.8, 0.9] output = %w(c1 c1 c2 c2 c3 c4 c5) it 'should return correct css for hamer reputations' do hamer_input.each_with_index do |e, i| expect(get_css_style_for_hamer_reputation(e)).to eq(output[i]) end end it 'should return correct css for luaw reputations' do lauw_input.each_with_index do |e, i| expect(get_css_style_for_lauw_reputation(e)).to eq(output[i]) end end end end ######################### # Functional Cases ######################### describe GradesHelper, type: :feature do before(:each) do @assignment = create(:assignment) @assignment_team = create(:assignment_team, assignment: @assignment) @participant = create(:participant, assignment: @assignment) create(:team_user, team: @assignment_team, user: User.find(@participant.user_id)) login_as(@participant.name) visit '/student_task/list' expect(page).to have_content 'final2' click_link('final2') end describe 'case 1' do it "Javascript should work on grades Alternate View", js: true do expect(page).to have_content 'Alternate View' expect(page).to have_content 'Review' click_link('Alternate View') expect(page).to have_content 'Grade for submission' end end describe 'case 2' do it "Student should be able to view scores", js: true do expect(page).to have_content 'Your scores' click_link('Your scores') expect(page).to have_content '0.00%' end end end
Test Plans
The main goal of testing was to ensure functionality was maintained after refactoring was completed. The following test cases exercise the functionality of the code sections which were refactored. Automated versions of these tests were written using RSpec/FactoryGirl/Selenium and appear in on the Github repository associated with this project. The functional tests may also be run manually per the descriptions below.
Functional Tests
Test Case 1 | |
---|---|
Test URL | http://csc517.eastus.cloudapp.azure.com:8080 |
Test Type | Functional |
Scenario | Test to see if javascript changes for view_team.html.erb are working |
Pre-Conditions |
Make sure student to test with has the following:
|
Description |
|
Test Execution using existing login |
|
Test Case 2 | |
Test Type | Functional |
Scenario | Test _participant.html.erb to ensure it is working after refactoring logic code |
Pre-Conditions |
|
Description |
|
Test Execution using existing login |
|
Unit Tests
Unit tests were required per the project assignment for helper methods which previously had no associated unit tests. These tests verified that each discrete method returned the proper values. The tests were written using RSpec/FactoryGirl and are listed in the code above. A summary of the test cases is given below.
Unit Test Summary | |||
---|---|---|---|
Method | Parameters | Purpose | Tested Scenarios |
get_accordion_title | last_topic, new_topic | Render a proper partial for questionnaires | last_topic=nil; new_topic=nil; neither=nil |
has_team_and_metareview? | params[:id] | Determine whether a team and reviews exist for a given assignment | find an assignment given assignment_id; find an assignment given participant_id; return 1 for a case with a team but no meta review; return 1 for a case with a meta review but no team; return 2 for a case with a team and metareview |
participant | params[:id] | Returns a participant given an id | find a newly created participant |
rscore_review | params[:id] | returns all the reviews for a participant based on id | return reviews from a participant with reviews; return nothing for a participant without reviews. |
rscore_metareview | params[:id] | returns all the metareviews for a participant based on id | return metareviews from a participant with metareviews; return nothing for a participant without metareviews. |
rscore_feedback | params[:id] | returns all the author feedback for a participant based on id | returns feedback for a participant with feedback; return nothing for a participant without feedback. |
rscore_teammate | params[:id] | returns teammate reviews for a participant based on id | returns teammate reviews for a participant with teammate reviews; return nothing for a participant without teammate reviews. |
p_total_score | params[:id] | returns grade for a participant based on id | returns grade for a participant with grade; returns total_score for a participant without grade. |
p_title | params[:id] | returns a pertinent title for the grades view | returns text title for a participant with grade; returns nothing for a participant without grade. |
get_css_style_for_hamer_reputation | reputation_value | returns a CSS value for formatting based on reputation | rep -0.1 yields CSS c1; rep 0 yields CSS c1; rep 0.5 yields CSS c2; rep 1 yields CSS c2; rep 1.5 yields CSS c3, rep 2 yields CSS c4; rep 2.1 yields css c5 |
get_css_style_for_lauw_reputation | reputation_value | returns a CSS value for formatting based on reputation | rep -0.1 yields CSS c1; rep 0 yields CSS c1; rep 0.2 yields CSS c2; rep 0.4 yields CSS c2; rep 0.6 yields CSS c3, rep 0.8 yields CSS c4; rep 0.9 yields css c5 |