Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Spring 2021

2021-03-11T22:25:51Z

Svshingt: Created page with "== OSS Projects == * CSC/ECE 517 Spring 2021 - E2106. Fix view in student_task/list page"

== OSS Projects ==
* [[CSC/ECE 517 Spring 2021 - E2106. Fix view in student_task/list page]]

MainPage

2021-03-11T22:24:37Z

Svshingt: /* Expertiza */

==Expertiza==
* [[Expertiza documentation]]

* [[CSC/ECE 517 Summer 2008]]
* [[CSC/ECE 517 Fall 2010]]
* [[CSC/ECE 517 Fall 2011]]
* [[CSC/ECE 517 Fall 2012]]
* [[CSC/ECE 517 Fall 2013]]
* [[CSC/ECE 517 Fall 2014]]
* [[CSC/ECE 517 Fall 2015]]
* [[CSC/ECE 517 Fall 2016]]
* [[CSC/ECE 517 Spring 2014]]
* [[CSC/ECE 517 Spring 2015]]
* [[CSC/ECE 517 Spring 2016]]
* [[CSC/ECE 517 Spring 2017]]
* [[CSC/ECE 517 Fall 2017]]
* [[CSC/Independent Study Spring 2018]]
* [[CSC/ECE 517 Spring 2018]]
* [[CSC/ECE 517 Fall 2018]]
* [[CSC/ECE 517 Spring 2019]]
* [[CSC/ECE 517 Fall 2019]]
* [[CSC/ECE 517 Spring 2020]]
* [[CSC/ECE 517 Summer 2020]]
* [[CSC/ECE 517 Fall 2020]]
* [[CSC/ECE 517 Spring 2021]]
* [[CSC 456 Spring 2011|CSC 456 Spring 2012]]
* [[ECE 633]]
* [[KCU]]
* [[Progress reports]]
* [[ReactJs Frontend]]
* [[Front-End/Back-End]]

==Application Behavior==
* [[Grading]]

==Metaprogramming==
* [[CSC/ECE_517_Spring_2013/ch1b_1k_hf|Lecture on Metaprogramming]]

==Development==

''Expertiza now has a Java dependency, so the machine you are using to develop Expertiza on should have the JVM installed.''

