CSC/ECE 517 Fall 2021 - E2160. Implementing and testing import export controllers: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
Line 76: Line 76:
=== Models ===
=== Models ===


*ImportFileController is changed and has been made more generalized, many case statements are removed. The ImprortFileController is made so small and understandable. The following code changes are follows:
*ImportFileController is changed and has been made more generalized, many case statements are removed. The ImprortFileController is made so small and understandable. The import functionality is moved into the corresponding models. Previously  code was present in different modules which was confusing now the functionality was all in one place. The following code changes are follows:


*Previous Implmentation
*Previous Implmentation
Line 312: Line 312:




*Questionnaire.rb has a #import method that is now routed through the ImportFileController and the actual act of importing works. We removed all functionality related to importing from the QuestionnaireController and QuestionnaireHelper.  
With these new changes to #import functionality. The following modules review_response_map.rb, course_team.rb,assignment_team.rb,assignment_participant.rb have the  "self.import" which will use the import functionality in the ImportFileController and they are made as concise as possible. Each module has specific checks while importing the file, so we can't have same code for all the module.
 
*We removed the ImportFileHelper and ImportTopicsHelper and moved that functionality into the corresponding models. Having the code segmented made things confusing since none of the functionality was all in one place. 
 
*We removed import functionality from '''question.rb''' because there is no way to import a question out of context from a questionnaire. Importing a questionnaire, means importing questions to fill that questionnaire.  
 
*SignUpSheet no longer has an import method. That functionality was never used and does not actually currently work in the production version of Expertiza. After discussing with our mentor, we were instructed to removed the import link on the front-end, the import method, and all related tests.  
 
*The previous way of determining required import fields were to populate the @expected_fields variable which was incredibly hard to find in the code. We have eliminated the need for that variable and have removed all instances of it.
 
*We have made things as generic as possible in the .html.erb files so that you can be in any model and the code works on the front end seamlessly. This is done by adding these three methods to each model that has an import method:
 
<pre>
  def self.required_import_fields
    {"teammembers" => "Team Members"}
  end
 
  def self.optional_import_fields(id=nil)
    {"teamname" => "Team Name"}
  end
 
  def self.import_options
    {"handle_dups" => {"display" => "Handle Duplicates",
                      "options" => {"ignore" => "Ignore new team name",
                                    "replace" => "Replace the existing team with the new team",
                                    "insert" => "Insert any new team members into the existing team",
                                    "rename" => "Rename the new team and import"}}}
  end
</pre>
 
The above content is specific to the course_team.rb but the three method names are consistent to all the models and are called on the front end.
 
*The models that have a "self.import" method that does not branch out into other controllers beside ImportFileController will be looked at to make sure they are as concise as possible. None of them can be all the same because they all need to have checks specific to what they need to import. We can, however, make sure that similar models, like assignment_team and course_team, have similar imports. That is what we have done for assignment_team/course_team and assignment_participant/course_participant.
 
 
Now, these are the final models/places where a user may import a file into Expertiza:
 
- assignment_participant
 
- assignment_team
 
- course_participant
 
- course_team
 
- review_response_map
 
- metareview_response_map
 
- sign_up_topic
 
- user
 
- questionnaire
 
 
*We have updated some text on the front end related to questionnaire. The import link did not match the capitalization format in the rest Expertiza.
 
=== Code Climate ===
'''Note: Code Climate was not running on Expertiza's beta branch for the last four days of the project. We have done the best we could to removed extraneous lines and white spaces.'''
 
There were many code climate issues in reference to our project. We have managed to fix 46 issues as a byproduct of refactoring the code. Here is a list of them with their frequency put in parenthesis':
 
* Method get_questions_from_csv has a Cognitive Complexity of 62 (exceeds 5 allowed). Consider refactoring.
 
* Method get_questions_from_csv has 41 lines of code (exceeds 25 allowed). Consider refactoring.
 
* File import_file_controller.rb has 278 lines of code (exceeds 250 allowed). Consider refactoring.
 
* Avoid deeply nested control flow statements. (3)
 
* Similar blocks of code found in 2 locations. Consider refactoring. (2)
 
* Unescaped parameter value
 
* Useless assignment to variable - a.
 
* Cyclomatic complexity for get_questions_from_csv is too high. [18/6]
 
* Assignment Branch Condition size for get_questions_from_csv is too high. [43.3/15]
 
* Block has too many lines. [36/25]
 
* Avoid more than 3 levels of block nesting. (3)
 
* Perceived complexity for get_questions_from_csv is too high. [16/7]
 
* Align elsif with if.
 
* Space missing after comma. (13)
 
* Line is too long. [172/160]
 
* Method has too many lines. [108/60]
 
* Use the return of the conditional for variable assignment and comparison.
 
* Move @optional_count = 0 out of the conditional. (2)
 
