CSC517 Spring 2019/E1923 New Framework For Import/Export: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
m (→‎Export Changes: fix typo)
 
(65 intermediate revisions by one other user not shown)
Line 1: Line 1:
== Project Introduction ==
== Project Introduction ==
=== Team ===
=== Team ===
*Drew Marshburn, rdmarshb
*Veeha Khanna, vkhanna
*Veeha Khanna, vkhanna
*Randy Paluszkiewicz, rpalusz
*Randy Paluszkiewicz, rpalusz
*Drew Marshburn, rdmarshb
*Andrew Miller, asmille5
*Andrew Miller, asmille5
Link to our video:
https://youtu.be/1nSnjta_zIc


== Project Purpose ==
== Project Purpose ==
Line 19: Line 22:
== Existing Import Functionality ==
== Existing Import Functionality ==


Existing import functionality is primarily routed through the import_file_controller and an import class method for various models.
Existing import functionality is primarily routed through the ImportFileController and an import class method for various models.


SignUpTopic and User, rely on helper classes that extract attributes from a hash and create an ActiveRecord object.
SignUpTopic and User, rely on helper classes that extract attributes from a hash and create an ActiveRecord object.
Line 27: Line 30:
=== Controllers ===
=== Controllers ===


*'''import_file_controller'''
*'''import_file_controller''', the list of methods in the controller are the following:
** File processing methods:
** File processing methods:
*** #get_delimiter - Sets proper delimiter for filetype
*** #get_delimiter - Sets proper delimiter for filetype
Line 38: Line 41:
*** #import - Larger controller of import, sets error messages and displays.
*** #import - Larger controller of import, sets error messages and displays.


*'''questionnaires_controller'''
*'''questionnaires_controller''', the list of methods in the controller are the following:
** ::import - Allows import from CSV using QuestionnaireHelper (Appears to be deprecated/unused)
** ::import - Allows import from CSV using QuestionnaireHelper (Appears to be deprecated/unused)


=== Helpers ===
=== Helpers ===
*'''import_file_helper'''
*'''import_file_helper''', the list of methods in the file are the following:
** ::define_attributes - Sets and returns attributes for User object from hash.
** ::define_attributes - Sets and returns attributes for User object from hash.
** ::create_new_user - Makes a user object in the database.
** ::create_new_user - Makes a user object in the database.


*'''import_topics_helper'''
*'''import_topics_helper''', the list of methods in the file are the following:
** ::define_attributes - Sets and returns attributes for a SignUpTopic from hash.
** ::define_attributes - Sets and returns attributes for a SignUpTopic from hash.
** ::create_new_sign_up_topic - Makes SignUpTopic objects in the database.
** ::create_new_sign_up_topic - Makes SignUpTopic objects in the database.


*'''questionnaire_helper''' (Appears to be deprecated/unused)
*'''questionnaire_helper''' (Appears to be deprecated/unused), the list of methods in the file are the following:
** ::get_questions_from_csv - Allows Question and QuestionAdvice import/creation with CSV file.
** ::get_questions_from_csv - Allows Question and QuestionAdvice import/creation with CSV file.


=== Models ===
=== Models ===


All models have an ::import method called by the ImportFileController. In addition, SignUpTopic and User objects rely on their helpers, which are mentioned above.
All these models have an ::import method called by the ImportFileController. In addition, SignUpTopic and User objects rely on their helpers, which are mentioned above.


*'''assignment_participant'''
*'''assignment_participant'''
Line 71: Line 74:
== Proposed Import Changes ==
== Proposed Import Changes ==


The import functionality should be routed from the ImportFileController. Everything should still work as it does now after our changes. Luckily, most import functionality is routed through the ImportFileController and import class methods on models that are being imported. It makes sense for each model to have its own import method because each model is know what to expect for itself. Making things overly generic will end up contradicting the DRY principle.  
The import functionality should be routed through the ImportFileController. Everything should still work as it does now after our changes; we are cleaning up code and may move functionality to a different location but nothing will get removed permanently. Luckily, most import functionality is routed through the ImportFileController and import class methods on models that are being imported. It makes sense for each model to have its own import method because each model knows what to expect for itself. Making things overly generic will end up contradicting the DRY principle and lead to many nested if statements.  