* [[Setting Up a Development Machine]]
* [[Creating a Linux Development Environment for Expertiza - Installation Guide]]
* [[Using git and github for projects]]
* [[Using heroku to deploy your projects]]
* [[How to Begin a Project from the Current Expertiza Repository]]
* [[Git]]
* [[How to Change a User's Password on a Development Machine]]
* [[Debugging Rails]]
* [http://rajanalwan.com/ui_guidelines/ Design Template]

==Production==
* [[Deploying to Production]]
* [[Downloading Production Data]]
* [[Accessing the Production Server]]

==Testing==
* [[Using Cucumber with Expertiza]]
* [[Rails Testing Overview]]
* [[Expertiza Continuous Integration]]

==References==
* [[Object-Oriented Design and Programming]]

CSC/ECE 517 Fall 2019 - E1991 Improvements to anonymized view

2021-03-07T22:07:20Z

Svshingt: Blanked the page

CSC/ECE 517 Fall 2020

2020-11-20T15:13:06Z

Svshingt:

== Writing Assignments ==
* [[CSC/ECE 517 Fall 2020 - E2070. Refactor response_controller.rb]]
* [[CSC/ECE 517 Fall 2020 - E2068. Refactor quiz_questionnaires_controller.rb]]
* [[CSC/ECE 517 Fall 2020 - E2062. Add test cases to review_mapping_helper.rb]]
* [[CSC/ECE 517 Fall 2020 - E2063. Refactor tree-display.js and tree_display_controller.rb]]
* [[CSC/ECE 517 Fall 2020 - E2064. Refactor reputation_web_service_controller.rb]]
* [[CSC/ECE 517 Fall 2020 - E2065. Fix view in student_task/list page]]
* [[CSC/ECE 517 Fall 2020 - E2066. Refactor lottery controller.rb]]
* [[CSC/ECE 517 Fall 2020 - E2067. Refactor student_teams_controller.rb]]
* [[CSC/ECE 517 Fall 2020 - E2073. Refactor course_controller.rb]]
* [[CSC/ECE 517 Fall 2020 - E2057. Restrict TAs’ ability to change their own grade and limit file-size upload]]
* [[CSC/ECE 517 Fall 2020 - E2052. Remove multiple topics at a time]]
* [[CSC/ECE 517 Fall 2020 - E2052. Remove multiple topics at a time]]
* [[CSC/ECE 517 Fall 2020 - E2056 Account_Request_Controller.rb]]
* [[CSC/ECE 517 Fall 2020 - E2059. Email notification to reviewers and instructors.rb]]
* [[CSC/ECE 517 Fall 2020 - E2058. Two issues related to assignment management]]
* [[CSC/ECE 517 Fall 2020 - E2071. Improve assessment360_controller.rb]]
* [[CSC/ECE 517 Fall 2020 - E2054. Auto generate submission directory names based on assignment names]]
* [[CSC/ECE 517 Fall 2020 - E2060. Review report should link to the usual view for reviews]]

== Final Project Design Documents ==
* [[CSC/ECE 517 Fall 2020 - E2082. Track time between successive tag assignments]]
* [[CSC/ECE 517 Fall 2020 - E2078. Improve self-review Link peer review & self-review to derive grades]]
* [[CSC/ECE 517 Fall 2020 - E2083. Revision_planning_tool_E2016]]
* [[CSC/ECE 517 Fall 2020 - E2085. Allow reviewers to bid on what to review]]
* [[CSC/ECE 517 Fall 2020 - E2086. Let course staff as well as students do reviews]]
* [[CSC/ECE 517 Fall 2020 - SQLFE. Refactor Submission.java]]
* [[CSC/ECE 517 Fall 2020 - E2075. calibration submissions should be copied along with calibration assignments]]
* [[CSC/ECE 517 Fall 2020 - E2076. Allow users to create an account and submit work to an "assignment" (e.g., for conference reviewing)]]
* [[CSC/ECE 517 Fall 2020 - E2080. Track the time students look at other submissions]]
* [[CSC/ECE 517 Fall 2020 - E2081. Add a "cake" item type to rubrics]]
* [[CSC/ECE 517 Fall 2020 - E2079. Improve Search Facility In Expertiza]]
* [[CSC/ECE 517 Fall 2020 - E2087. Conflict notification. Improve Search Facility In Expertiza]]
* [[CSC/ECE 517 Fall 2020 - E2084 sort instructor reports by name ID score etc]]

CSC/ECE 517 Fall 2020

2020-09-30T19:29:40Z

Svshingt: Created page with "* Dummy URL"

* [[Dummy URL]]

MainPage

2020-09-30T19:28:30Z

Svshingt: /* Expertiza */

==Expertiza==
* [[Expertiza documentation]]

* [[CSC/ECE 517 Summer 2008]]
* [[CSC/ECE 517 Fall 2010]]
* [[CSC/ECE 517 Fall 2011]]
* [[CSC/ECE 517 Fall 2012]]
* [[CSC/ECE 517 Fall 2013]]
* [[CSC/ECE 517 Fall 2014]]
* [[CSC/ECE 517 Fall 2015]]
* [[CSC/ECE 517 Fall 2016]]
* [[CSC/ECE 517 Spring 2014]]
* [[CSC/ECE 517 Spring 2015]]
* [[CSC/ECE 517 Spring 2016]]
* [[CSC/ECE 517 Spring 2017]]
* [[CSC/ECE 517 Fall 2017]]
* [[CSC/Independent Study Spring 2018]]
* [[CSC/ECE 517 Spring 2018]]
* [[CSC/ECE 517 Fall 2018]]
* [[CSC/ECE 517 Spring 2019]]
* [[CSC/ECE 517 Fall 2019]]
* [[CSC/ECE 517 Spring 2020]]
* [[CSC/ECE 517 Fall 2020]]
* [[CSC 456 Spring 2011|CSC 456 Spring 2012]]
* [[ECE 633]]
* [[KCU]]
* [[Progress reports]]
* [[ReactJs Frontend]]
* [[Front-End/Back-End]]

==Application Behavior==
* [[Grading]]

==Metaprogramming==
* [[CSC/ECE_517_Spring_2013/ch1b_1k_hf|Lecture on Metaprogramming]]

==Development==

''Expertiza now has a Java dependency, so the machine you are using to develop Expertiza on should have the JVM installed.''

* [[Setting Up a Development Machine]]
* [[Creating a Linux Development Environment for Expertiza - Installation Guide]]
* [[Using git and github for projects]]
* [[Using heroku to deploy your projects]]
* [[How to Begin a Project from the Current Expertiza Repository]]
* [[Git]]
* [[How to Change a User's Password on a Development Machine]]
* [[Debugging Rails]]
* [http://rajanalwan.com/ui_guidelines/ Design Template]

==Production==
* [[Deploying to Production]]
* [[Downloading Production Data]]
* [[Accessing the Production Server]]

==Testing==
* [[Using Cucumber with Expertiza]]
* [[Rails Testing Overview]]
* [[Expertiza Continuous Integration]]

==References==
* [[Object-Oriented Design and Programming]]

Due dates

2020-06-11T21:10:31Z

Svshingt: /* Due Dates Variable Documentation */

The Due Dates table gives information regarding the due dates associated to each of the assignment.
== Due Dates Variable Documentation ==

{| class="wikitable"
!Field Name !!Type !!Description
|-
!id
|int(11)
|Unique ID for each Due Date record
|-
!due_at
|datetime
|Deadline for submission
|-
!deadline_type_id
|int(11)
|whether a submission deadline, a review deadline, etc.
|-
!parent_id
|int(11)
|the assignment that the deadline is related to
|-
!submission_allowed_id
|int(11)
|this is "OK", "Late" or "No", depending on whether submission is allowed before this due date
|-
!review_allowed_id
|int(11)
|this is "No", "Late" or "OK", depending on whether reviewing is allowed before this due date
|-
!review_of_review_allowed_id
|int(11)
|this is "OK", "Late" or "No", depending on whether meta-reviewing is allowed before this due date
|-
!round
|int(11)
|Number of review rounds
|-
!flag
|tinyint(1)
|Not clear what this is. It is zero in all due dates except some between 2010/11/15 and 2011/05/25.
|-
!threshold
|int(11)
|How many hours before the due date a reminder is sent to participants
|-
!delayed_job_id
|int(11)
|ID of the delayed job, presumably the job that sends reminders of this due date.
|-
!deadline_name
|varchar(255)
|Name of the deadline. This is either NULL or the empty string in all deadlines.
|-
!description_url
|varchar(255)
|URL of the description
|-
!quiz_allowed_id
|int(11)
|ID of the quiz associated to the Due Date.
|-
!teammate_review_allowed_id
|int(11)
|The ID of the teammate whose review needs to be done.
|-
!type
|int(11)
|Type of the Due Date.
|}

== E/R diagram for Parents Tables ==
Tables referred by the Due Date Table as Foreign Key Relationship.

[[File:due_dates_imported.png]]

== E/R diagram for Child Tables ==
No Tables refer the Due Date Table as Foreign Key Relationship.

Back to [http://wiki.expertiza.ncsu.edu/index.php/Documentation_on_Database_Tables Database Tables] Main page.

Assignment questionnaires

2020-05-27T18:21:21Z

Svshingt:

The assignment_questionnaires table defines the joins between a [[questionnaires|questionnaire]] and an [[assignments|assignment]]. It is also used to associate default limits for the notifications an instructor receives.
==Assignment Questionnaires variable documentation==

{| class="wikitable"
!Field Name !!Type !!Description
|-
!id
|int(11)
|The unique record id
|-
!assignment_id
|int(11)
|The ID of the [[assignments|assignment]] in this relationship. A null value is used to define the default limits
|-
!questionnaire_id
|int(11)
|The ID of the [[questionnaires|questionnaire]] in this relationship. A null value is used to define the default limits
|-
!user_id
|int(11)
|The ID of the [[users|instructor]] who created the relationship
|-
!notification_limit
|int(11)
|A percent difference between an awarded score and the current average score as an integer. This defines at which point the instructor receives a notification that a response falls outside the acceptable range
|-
!questionnaire_weight
|int(11)
|A percentage that the [[questionnaires|questionnaire]] contributes to the overall score. When both assignment_id and questionnaire_id are null, this relationship defines the defaults set by the instructor.
|-
!used_in_round
|int(11)
|Questionnaire used in which particular round.
|-
!dropdown
|tinyint(1)
|Indicating whether a dropdown is present or not.
|}

== E/R diagram for Parent Tables ==
The table Assignment_questionnaire has not referred to any other table.

== E/R diagram for Child Tables ==
Tables which refer the Assignment_questionnaires Table as Foreign Key Relationship.

[[File:assignment_questionnaire_imported.png]]

Back to the [[Documentation_on_Database_Tables|database documentation]]

Response maps

2020-02-08T05:55:13Z

Svshingt: /* Response maps variable documentation */

Maps a connection between the [[participants]] as reviewers and [[participants]] or [[teams]] as reviewees

==Response maps variable documentation==

{| class="wikitable"
!Field Name !!Type !!Description
|-
!id
|int(11)
|The unique record id
|-
!reviewed_object_id
|int(11)
|The object being reviewed in the [[responses|response]]. Possible objects include other ResponseMaps or [[assignments]]
|-
!reviewer_id
|int(11)
|The [[participants|participant]] (actually AssignmentParticipant) providing the response
|-
!reviewee_id
|int(11)
|The [[teams|team]] (AssignmentTeam) receiving the response
|-
!type
|varchar(255)
|Used for subclassing the response map. Available subclasses are ReviewResponseMap, MetareviewResponseMap, FeedbackResponseMap, TeammateReviewResponseMap
|-
!created_at
|DATETIME
|Date and Time for when the record was created
|-
!updated_at
|DATETIME
|Date and Time when the last update was made
|-
!calibrate_to
|tinyint(1)
|Tells whether the record will be used for calibration or not.
|}

Back to the [[Documentation_on_Database_Tables|database documentation]]

Response maps

2020-02-08T05:50:02Z

Svshingt: /* Response maps variable documentation */

Maps a connection between the [[participants]] as reviewers and [[participants]] or [[teams]] as reviewees

==Response maps variable documentation==

{| class="wikitable"
!Field Name !!Type !!Description
|-
!id
|int(11)
|The unique record id
|-
!reviewed_object_id
|int(11)
|The object being reviewed in the [[responses|response]]. Possible objects include other ResponseMaps or [[assignments]]
|-
!reviewer_id
|int(11)
|The [[participants|participant]] (actually AssignmentParticipant) providing the response
|-
!reviewee_id
|int(11)
|The [[teams|team]] (AssignmentTeam) receiving the response
|-
!type
|varchar(255)
|Used for subclassing the response map. Available subclasses are ReviewResponseMap, MetareviewResponseMap, FeedbackResponseMap, TeammateReviewResponseMap
|-
!created_at
|DATETIME
|Date and Time for when the record was created
|-
!updated_at
|DATETIME
|Date and Time when the last update was made
|-
!calibrate_to
|BIT
|Tells whether the record will be used for calibration or not.
|}

Back to the [[Documentation_on_Database_Tables|database documentation]]

CSC/ECE 517 Fall 2019 - E1998. Weights in grade calculation

2019-11-11T23:16:26Z

Svshingt: /* Test Plan */

This wiki page has been created to document the changes that will be made under E1998 - Weights in grade calculation as a part of the final project for Fall 2019, CSC/ECE 517.

== '''About Expertiza''' ==

Expertiza is an open source project which can be used by instructors for a multitude of use cases such as creating assignments of various types, creating rubrics, questionnaires, allowing teams to be created (for group assignments) etc. It also allows grading of the assignments created, and provides the ability to let students peer review other students' work. Additionally, it also allows an instructor to export data for various objects created into multiple formats for further analysis.

== '''Project Overview''' ==

'''Background and Problem Statement'''

The questionnaire/rubric creation feature of Expertiza also allows weights to be added to the questions being created, thereby allowing an instructor to create a questionnaire with relative ordering among the questions from a grading standpoint i.e., correctly answering a question with a higher weight will fetch more marks as compared to correctly answering a question with a lower weight. However, it has been observed that when grading is done for questionnaires, the module responsible for grading seems to ignore the weights associated with questions.

'''Goal of the Project'''

The goal of this project is to understand the cause of the aforementioned issue, and to ensure that the weights associated with questions are taken into consideration when grading all types of questionnaires involving weights.

== '''Current Implementation''' ==

== '''Proposed Solution''' ==

== '''Files which require modification (Tentative)''' ==

== '''Test Plan''' ==

* There are many tests to check the functionalities which get involved in calculating grades using weights
* Currently all the test cases are passing because system considers weights as 1 to pass the validations
* For the future work, we will be writing a new method which will be calculating scores considering weights
* For the private methods, which will return the weighted scores for some particular types of questions, there is no need of tests
* But functions which will return the entire score of an assignment will require tests

== '''Quick Links''' ==

* Forked Repository Link: https://github.com/theSaurabhMhatre/expertiza
* Pull Request Link: To be created
* Demo Link: To be created

== '''Team''' ==

Our team:

*Sanket Pai (sgpai2@ncsu.edu)
*Sanveg Rane (ssrane2@ncsu.edu)
*Saurabh Mhatre (smhatre@ncsu.edu)
*Saurabh Shingte (svshingt@ncsu.edu)

Mentor: Carmen Bentley (cnaiken@ncsu.edu)

CSC/ECE 517 Fall 2019

2019-11-11T20:18:19Z

Svshingt:

* [[CSC/ECE 517 Fall 2019 - Project E1947. Refactor quiz_questionnaire_controller.rb]]
* [[CSC/ECE 517 Fall 2019 - Project E1965. Review report should link to the usual view for reviews]]
* [[CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb]]
* [[CSC/ECE 517 Fall 2019 - E1972. OSS project J. Skellington: Accessing Assignment Rubrics]]
* [[CSC/ECE 517 Fall 2019 - E1961. Email notification to reviewers and instructors]]
* [[CSC/ECE 517 Fall 2019 - E1971. OSS project Finklestein: Instructors & Institutions]]
* [[CSC/ECE 517 Fall 2019 - E1953. Tagging report for student]]
* [[CSC/ECE 517 Fall 2019 - E1955.Write unit tests for student_task.rb]]
* [[CSC/ECE 517 Fall 2019 - E1954. Auto-generate submission directory names based on assignment names]]
* [[CSC/ECE 517 Fall 2019 - E1958. Two issues related to assignment management]]
* [[CSC/ECE 517 Fall 2019 - E1948. Refactor review_mapping_helper.rb]]
* [[CSC/ECE 517 Fall 2019 - E1959. Intelligent copying of assignments without topics]]
* [[CSC/ECE 517 Fall 2019 - E1968. Fixes for adding members to teams]]
* [[CSC/ECE 517 Fall 2019 - E1969. Fixes for reviews not being available]]
* [[CSC/ECE 517 Fall 2019 - E1951. Remove multiple topics at a time]]
* [[CSC/ECE 517 Fall 2019 - E1957. Time travel Not Allowed..!!! Restrict TAs’ ability to change their own grade + limit file-size upload]]
* [[CSC/ECE 517 Fall 2019 - E1963. Changing assignment participant role]]
* [[CSC/ECE 517 Fall 2019 - E1941. Issues related to topic deadlines]]
* [[CSC/ECE 517 Fall 2019 - E1966. Tabbed_reviews partial file refactor for displaying the alternate view of reviews]]
* [[CSC/ECE 517 Fall 2019 - E1962. Email notification upon account creation]]
* [[CSC/ECE 517 Fall 2019 - E1967. Fix glitches in author feedback]]
* [[CSC/ECE 517 Fall 2019 - E1960. Create new late policy successfully and fixing "Back" link]]
* [[CSC/ECE 517 Fall 2019 - E1939. OSS Project Juniper: Bookmark enhancements]]
* [[CSC/ECE 517 Fall 2019 - M1950. Support Asynchronous Web Assembly Compilation]]
* [[CSC/ECE 517 Fall 2019 - E1938. OSS project Duke Blue: Fix import glitches]]
* [[CSC/ECE 517 Fall 2019 - E1985. Let course staff and students do reviews]]
* [[CSC/ECE 517 Fall 2019 - M1951. Implement missing OffscreenCanvas APIs]]
* [[CSC/ECE 517 Fall 2019 - E1940. Improving email notification]]
* [[CSC/ECE 517 Fall 2019 - E1956. There is no shortcut to get free review points: Review Assignment Bug]]
* [[CSC/ECE 517 Fall 2019 - E1942. Refactor stage deadlines in assignment.rb]]
* [[CSC/ECE 517 Fall 2019 - M1952. Missing DOM features project]]
* [[CSC/ECE 517 Fall 2019 - E1945. Refactor users_controller.rb]]
* [[CSC/ECE 517 Fall 2019 - E1964. Export review scores for projects]]
* [[CSC/ECE 517 Fall 2019 - E1949. Write Unit Tests for Importing assignment participants and import glitches]]
* [[CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller]]
* [[CSC/ECE 517 Fall 2019 - E1946. Refactor Questionnaire controller]]
* [[CSC/ECE 517 Fall 2019 - E1982. Regulate changing of rubrics while projects are in progress]]
* [[CSC/ECE 517 Fall 2019 - E1989. Track the time students look at other submissions]]
* [[CSC/ECE 517 Fall 2019 - E1990. Integrate suggestion detection algorithm]]
* [[CSC/ECE 517 Fall 2019 - E1995. Tests for email functionality]]
* [[CSC/ECE 517 Fall 2019 - E1993 Track Time Between Successive Tag Assignments]]
* [[CSC/ECE 517 Fall 2019 - E1979. Completion/Progress view]]
* [[CSC/ECE 517 Fall 2019 - E1980. Sort instructor reports by name, ID, score, etc.]]
* [[CSC/ECE 517 Fall 2019 - E1984. Improve self-review Link peer review & self-review to derive grades]]
* [[CSC/ECE 517 Fall 2019 - E1994. Mentor management for assignments without topics]]
* [[CSC/ECE 517 Fall 2019 - E1997. Issues related to meta-reviewing]]
* [[CSC/ECE 517 Fall 2019 - E1981. Student-generated questions added to rubric]]
* [[CSC/ECE 517 Fall 2019 - E1996. Enhancements to review grader]]
* [[CSC/ECE 517 Fall 2019 - E1987. Improving search facility in Expertiza]]
* [[CSC/ECE 517 Fall 2019 - E1978. Fix issues related to deadlines and late policies]]
* [[CSC/ECE 517 Fall 2019 - E1991. Improvements to anonymized view]]
* [[CSC/ECE 517 Fall 2019 - E1998. Weights in grade calculation]]
* [[CSC/ECE 517 Fall 2019 - E1986. Allow reviewers to bid on what to review]]
* [[CSC/ECE 517 Fall 2019 -E1974 Allow users to create an account and submit work to an ”assignment” Design]]
* [[CSC/ECE 517 Fall 2019 -E1988 Allow reviewer to say review can be shown to class as an example]]
* [[CSC/ECE 517 Fall 2019 - E1975. Generalize Review Versioning]]

CSC/ECE 517 Fall 2019

2019-11-10T02:21:30Z

Svshingt:

* [[CSC/ECE 517 Fall 2019 - Project E1947. Refactor quiz_questionnaire_controller.rb]]
* [[CSC/ECE 517 Fall 2019 - Project E1965. Review report should link to the usual view for reviews]]
* [[CSC/ECE 517 Fall 2019 - Project E1943. Refactor sign up sheet controller.rb]]
* [[CSC/ECE 517 Fall 2019 - E1972. OSS project J. Skellington: Accessing Assignment Rubrics]]
* [[CSC/ECE 517 Fall 2019 - E1961. Email notification to reviewers and instructors]]
* [[CSC/ECE 517 Fall 2019 - E1971. OSS project Finklestein: Instructors & Institutions]]
* [[CSC/ECE 517 Fall 2019 - E1953. Tagging report for student]]
* [[CSC/ECE 517 Fall 2019 - E1955.Write unit tests for student_task.rb]]
* [[CSC/ECE 517 Fall 2019 - E1954. Auto-generate submission directory names based on assignment names]]
* [[CSC/ECE 517 Fall 2019 - E1958. Two issues related to assignment management]]
* [[CSC/ECE 517 Fall 2019 - E1948. Refactor review_mapping_helper.rb]]
* [[CSC/ECE 517 Fall 2019 - E1959. Intelligent copying of assignments without topics]]
* [[CSC/ECE 517 Fall 2019 - E1968. Fixes for adding members to teams]]
* [[CSC/ECE 517 Fall 2019 - E1969. Fixes for reviews not being available]]
* [[CSC/ECE 517 Fall 2019 - E1951. Remove multiple topics at a time]]
* [[CSC/ECE 517 Fall 2019 - E1957. Time travel Not Allowed..!!! Restrict TAs’ ability to change their own grade + limit file-size upload]]
* [[CSC/ECE 517 Fall 2019 - E1963. Changing assignment participant role]]
* [[CSC/ECE 517 Fall 2019 - E1941. Issues related to topic deadlines]]
* [[CSC/ECE 517 Fall 2019 - E1966. Tabbed_reviews partial file refactor for displaying the alternate view of reviews]]
* [[CSC/ECE 517 Fall 2019 - E1962. Email notification upon account creation]]
* [[CSC/ECE 517 Fall 2019 - E1967. Fix glitches in author feedback]]
* [[CSC/ECE 517 Fall 2019 - E1960. Create new late policy successfully and fixing "Back" link]]
* [[CSC/ECE 517 Fall 2019 - E1939. OSS Project Juniper: Bookmark enhancements]]
* [[CSC/ECE 517 Fall 2019 - M1950. Support Asynchronous Web Assembly Compilation]]
* [[CSC/ECE 517 Fall 2019 - E1938. OSS project Duke Blue: Fix import glitches]]
* [[CSC/ECE 517 Fall 2019 - M1951. Implement missing OffscreenCanvas APIs]]
* [[CSC/ECE 517 Fall 2019 - E1940. Improving email notification]]
* [[CSC/ECE 517 Fall 2019 - E1956. There is no shortcut to get free review points: Review Assignment Bug]]
* [[CSC/ECE 517 Fall 2019 - E1942. Refactor stage deadlines in assignment.rb]]
* [[CSC/ECE 517 Fall 2019 - M1952. Missing DOM features project]]
* [[CSC/ECE 517 Fall 2019 - E1945. Refactor users_controller.rb]]
* [[CSC/ECE 517 Fall 2019 - E1964. Export review scores for projects]]
* [[CSC/ECE 517 Fall 2019 - E1949. Write Unit Tests for Importing assignment participants and import glitches]]
* [[CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller]]
* [[CSC/ECE 517 Fall 2019 - E1946. Refactor Questionnaire controller]]
* [[CSC/ECE 517 Fall 2019 - E1982. Regulate changing of rubrics while projects are in progress]]
* [[CSC/ECE 517 Fall 2019 - E1989. Track the time students look at other submissions]]
* [[CSC/ECE 517 Fall 2019 - E1990. Integrate suggestion detection algorithm]]
* [[CSC/ECE 517 Fall 2019 - E1995. Tests for email functionality]]
* [[CSC/ECE 517 Fall 2019 - E1979. Completion/Progress view]]
* [[CSC/ECE 517 Fall 2019 - E1995. Weights in grade calculation]]

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-11-06T17:00:23Z

Svshingt: /* Details of the changes made */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.
sd
== About Expertiza==

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

== Description of the project ==

The focus of the project is on a controller named ReviewMappingController and the primary goal is to make changes to the internal structure of the controller to make it easier to read and cheaper to maintain without changing its observable behavior. This can be achieved through refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code, etc.

The number of routes directed to review_mapping_controller is illustrated here and since it is intertwined with a lot of other parts in the code base, it is quite imperative that this code needs to be refactored.

[[File:complexity.png]]

== Work-Plan Followed ==
[[File:FlowchartOODD.jpg]]
 
We were tasked to refactor the review_mapping_controller.rb and solve any cascading issues or bugs we could find. We followed the above work plan to complete this task. There were many times when all the Rspec and Cucumber tests passed locally but ran into an issue when we uploaded the changes on GitHub. Prompt feedback from the TRAVIS CI helped us recognize the issue. Then we went on local machine and followed the whole process of refactoring again. In this way, we covered every refactoring we did and ensured that the TRAVIS CI get passed with minimum issues on the code-climate.

== Files modified/created in the current project ==

1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of '''long''' and '''complex methods''' such as '''peer_review_strategy''' and '''automatic_review_mapping''' were refactored from this controller along with the '''removal''' of some '''non-related methods''' such as '''add_calibration''' and '''assign_quiz_dynamically'''. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

After refactoring the '''Review_Mapping_Controller.rb''', there were some tests still present in the spec file of this controller. So, we '''removed such tests''' from the '''review_mapping_controller_spec.rb''' to the appropriate spec file.

=== AssignQuizController ===

'''assign_quiz_dynamically''' (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

'''Add_Calibration''' is a nuanced method and has seemingly '''different functionality''' than '''Review Mapping''' Controller. Methods having a different purpose than review_mapping or helping review_mapping should not be present in this controller. So we '''moved''' this method '''into''' a '''separate controller'''.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

<pre>
assign_quiz_dynamically_assign_quiz_index POST /assign_quiz/assign_quiz_dynamically(.:format) assign_quiz#assign_quiz_dynamically
</pre>

=== views/partials ===

Routes were changed in the views and partials.

==== Modified View Files: ====
app/views/student_quizzes/_set_dynamic_quiz.html.erb 
app/views/assignments/edit/_calibration.html.erb 

== Details of the changes made==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. before: [[File:3bef.jpg]] after: [[File:3aft.jpg]] 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 
before: [[File:4bef.jpg]] after: [[File:4aft.jpg]] 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything. 
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability. 
<pre>
## Helper Method for generating a random participant which is to be used in peer_review_strategy method.
def gen_random_participant_id(iterator, participants_hash, num_participants, participants)
if iterator.zero?
rand_num = rand(0..num_participants - 1)
else
min_value = participants_hash.values.min
# get the temp array including indices of participants, each participant has minimum review number in hash table.
participants_with_min_assigned_reviews = []
participants.each do |participant|
participants_with_min_assigned_reviews << participants.index(participant) if participants_hash[participant.id] == min_value
end
# if participants_with_min_assigned_reviews is blank
no_particpants = participants_with_min_assigned_reviews.empty?
# or only one element in participants_with_min_assigned_reviews, prohibit one student to review his/her own artifact
participant_is_owner = (participants_with_min_assigned_reviews.size == 1 and TeamsUser.exists?(team_id: team.id, user_id: participants[participants_with_min_assigned_reviews[0]].user_id))
rand_num = if no_particpants or participant_is_owner
# use original method to get random number
rand(0..num_participants - 1)
else
# rand_num should be the position of this participant in original array
participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size - 1)]
end
end
return rand_num
end
</pre>
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
       *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern 
       *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method. 
<pre>
# Helper Method to check num_reviews_per_student and num_reviews_per_submission arguments passed in by params hash.
def check_num_reviews_args(num_reviews_per_student, num_reviews_per_submission, teams)
has_error_not_raised = true
# check for exit paths first
if num_reviews_per_student == 0 and num_reviews_per_submission == 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
has_error_not_raised = false
elsif num_reviews_per_student != 0 and num_reviews_per_submission != 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
has_error_not_raised = false
elsif num_reviews_per_student >= teams.size
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
flash[:error] = 'You cannot set the number of reviews done ' \
'by each student to be greater than or equal to total number of teams ' \
'[or "participants" if it is an individual assignment].'
has_error_not_raised = false
end
end
</pre>
7. Modularized helper methods into a module and was mixed in the ReviewMappingController Class. 
       *Only some of the methods written in the class have external usage i.e called by another controllers, views etc. 
       *The other methods are just helpers and as such moved into a Helper method module and mixed in the class 
Before Modularization
<pre>
class ReviewMappingController < ApplicationController
...................
...................
512 Lines & 25 Methods Defined (After moving a few methods into separate controllers)
...................
...................
end
</pre>
After Modularization
<pre>
module Helper_methods
...................
5 Methods and 170 lines
...................
end

class ReviewMappingController < ApplicationController
include Helper_methods
...................
340 Lines & 20 Methods (All of those are used elsewhere directly in the application)
...................
end
</pre>

8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle. 
       *Internally used methods were not tested 
       *Tests for newly created controllers have been moved into a separate spec files. 

9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
       *This method assigns a quiz(Stored as an assignment object) to the participant. 
       *This method is not related to ReviewMapping functionality, so it was made into a new controller. 

10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
       *The controller paths present in the views/partials have to be changed to not to break the functionality 
       *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically 

View File Affected by the creation of AssinQuizController
[[File:View.png]]

11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

== Test Plan ==

=== Rspec Unit Tests ===

Since this is a Refactoring Project, We made sure that the changes made did not break any functionality.

[[File:testplan.png]]

Note: Tests have been run for three controllers (one existing and two new).

=== Capybara Integration and Functional Tests ===

As the controller routes have been modified in the routes.rb and the other view files, there are potential chances of failures in Integration tests.

However, no such failure has been reported by Travis build.

[[File:Travis CI.png]]

[[File:FeaturesCI.png]]

== Code Coverage ==

Code Coverage for Controllers section climbed up.

[https://coveralls.io/builds/26607665] # Link for the COVERALLS stats of our pull request.

[[File:CCC1.png]]

== Project Mentor ==

Ramya Vijayakumar (rvijaya4@ncsu.edu)

== Team Members ==

Yaswanth Soodini (ysoodin@ncsu.edu)

Saurabh Shingte (svshingt@ncsu.edu)

Vivek Karri (vkarri@ncsu.edu)

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-11-03T02:03:32Z

Svshingt: /* Details of the changes made */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.
sd
== About Expertiza==

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

== Description of the project ==

The focus of the project is on a controller named ReviewMappingController and the primary goal is to make changes to the internal structure of the controller to make it easier to read and cheaper to maintain without changing it's observable behaviour. This can be achieved though refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code, etc.

The number of routes directed to review_mapping_controller is illustrated here:

[[File:complexity.png]]

== Work-Plan Followed ==
[[File:FlowchartOODD.jpg]]
 
We were tasked to refactor the review_mapping_controller.rb and solve any cascading issues or bugs we could find. We followed the above work plan to complete this task. There were many times when all the Rspec and Cucumber tests passed locally but ran into an issue when we uploaded the changes on GitHub. Prompt feedback from the TRAVIS CI helped us recognize the issue. Then we went on local machine and followed the whole process of refactoring again. In this way, we covered every refactoring we did and ensured that the TRAVIS CI get passed with minimum issues on the code-climate.

== Files modified/created in the current project ==

1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of '''long''' and '''complex methods''' such as '''peer_review_strategy''' and '''automatic_review_mapping''' were refactored from this controller along with the '''removal''' of some '''non-related methods''' such as '''add_calibration''' and '''assign_quiz_dynamically'''. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

After refactoring the '''Review_Mapping_Controller.rb''', there were some tests still present in the spec file of this controller. So, we '''removed such tests''' from the '''review_mapping_controller_spec.rb''' to the appropriate spec file.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

'''Add_Calibration''' is a nuanced method and has seemingly '''different functionality''' than '''Review Mapping''' Controller. Methods having a different purpose than review_mapping or helping review_mapping should not be present in this controller. So we '''moved''' this method '''into''' a '''separate controller'''.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of the changes made==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. [[File:2changes.JPG]] 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. [[File:2changed.JPG]]
 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. before: [[File:3bef.jpg]] after: [[File:3aft.jpg]] 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 
before: [[File:4bef.jpg]] after: [[File:4aft.jpg]] 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything. 
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability. 
<pre>
## Helper Method for generating a random participant which is to be used in peer_review_strategy method.
def gen_random_participant_id(iterator, participants_hash, num_participants, participants)
if iterator.zero?
rand_num = rand(0..num_participants - 1)
else
min_value = participants_hash.values.min
# get the temp array including indices of participants, each participant has minimum review number in hash table.
participants_with_min_assigned_reviews = []
participants.each do |participant|
participants_with_min_assigned_reviews << participants.index(participant) if participants_hash[participant.id] == min_value
end
# if participants_with_min_assigned_reviews is blank
no_particpants = participants_with_min_assigned_reviews.empty?
# or only one element in participants_with_min_assigned_reviews, prohibit one student to review his/her own artifact
participant_is_owner = (participants_with_min_assigned_reviews.size == 1 and TeamsUser.exists?(team_id: team.id, user_id: participants[participants_with_min_assigned_reviews[0]].user_id))
rand_num = if no_particpants or participant_is_owner
# use original method to get random number
rand(0..num_participants - 1)
else
# rand_num should be the position of this participant in original array
participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size - 1)]
end
end
return rand_num
end
</pre>
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
       *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern 
       *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method. 
<pre>
# Helper Method to check num_reviews_per_student and num_reviews_per_submission arguments passed in by params hash.
def check_num_reviews_args(num_reviews_per_student, num_reviews_per_submission, teams)
has_error_not_raised = true
# check for exit paths first
if num_reviews_per_student == 0 and num_reviews_per_submission == 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
has_error_not_raised = false
elsif num_reviews_per_student != 0 and num_reviews_per_submission != 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
has_error_not_raised = false
elsif num_reviews_per_student >= teams.size
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
flash[:error] = 'You cannot set the number of reviews done ' \
'by each student to be greater than or equal to total number of teams ' \
'[or "participants" if it is an individual assignment].'
has_error_not_raised = false
end
end
</pre>
7. Modularized helper methods into a module and was mixed in the ReviewMappingController Class. 
       *Only some of the methods written in the class have external usage i.e called by another controllers, views etc. 
       *The other methods are just helpers and as such moved into a Helper method module and mixed in the class 
Before Modularization
<pre>
class ReviewMappingController < ApplicationController
...................
...................
512 Lines & 25 Methods Defined (After moving a few methods into separate controllers)
...................
...................
end
</pre>
After Modularization
<pre>
module Helper_methods
...................
5 Methods and 170 lines
...................
end

class ReviewMappingController < ApplicationController
include Helper_methods
...................
340 Lines & 20 Methods (All of those are used elsewhere directly in the application)
...................
end
</pre>

8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle. 
       *Internally used methods were not tested 
       *Tests for newly created controllers have been moved into a separate spec files. 

9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
       *This method assigns a quiz(Stored as an assignment object) to the participant. 
       *This method is not related to ReviewMapping functionality, so it was made into a new controller. 

10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
       *The controller paths present in the views/partials have to be changed to not to break the functionality 
       *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically 

View File Affected by the creation of AssinQuizController
[[File:View.png]]

11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

== Test Plan ==

=== Rspec Unit Tests ===

Since this is a Refactoring Project, We made sure that the changes made did not break any functionality.

[[File:testplan.png]]

Note: Tests have been run for three controllers (one existing and two new).

=== Capybara Integration and Functional Tests ===

As the controller routes have been modified in the routes.rb and the other view files, there are potential chances of failures in Integration tests.

However, no such failure has been reported by Travis build.

[[File:Travis CI.png]]

[[File:FeaturesCI.png]]

== Code Coverage ==

Code Coverage for Controllers section climbed up.

[https://coveralls.io/builds/26607665] # Link for the COVERALLS stats of our pull request.

[[File:CCC1.png]]

== Project Mentor ==

Ramya Vijayakumar (rvijaya4@ncsu.edu)

== Team Members ==

Yaswanth Soodini (ysoodin@ncsu.edu)

Saurabh Shingte (svshingt@ncsu.edu)

Vivek Karri (vkarri@ncsu.edu)

File:2changed.JPG

2019-11-03T02:03:18Z

Svshingt:

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-11-03T01:57:47Z

Svshingt: /* Details of the changes made */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.
sd
== About Expertiza==

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

== Description of the project ==

The focus of the project is on a controller named ReviewMappingController and the primary goal is to make changes to the internal structure of the controller to make it easier to read and cheaper to maintain without changing it's observable behaviour. This can be achieved though refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code, etc.

The number of routes directed to review_mapping_controller is illustrated here:

[[File:complexity.png]]

== Work-Plan Followed ==
[[File:FlowchartOODD.jpg]]
 
We were tasked to refactor the review_mapping_controller.rb and solve any cascading issues or bugs we could find. We followed the above work plan to complete this task. There were many times when all the Rspec and Cucumber tests passed locally but ran into an issue when we uploaded the changes on GitHub. Prompt feedback from the TRAVIS CI helped us recognize the issue. Then we went on local machine and followed the whole process of refactoring again. In this way, we covered every refactoring we did and ensured that the TRAVIS CI get passed with minimum issues on the code-climate.

== Files modified/created in the current project ==

1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of '''long''' and '''complex methods''' such as '''peer_review_strategy''' and '''automatic_review_mapping''' were refactored from this controller along with the '''removal''' of some '''non-related methods''' such as '''add_calibration''' and '''assign_quiz_dynamically'''. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

After refactoring the '''Review_Mapping_Controller.rb''', there were some tests still present in the spec file of this controller. So, we '''removed such tests''' from the '''review_mapping_controller_spec.rb''' to the appropriate spec file.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

'''Add_Calibration''' is a nuanced method and has seemingly '''different functionality''' than '''Review Mapping''' Controller. Methods having a different purpose than review_mapping or helping review_mapping should not be present in this controller. So we '''moved''' this method '''into''' a '''separate controller'''.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of the changes made==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. [[File:2changes.JPG]] 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 
 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. before: [[File:3bef.jpg]] after: [[File:3aft.jpg]] 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 
before: [[File:4bef.jpg]] after: [[File:4aft.jpg]] 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything. 
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability. 
<pre>
## Helper Method for generating a random participant which is to be used in peer_review_strategy method.
def gen_random_participant_id(iterator, participants_hash, num_participants, participants)
if iterator.zero?
rand_num = rand(0..num_participants - 1)
else
min_value = participants_hash.values.min
# get the temp array including indices of participants, each participant has minimum review number in hash table.
participants_with_min_assigned_reviews = []
participants.each do |participant|
participants_with_min_assigned_reviews << participants.index(participant) if participants_hash[participant.id] == min_value
end
# if participants_with_min_assigned_reviews is blank
no_particpants = participants_with_min_assigned_reviews.empty?
# or only one element in participants_with_min_assigned_reviews, prohibit one student to review his/her own artifact
participant_is_owner = (participants_with_min_assigned_reviews.size == 1 and TeamsUser.exists?(team_id: team.id, user_id: participants[participants_with_min_assigned_reviews[0]].user_id))
rand_num = if no_particpants or participant_is_owner
# use original method to get random number
rand(0..num_participants - 1)
else
# rand_num should be the position of this participant in original array
participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size - 1)]
end
end
return rand_num
end
</pre>
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
       *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern 
       *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method. 
<pre>
# Helper Method to check num_reviews_per_student and num_reviews_per_submission arguments passed in by params hash.
def check_num_reviews_args(num_reviews_per_student, num_reviews_per_submission, teams)
has_error_not_raised = true
# check for exit paths first
if num_reviews_per_student == 0 and num_reviews_per_submission == 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
has_error_not_raised = false
elsif num_reviews_per_student != 0 and num_reviews_per_submission != 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
has_error_not_raised = false
elsif num_reviews_per_student >= teams.size
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
flash[:error] = 'You cannot set the number of reviews done ' \
'by each student to be greater than or equal to total number of teams ' \
'[or "participants" if it is an individual assignment].'
has_error_not_raised = false
end
end
</pre>
7. Modularized helper methods into a module and was mixed in the ReviewMappingController Class. 
       *Only some of the methods written in the class have external usage i.e called by another controllers, views etc. 
       *The other methods are just helpers and as such moved into a Helper method module and mixed in the class 
Before Modularization
<pre>
class ReviewMappingController < ApplicationController
...................
...................
512 Lines & 25 Methods Defined (After moving a few methods into separate controllers)
...................
...................
end
</pre>
After Modularization
<pre>
module Helper_methods
...................
5 Methods and 170 lines
...................
end

class ReviewMappingController < ApplicationController
include Helper_methods
...................
340 Lines & 20 Methods (All of those are used elsewhere directly in the application)
...................
end
</pre>

8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle. 
       *Internally used methods were not tested 
       *Tests for newly created controllers have been moved into a separate spec files. 

9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
       *This method assigns a quiz(Stored as an assignment object) to the participant. 
       *This method is not related to ReviewMapping functionality, so it was made into a new controller. 

10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
       *The controller paths present in the views/partials have to be changed to not to break the functionality 
       *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically 

View File Affected by the creation of AssinQuizController
[[File:View.png]]

11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

== Test Plan ==

=== Rspec Unit Tests ===

Since this is a Refactoring Project, We made sure that the changes made did not break any functionality.

[[File:testplan.png]]

Note: Tests have been run for three controllers (one existing and two new).

=== Capybara Integration and Functional Tests ===

As the controller routes have been modified in the routes.rb and the other view files, there are potential chances of failures in Integration tests.

However, no such failure has been reported by Travis build.

[[File:Travis CI.png]]

[[File:FeaturesCI.png]]

== Code Coverage ==

Code Coverage for Controllers section climbed up.

[https://coveralls.io/builds/26607665] # Link for the COVERALLS stats of our pull request.

[[File:CCC1.png]]

== Project Mentor ==

Ramya Vijayakumar (rvijaya4@ncsu.edu)

== Team Members ==

Yaswanth Soodini (ysoodin@ncsu.edu)

Saurabh Shingte (svshingt@ncsu.edu)

Vivek Karri (vkarri@ncsu.edu)

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-11-03T01:50:54Z

Svshingt: /* Details of the changes made */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.
sd
== About Expertiza==

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

== Description of the project ==

The focus of the project is on a controller named ReviewMappingController and the primary goal is to make changes to the internal structure of the controller to make it easier to read and cheaper to maintain without changing it's observable behaviour. This can be achieved though refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code, etc.

The number of routes directed to review_mapping_controller is illustrated here:

[[File:complexity.png]]

== Work-Plan Followed ==
[[File:FlowchartOODD.jpg]]
 
We were tasked to refactor the review_mapping_controller.rb and solve any cascading issues or bugs we could find. We followed the above work plan to complete this task. There were many times when all the Rspec and Cucumber tests passed locally but ran into an issue when we uploaded the changes on GitHub. Prompt feedback from the TRAVIS CI helped us recognize the issue. Then we went on local machine and followed the whole process of refactoring again. In this way, we covered every refactoring we did and ensured that the TRAVIS CI get passed with minimum issues on the code-climate.

== Files modified/created in the current project ==

1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of '''long''' and '''complex methods''' such as '''peer_review_strategy''' and '''automatic_review_mapping''' were refactored from this controller along with the '''removal''' of some '''non-related methods''' such as '''add_calibration''' and '''assign_quiz_dynamically'''. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

After refactoring the '''Review_Mapping_Controller.rb''', there were some tests still present in the spec file of this controller. So, we '''removed such tests''' from the '''review_mapping_controller_spec.rb''' to the appropriate spec file.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

'''Add_Calibration''' is a nuanced method and has seemingly '''different functionality''' than '''Review Mapping''' Controller. Methods having a different purpose than review_mapping or helping review_mapping should not be present in this controller. So we '''moved''' this method '''into''' a '''separate controller'''.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of the changes made==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 
[[File:2changes.JPG]] 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. before: [[File:3bef.jpg]] after: [[File:3aft.jpg]] 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 
before: [[File:4bef.jpg]] after: [[File:4aft.jpg]] 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything. 
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability. 
<pre>
## Helper Method for generating a random participant which is to be used in peer_review_strategy method.
def gen_random_participant_id(iterator, participants_hash, num_participants, participants)
if iterator.zero?
rand_num = rand(0..num_participants - 1)
else
min_value = participants_hash.values.min
# get the temp array including indices of participants, each participant has minimum review number in hash table.
participants_with_min_assigned_reviews = []
participants.each do |participant|
participants_with_min_assigned_reviews << participants.index(participant) if participants_hash[participant.id] == min_value
end
# if participants_with_min_assigned_reviews is blank
no_particpants = participants_with_min_assigned_reviews.empty?
# or only one element in participants_with_min_assigned_reviews, prohibit one student to review his/her own artifact
participant_is_owner = (participants_with_min_assigned_reviews.size == 1 and TeamsUser.exists?(team_id: team.id, user_id: participants[participants_with_min_assigned_reviews[0]].user_id))
rand_num = if no_particpants or participant_is_owner
# use original method to get random number
rand(0..num_participants - 1)
else
# rand_num should be the position of this participant in original array
participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size - 1)]
end
end
return rand_num
end
</pre>
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
       *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern 
       *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method. 
<pre>
# Helper Method to check num_reviews_per_student and num_reviews_per_submission arguments passed in by params hash.
def check_num_reviews_args(num_reviews_per_student, num_reviews_per_submission, teams)
has_error_not_raised = true
# check for exit paths first
if num_reviews_per_student == 0 and num_reviews_per_submission == 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
has_error_not_raised = false
elsif num_reviews_per_student != 0 and num_reviews_per_submission != 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
has_error_not_raised = false
elsif num_reviews_per_student >= teams.size
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
flash[:error] = 'You cannot set the number of reviews done ' \
'by each student to be greater than or equal to total number of teams ' \
'[or "participants" if it is an individual assignment].'
has_error_not_raised = false
end
end
</pre>
7. Modularized helper methods into a module and was mixed in the ReviewMappingController Class. 
       *Only some of the methods written in the class have external usage i.e called by another controllers, views etc. 
       *The other methods are just helpers and as such moved into a Helper method module and mixed in the class 
Before Modularization
<pre>
class ReviewMappingController < ApplicationController
...................
...................
512 Lines & 25 Methods Defined (After moving a few methods into separate controllers)
...................
...................
end
</pre>
After Modularization
<pre>
module Helper_methods
...................
5 Methods and 170 lines
...................
end

class ReviewMappingController < ApplicationController
include Helper_methods
...................
340 Lines & 20 Methods (All of those are used elsewhere directly in the application)
...................
end
</pre>

8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle. 
       *Internally used methods were not tested 
       *Tests for newly created controllers have been moved into a separate spec files. 

9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
       *This method assigns a quiz(Stored as an assignment object) to the participant. 
       *This method is not related to ReviewMapping functionality, so it was made into a new controller. 

10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
       *The controller paths present in the views/partials have to be changed to not to break the functionality 
       *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically 

View File Affected by the creation of AssinQuizController
[[File:View.png]]

11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

== Test Plan ==

=== Rspec Unit Tests ===

Since this is a Refactoring Project, We made sure that the changes made did not break any functionality.

[[File:testplan.png]]

Note: Tests have been run for three controllers (one existing and two new).

=== Capybara Integration and Functional Tests ===

As the controller routes have been modified in the routes.rb and the other view files, there are potential chances of failures in Integration tests.

However, no such failure has been reported by Travis build.

[[File:Travis CI.png]]

[[File:FeaturesCI.png]]

== Code Coverage ==

Code Coverage for Controllers section climbed up.

[https://coveralls.io/builds/26607665] # Link for the COVERALLS stats of our pull request.

[[File:CCC1.png]]

== Project Mentor ==

Ramya Vijayakumar (rvijaya4@ncsu.edu)

== Team Members ==

Yaswanth Soodini (ysoodin@ncsu.edu)

Saurabh Shingte (svshingt@ncsu.edu)

Vivek Karri (vkarri@ncsu.edu)

File:2changes.JPG

2019-11-03T01:49:37Z

Svshingt:

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-11-03T01:43:43Z

Svshingt: /* Details of the changes made */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.
sd
== About Expertiza==

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

== Description of the project ==

The focus of the project is on a controller named ReviewMappingController and the primary goal is to make changes to the internal structure of the controller to make it easier to read and cheaper to maintain without changing it's observable behaviour. This can be achieved though refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code, etc.

The number of routes directed to review_mapping_controller is illustrated here:

[[File:complexity.png]]

== Work-Plan Followed ==
[[File:FlowchartOODD.jpg]]
 
We were tasked to refactor the review_mapping_controller.rb and solve any cascading issues or bugs we could find. We followed the above work plan to complete this task. There were many times when all the Rspec and Cucumber tests passed locally but ran into an issue when we uploaded the changes on GitHub. Prompt feedback from the TRAVIS CI helped us recognize the issue. Then we went on local machine and followed the whole process of refactoring again. In this way, we covered every refactoring we did and ensured that the TRAVIS CI get passed with minimum issues on the code-climate.

== Files modified/created in the current project ==

1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of '''long''' and '''complex methods''' such as '''peer_review_strategy''' and '''automatic_review_mapping''' were refactored from this controller along with the '''removal''' of some '''non-related methods''' such as '''add_calibration''' and '''assign_quiz_dynamically'''. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

After refactoring the '''Review_Mapping_Controller.rb''', there were some tests still present in the spec file of this controller. So, we '''removed such tests''' from the '''review_mapping_controller_spec.rb''' to the appropriate spec file.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

'''Add_Calibration''' is a nuanced method and has seemingly '''different functionality''' than '''Review Mapping''' Controller. Methods having a different purpose than review_mapping or helping review_mapping should not be present in this controller. So we '''moved''' this method '''into''' a '''separate controller'''.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of the changes made==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. before: [[File:3bef.jpg]] after: [[File:3aft.jpg]] 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 
before: [[File:4bef.jpg]] after: [[File:4aft.jpg]] 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything. 
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability. 
<pre>
## Helper Method for generating a random participant which is to be used in peer_review_strategy method.
def gen_random_participant_id(iterator, participants_hash, num_participants, participants)
if iterator.zero?
rand_num = rand(0..num_participants - 1)
else
min_value = participants_hash.values.min
# get the temp array including indices of participants, each participant has minimum review number in hash table.
participants_with_min_assigned_reviews = []
participants.each do |participant|
participants_with_min_assigned_reviews << participants.index(participant) if participants_hash[participant.id] == min_value
end
# if participants_with_min_assigned_reviews is blank
no_particpants = participants_with_min_assigned_reviews.empty?
# or only one element in participants_with_min_assigned_reviews, prohibit one student to review his/her own artifact
participant_is_owner = (participants_with_min_assigned_reviews.size == 1 and TeamsUser.exists?(team_id: team.id, user_id: participants[participants_with_min_assigned_reviews[0]].user_id))
rand_num = if no_particpants or participant_is_owner
# use original method to get random number
rand(0..num_participants - 1)
else
# rand_num should be the position of this participant in original array
participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size - 1)]
end
end
return rand_num
end
</pre>
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
       *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern 
       *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method. 
<pre>
# Helper Method to check num_reviews_per_student and num_reviews_per_submission arguments passed in by params hash.
def check_num_reviews_args(num_reviews_per_student, num_reviews_per_submission, teams)
has_error_not_raised = true
# check for exit paths first
if num_reviews_per_student == 0 and num_reviews_per_submission == 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
has_error_not_raised = false
elsif num_reviews_per_student != 0 and num_reviews_per_submission != 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
has_error_not_raised = false
elsif num_reviews_per_student >= teams.size
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
flash[:error] = 'You cannot set the number of reviews done ' \
'by each student to be greater than or equal to total number of teams ' \
'[or "participants" if it is an individual assignment].'
has_error_not_raised = false
end
end
</pre>
7. Modularized helper methods into a module and was mixed in the ReviewMappingController Class. 
       *Only some of the methods written in the class have external usage i.e called by another controllers, views etc. 
       *The other methods are just helpers and as such moved into a Helper method module and mixed in the class 
Before Modularization
<pre>
class ReviewMappingController < ApplicationController
...................
...................
512 Lines & 25 Methods Defined (After moving a few methods into separate controllers)
...................
...................
end
</pre>
After Modularization
<pre>
module Helper_methods
...................
5 Methods and 170 lines
...................
end

class ReviewMappingController < ApplicationController
include Helper_methods
...................
340 Lines & 20 Methods (All of those are used elsewhere directly in the application)
...................
end
</pre>

8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle. 
       *Internally used methods were not tested 
       *Tests for newly created controllers have been moved into a separate spec files. 

9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
       *This method assigns a quiz(Stored as an assignment object) to the participant. 
       *This method is not related to ReviewMapping functionality, so it was made into a new controller. 

10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
       *The controller paths present in the views/partials have to be changed to not to break the functionality 
       *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically 

View File Affected by the creation of AssinQuizController
[[File:View.png]]

11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

== Test Plan ==

=== Rspec Unit Tests ===

Since this is a Refactoring Project, We made sure that the changes made did not break any functionality.

[[File:testplan.png]]

Note: Tests have been run for three controllers (one existing and two new).

=== Capybara Integration and Functional Tests ===

As the controller routes have been modified in the routes.rb and the other view files, there are potential chances of failures in Integration tests.

However, no such failure has been reported by Travis build.

[[File:Travis CI.png]]

[[File:FeaturesCI.png]]

== Code Coverage ==

Code Coverage for Controllers section climbed up.

[https://coveralls.io/builds/26607665] # Link for the COVERALLS stats of our pull request.

[[File:CCC1.png]]

== Project Mentor ==

Ramya Vijayakumar (rvijaya4@ncsu.edu)

== Team Members ==

Yaswanth Soodini (ysoodin@ncsu.edu)

Saurabh Shingte (svshingt@ncsu.edu)

Vivek Karri (vkarri@ncsu.edu)

File:4bef.jpg

2019-11-03T01:39:29Z

Svshingt:

File:4aft.jpg

2019-11-03T01:39:15Z

Svshingt: Latest

Latest

File:3bef.jpg

2019-11-03T01:38:54Z

Svshingt:

File:3aft.jpg

2019-11-03T01:38:38Z

Svshingt: Latest

Latest

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-11-03T01:20:57Z

Svshingt: /* Files modified/created in the current project */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.
sd
== About Expertiza==

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

== Description of the project ==

The focus of the project is on a controller named ReviewMappingController and the primary goal is to make changes to the internal structure of the controller to make it easier to read and cheaper to maintain without changing it's observable behaviour. This can be achieved though refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code, etc.

The number of routes directed to review_mapping_controller is illustrated here:

[[File:complexity.png]]

== Work-Plan Followed ==
[[File:FlowchartOODD.jpg]]
 
We were tasked to refactor the review_mapping_controller.rb and solve any cascading issues or bugs we could find. We followed the above work plan to complete this task. There were many times when all the Rspec and Cucumber tests passed locally but ran into an issue when we uploaded the changes on GitHub. Prompt feedback from the TRAVIS CI helped us recognize the issue. Then we went on local machine and followed the whole process of refactoring again. In this way, we covered every refactoring we did and ensured that the TRAVIS CI get passed with minimum issues on the code-climate.

== Files modified/created in the current project ==

1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of '''long''' and '''complex methods''' such as '''peer_review_strategy''' and '''automatic_review_mapping''' were refactored from this controller along with the '''removal''' of some '''non-related methods''' such as '''add_calibration''' and '''assign_quiz_dynamically'''. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

After refactoring the '''Review_Mapping_Controller.rb''', there were some tests still present in the spec file of this controller. So, we '''removed such tests''' from the '''review_mapping_controller_spec.rb''' to the appropriate spec file.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

'''Add_Calibration''' is a nuanced method and has seemingly '''different functionality''' than '''Review Mapping''' Controller. Methods having a different purpose than review_mapping or helping review_mapping should not be present in this controller. So we '''moved''' this method '''into''' a '''separate controller'''.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of the changes made==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything. 
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability. 
<pre>
## Helper Method for generating a random participant which is to be used in peer_review_strategy method.
def gen_random_participant_id(iterator, participants_hash, num_participants, participants)
if iterator.zero?
rand_num = rand(0..num_participants - 1)
else
min_value = participants_hash.values.min
# get the temp array including indices of participants, each participant has minimum review number in hash table.
participants_with_min_assigned_reviews = []
participants.each do |participant|
participants_with_min_assigned_reviews << participants.index(participant) if participants_hash[participant.id] == min_value
end
# if participants_with_min_assigned_reviews is blank
no_particpants = participants_with_min_assigned_reviews.empty?
# or only one element in participants_with_min_assigned_reviews, prohibit one student to review his/her own artifact
participant_is_owner = (participants_with_min_assigned_reviews.size == 1 and TeamsUser.exists?(team_id: team.id, user_id: participants[participants_with_min_assigned_reviews[0]].user_id))
rand_num = if no_particpants or participant_is_owner
# use original method to get random number
rand(0..num_participants - 1)
else
# rand_num should be the position of this participant in original array
participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size - 1)]
end
end
return rand_num
end
</pre>
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
       *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern 
       *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method. 
<pre>
# Helper Method to check num_reviews_per_student and num_reviews_per_submission arguments passed in by params hash.
def check_num_reviews_args(num_reviews_per_student, num_reviews_per_submission, teams)
has_error_not_raised = true
# check for exit paths first
if num_reviews_per_student == 0 and num_reviews_per_submission == 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
has_error_not_raised = false
elsif num_reviews_per_student != 0 and num_reviews_per_submission != 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
has_error_not_raised = false
elsif num_reviews_per_student >= teams.size
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
flash[:error] = 'You cannot set the number of reviews done ' \
'by each student to be greater than or equal to total number of teams ' \
'[or "participants" if it is an individual assignment].'
has_error_not_raised = false
end
end
</pre>
7. Modularized helper methods into a module and was mixed in the ReviewMappingController Class. 
       *Only some of the methods written in the class have external usage i.e called by another controllers, views etc. 
       *The other methods are just helpers and as such moved into a Helper method module and mixed in the class 
Before Modularization
<pre>
class ReviewMappingController < ApplicationController
...................
...................
512 Lines & 25 Methods Defined (After moving a few methods into separate controllers)
...................
...................
end
</pre>
After Modularization
<pre>
module Helper_methods
...................
5 Methods and 170 lines
...................
end

class ReviewMappingController < ApplicationController
include Helper_methods
...................
340 Lines & 20 Methods (All of those are used elsewhere directly in the application)
...................
end
</pre>

8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle. 
       *Internally used methods were not tested 
       *Tests for newly created controllers have been moved into a separate spec files. 

9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
       *This method assigns a quiz(Stored as an assignment object) to the participant. 
       *This method is not related to ReviewMapping functionality, so it was made into a new controller. 

10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
       *The controller paths present in the views/partials have to be changed to not to break the functionality 
       *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically 

View File Affected by the creation of AssinQuizController
[[File:View.png]]

11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

== Test Plan ==

=== Rspec Unit Tests ===

Since this is a Refactoring Project, We made sure that the changes made did not break any functionality.

[[File:testplan.png]]

Note: Tests have been run for three controllers (one existing and two new).

=== Capybara Integration and Functional Tests ===

As the controller routes have been modified in the routes.rb and the other view files, there are potential chances of failures in Integration tests.

However, no such failure has been reported by Travis build.

[[File:Travis CI.png]]

[[File:FeaturesCI.png]]

== Code Coverage ==

Code Coverage for Controllers section climbed up.

[https://coveralls.io/builds/26607665] # Link for the COVERALLS stats of our pull request.

[[File:CCC1.png]]

== Project Mentor ==

Ramya Vijayakumar (rvijaya4@ncsu.edu)

== Team Members ==

Yaswanth Soodini (ysoodin@ncsu.edu)

Saurabh Shingte (svshingt@ncsu.edu)

Vivek Karri (vkarri@ncsu.edu)

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-11-03T01:15:45Z

Svshingt: /* Files modified/created in the current project */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.
sd
== About Expertiza==

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

== Description of the project ==

The focus of the project is on a controller named ReviewMappingController and the primary goal is to make changes to the internal structure of the controller to make it easier to read and cheaper to maintain without changing it's observable behaviour. This can be achieved though refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code, etc.

The number of routes directed to review_mapping_controller is illustrated here:

[[File:complexity.png]]

== Work-Plan Followed ==
[[File:FlowchartOODD.jpg]]
 
We were tasked to refactor the review_mapping_controller.rb and solve any cascading issues or bugs we could find. We followed the above work plan to complete this task. There were many times when all the Rspec and Cucumber tests passed locally but ran into an issue when we uploaded the changes on GitHub. Prompt feedback from the TRAVIS CI helped us recognize the issue. Then we went on local machine and followed the whole process of refactoring again. In this way, we covered every refactoring we did and ensured that the TRAVIS CI get passed with minimum issues on the code-climate.

== Files modified/created in the current project ==

1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of '''long''' and '''complex methods''' such as '''peer_review_strategy''' and '''automatic_review_mapping''' were refactored from this controller along with the '''removal''' of some '''non-related methods''' such as '''add_calibration''' and '''assign_quiz_dynamically'''. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

After refactoring the '''Review_Mapping_Controller.rb''', there were some tests still present in the spec file of this controller. So, we '''removed such tests''' from the '''review_mapping_controller_spec.rb''' to the appropriate spec file.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

Add Calibration is a nuanced method that has seemingly different functionality than Review Mapping Controller and hence moved into a separate controller.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of the changes made==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything. 
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability. 
<pre>
## Helper Method for generating a random participant which is to be used in peer_review_strategy method.
def gen_random_participant_id(iterator, participants_hash, num_participants, participants)
if iterator.zero?
rand_num = rand(0..num_participants - 1)
else
min_value = participants_hash.values.min
# get the temp array including indices of participants, each participant has minimum review number in hash table.
participants_with_min_assigned_reviews = []
participants.each do |participant|
participants_with_min_assigned_reviews << participants.index(participant) if participants_hash[participant.id] == min_value
end
# if participants_with_min_assigned_reviews is blank
no_particpants = participants_with_min_assigned_reviews.empty?
# or only one element in participants_with_min_assigned_reviews, prohibit one student to review his/her own artifact
participant_is_owner = (participants_with_min_assigned_reviews.size == 1 and TeamsUser.exists?(team_id: team.id, user_id: participants[participants_with_min_assigned_reviews[0]].user_id))
rand_num = if no_particpants or participant_is_owner
# use original method to get random number
rand(0..num_participants - 1)
else
# rand_num should be the position of this participant in original array
participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size - 1)]
end
end
return rand_num
end
</pre>
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
       *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern 
       *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method. 