* Move contents_hash = eval(params[:contents_hash]) out of the conditional. (6)
 
* Convert if nested inside else to elsif.
 
* Don't use parentheses around the condition of an if. (3)


== Test Plan ==
== Test Plan ==

Revision as of 22:54, 29 November 2021

Team

  • Akash Sarda, aksarda
  • Sairam Sakhamuri, svsakham
  • Sidhant Arora, sarora22
  • Sai Harsha Nadendla, snadend2

Import and Export Functionality

In Expertiza, many kinds of files can be imported or exported: lists of users for whom accounts are to be created, lists of teams that have been created or need to be created, lists of topics that users are to be able to sign up for, or instructor or peer scores that have been given for a particular assignment. Historically, all of these import and export methods were written individually, using similar code patterns. The instructor might end up adding those in excel or having that data in excel - this functionality allows the expertiza to accept that csv file and tabulate it infront of the user. The user can then select to assign columns to that data and then import it into the database. Eg. list of topics or feedback comments from the students can be tabular if it is say a MCQ questionnaire. The instructor will then have to either enter each entry manually through the UI or can upload this file to automate it.

The export functionality is however is already quite refactored to suit the strategy pattern, which is not changed by the previous group as well.

Problem Description

Initially the different classes (Topics, Assignments, Course Participants) had different implementations of taking in the csv file which was tightly coupled with the feature / model class. However, it was discovered that instead of defining the new method again and again, this process can be automated, by pushing common code of reading the headers and writing to the database into a single generic function. This would basically make it really easy to add this functionality to other model classes and to the new features yet to come to Expertiza.

Existing Import Functionality

The current import functionality is done through the ImportFileController and there are different import class methods implemented in different models that are performing import function.

In the SignUpTopic and User models use helper classes and the attributes are taken from a hash and new ActiveRecord object is created.

Controllers

  • import_file_controller, methods:
    • Methods used to process files:
      • #get_delimiter - Sets delimiter for filetype
      • #parse_line - Processes line of the file
      • #parse_to_grid - parses the file into 2D array
      • #parse_to_hash - parses file into hash where 'header' stores header row and 'body' stores all contents.
      • #hash_rows_with_headers - Creates hash for each row of file. Keys are headers, values are row values.
    • Import methods:
      • #import_from_hash - Primary import functionality. Creates objects for hashed rows (from #hash_rows_with_headers).
      • #import - Larger controller of import, sets error messages and displays.

Helpers

  • import_file_helper, the list of methods in the file are the following:
    • ::define_attributes - Sets and returns attributes for User object from hash.
    • ::create_new_user - Makes a user object in the database.
  • import_topics_helper, the list of methods in the file are the following:
    • ::define_attributes - Sets and returns attributes for a SignUpTopic from hash.
    • ::create_new_sign_up_topic - Makes SignUpTopic objects in the database.

Models

We have found that the below mentioned models are using the import functionality defined in ImportFileController. SignUpTopic and User are dependent on the helper methods to use import functionality.

  • assignment_participant
  • assignment_team
  • course_participant
  • course_team
  • team
  • metareview_response_map
  • question
  • review_response_map
  • sign_up_sheet
  • sign_up_topic
  • user

Design Plan

The design plan is to use Strategy patter to implement import functionality, so that all the import requests present in different models are routed through the ImportFileController. Right now since the import functionality is implemented in various model methods this leads to many if and else statements to check the type of model. So, we intent to generalize the import functionality by placing common code in the ImportFileController. But each model will still have its own import method. With this approach the redundancy is reduced by moving common code to ImportFileController and code will become DRY.

Other helper methods such as ImportFileHelper and ImportTopicHelper that are used to perform import functionality will also be removed, which keeps import functionality consistent. We will be using method overloading and overriding for the methods in ImportFileController to eliminate unnecessary if and else blocks.

To summarize our plan of changes:

  • Redirect all import calls through ImportFileController
  • Refactor ImportFileController by removing the redundant code and making code generic.
  • Insert object creation conditions into all relevant ::import functions and into the ImportFileController form.


What has been Implemented