We intend to remove other helpers and files, such as the ImportFileHelper, the ImportTopicHelper, and the QuestionnaireHelper to keep import routing consistent. We will also remove the QuestionnaireController and route that import through the ImportFileController. In addition, we will update ImportFileController methods (in particular, #hash_rows_with_headers and #import_from_hash) to make better use of polymorphism and eliminate large and unnecessary if/else blocks.
We intend to remove other helpers and files, such as the ImportFileHelper, the ImportTopicHelper, and the QuestionnaireHelper to keep import routing consistent. We will also remove the QuestionnaireController and route that import through the ImportFileController. In addition, we will update ImportFileController methods (in particular, #hash_rows_with_headers and #import_from_hash) to make better use of polymorphism and eliminate large and unnecessary if/else blocks.
Line 83: Line 86:
* Insert object creation conditions into all relevant ::import functions and into the ImportFileController form.
* Insert object creation conditions into all relevant ::import functions and into the ImportFileController form.
* Add ability for ImportFileController to import Question objects for Questionnaires. (Add question view to views/import_file, introduce Question logic into the controller.)
* Add ability for ImportFileController to import Question objects for Questionnaires. (Add question view to views/import_file, introduce Question logic into the controller.)
** Note: Why does QuizQuestionnaire have a different view?


Visually this is what is happening right now (with some files missing for brevity's sake):
Visually this is what is happening right now (with some files missing for brevity's sake):
Line 93: Line 95:
[[Image: goodUML.png]]
[[Image: goodUML.png]]


=== What has been done so far ===
== What has been done==
We have rerouted how the Questionnaire import works so that it now gets routed to the ImportFileController instead of the questionnaires_controller. We have asked our mentor for some test files to try importing and seeing if what we did works.  
=== Models ===
*In the shared.js file there was a method #checkIfUserColumnDuplicate() which we changed to #checkForDuplicates(field_count). This method now checks to make sure that each column header is unique regardless of which model you are in.
 
*In the ImportFileController we moved the #start method to the beginning of the file for consistency and readability.
 
*The #import_from_hash and the #show method inside the ImportFileController is markedly shorter now then when we found it. We removed most of the case statements. The below images are in the respective order.
 
[[Image: importmethod.png]]
 
[[Image: showmethod.png]]
 
*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.
 
*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
 
=== Views ===
 
*'''start.html.erb''' file has no more case statements by the @model type and everything has become generic.
 
*We removed all the partials related to import_file. Which has lead the '''show.html.erb''' file to have no more case statements and everything has become generic. The removed files are:
 
- _metareviewer.html.erb
 
- _participant.html.erb
 
- _reviewer.html.erb
 
- _sign_up_topic.html.erb


We have also started refactoring the #show and #import_from_hash. The first model that we will refactor is '''SignUpTopic''' since it is the most complicated and messy. There is a lot of hard coded information in these files so there is only so much we can do without breaking everything. Our goal is it mimic the current functionality in a cleaner more DRY way.  
- _team.html.erb


- _user.html.erb


In the #show method this is what we wanted to clean up:
*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.


[[Image: show.png]]
=== 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':


What is happening here is that there are three optional columns that the imported file could have. To know if they are a part of that file the user checks a check box; when this happens this bit of code counts how many extra columns there are by incrementing '@optional_count' by one. This is very messy and not good coding practice.  
* Method get_questions_from_csv has a Cognitive Complexity of 62 (exceeds 5 allowed). Consider refactoring.


This is how we have changed it:
* Method get_questions_from_csv has 41 lines of code (exceeds 25 allowed). Consider refactoring.


[[Image: newShow.png]]
* File import_file_controller.rb has 278 lines of code (exceeds 250 allowed). Consider refactoring.


Now for loop something.  
* Avoid deeply nested control flow statements. (3)


In the #import_from_hash this is what we wanted to clean up:
* Similar blocks of code found in 2 locations. Consider refactoring. (2)


[[Image: hash.png]]
* Unescaped parameter value


What is happening here is that depending on the value of '@optional_count' the appropriate number of headers will be generated when parsing the file (given a header is not present) and then #hash_row_with_headers is called. This is again very repetitive and long.  
* Useless assignment to variable - a.


This is what we have changed it to:
* Cyclomatic complexity for get_questions_from_csv is too high. [18/6]


[[Image: newHash.png]]
* Assignment Branch Condition size for get_questions_from_csv is too high. [43.3/15]


== Existing Export Functionality ==
* Block has too many lines. [36/25]


These are the files that contain an method with export in the name. The Export functionality, unlike the Import functionality, is different in every place it was implemented.  
* Avoid more than 3 levels of block nesting. (3)


=== Controllers ===
* 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)


*'''export_file_controller'''
* Convert if nested inside else to elsif.
** File processing methods:
*** #find_delim_filename - Find delimiter and filename
*** #start - Assign titles to models for display
** Export methods:
*** #exportdetails - Exports details from assignments
*** #export - Exports the options parameter for the following models: Assignment, AssignmentParticipant, AssignmentTeam, CourseParticipant, CourseTeam, MetareviewResponseMap, Question, ReviewResponseMap, User, Team
*** #export_advice - Exports question advice data to CSV file


* Don't use parentheses around the condition of an if. (3)


*'''106_update_controller_for_export'''
== Export Changes ==
** ::up - It seems to update permissions for export by for administrative assignments
** ::down - Empty method


=== Helpers ===
The goal was to get all the export functionality routed through one place, the ExportFileController. The overall effect would have been similar to what we hope to accomplish when updating the import functionality.


*'''export_file_helper'''
We ran out of time to work on this functionality.
** It is empty


=== Models ===
== Test Plan ==
'''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.'''


All models have some kind of an ::export method:
All these models have an ::import method that we amended and as a result we need to update the tests. We added or amended tests in their respective .spec files to make sure that the ::import method has 100% coverage.


*'''assignment'''
*'''assignment_participant'''
*'''assignment_team'''
*'''assignment_team'''
*'''course_participant'''
*'''course_team'''
*'''course_team'''
*'''team'''
*'''metareview_response_map'''
*'''metareview_response_map'''
*'''participant'''
*'''question'''
*'''review_response_map'''
*'''review_response_map'''
*'''sign_up_topic'''
*'''user'''
*'''user'''
*'''team'''
*'''question_advice'''
== Proposed Export Changes ==
The goal is to get all the export functionality routed through one place, the ExportFileController. The overall effect will be similar to what we hope to achomplish when updating the Import functionality.
We will work on Import before we start with Export.
== Test Plan ==
Every file we are amending has test cases corresponding to the import/export functionalities. Since we are rearranging import/export functionality and not re-creating it tests will similarly be rearranged so they keep passing and are being represented in the correct file.


If we end up adding new functionality we will add new tests accordingly, we do not foresee a need for this yet.
There is one failing test about exporting and we have not touched export functionality. There is another failing test about a review tab that we didn't touch.


== Deployment ==
== Deployment ==


The project is deployed on an NCSU VCL server. It can be accessed at http://152.46.16.252:8080/.
The project is deployed on an NCSU VCL server. It can be accessed at http://152.46.17.230:8080/.


A sample instructor login is:
A sample instructor login is:

Latest revision as of 06:46, 27 March 2024

Project Introduction

Team

  • Drew Marshburn, rdmarshb
  • Veeha Khanna, vkhanna
  • Randy Paluszkiewicz, rpalusz
  • Andrew Miller, asmille5

Link to our video: https://youtu.be/1nSnjta_zIc

Project Purpose

The export/import feature is the most helpful feature for instructors to set up assignments. The instructors usually have a list of students, teams, etc from their learning management system. Being able to export/import these into expertiza saves a lot of time when setting up an assignment.


Expertiza provides multiple export and import features for eg. export students, teams etc. Essentially what it does is it fetches some data from database and save it as a file in desired format. However, same functionality is implemented multiple times for exporting and importing different things.

The aim of this project is to design a generic export/import feature. What you need to do is implement a framework which supports exporting and importing functionality based on input. There are substantial degrees of freedom in this project. You need to design the module which will take the database table names, column names from which data needs to be exported or the imported into. So that this module can be used for every export/import feature that expertiza provides.

Most imports and exports just import or export a single table. But sometimes exports involve “details.” For example, when questionnaires are imported or exported, the “advice” (guidelines on what characteristics of work merit each score) may be imported or exported along with the questionnaire. I suspect that all instances of import/export of multiple tables are called “details” in the current code. Provide a mechanism so that “details” can be imported or exported along with the main table involved.

Existing Import Functionality

Existing import functionality is primarily routed through the ImportFileController and an import class method for various models.

SignUpTopic and User, rely on helper classes that extract attributes from a hash and create an ActiveRecord object.

Questionnaire relies on a helper method that can import Question objects (objects that make up a Questionnaire) from a CSV and adjust the size of the associated QuestionAdvice (the words that pop up after you pick a certain number of stars). However, these functions might be deprecated, as it appears that Question importing is now routed through the ImportFileController unsuccessfully. More detail about specific functions is provided below.

Controllers

  • import_file_controller, the list of methods in the controller are the following:
    • File processing methods:
      • #get_delimiter - Sets proper delimiter for filetype
      • #parse_line - Processes line (row) of the file
      • #parse_to_grid - Turns file into 2D array
      • #parse_to_hash - Turns 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.
  • questionnaires_controller, the list of methods in the controller are the following:
    • ::import - Allows import from CSV using QuestionnaireHelper (Appears to be deprecated/unused)

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.
  • questionnaire_helper (Appears to be deprecated/unused), the list of methods in the file are the following:
    • ::get_questions_from_csv - Allows Question and QuestionAdvice import/creation with CSV file.

Models

All these models have an ::import method called by the ImportFileController. In addition, SignUpTopic and User objects rely on their helpers, which are mentioned above.

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

Proposed Import Changes

The import functionality should be routed through the ImportFileController. Everything should still work as it does now after our changes; we are cleaning up code and may move functionality to a different location but nothing will get removed permanently. Luckily, most import functionality is routed through the ImportFileController and import class methods on models that are being imported. It makes sense for each model to have its own import method because each model knows what to expect for itself. Making things overly generic will end up contradicting the DRY principle and lead to many nested if statements.

We intend to remove other helpers and files, such as the ImportFileHelper, the ImportTopicHelper, and the QuestionnaireHelper to keep import routing consistent. We will also remove the QuestionnaireController and route that import through the ImportFileController. In addition, we will update ImportFileController methods (in particular, #hash_rows_with_headers and #import_from_hash) to make better use of polymorphism and eliminate large and unnecessary if/else blocks.

We will also insert more specific object creation specifications. Consider if a user's import contains objects that do not exist in the database. We will provide an option for the user to specify whether or not new/dependent objects should be created and account for this boolean in relevant ::import functions for the models.

To summarize our changes:

  • Route all import traffic through ImportFileController and ::import calls on models.
  • Refactor ImportFileController.
  • Insert object creation conditions into all relevant ::import functions and into the ImportFileController form.
  • Add ability for ImportFileController to import Question objects for Questionnaires. (Add question view to views/import_file, introduce Question logic into the controller.)

Visually this is what is happening right now (with some files missing for brevity's sake):

What we want it to look like:

What has been done

Models

  • In the shared.js file there was a method #checkIfUserColumnDuplicate() which we changed to #checkForDuplicates(field_count). This method now checks to make sure that each column header is unique regardless of which model you are in.
  • In the ImportFileController we moved the #start method to the beginning of the file for consistency and readability.
  • The #import_from_hash and the #show method inside the ImportFileController is markedly shorter now then when we found it. We removed most of the case statements. The below images are in the respective order.

  • 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.
  • 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:
 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

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

Views

  • start.html.erb file has no more case statements by the @model type and everything has become generic.
  • We removed all the partials related to import_file. Which has lead the show.html.erb file to have no more case statements and everything has become generic. The removed files are:

- _metareviewer.html.erb

- _participant.html.erb

- _reviewer.html.erb

- _sign_up_topic.html.erb

- _team.html.erb

- _user.html.erb

  • 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)

Export Changes

The goal was to get all the export functionality routed through one place, the ExportFileController. The overall effect would have been similar to what we hope to accomplish when updating the import functionality.

We ran out of time to work on this functionality.

Test Plan

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.

All these models have an ::import method that we amended and as a result we need to update the tests. We added or amended tests in their respective .spec files to make sure that the ::import method has 100% coverage.

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

There is one failing test about exporting and we have not touched export functionality. There is another failing test about a review tab that we didn't touch.

Deployment

The project is deployed on an NCSU VCL server. It can be accessed at http://152.46.17.230:8080/.

A sample instructor login is:

  • Username: instructor6
  • Password: password