<pre>
# Helper Method to check num_reviews_per_student and num_reviews_per_submission arguments passed in by params hash.
def check_num_reviews_args(num_reviews_per_student, num_reviews_per_submission, teams)
has_error_not_raised = true
# check for exit paths first
if num_reviews_per_student == 0 and num_reviews_per_submission == 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
has_error_not_raised = false
elsif num_reviews_per_student != 0 and num_reviews_per_submission != 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
has_error_not_raised = false
elsif num_reviews_per_student >= teams.size
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
flash[:error] = 'You cannot set the number of reviews done ' \
'by each student to be greater than or equal to total number of teams ' \
'[or "participants" if it is an individual assignment].'
has_error_not_raised = false
end
end
</pre>
7. Modularized helper methods into a module and was mixed in the ReviewMappingController Class. 
       *Only some of the methods written in the class have external usage i.e called by another controllers, views etc. 
       *The other methods are just helpers and as such moved into a Helper method module and mixed in the class 
Before Modularization
<pre>
class ReviewMappingController < ApplicationController
...................
...................
512 Lines & 25 Methods Defined (After moving a few methods into separate controllers)
...................
...................
end
</pre>
After Modularization
<pre>
module Helper_methods
...................
5 Methods and 170 lines
...................
end

class ReviewMappingController < ApplicationController
include Helper_methods
...................
340 Lines & 20 Methods (All of those are used elsewhere directly in the application)
...................
end
</pre>

