CSC/ECE 517 Fall 2022 - E2277. Further Refactoring questionnaires controller.rb: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
(Created page with "==Introduction== This page gives a description of the changes made for the ''questionnaires_controller.rb'' of Expertiza based OSS project. ===Expertiza Background === Expert...")
 
No edit summary
 
(66 intermediate revisions by 3 users not shown)
Line 1: Line 1:
==Introduction==
==Introduction==
This page gives a description of the changes made for the ''questionnaires_controller.rb'' of Expertiza based OSS project.
This page gives a description of the changes made related to the CRUD operations related to the questionnaire of the Expertiza-based OSS project.


===Expertiza Background ===
===Expertiza Background ===
Expertiza is a web application where students can submit and peer-review learning objects (articles, codes, websites, etc). Instructors add and grade the assignments submitted by students to Expertiza. Students can be assigned in teams based on their selection of the topics. It has functionalities such as peer reviews in which students can provide feedback on other's work which helps peer in better developing the project. It is supported by the National Science Foundation.
Expertiza is a web application where students can submit and peer-review learning objects (articles, codes, websites, etc). Instructors add and grade the assignments submitted by students to Expertiza. Students can be assigned in teams based on their selection of the topics. It has functionalities such as peer reviews in which students can provide feedback on others' work which helps peer in better developing the project. The National Science Foundation supports it.


=== Problem Statement ===
== Problems Encountered ==


<b>What is needed:</b> This project builds on E2125, and should begin with the refactoring done by that project.  That project focused on simplifying the methods in review_mapping_helper, while this project looks at making the code more understandable and transparent.
This project builds on E2257.  That project focused only on simplifying the methods in questionnaires_controller, while this project's objective is wider. It involves looking at any code related to the questionnaire and performing bug fixes, refactoring and cleanup. This code also introduces the changes made in E2257, as PR related to that required some changes to be made before merging. This was done as some of the current changes build upon changes made in the previous round.


* Method <b><i>get_data_for_review_report</i></b> marshals a lot of data together and passes back a data structure.  It is used in views/reports/_review_report.html.erb, but it is quite difficult to see how the data is being displayed.  It violates the <b>Expert pattern</b>, because the view needs to know how to break apart the structure that is passed to it.  Doing the same thing with partials in views/reports would make the code easier to follow
<b> Suggestion provided in the documentation of the problem </b>  
* The next three methods involve team <i>“color”</i>.  Color-coding is explained on this page and this page for the E1789 project and this page for E1815.  The code for these methods is not at all clear, and should be refactored.  And please use the American spelling “color”.
* Use of temporary variables instead of instance variables wherever possible to narrow their scope.
* Various method names begin with <b><i>get</i></b>.  This is not Ruby-like.  Change the names to something more appropriate.
* question.rb contains a large number of constants. Make explicit comments on their usage, and remove them if not used.
* The <b><i>get_awarded_review_score</i></b> computes an overall score based upon scores awarded in individual rounds. This is one of many places in Expertiza where scores are being calculated.  Score-calculation code for multiple rounds is being standardized now, in response_map.rb.  Contact Nicholas Himes (nnhimes@ncsu.edu) for particulars.  Change this method to use the new code.
* Since a controller should only create objects of one type the questions_controller could be introduced to create and edit questions moving the logic of dealing with individual questions into a separate controller and improving the separation of responsibility.
* The method <b><i>sort_reviewer_by_review_volume_desc</i></b> should be generalized so that it can sort by any metric, not just review volume.  Other metrics might include number of suggestions, or number of suggestions + number of problems detected. This method should not be counting the number of review rounds!  Since other places in the code will need to know the number of review rounds, it should be calculated somewhere else in the system.
* The next several methods generate charts.  They are cohesive enough that they should be in their own separate file, either another helper or a mixin.
* There is a method <b><i>list_review_submissions</i></b>. It’s not at all clear that this method is needed, though it is used in one view.  Look on the Expertiza wiki and see if there is a better way.
* Also there are several methods for <b><i>feedback_response_maps</i></b>. It is not at all clear why they are here. Look on the Expertiza wiki for the documentation, and see if you can replace them by calls to other methods, or at least make it clearer what they are doing.
* The file ends with three small classes being defined.  There are no comments at all to explain what is being done. Look them up on the Expertiza wiki and refactor or comment them, whichever seems more appropriate.


==Implementation==
<b> What we have considered </b>
*Checked the flow of the code, by adding comments to figure out unused functions.
*Check for unused constants.
*Checked code climate issues found.
*Found bugs where expected behaviour wasn't happening.