Models

  • ImportFileController is changed and has been made more generalized, many case statements are removed. The ImprortFileController is made so small and understandable. The import functionality is moved into the corresponding models. Previously code was present in different modules which was confusing now the functionality was all in one place. The following code changes are follows:
  • Previous Implmentation
  def import_from_hash(session, params)
    if params[:model] == "AssignmentTeam" or params[:model] == "CourseTeam"
      contents_hash = eval(params[:contents_hash])
      @header_integrated_body = hash_rows_with_headers(contents_hash[:header],contents_hash[:body])
      errors = []
      begin
        @header_integrated_body.each do |row_hash|
          if params[:model] == "AssignmentTeam"
            teamtype = AssignmentTeam
          else
            teamtype = CourseTeam
          end
          options = eval(params[:options])
          options[:has_teamname] = params[:has_teamname]
          Team.import(row_hash, params[:id], options, teamtype)
        end
      rescue
        errors << $ERROR_INFO
      end
      elsif params[:model] == "ReviewResponseMap"
        contents_hash = eval(params[:contents_hash])
        @header_integrated_body = hash_rows_with_headers(contents_hash[:header],contents_hash[:body])
        errors = []
        begin
          @header_integrated_body.each do |row_hash|
            ReviewResponseMap.import(row_hash,session,params[:id])
          end
        rescue
          errors << $ERROR_INFO
        end
    elsif params[:model] == "MetareviewResponseMap"
      contents_hash = eval(params[:contents_hash])
      @header_integrated_body = hash_rows_with_headers(contents_hash[:header],contents_hash[:body])
      errors = []
      begin
        @header_integrated_body.each do |row_hash|
          MetareviewResponseMap.import(row_hash,session,params[:id])
        end
      rescue
        errors << $ERROR_INFO
      end
    elsif params[:model] == 'SignUpTopic' || params[:model] == 'SignUpSheet'
      contents_hash = eval(params[:contents_hash])
      if params[:has_header] == 'true'
        @header_integrated_body = hash_rows_with_headers(contents_hash[:header],contents_hash[:body])
      else
        if params[:optional_count] == '0'
          new_header = [params[:select1], params[:select2], params[:select3]]
          @header_integrated_body = hash_rows_with_headers(new_header,contents_hash[:body])
        elsif params[:optional_count] == '1'
          new_header = [params[:select1], params[:select2], params[:select3], params[:select4]]
          @header_integrated_body = hash_rows_with_headers(new_header,contents_hash[:body])
        elsif params[:optional_count] == '2'
          new_header = [params[:select1], params[:select2], params[:select3], params[:select4], params[:select5]]
          @header_integrated_body = hash_rows_with_headers(new_header,contents_hash[:body])
        elsif params[:optional_count] == '3'
          new_header = [params[:select1], params[:select2], params[:select3], params[:select4], params[:select5], params[:select6]]
          @header_integrated_body = hash_rows_with_headers(new_header,contents_hash[:body])
        end
      end
      errors = []
      begin
        @header_integrated_body.each do |row_hash|
          session[:assignment_id] = params[:id]
          Object.const_get(params[:model]).import(row_hash, session, params[:id])
        end
      rescue
        errors << $ERROR_INFO
      end
    elsif params[:model] == 'AssignmentParticipant' || params[:model] == 'CourseParticipant'
      contents_hash = eval(params[:contents_hash])
      if params[:has_header] == 'true'
        @header_integrated_body = hash_rows_with_headers(contents_hash[:header], contents_hash[:body])
      else
        new_header = [params[:select1], params[:select2], params[:select3], params[:select4]]
        @header_integrated_body = hash_rows_with_headers(new_header, contents_hash[:body])
      end
      errors = []
      begin
        if params[:model] == 'AssignmentParticipant'
          @header_integrated_body.each do |row_hash|
            AssignmentParticipant.import(row_hash, session, params[:id])
          end
        elsif params[:model] == 'CourseParticipant'
          @header_integrated_body.each do |row_hash|
            CourseParticipant.import(row_hash, session, params[:id])
          end
        end
      rescue
        errors << $ERROR_INFO
      end
    else # params[:model] = "User"
      contents_hash = eval(params[:contents_hash])
      if params[:has_header] == 'true'
        @header_integrated_body = hash_rows_with_headers(contents_hash[:header],contents_hash[:body])
      else
        new_header = [params[:select1], params[:select2], params[:select3]]
        @header_integrated_body = hash_rows_with_headers(new_header, contents_hash[:body])
      end
      errors = []
      begin
        @header_integrated_body.each do |row_hash|
          User.import(row_hash, nil, session)
        end
      rescue StandardError
        errors << $ERROR_INFO
      end
  end
  • Current Implementation
  def import
    errors = import_from_hash(session, params)
    err_msg = "The following errors were encountered during import.Other records may have been added. A second submission will not duplicate these records."
    errors.each do |error|
      err_msg = err_msg + error.to_s
    end
    err_msg += </ul>
    if errors.empty?
      ExpertizaLogger.info LoggerMessage.new(controller_name, session[:user].name, "The file has been successfully imported.", request)
      undo_link("The file has been successfully imported.")
    else
      ExpertizaLogger.error LoggerMessage.new(controller_name, session[:user].name, err_msg, request)
      flash[:error] = err_msg
    end
    redirect_to session[:return_to]
  end
  • Assignment Participant is changed to use the newly implemented #import functionality. Changes are as shown below:
  def self.import(row_hash, session, id)
    raise ArgumentError, "Record does not contain required items." if row_hash.length < self.required_import_fields.length
    user = User.find_by(name: row_hash[:name])
    user = User.import(row_hash, session, nil) if user.nil?
    raise ImportError, "The assignment with id #{id} was not found." if Assignment.find(id).nil?
    unless AssignmentParticipant.exists?(user_id: user.id, parent_id: id)
      new_part = AssignmentParticipant.new(user_id: user.id, parent_id: id)
      new_part.set_handle
    end
  end

  def self.required_import_fields
    {"name" => "Name",
     "fullname" => "Full Name",
     "email" => "Email"}
  end

  def self.optional_import_fields(id=nil)
    {}
  end

  def self.import_options
    {}
  end
  • Assignment Team is also changed to use the newly implemented #import functionality.
  def self.import(row_hash, session = nil, id, options)
    raise ArgumentError, "Record does not contain required items." if row_hash.length < self.required_import_fields.length
    raise ImportError, "The assignment with the id \"" + id.to_s + "\" was not found. <a href='/assignment/new'>Create</a> this assignment?" if Assignment.find_by(id: id).nil?
    Team.import_helper(row_hash, id, options, prototype)
  end

  def self.required_import_fields
    {"teammembers" => "Team Members"}
  end

  def self.optional_import_fields(id=nil)
    {"teamname" => "Team Name"}
  end

  def self.import_options
     {"handle_dups" => {"display" => "Handle Duplicates",
                      "options" => {"ignore" => "Ignore new team name",
                                      "replace" => "Replace the existing team with the new team",
                                      "insert" => "Insert any new team members into the existing team",
                                      "rename" => "Rename the new team and import"}}}
  end
  • Course_team is is also changed to use the newly implemented #import functionality.