8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle. 
       *Internally used methods were not tested 
       *Tests for newly created controllers have been moved into a separate spec files. 

9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
       *This method assigns a quiz(Stored as an assignment object) to the participant. 
       *This method is not related to ReviewMapping functionality, so it was made into a new controller. 

10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
       *The controller paths present in the views/partials have to be changed to not to break the functionality 
       *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically 

View File Affected by the creation of AssinQuizController
[[File:View.png]]

11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

== Test Plan ==

=== Rspec Unit Tests ===

Since this is a Refactoring Project, We made sure that the changes made did not break any functionality.

[[File:testplan.png]]

Note: Tests have been run for three controllers (one existing and two new).

=== Capybara Integration and Functional Tests ===

As the controller routes have been modified in the routes.rb and the other view files, there are potential chances of failures in Integration tests.

However, no such failure has been reported by Travis build.

[[File:Travis CI.png]]

[[File:FeaturesCI.png]]

== Code Coverage ==

Code Coverage for Controllers section climbed up.

[https://coveralls.io/builds/26607665] # Link for the COVERALLS stats of our pull request.

[[File:CCC1.png]]

== Project Mentor ==

Ramya Vijayakumar (rvijaya4@ncsu.edu)

== Team Members ==

Yaswanth Soodini (ysoodin@ncsu.edu)

Saurabh Shingte (svshingt@ncsu.edu)

Vivek Karri (vkarri@ncsu.edu)

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-11-03T01:09:18Z

Svshingt: /* Files modified/created in the current project */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.
sd
== About Expertiza==

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

== Description of the project ==

The focus of the project is on a controller named ReviewMappingController and the primary goal is to make changes to the internal structure of the controller to make it easier to read and cheaper to maintain without changing it's observable behaviour. This can be achieved though refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code, etc.

The number of routes directed to review_mapping_controller is illustrated here:

[[File:complexity.png]]

== Work-Plan Followed ==
[[File:FlowchartOODD.jpg]]
 
We were tasked to refactor the review_mapping_controller.rb and solve any cascading issues or bugs we could find. We followed the above work plan to complete this task. There were many times when all the Rspec and Cucumber tests passed locally but ran into an issue when we uploaded the changes on GitHub. Prompt feedback from the TRAVIS CI helped us recognize the issue. Then we went on local machine and followed the whole process of refactoring again. In this way, we covered every refactoring we did and ensured that the TRAVIS CI get passed with minimum issues on the code-climate.

== Files modified/created in the current project ==

1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of long and complex methods such as peer_review_strategy and automatic_review_mapping were refactored from this controller along with the removal of some non-related methods such as add_calibration and assign_quiz_dynamically. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

After refactoring the Review_Mapping_Controller.rb, there were some tests still present in the spec file of this controller. So, we removed such tests from the review_mapping_controller_spec.rb to the appropriate spec file.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

Add Calibration is a nuanced method that has seemingly different functionality than Review Mapping Controller and hence moved into a separate controller.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of the changes made==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything. 
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability. 
<pre>
## Helper Method for generating a random participant which is to be used in peer_review_strategy method.
def gen_random_participant_id(iterator, participants_hash, num_participants, participants)
if iterator.zero?
rand_num = rand(0..num_participants - 1)
else
min_value = participants_hash.values.min
# get the temp array including indices of participants, each participant has minimum review number in hash table.
participants_with_min_assigned_reviews = []
participants.each do |participant|
participants_with_min_assigned_reviews << participants.index(participant) if participants_hash[participant.id] == min_value
end
# if participants_with_min_assigned_reviews is blank
no_particpants = participants_with_min_assigned_reviews.empty?
# or only one element in participants_with_min_assigned_reviews, prohibit one student to review his/her own artifact
participant_is_owner = (participants_with_min_assigned_reviews.size == 1 and TeamsUser.exists?(team_id: team.id, user_id: participants[participants_with_min_assigned_reviews[0]].user_id))
rand_num = if no_particpants or participant_is_owner
# use original method to get random number
rand(0..num_participants - 1)
else
# rand_num should be the position of this participant in original array
participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size - 1)]
end
end
return rand_num
end
</pre>
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
       *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern 
       *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method. 