===Flowchart===
Since this is an important piece of code with usage at a lot of places we had to be very careful with making changes. We were able to fix most of the bugs and issues we found, but there still exist a few that we didn't have enough time to fix. These have been added to future work so that it can be easier for anyone who picks up the project.


The following process is carried out to complete the project-
<b> The issues we have handled are as follows: </b>
*Presence of many unused functions in the questionnaire questionnaire_controller.rb
*Presence of many unused constants in the model questions.rb
*Unable to update questionnaire parameters.
*Error in the logic used to assign sequence numbers to questions in a questionnaire.
*High severity issue in the way questionnaire and question objects are created (Code Climate)
*Updates made to issues fixed in E2257


[[File:flowchart.jpeg]]
==Files Changed==


===Refactoring===
*app/controllers/questionnaires_controller.rb
*app/models/question.rb
*app/helpers/questionnaire_helper.rb
*app/views/questionnaires/_questionnaire.html.erb
*spec/controllers/questionnaires_controller_spec.rb


== Files Added ==
*app/helpers/question_helper.rb


====Refactoring method====
==Implementation==
All the method names that are refactored are mentioned below-


  review_ get_data_for_review_report
=== Remove Unused Functions ===
The following functions in questionnaire_controller.rb controller was found to be never used and was hence removed. They checked for calls throughout the project and ran the code and testing functionalities after commenting it out, to be sure.


Since this method marshals a lot of data together and passes back a data structure, it is quite difficult to see how the data is being displayed. Therefore what we were thinking is to remove this existing method, create new partials which renders specific data for each individual tasks performed in the current method and pass it to the views file. Similar to what we do Expert pattern.
[[File:2277_3.png]]


[[File:2277_4.png]]


------------------------------------------------------------------------------------------------------------------------
[[File:2277_5.png]]


[[File:2277_6.png]]


  get_team_colour
=== Remove Unused Constants ===
A similar approach to the ones above was performed to figure out the unused constants before removing them. They were all present in the questions.rb model.


Since the method is not particularly clear we will be refactoring the method in such a way that no code is repeated, no conditions is checked multiple times and finally move this piece of code to review_response_maps.rb to decrease the total length of the file (>450 line).
[[File:2277_7.png]]


=== Questionnaire update bug ===
There was a bug where the questionnaire parameters were getting passed back to the function to get the parameters updated. This was happening as the submit button for that section was activated only during the "creation" of a new questionnaire. The edit was using the same page and thus didn't have the submit which was supposed to send values to the controller. This was fixed by adding a separate button to submit during the edit.


------------------------------------------------------------------------------------------------------------------------
[[File:2257_Orig_html.png]]


The code was updated from being as follows:


To remove 'get' from the below methods to something ruby like methods( below suggestions are subject to change)
[[File:2277_Updated_html.png]]


  get_data_for_review_report                      to    data_for_review_report
=== Bug in Sequence number assignment ===
  get_team_colour                                to    team_colour
The initial logic for assigning the sequence number for a question was to just assign it with (no_of_existing_questions)+1. This won't function properly in cases where the instructor deletes questions in the middle and then adds a question.
  get_link_updated_at                            to    link_updated_at
  get_team_reviewed_link_name                    to    team_reviewed_link_name
  get_awarded_review_score                        to    awarded_review_score
  get_review_metrics                              to    review_metrics
  get_each_review_and_feedback_response_map      to    review_and_feedback_response_map
  get_certain_review_and_feedback_response_map    to   certain_review_and_feedback_response_map


Since methods starting with the the word 'get' are not ruby-esque and may cause unintended side-effects, we'll be changing the method names
[[File:2277_8.png]]


This issue was fixed by find the max sequence number and incrementing that to get the sequence number of a new question.


------------------------------------------------------------------------------------------------------------------------
[[File:2277_9.png]]


===  Codeclimate high severity issue because of usage of the following method "const_get" ===
This problem was solved by introducing factory methods for both Questionnaire and Questions object. These methods were then added to the corresponding helper functions. The initial code to create an object was as follows.


  get_awarded_review_score
[[File:2277_1.png]]


This method computes an overall score based upon scores awarded in individual rounds. Since this task is already being done in response_map.rb, we'll using this rb file's method and remove the repeated method from this file.
After the introduction of the factory method.


[[File:2277_2.png]]


------------------------------------------------------------------------------------------------------------------------
The following questionnaire factory method was added to questionnaire_helper.rb


[[File:2277_Questionnaire_factory.png]]


  sort_reviewer_by_review_volume_desc