def self.import(row_hash, session, id, options)
      raise ArgumentError, "Record does not contain required items." if row_hash.length < self.required_import_fields.length
      raise ImportError, "The course with the id \"" + id.to_s + "\" was not found. <a href='/course/new'>Create</a> this course?" if Course.find(id).nil?
      Team.import_helper(row_hash, id, options, prototype)
  end

  def self.required_import_fields
      {"teammembers" => "Team Members"}
  end

  def self.optional_import_fields(id=nil)
      {"teamname" => "Team Name"}
  end

  def self.import_options
      {"handle_dups" => {"display" => "Handle Duplicates",
                         "options" => {"ignore" => "Ignore new team name",
                                       "replace" => "Replace the existing team with the new team",
                                       "insert" => "Insert any new team members into the existing team",
                                       "rename" => "Rename the new team and import"}}}
  end
  • Review response map's #import functionality is also updated to use the newly implemented #import functionality.

  def self.import(row_hash, _session, assignment_id)
    raise ArgumentError, "Record does not contain required items." if row_hash.length < self.required_import_fields.length
    reviewee_user_name = row_hash[:reviewee].to_s
    reviewee_user = User.find_by(name: reviewee_user_name)
    raise ArgumentError, "Cannot find reviewee user." unless reviewee_user
	@@ -57,7 +58,7 @@ def self.import(row_hash, _session, assignment_id)
      team_node = TeamNode.create(parent_id: assignment_id, node_object_id: reviewee_team.id)
      TeamUserNode.create(parent_id: team_node.id, node_object_id: t_user.id)
    end
    row_hash[:reviewers].split.each do |reviewer|
      reviewer_user_name = reviewer.to_s
      reviewer_user = User.find_by(name: reviewer_user_name)
      raise ArgumentError, "Cannot find reviewer user." unless reviewer_user
	@@ -71,6 +72,20 @@ def self.import(row_hash, _session, assignment_id)
    end
  end


With these new changes to #import functionality. The following modules review_response_map.rb, course_team.rb,assignment_team.rb,assignment_participant.rb have the "self.import" which will use the import functionality in the ImportFileController and they are made as concise as possible. Each module has specific checks while importing the file, so we can't have same code for all the module.

Test Plan

The import method has been implemented in a bunch of models which have been listed above. After preliminary analysis, we assume that the import functionality in a few of the models might have to be amended. These models are:

  • assignment_participant
  • assignment_team
  • course_participant
  • course_team
  • team
  • metareview_response_map
  • review_response_map
  • sign_up_topic
  • user

We will update the test plan for those models in which the import code is amended to fit into the new framework. If we amend the import code in any other model, we will also update the tests in their respective .spec files to ensure 100% coverage. We also plan to create a spec file for the import_file_controller.

Scenarios:

  1. when assignment found and assignment participant does not exist, creates a new user and participant
  2. when assignment cannot be found, creates a new user then raises an ImportError
  3. when the assignment team does not have the required fields, raises ArgumentError
  4. check what import/export actions are allowed for admin, instructor, TA, student
  5. when course found and course participant does not exist, creates a new user and participant
  6. when the course team does not have the required fields, raises ArgumentError