<pre>
# Helper Method to check num_reviews_per_student and num_reviews_per_submission arguments passed in by params hash.
def check_num_reviews_args(num_reviews_per_student, num_reviews_per_submission, teams)
has_error_not_raised = true
# check for exit paths first
if num_reviews_per_student == 0 and num_reviews_per_submission == 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
has_error_not_raised = false
elsif num_reviews_per_student != 0 and num_reviews_per_submission != 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
has_error_not_raised = false
elsif num_reviews_per_student >= teams.size
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
flash[:error] = 'You cannot set the number of reviews done ' \
'by each student to be greater than or equal to total number of teams ' \
'[or "participants" if it is an individual assignment].'
has_error_not_raised = false
end
end
</pre>
7. Modularized helper methods into a module and was mixed in the ReviewMappingController Class. 
       *Only some of the methods written in the class have external usage i.e called by another controllers, views etc. 
       *The other methods are just helpers and as such moved into a Helper method module and mixed in the class 
Before Modularization
<pre>
class ReviewMappingController < ApplicationController
...................
...................
512 Lines & 25 Methods Defined (After moving a few methods into separate controllers)
...................
...................
end
</pre>
After Modularization
<pre>
module Helper_methods
...................
5 Methods and 170 lines
...................
end

class ReviewMappingController < ApplicationController
include Helper_methods
...................
340 Lines & 20 Methods (All of those are used elsewhere directly in the application)
...................
end
</pre>