The following question factory method was added to question_helper.rb


This method is currently sorting the reviewer using review volume. We'll be generalizing this method such that it takes 1 more additional metrics and sort reviewers using the provided metrics
[[File:2277_Question_factory.png]]


===  Acting on Feedback from previous Pull Request [https://github.com/expertiza/expertiza/pull/2472 (PR#2472)]. ===
# Adjusted method names as suggested
# Revised the comments section
# Removed unnecessary constants used in questions.rb


------------------------------------------------------------------------------------------------------------------------
==Testing==
 
 
The below methods
 
  initialize_volume_metric_chart_elements
  display_volume_metric_chart
  initialize_review_metrics_chart_elements
  display_review_metrics_chart
  prepare_chart_data
  prepare_chart_options
 
These methods are specific to to creating and editing charts so they should have their own helper/mixin so we'll be moving these methods out of review_mapping_helper.rb to some other helper/mixin.
 
 
------------------------------------------------------------------------------------------------------------------------
 
 
  list_review_submissions
 
We'll be understanding and refactoring this method as currently its not known where this method is being used. If we find that this method is not being used, then we'll remove it from the file.
 
 
------------------------------------------------------------------------------------------------------------------------
 
 
The below methods
 
  get_each_review_and_feedback_response_map
  get_certain_review_and_feedback_response_map
 
These methods use 'feedback_response_maps' and its not clear how and where they are used. So our task is to find out how and where these methods are being used and how we can implement it better.
 
 
------------------------------------------------------------------------------------------------------------------------
 
 
The below classes
 
  class ReviewStrategy
    attr_accessor :participants, :teams
    def initialize(participants, teams, review_num)
      @participants = participants
      @teams = teams
      @review_num = review_num
    end
  end
 
  class StudentReviewStrategy < ReviewStrategy
    def reviews_per_team
      (@participants.size * @review_num * 1.0 / @teams.size).round
    end
    def reviews_needed
      @participants.size * @review_num
    end
    def reviews_per_student
      @review_num
    end
  end
 
  class TeamReviewStrategy < ReviewStrategy
    def reviews_per_team
      @review_num
    end
    def reviews_needed
      @teams.size * @review_num
    end
    def reviews_per_student
      (@teams.size * @review_num * 1.0 / @participants.size).round
    end
  end
 
Its not known where are being used so we'll be removing these classes or provide better documentation for these classes if their application is found.


''''' Testing Plan '''''
# Since we are removing the use of  'Object.const_get' we ran the tests again to see that it is functioning properly.
# Removed tests that were testing the methods which are not used in the workflow
# Tests were changed to handle the changes made with the sequence number assignment.


------------------------------------------------------------------------------------------------------------------------
=== RSPEC Test Cases ===
 
The current version of expertiza included tests for the questionnaire_controller and qeustions_controller. we plan to modify a few of these tests so that it fits the changes that we have proposed.
===Function placed in the view as partials===  
 
The following method has been removed from the '''review_mapping_helper.rb''' and added to the views as partials:


To run the tests, run the following commands from the expertiza directory
<pre>
<pre>
  def create_report_table_header(headers = {})
rspec ./spec/controllers/questionnaires_controller_spec.rb
    table_header = "<div class = 'reviewreport'>\
                    <table width='100% cellspacing='0' cellpadding='2' border='0' class='table table-striped'>\
                    <tr bgcolor='#CCCCCC'>"
    headers.each do |header, percentage|
      table_header += if percentage
                        "<th width = #{percentage}>\
                        #{header.humanize}\
                                        </th>"
                      else
                        "<th>\
                        #{header.humanize}\
                                        </th>"
                      end
    end
    table_header += "</tr>"
    table_header.html_safe
  end
</pre>
</pre>
The following is the code available in views in the file '''_report_table_header.html.erb''':
<pre>
<pre>
<% table_header = "<div class = 'reviewreport'><table width='100% cellspacing='0' cellpadding='2' border='0' class='table table-striped'><tr bgcolor='#CCCCCC'>" %>
rspec ./spec/controllers/questions_controller_spec.rb
<%  headers.each do |header, percentage| %>
<% table_header += if percentage %>
  <% "<th width = #{percentage}> #{header.humanize} </th>" %>
<% else %>
  <%"<th> #{header.humanize} </th>" %>
<% end %>
<% end %>
<%  table_header += "</tr>"%>
<% return table_header.html_safe %>
</pre>
</pre>


==Testing==
===Manual Testing===


===RSpec Testing===
[https://drive.google.com/file/d/1iPYWqkC5bcAcpwJomyGVI_wUGo28IKBN/view?usp=share_link Video Link]


RSpec testing was carried out for the methods that had been refactored. Given below are the tests which are written for the functions that are modified:
''''' Testing as Instructor'''''
# Login as Instructor.
# Click on Questionnaires.
# Choose a topic and create a new questionnaire.
# Check the save functionality.
# Add a new question to the questionnaire and check the save question functionality.
# delete a question and check if it's reflected properly.
# change the questionnaires name and try to update the questionnaire.


<pre>
==SOLID Principle==
describe 'My Test Cases' do
  before(:each) do
    create(:instructor)
    create(:role_of_student)
    login_as("instructor6")
    visit '/tree_display/list'
    click_link 'View reports'
    expect(page).to have_current_path('/reports/response_report')
    click_link 'View'
  end


  it "can display review metrics", js: true do
SOLID is a set of rules that guide us on how to achieve that in object-oriented programming. Using SOLID guidelines you can avoid bad design in your code and give a well-structured architecture to your design. Bad design leads to an inflexible and brittle codebase, a simple and small change can result in bugs with bad design.
    expect(page).to have_content('Metrics')
  end


  it "can display review grades of each round", js: true do
We implemented the single responsibility principle to divide the questionnaires_controller into questionnaires and questions controller thus ensuring that each controller does only one job.
    expect(page).to have_content('Score awarded')
  end


  it "can display review summary", js: true do
==Factory Design pattern==
    expect(page).to have_content('Reviews done')
  end


  it "can display review summary", js: true do
In Factory pattern, we create objects without exposing the creation logic to the client and refer to a newly created object using a common interface.
    expect(page).to have_content('Reviewer')
  end


  it "can display review summary", js: true do
We implemented the factory design pattern to create questionnaire objects based on the type selected by the user. We implemented a similar approach for the creation of question objects as well.
    expect(page).to have_content('Team reviewed')
  end
end


== Future Work ==
*Questionnaire delete functionality does not work, takes you to a broken link error on clicking the delete button.
*View/Edit advice button is also not working, again takes you to a error page page which claims route doesn't exist.


</pre>
==References==
 
===Manual Testing===
 
To be carried out.


==Design Pattern==
* [https://github.com/kailash1998s/expertiza GitHub]


A design pattern is a general repeatable solution to a commonly occurring problem in software design. It is a description or template for how to solve a problem that can be used in many different situations.
* [https://github.com/expertiza/expertiza/pull/2492 github PR]
 
During the process of refactoring methods as well as method names,'''Strategy Pattern''' was used in the implementation. The Strategy pattern is most useful when you want to provide multiple ways of processing a request, without hard-coding knowledge about those different methods into the object that handles the request.
 
 
==References==


* [ GitHub]
* [http://152.7.178.109:8080 server running updated code]


* [https://www.rubyguides.com/2015/12/ruby-refactoring/ Introduction to Refractoring in Ruby]
* [https://www.rubyguides.com/2015/12/ruby-refactoring/ Introduction to Refactoring in Ruby]


* [http://rspec.info/documentation/3.8/rspec-core/ Rspec Documentation]
* [http://rspec.info/documentation/3.8/rspec-core/ Rspec Documentation]

Latest revision as of 02:49, 12 December 2022

Introduction

This page gives a description of the changes made related to the CRUD operations related to the questionnaire of the Expertiza-based OSS project.

Expertiza Background

Expertiza is a web application where students can submit and peer-review learning objects (articles, codes, websites, etc). Instructors add and grade the assignments submitted by students to Expertiza. Students can be assigned in teams based on their selection of the topics. It has functionalities such as peer reviews in which students can provide feedback on others' work which helps peer in better developing the project. The National Science Foundation supports it.

Problems Encountered

This project builds on E2257. That project focused only on simplifying the methods in questionnaires_controller, while this project's objective is wider. It involves looking at any code related to the questionnaire and performing bug fixes, refactoring and cleanup. This code also introduces the changes made in E2257, as PR related to that required some changes to be made before merging. This was done as some of the current changes build upon changes made in the previous round.

Suggestion provided in the documentation of the problem

  • Use of temporary variables instead of instance variables wherever possible to narrow their scope.
  • question.rb contains a large number of constants. Make explicit comments on their usage, and remove them if not used.
  • Since a controller should only create objects of one type the questions_controller could be introduced to create and edit questions moving the logic of dealing with individual questions into a separate controller and improving the separation of responsibility.

What we have considered

  • Checked the flow of the code, by adding comments to figure out unused functions.
  • Check for unused constants.
  • Checked code climate issues found.
  • Found bugs where expected behaviour wasn't happening.

Since this is an important piece of code with usage at a lot of places we had to be very careful with making changes. We were able to fix most of the bugs and issues we found, but there still exist a few that we didn't have enough time to fix. These have been added to future work so that it can be easier for anyone who picks up the project.

The issues we have handled are as follows:

  • Presence of many unused functions in the questionnaire questionnaire_controller.rb
  • Presence of many unused constants in the model questions.rb
  • Unable to update questionnaire parameters.
  • Error in the logic used to assign sequence numbers to questions in a questionnaire.
  • High severity issue in the way questionnaire and question objects are created (Code Climate)
  • Updates made to issues fixed in E2257

Files Changed

  • app/controllers/questionnaires_controller.rb
  • app/models/question.rb
  • app/helpers/questionnaire_helper.rb
  • app/views/questionnaires/_questionnaire.html.erb
  • spec/controllers/questionnaires_controller_spec.rb

Files Added

  • app/helpers/question_helper.rb

Implementation

Remove Unused Functions

The following functions in questionnaire_controller.rb controller was found to be never used and was hence removed. They checked for calls throughout the project and ran the code and testing functionalities after commenting it out, to be sure.

Remove Unused Constants

A similar approach to the ones above was performed to figure out the unused constants before removing them. They were all present in the questions.rb model.

Questionnaire update bug

There was a bug where the questionnaire parameters were getting passed back to the function to get the parameters updated. This was happening as the submit button for that section was activated only during the "creation" of a new questionnaire. The edit was using the same page and thus didn't have the submit which was supposed to send values to the controller. This was fixed by adding a separate button to submit during the edit.

The code was updated from being as follows:

Bug in Sequence number assignment

The initial logic for assigning the sequence number for a question was to just assign it with (no_of_existing_questions)+1. This won't function properly in cases where the instructor deletes questions in the middle and then adds a question.

This issue was fixed by find the max sequence number and incrementing that to get the sequence number of a new question.

Codeclimate high severity issue because of usage of the following method "const_get"

This problem was solved by introducing factory methods for both Questionnaire and Questions object. These methods were then added to the corresponding helper functions. The initial code to create an object was as follows.

After the introduction of the factory method.

The following questionnaire factory method was added to questionnaire_helper.rb

The following question factory method was added to question_helper.rb

Acting on Feedback from previous Pull Request (PR#2472).

  1. Adjusted method names as suggested
  2. Revised the comments section
  3. Removed unnecessary constants used in questions.rb

Testing

Testing Plan

  1. Since we are removing the use of 'Object.const_get' we ran the tests again to see that it is functioning properly.
  2. Removed tests that were testing the methods which are not used in the workflow
  3. Tests were changed to handle the changes made with the sequence number assignment.

RSPEC Test Cases

The current version of expertiza included tests for the questionnaire_controller and qeustions_controller. we plan to modify a few of these tests so that it fits the changes that we have proposed.

To run the tests, run the following commands from the expertiza directory 

rspec ./spec/controllers/questionnaires_controller_spec.rb

rspec ./spec/controllers/questions_controller_spec.rb

Manual Testing

Video Link

Testing as Instructor

  1. Login as Instructor.
  2. Click on Questionnaires.
  3. Choose a topic and create a new questionnaire.
  4. Check the save functionality.
  5. Add a new question to the questionnaire and check the save question functionality.
  6. delete a question and check if it's reflected properly.
  7. change the questionnaires name and try to update the questionnaire.

SOLID Principle

SOLID is a set of rules that guide us on how to achieve that in object-oriented programming. Using SOLID guidelines you can avoid bad design in your code and give a well-structured architecture to your design. Bad design leads to an inflexible and brittle codebase, a simple and small change can result in bugs with bad design.

We implemented the single responsibility principle to divide the questionnaires_controller into questionnaires and questions controller thus ensuring that each controller does only one job.

Factory Design pattern

In Factory pattern, we create objects without exposing the creation logic to the client and refer to a newly created object using a common interface.

We implemented the factory design pattern to create questionnaire objects based on the type selected by the user. We implemented a similar approach for the creation of question objects as well.

Future Work

  • Questionnaire delete functionality does not work, takes you to a broken link error on clicking the delete button.
  • View/Edit advice button is also not working, again takes you to a error page page which claims route doesn't exist.

References

Team Members

Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Fall_2022_-_E2277._Further_Refactoring_questionnaires_controller.rb&oldid=147282"