8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle. 
       *Internally used methods were not tested 
       *Tests for newly created controllers have been moved into a separate spec files. 

9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
       *This method assigns a quiz(Stored as an assignment object) to the participant. 
       *This method is not related to ReviewMapping functionality, so it was made into a new controller. 

10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
       *The controller paths present in the views/partials have to be changed to not to break the functionality 
       *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically 

View File Affected by the creation of AssinQuizController
[[File:View.png]]

11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

== Test Plan ==

=== Rspec Unit Tests ===

Since this is a Refactoring Project, We made sure that the changes made did not break any functionality.

[[File:testplan.png]]

Note: Tests have been run for three controllers (one existing and two new).

=== Capybara Integration and Functional Tests ===

As the controller routes have been modified in the routes.rb and the other view files, there are potential chances of failures in Integration tests.

However, no such failure has been reported by Travis build.

[[File:Travis CI.png]]

[[File:FeaturesCI.png]]

== Code Coverage ==

Code Coverage for Controllers section climbed up.

[https://coveralls.io/builds/26607665] # Link for the COVERALLS stats of our pull request.

[[File:CCC1.png]]

== Project Mentor ==

Ramya Vijayakumar (rvijaya4@ncsu.edu)

== Team Members ==

Yaswanth Soodini (ysoodin@ncsu.edu)

Saurabh Shingte (svshingt@ncsu.edu)

Vivek Karri (vkarri@ncsu.edu)

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-11-03T01:04:11Z

Svshingt: /* Work-Plan Followed */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.
sd
== About Expertiza==

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

== Description of the project ==

The focus of the project is on a controller named ReviewMappingController and the primary goal is to make changes to the internal structure of the controller to make it easier to read and cheaper to maintain without changing it's observable behaviour. This can be achieved though refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code, etc.

The number of routes directed to review_mapping_controller is illustrated here:

[[File:complexity.png]]

== Work-Plan Followed ==
[[File:FlowchartOODD.jpg]]
 
We were tasked to refactor the review_mapping_controller.rb and solve any cascading issues or bugs we could find. We followed the above work plan to complete this task. There were many times when all the Rspec and Cucumber tests passed locally but ran into an issue when we uploaded the changes on GitHub. Prompt feedback from the TRAVIS CI helped us recognize the issue. Then we went on local machine and followed the whole process of refactoring again. In this way, we covered every refactoring we did and ensured that the TRAVIS CI get passed with minimum issues on the code-climate.

== Files modified/created in the current project ==

1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of long and complex methods such as peer_review_strategy and automatic_review_mapping were refactored from this controller along with the removal of some non-related methods such as add_calibration and assign_quiz_dynamically. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

Tests were removed from this file which belongs to the isolated methods.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

Add Calibration is a nuanced method that has seemingly different functionality than Review Mapping Controller and hence moved into a separate controller.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of the changes made==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything. 
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability. 
<pre>
## Helper Method for generating a random participant which is to be used in peer_review_strategy method.
def gen_random_participant_id(iterator, participants_hash, num_participants, participants)
if iterator.zero?
rand_num = rand(0..num_participants - 1)
else
min_value = participants_hash.values.min
# get the temp array including indices of participants, each participant has minimum review number in hash table.
participants_with_min_assigned_reviews = []
participants.each do |participant|
participants_with_min_assigned_reviews << participants.index(participant) if participants_hash[participant.id] == min_value
end
# if participants_with_min_assigned_reviews is blank
no_particpants = participants_with_min_assigned_reviews.empty?
# or only one element in participants_with_min_assigned_reviews, prohibit one student to review his/her own artifact
participant_is_owner = (participants_with_min_assigned_reviews.size == 1 and TeamsUser.exists?(team_id: team.id, user_id: participants[participants_with_min_assigned_reviews[0]].user_id))
rand_num = if no_particpants or participant_is_owner
# use original method to get random number
rand(0..num_participants - 1)
else
# rand_num should be the position of this participant in original array
participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size - 1)]
end
end
return rand_num
end
</pre>
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
       *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern 
       *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method. 
<pre>
# Helper Method to check num_reviews_per_student and num_reviews_per_submission arguments passed in by params hash.
def check_num_reviews_args(num_reviews_per_student, num_reviews_per_submission, teams)
has_error_not_raised = true
# check for exit paths first
if num_reviews_per_student == 0 and num_reviews_per_submission == 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
has_error_not_raised = false
elsif num_reviews_per_student != 0 and num_reviews_per_submission != 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
has_error_not_raised = false
elsif num_reviews_per_student >= teams.size
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
flash[:error] = 'You cannot set the number of reviews done ' \
'by each student to be greater than or equal to total number of teams ' \
'[or "participants" if it is an individual assignment].'
has_error_not_raised = false
end
end
</pre>
7. Modularized helper methods into a module and was mixed in the ReviewMappingController Class. 
       *Only some of the methods written in the class have external usage i.e called by another controllers, views etc. 
       *The other methods are just helpers and as such moved into a Helper method module and mixed in the class 
Before Modularization
<pre>
class ReviewMappingController < ApplicationController
...................
...................
512 Lines & 25 Methods Defined (After moving a few methods into separate controllers)
...................
...................
end
</pre>
After Modularization
<pre>
module Helper_methods
...................
5 Methods and 170 lines
...................
end

class ReviewMappingController < ApplicationController
include Helper_methods
...................
340 Lines & 20 Methods (All of those are used elsewhere directly in the application)
...................
end
</pre>

8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle. 
       *Internally used methods were not tested 
       *Tests for newly created controllers have been moved into a separate spec files. 

9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
       *This method assigns a quiz(Stored as an assignment object) to the participant. 
       *This method is not related to ReviewMapping functionality, so it was made into a new controller. 

10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
       *The controller paths present in the views/partials have to be changed to not to break the functionality 
       *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically 

View File Affected by the creation of AssinQuizController
[[File:View.png]]

11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

== Test Plan ==

=== Rspec Unit Tests ===

Since this is a Refactoring Project, We made sure that the changes made did not break any functionality.

[[File:testplan.png]]

Note: Tests have been run for three controllers (one existing and two new).

=== Capybara Integration and Functional Tests ===

As the controller routes have been modified in the routes.rb and the other view files, there are potential chances of failures in Integration tests.

However, no such failure has been reported by Travis build.

[[File:Travis CI.png]]

[[File:FeaturesCI.png]]

== Code Coverage ==

Code Coverage for Controllers section climbed up.

[https://coveralls.io/builds/26607665] # Link for the COVERALLS stats of our pull request.

[[File:CCC1.png]]

== Project Mentor ==

Ramya Vijayakumar (rvijaya4@ncsu.edu)

== Team Members ==

Yaswanth Soodini (ysoodin@ncsu.edu)

Saurabh Shingte (svshingt@ncsu.edu)

Vivek Karri (vkarri@ncsu.edu)

File:FlowchartOODD.jpg

2019-11-03T00:56:19Z

Svshingt:

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-11-03T00:55:00Z

Svshingt:

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.
sd
== About Expertiza==

[http://expertiza.ncsu.edu/ Expertiza] is an open source project based on [http://rubyonrails.org/ Ruby on Rails] framework. Expertiza allows the instructor to create new assignments and customize new or existing assignments. It also allows the instructor to create a list of topics the students can sign up for. Students can form teams in Expertiza to work on various projects and assignments. Students can also peer review other students' submissions. Expertiza supports submission across various document types, including the URLs and wiki pages.

== Description of the project ==

The focus of the project is on a controller named ReviewMappingController and the primary goal is to make changes to the internal structure of the controller to make it easier to read and cheaper to maintain without changing it's observable behaviour. This can be achieved though refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code, etc.

The number of routes directed to review_mapping_controller is illustrated here:

[[File:complexity.png]]

== Work-Plan Followed ==

== Files modified/created in the current project ==

1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of long and complex methods such as peer_review_strategy and automatic_review_mapping were refactored from this controller along with the removal of some non-related methods such as add_calibration and assign_quiz_dynamically. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

Tests were removed from this file which belongs to the isolated methods.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

Add Calibration is a nuanced method that has seemingly different functionality than Review Mapping Controller and hence moved into a separate controller.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of the changes made==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything. 
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability. 
<pre>
## Helper Method for generating a random participant which is to be used in peer_review_strategy method.
def gen_random_participant_id(iterator, participants_hash, num_participants, participants)
if iterator.zero?
rand_num = rand(0..num_participants - 1)
else
min_value = participants_hash.values.min
# get the temp array including indices of participants, each participant has minimum review number in hash table.
participants_with_min_assigned_reviews = []
participants.each do |participant|
participants_with_min_assigned_reviews << participants.index(participant) if participants_hash[participant.id] == min_value
end
# if participants_with_min_assigned_reviews is blank
no_particpants = participants_with_min_assigned_reviews.empty?
# or only one element in participants_with_min_assigned_reviews, prohibit one student to review his/her own artifact
participant_is_owner = (participants_with_min_assigned_reviews.size == 1 and TeamsUser.exists?(team_id: team.id, user_id: participants[participants_with_min_assigned_reviews[0]].user_id))
rand_num = if no_particpants or participant_is_owner
# use original method to get random number
rand(0..num_participants - 1)
else
# rand_num should be the position of this participant in original array
participants_with_min_assigned_reviews[rand(0..participants_with_min_assigned_reviews.size - 1)]
end
end
return rand_num
end
</pre>
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
       *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern 
       *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method. 
<pre>
# Helper Method to check num_reviews_per_student and num_reviews_per_submission arguments passed in by params hash.
def check_num_reviews_args(num_reviews_per_student, num_reviews_per_submission, teams)
has_error_not_raised = true
# check for exit paths first
if num_reviews_per_student == 0 and num_reviews_per_submission == 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student)."
has_error_not_raised = false
elsif num_reviews_per_student != 0 and num_reviews_per_submission != 0
flash[:error] = "Please choose either the number of reviews per student or the number of reviewers per team (student), not both."
has_error_not_raised = false
elsif num_reviews_per_student >= teams.size
# Exception detection: If instructor want to assign too many reviews done
# by each student, there will be an error msg.
flash[:error] = 'You cannot set the number of reviews done ' \
'by each student to be greater than or equal to total number of teams ' \
'[or "participants" if it is an individual assignment].'
has_error_not_raised = false
end
end
</pre>
7. Modularized helper methods into a module and was mixed in the ReviewMappingController Class. 
       *Only some of the methods written in the class have external usage i.e called by another controllers, views etc. 
       *The other methods are just helpers and as such moved into a Helper method module and mixed in the class 
Before Modularization
<pre>
class ReviewMappingController < ApplicationController
...................
...................
512 Lines & 25 Methods Defined (After moving a few methods into separate controllers)
...................
...................
end
</pre>
After Modularization
<pre>
module Helper_methods
...................
5 Methods and 170 lines
...................
end

class ReviewMappingController < ApplicationController
include Helper_methods
...................
340 Lines & 20 Methods (All of those are used elsewhere directly in the application)
...................
end
</pre>

8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle. 
       *Internally used methods were not tested 
       *Tests for newly created controllers have been moved into a separate spec files. 

9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
       *This method assigns a quiz(Stored as an assignment object) to the participant. 
       *This method is not related to ReviewMapping functionality, so it was made into a new controller. 

10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
       *The controller paths present in the views/partials have to be changed to not to break the functionality 
       *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically 

View File Affected by the creation of AssinQuizController
[[File:View.png]]

11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

== Test Plan ==

=== Rspec Unit Tests ===

Since this is a Refactoring Project, We made sure that the changes made did not break any functionality.

[[File:testplan.png]]

Note: Tests have been run for three controllers (one existing and two new).

=== Capybara Integration and Functional Tests ===

As the controller routes have been modified in the routes.rb and the other view files, there are potential chances of failures in Integration tests.

However, no such failure has been reported by Travis build.

[[File:Travis CI.png]]

[[File:FeaturesCI.png]]

== Code Coverage ==

Code Coverage for Controllers section climbed up.

[https://coveralls.io/builds/26607665] # Link for the COVERALLS stats of our pull request.

[[File:CCC1.png]]

== Project Mentor ==

Ramya Vijayakumar (rvijaya4@ncsu.edu)

== Team Members ==

Yaswanth Soodini (ysoodin@ncsu.edu)

Saurabh Shingte (svshingt@ncsu.edu)

Vivek Karri (vkarri@ncsu.edu)

File:4 aft.jpg

2019-11-03T00:48:31Z

Svshingt:

File:3 aft.jpg

2019-11-03T00:47:38Z

Svshingt:

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-11-02T01:39:51Z

Svshingt: /* Details of the changes made */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.

== Expertiza Background==

Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NC State University. It’s an open-source project developed on Ruby on Rails platform and its codebase is available on Github. It facilitates peer review among the students allowing them to improve their work from the feedback. It also assists the faculty in customizing the specifications for the projects and Assignments.

== Description of the current project ==

The following is an Expertiza based OSS project which deals primarily with a controller namely review_mapping_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code etc among the other things. The goal of this project is to attempt to make this part of the application easier to read and maintain.

== Files modified in the current project ==

A controller and a helper file were modified for this project namely: 
1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of long and complex methods such as peer_review_strategy and automatic_review_mapping were refactored from this controller along with the removal of some non-related methods such as add_calibration and assign_quiz_dynamically. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

Tests were removed from this file which belongs to the isolated methods.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

Add Calibration is a nuanced method that has seemingly different functionality than Review Mapping Controller and hence moved into a separate controller.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of the changes made==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
       *A random participant_id is generated from the possible pool of candidates but the code block for that is kind of a query, i.e. it does not change or set anything. 
       *And it is equally complex enough to confuse the reader. So this has been put into a helper method with an expressive name to increase readability. 

6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
       *Parameters such as num_reviews_per_student, num_calibrated_artifacts etc passed are first verified to check if they are in acceptable range or pattern 
       *To increase readability, the not-so good looking sets of if-else statements have been moved into check_num_reviews_args method. 

7. Modularized helper methods (which are not accessed by any other functionality outside the review_mapping_controller) into a module and was mixed in the ReviewMappingController Class. 
       *Only some of the methods written in the class have external usage i.e called by another controllers, views etc. 
       *The other methods are just helpers and as such moved into a Helper method module and mixed in the class 

8. Abided to the principles of Magic Tricks of testing and did not test any internally used methods, The other tests are written were already following this principle. 
       *Internally used methods were not tested 
       *Tests for newly created controllers have been moved into a separate spec files. 

9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
       *This method assigns a quiz(Stored as an assignment object) to the participant. 
       *This method is not related to ReviewMapping functionality, so it was made into a new controller. 

10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
       *The controller paths present in the views/partials have to be changed to not to break the functionality 
       *The controller paths in views/partials/routes have been changed for the newly created controller of AssignQuizDynamically 

11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-10-30T16:31:59Z

Svshingt:

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.

== Expertiza Background==

Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NC State University. It’s an open-source project developed on Ruby on Rails platform and its codebase is available on Github. It facilitates peer review among the students allowing them to improve their work from the feedback. It also assists the faculty in customizing the specifications for the projects and Assignments.

== Description of the current project ==

The following is an Expertiza based OSS project which deals primarily with a controller namely review_mapping_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code etc among the other things. The goal of this project is to attempt to make this part of the application easier to read and maintain.

== Files modified in the current project ==

A controller and a helper file were modified for this project namely: 
1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of long and complex methods such as peer_review_strategy and automatic_review_mapping were refactored from this controller along with the removal of some non-related methods such as add_calibration and assign_quiz_dynamically. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

Tests were removed from this file which belongs to the isolated methods.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

Add Calibration is a nuanced method that has seemingly different functionality than Review Mapping Controller and hence moved into a separate controller.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of changes ==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *It was also used in review_mapping_controller.rb to check if student has selected any particular topic. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
7. Modularized helper methods (which are not accessed by any other functionality outside the review_mapping_controller) into a module and was mixed in the ReviewMappingController Class. 
8. Abided to the principles of Magic Tricks of testing specified in wand did not test any internally used methods, The other tests are written were already following this principle. 
9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

File:4 bef.PNG

2019-10-30T16:07:14Z

Svshingt:

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-10-30T05:37:26Z

Svshingt: /* Details of changes */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.

== Expertiza Background==

Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NC State University. It’s an open-source project developed on Ruby on Rails platform and its codebase is available on Github. It facilitates peer review among the students allowing them to improve their work from the feedback. It also assists the faculty in customizing the specifications for the projects and Assignments.

== Description of the current project ==

The following is an Expertiza based OSS project which deals primarily with a controller namely review_mapping_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code etc among the other things. The goal of this project is to attempt to make this part of the application easier to read and maintain.

== Files modified in the current project ==

A controller and a helper file were modified for this project namely: 
1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of long and complex methods such as peer_review_strategy and automatic_review_mapping were refactored from this controller along with the removal of some non-related methods such as add_calibration and assign_quiz_dynamically. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

Tests were removed from this file which belongs to the isolated methods.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

Add Calibration is a nuanced method that has seemingly different functionality than Review Mapping Controller and hence moved into a separate controller.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of changes ==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy from review_mapping_controller_spec.rb. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case in above method from review_mapping_controller.rb contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
7. Modularized helper methods (which are not accessed by any other functionality outside the review_mapping_controller) into a module and was mixed in the ReviewMappingController Class. 
8. Abided to the principles of Magic Tricks of testing specified in wand did not test any internally used methods, The other tests are written were already following this principle. 
9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-10-30T04:12:24Z

Svshingt: /* Details of changes */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.

== Expertiza Background==

Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NC State University. It’s an open-source project developed on Ruby on Rails platform and its codebase is available on Github. It facilitates peer review among the students allowing them to improve their work from the feedback. It also assists the faculty in customizing the specifications for the projects and Assignments.

== Description of the current project ==

The following is an Expertiza based OSS project which deals primarily with a controller namely review_mapping_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code etc among the other things. The goal of this project is to attempt to make this part of the application easier to read and maintain.

== Files modified in the current project ==

A controller and a helper file were modified for this project namely: 
1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of long and complex methods such as peer_review_strategy and automatic_review_mapping were refactored from this controller along with the removal of some non-related methods such as add_calibration and assign_quiz_dynamically. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

Tests were removed from this file which belongs to the isolated methods.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

Add Calibration is a nuanced method that has seemingly different functionality than Review Mapping Controller and hence moved into a separate controller.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of changes ==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
       *Switch case contained all the actions having the same output for around 70% of cases. 
       *Hence, replaced the switch statements and initialized a list with those switch cases. 
       *Even the space complexity increased, the tradeoff got balanced because if someone has to change some actions, he will have to just add or remove the action name from the allowed_actions list. 

5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
7. Modularized helper methods (which are not accessed by any other functionality outside the review_mapping_controller) into a module and was mixed in the ReviewMappingController Class. 
8. Abided to the principles of Magic Tricks of testing specified in wand did not test any internally used methods, The other tests are written were already following this principle. 
9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-10-30T04:01:18Z

Svshingt: /* Details of changes */

This wiki page is for the description of changes made under E1944 OSS assignment for Fall 2019, CSC/ECE 517.

== Expertiza Background==

Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NC State University. It’s an open-source project developed on Ruby on Rails platform and its codebase is available on Github. It facilitates peer review among the students allowing them to improve their work from the feedback. It also assists the faculty in customizing the specifications for the projects and Assignments.

== Description of the current project ==

The following is an Expertiza based OSS project which deals primarily with a controller namely review_mapping_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code etc among the other things. The goal of this project is to attempt to make this part of the application easier to read and maintain.

== Files modified in the current project ==

A controller and a helper file were modified for this project namely: 
1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of long and complex methods such as peer_review_strategy and automatic_review_mapping were refactored from this controller along with the removal of some non-related methods such as add_calibration and assign_quiz_dynamically. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

Tests were removed from this file which belongs to the isolated methods.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== ReviewResponseMapController.rb ===

Add Calibration is a nuanced method that has seemingly different functionality than Review Mapping Controller and hence moved into a separate controller.

=== review_response_map_controller_spec.rb ===

Tests related to review_response_map were moved into this file.

=== routes.rb ===

New routes were added to newly created controllers.

=== views/partials ===

Routes were changed in the views and partials.

== Details of changes ==
1. Changed 'instructor = build(:instructor)' to ‘@instructor = build(:instructor, id: 1)’. 
       *Removed code redundancy. Two variables were being initialized containing the same value. One was in the before(:each) loop and other was being called in first three test cases. 
       *Replaced the one in the before(:each) loop by @instructor = build(:instructor, id: 1) and used @instructor class variable, wherever required. 

2. Changed :i_dont_care to :no_particular_topic 
       *:i_dont_care was used in the /app/views/student_review/_set_dynamic_review.html.erb as a flag to store if student is interested in any particular topic or doesn't care which topic to review. 
       *Since, name :i_dont_care was very difficult to understand, we replaced it with something logical such as :no_particular_topic. It gives hint about what the symbol stores. 

3. Removed cascading effects of above change from features spec 
       *Above changes caused ./spec/features/review_assignment_spec.rb this feature test to fail. 
       *This spec has used the above symbol to check if the list of available topics collapse or not after selecting the I don't care option. 

4. Created a variable named ‘allowed_actions’ in method choose_case(action_in_params) 
5. Refactored Peer_review_strategy by using a helper method gen_random_participant_id 
6. Refactored automatic_review_mapping by using a helper method check_num_reviews_args 
7. Modularized helper methods (which are not accessed by any other functionality outside the review_mapping_controller) into a module and was mixed in the ReviewMappingController Class. 
8. Abided to the principles of Magic Tricks of testing specified in wand did not test any internally used methods, The other tests are written were already following this principle. 
9. Isolated AssignQuizDynamically method into a separate controller as the functionality was not related to ReviewMappingController. 
10. Associated Specs/routes/views/partials have been modified to adapt the change in controllers. 
11. student_review_num sounds like a number or an id associated with a student review, while it actually stores the number of reviews that a student can perform. So it is renamed num_reviews_per_student. 
12. submission_review_num sounds like a number or an id associated with a submission review, while it actually stores the total number of reviews that can be performed on a single submission. So it is renamed num_reviews_per_submission. 
13. calibrated_artifacts_num sounds like a number or an id associated with the calibrated artifacts, while it actually stores the number of calibrated artifacts. So it is renamed num_calibrated_artifacts. Similarly, uncalibrated_artifacts_num is renamed num_uncalibrated_artifacts. 
14. participants_hash is not an appropriate name for a hash whose keys are participant ids and values are number of reviews performed by corresponding participants. So it is renamed num_reviews_by_participant_hash. 
15. Extracted a method make_review_strategy (from automatic_review_mapping_strategy) that returns a review_strategy based on the values of num_reviews_per_submission and num_reviews_per_submission. 
16. add_calibration is a method that changes the attribute of a ReviewResponseMap and has little to do with Review Mapping. So it is now put in a separate controller named ReviewResponseMapController. 
17. The name add_reviewer may lead the reader to think that the method adds a reviewer to a collection of reviewers (e.g. a list of reviewers). Changing the name to assign_reviewer_manually informs the reader that the method assigns a reviewer (to a submission) and hence improves the readability of the code.

CSC/ECE 517 Fall 2019

2019-10-29T04:11:40Z

Svshingt:

* [[CSC/ECE 517 Fall 2019 - Project E1947. Refactor quiz_questionnaire_controller.rb]]
* [[CSC/ECE 517 Fall 2019 - Project E1965. Review report should link to the usual view for reviews]]
* [[CSC/ECE 517 Fall 2019 - E1972. OSS project J. Skellington: Accessing Assignment Rubrics]]
* [[CSC/ECE 517 Fall 2019 - E1961. Email notification to reviewers and instructors]]
* [[CSC/ECE 517 Fall 2019 - E1971. OSS project Finklestein: Instructors & Institutions]]
* [[CSC/ECE 517 Fall 2019 - E1953. Tagging report for student]]
* [[CSC/ECE 517 Fall 2019 - E1955.Write unit tests for student_task.rb]]
* [[CSC/ECE 517 Fall 2019 - E1954. Auto-generate submission directory names based on assignment names]]
* [[CSC/ECE 517 Fall 2019 - E1958. Two issues related to assignment management]]
* [[CSC/ECE 517 Fall 2019 - E1948. Refactor review_mapping_helper.rb]]
* [[CSC/ECE 517 Fall 2019 - E1959. Intelligent copying of assignments without topics]]
* [[CSC/ECE 517 Fall 2019 - E1968. Fixes for adding members to teams]]
* [[CSC/ECE 517 Fall 2019 - E1969. Fixes for reviews not being available]]
* [[CSC/ECE 517 Fall 2019 - E1951. Remove multiple topics at a time]]
* [[CSC/ECE 517 Fall 2019 - E1957. Time travel Not Allowed..!!! Restrict TAs’ ability to change their own grade + limit file-size upload]]
* [[CSC/ECE 517 Fall 2019 - E1963. Changing assignment participant role]]
* [[CSC/ECE 517 Fall 2019 - E1941. Issues related to topic deadlines]]
* [[CSC/ECE 517 Fall 2019 - E1966. Tabbed_reviews partial file refactor for displaying the alternate view of reviews]]
* [[CSC/ECE 517 Fall 2019 - E1962. Email notification upon account creation]]
* [[CSC/ECE 517 Fall 2019 - E1967. Fix glitches in author feedback]]
* [[CSC/ECE 517 Fall 2019 - E1960. Create new late policy successfully and fixing "Back" link]]
* [[CSC/ECE 517 Fall 2019 - E1939. OSS Project Juniper: Bookmark enhancements]]
* [[CSC/ECE 517 Fall 2019 - M1950. Support Asynchronous Web Assembly Compilation]]
* [[CSC/ECE 517 Fall 2019 - E1938. OSS project Duke Blue: Fix import glitches]]
* [[CSC/ECE 517 Fall 2019 - M1951. Implement missing OffscreenCanvas APIs]]
* [[CSC/ECE 517 Fall 2019 - E1940. Improving email notification]]
* [[CSC/ECE 517 Fall 2019 - E1956. There is no shortcut to get free review points: Review Assignment Bug]]
* [[CSC/ECE 517 Fall 2019 - E1942. Refactor stage deadlines in assignment.rb]]
* [[CSC/ECE 517 Fall 2019 - M1952. Missing DOM features project]]
* [[CSC/ECE 517 Fall 2019 - E1945. Refactor users_controller.rb]]
* [[CSC/ECE 517 Fall 2019 - E1964. Export review scores for projects]]
* [[CSC/ECE 517 Fall 2019 - E1949. Write Unit Tests for Importing assignment participants and import glitches]]
* [[CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller]]

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-10-29T03:58:51Z

Svshingt:

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-10-29T03:52:58Z

Svshingt:

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-10-29T03:49:37Z

Svshingt:

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-10-29T03:48:05Z

Svshingt:

CSC/ECE 517 Fall 2019 - E1944. Refactor review mapping controller

2019-10-29T03:44:33Z

Svshingt:

This wiki page is for the description of changes made under E1555 OSS assignment for Fall 2015, CSC/ECE 517.

== Expertiza Background==

Expertiza is an educational web application created and maintained by the joint efforts of the students and the faculty at NC State University. It’s an open-source project developed on Ruby on Rails platform and its codebase is available on Github. It facilitates peer review among the students allowing them to improve their work from the feedback. It also assists the faculty in customizing the specifications for the projects and Assignments.

== Description of the current project ==

The following is an Expertiza based OSS project which deals primarily with a controller namely review_mapping_controller.rb. It focusses on refactoring some of the more complex methods, modifying some of the language to make it more Ruby friendly, removing redundant code etc among the other things. The goal of this project is to attempt to make this part of the application easier to read and maintain.

== Files modified in current project ==

A controller and a helper file were modified for this project namely: 
1. review_mapping_controller.rb 
2. review_mapping_controller_spec.rb 
3. assign_quiz_controller.rb 
4. assign_quiz_controller_spec.rb 
5. review_response_map_controller.rb 
6. review_response_map_controller_spec.rb 
7. routes.rb 
8. Other views & partials associated affected by these changes 

=== ReviewMappingController ===

This controller will map the submissions made by the teams to the students for facilitating peer-reviewing. A couple of long and complex methods such as peer_review_strategy and automatic_review_mapping were refactored from this controller along with the removal of some non-related methods such as add_calibration and assign_quiz_dynamically. Variable names have been changed and code has been modularized and helper methods were separated from the important methods into a module and were included in the class.

Test Cases were created for the newly created controllers such as assign_quiz_controller etc.

=== review_mapping_controller_spec.rb ===

Tests were removed from this file which belongs to the isolated methods.

=== AssignQuizController ===

Assigning Quizzes (Quizzes are also stored in the Assignment table) is not a seemingly/semantically related task to review mapping. Hence this was moved into a separate controller.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

=== assign_quiz_controller_spec.rb ===

Tests related to assign_quiz_controller were moved into this file.

== List of changes ==
We worked on the following work items(WIs) 
WI1 : Refactor calculate_all_penalties method into smaller methods 
WI2 : Move the repeated code in conflict_notification & edit methods to a separate method list_questions. 
WI3 : Refactor the code as per the Ruby style guidelines and incorporate the good practices 
WI4 : Test the conflict_notification method to test the changes made. 
WI5 : Move the repeated code in view and view_my_scores methods to a separate method retrieve_questions 
WI6 : Change the repeated varaible decalreation and put into single common block. 
WI7 : Change ambiguous variable names in more sensible variable names 

=== Solutions Implemented and Delivered ===

*Refactoring calculate_all_penalties method

This is used to calculate various penalty values for each assignment if penalty is applicable.

The following changes were made:

1. This method was very complex, performing too many functions within a single method and had to be broken into 3 smaller methods each having a more well defined function.
2. The following 3 methods were created after splitting the first method 
i. calculate_all_penalties 
ii. calculate_penatly_attributes 
iii. assign_all_penalties 
3. Changes were also made to make the code follow ruby style.The language was made more ruby friendly.
4. Finally some redundant code was commented out as it was non-functional.

Refactoring into smaller more specific methods:

[[File:Change6_new.png]]

Removal of non-functional code :

[[File:Change5_new.png]]

Change of language to make it more Ruby friendly:

[[File:Change1_new.png]]

*Move the redundant piece of code from conflict_notification & edit methods to a new method list_questions

The conflict_notification method is used to help the instructors decide if one of the reviews are unfair or inaccurate.
This was again split into 2 methods with some part of the code which is repeated in another method refactored into a new method.

[[File:Change3_new.png]]

Refactored #Created a method which was a duplicate in conflict_notification and edit methods

[[File:Change4_new.png]]

edit method:

This method is used to edit the questionnaires. This method again has code which is repeated in the conflict_notification method and thus the repeated section was split into a new method.

[[File:Change2_new.png]]

New method:
Refactored #Created a method which was a duplicate in conflict_notification and edit methods

[[File:Change4_new.png]]

Similar refactoring was performed to obtain the retrieve_questions method:

[[File:Latest1.png]]

This is the new method created after the above refactoring:

[[File:Latest2.png]]

== Testing Details==

=== RSpec ===
There were no existing test cases for the GradesController. We have added a new spec file 'grades_spec.rb' which covers testing scenario for the newly added method. The specs were run on the previous and current files and they return the same results implying that the refactored code does not break anything.
As the model was not changed, no test cases were added for the model.

=== UI Testing ===

Following steps needs to be performed to test this code from UI: 
1. Login as instructor. Create a course and an assignment under that course. 
2. Keep the has team checkbox checked while creating the assignment. Add a grading rubric to it. Add at least two students as participants to the assignment. 
3. Create topics for the assignment. 
4. Sign in as one of the students who were added to the assignment. 
5. Go to the assignment and sign up for a topic. 
6. Submit student's work by clicking 'Your work' under that assignment. 
7. Sign in as a different student which is participant of the assignment. 
8. Go to Assignments--><assignment name>-->Others' work (If the link is disabled, login as instructor and change the due date of the assignment to current time). 
9. Give reviews on first student's work. 
10. Login as instructor or first student to look at the review grades. 

== Scope for future improvement ==
1. The construct_table method in GradesHelper is not used anywhere. It has no reference in the project. So we feel it can be safely removed. 
2. The has_team_and_metareview? method in GradesHelper can be broken down into separate methods, one each for team and metareview. This will provide improved flexibility. It needs some analysis though, as both the entities(team & metareview) are currently checked in conjuction from all the views they are referenced from.