CSC/ECE 517/Spring 2023/E2308. Refactor course.rb and course team.rb models: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
 
(63 intermediate revisions by 3 users not shown)
Line 1: Line 1:
==E2308. Refactor course.rb and course_team.rb models==


This page provides a description of the Expertiza based OSS project.


__TOC__
== Team Contact ==
=== Team Members ===
* Kartik Rawool, (unityID:khrawool, GitHub:kartikrawool)
* Ameya Vaichalkar, (unityID:agvaicha, GitHub:ameyagv)
* Vikram Pande, (unityID:vspande, GitHub:vikrampande7)
=== Mentor ===
* Kartiki Bhandakkar, kbhanda3
== Overview of Expertiza ==
Expertiza is an open-source software written using Ruby on Rails which functions as a learning management software system. It has many different functions and abilities including the ability to create assignments, quizzes, assignment groups, and topics, and also a complete mechanism for providing peer reviews and feedback for other groups and other teammates. Part of its functionality is a system containing information about the course and its associated teams respectively. The <code>Course</code> and <code>CourseTeam</code> which are the models primarily addressed in this project are both critical components in providing this functionality.
== Description of Project ==
<code>course</code> model stores information about the instructor and institution and is associated with other models such as User, CourseParticipant, CourseTeam, Assignment, AssignmentParticipant, and Notification. The course model is responsible for completing a variety of tasks, including returning course teams, returning the submission directory for the course, viewing participants enrolled in the course, adding a participant to the course, copying the Assignment Participants to Course Participants, and checking whether a user is part of any CourseTeam. The problem description lists code smells that need to be fixed. We fixed some issues, such as deleting the unused methods and renaming methods to indicate their action. The renaming problem required us to change a few methods calls where the function to be renamed was used. Also, to reduce the cognitive complexity of the copy_assignment_participants method, we created a method to separate the raising error functionality. Comments were added to indicate the action of the methods.
The <code>course_team</code> is a subclass of the team class. CourseTeams are a type of team that an instructor can use throughout an entire semester, providing consistency in team membership over time. The course_team model is responsible for completing various tasks, including returning the course of the team, adding a participant to the CourseTeam, and copying the CourseTeam Participants to AssignmentTeam Participants. There are methods for importing and exporting data to/from CSV files for CourseTeam objects. The problem description lists code smells/issues that need to be fixed. The issues that we fixed were deleting unused methods, renaming methods, and changing method calls, changing rspec expectations, adding meaningful comments. To refactor methods, DRY principles were used. In addition, analogous changes were made in the assignment_team model.
== Files Modified ==
== CHANGES IN MODEL FILES ==
=== Changes to <code>app/models/course.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Checked and Removed <code>get_participant</code> method 
|Checked the usage of Method through IDE whether the function is used, the only call was from <code>course_spec.rb</code> and not from anywhere else. Hence, removed from <code>course.rb</code>
|[https://github.com/kartikrawool/expertiza/commit/6403edc25cf0edde623da63f23d92a1a57d67375 Commit]
|-
|2
|style="width: 30%"| Renamed <code>copy_participants</code> method
|Renamed <code>copy_participants</code> to <code>copy_assignment_participants</code> which makes more sense and a clear idea of how method works and what is the purpose of method.
|[https://github.com/kartikrawool/expertiza/commit/5177376c0f9a4f1890f49879bbbbfee0b28b8263 Commit]
|-
|3
|Removed <code>add_member</code> method from <code>course_team</code> model
|Assignment_team and Course_team are subclasses of Team. Both should follow the similar logic of adding members of respective teams. As of for, add_participants, usage is different for course.rb and course_team.rb, hence it should be present in both. However, to make the code more consistent among classes, team, course_team, and assignment_team, the add_member method should only be present in the team class.
|[https://github.com/expertiza/expertiza/pull/2548/commits/8fb9bf9a793b8c154bc2b640daa630624e008a86 Commit]
|-
|4
|Created method <code>raise_errors</code>
|To reduce Cognitive Complexity of method <code>copy_participants</code>
|[https://github.com/kartikrawool/expertiza/commit/7c1ae764d2d64e0246422934f292b9b2fe43a031 Commit]
|}
=== Changes to <code>app/models/course_team.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Removed <code>assignment_id</code>
|CourseTeam and AssignmentTeam inherit from Team class. The teams' table has an attribute <code>parent_id</code> that helps determine whether the team is AssigmentTeam or CourseTeam. Thus, there is no need for <code>assignment_id</code> in the CourseTeam class and no need for <code>course_id</code> in AssignmentTeam class. It will be a bad coding practice to call the assignment_id method on the CourseTeam object, calling the course_id method on the AssignmentTeam object as it would be inappropriate.
|[https://github.com/kartikrawool/expertiza/commit/ef9727542999c6cdbf2334f5b62ebb30fbfe8cdd Commit]
|-
|2
|Renamed <code>copy</code> method as <code>copy_to_assignment_team</code>
|As this method is copying a CourseTeam to an AssignmentTeam, renamed it as <code>copy_to_assignment_team</code>. A suggestion for copy method in AssignmentTeam would be to rename it as <code>copy_to_course_team</code>.
|[https://github.com/kartikrawool/expertiza/commit/372b8d44dfbc3b29c07f41500a5cc53d23cf577b#diff-56ab3d3b28f0be68663223f0a5ef9daa5c8d106caca412481562272ab2edfae1 Commit]<br>
|-
|3
|Added Import Functionality 
|Refactored import method, added methods import_options, optional_import_fields, import_options which are being called in the ImportFileController.
|[https://github.com/kartikrawool/expertiza/commit/972c66e47fbc22965490cc4da3ae69a56d076d6b Commit]
|}
=== Changes to <code>app/models/assignment_team.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Renamed <code>copy</code> method for AssignmentTeam
|Renamed <code>copy</code> for copying current AssignmentTeam to CourseTeam as <code>copy_to_course_team</code>
|[https://github.com/kartikrawool/expertiza/commit/b93df2590a7ab52f87a1df00512afe626fa81520 Commit]
|-
|2
|Added Import Functionality 
|Refactored import method, added methods import_options, optional_import_fields, import_options which are being called in the ImportFileController to get required and optional fields from the respective model.
|[https://github.com/kartikrawool/expertiza/commit/2bae1785a15caede251f81517ecf944302f80d89 Commit]
|}
=== Changes to <code>app/models/team.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Added Import Functionality 
|Refactored import_team_members and added import_helper, that is used for importing CourseTeam and AssignmentTeam objects.
|[https://github.com/kartikrawool/expertiza/commit/53a86107644d3a0b0bef28a42d499a0b7b386ebb Commit]
|}


__TOC__
== CHANGES IN RSPEC FILES ==
 
=== Changes to <code>spec/models/course_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Removed the test case for <code>get_paprticipant</code> method test case
|Removed the test case for <code>get_participant</code> method as the method was not used anywhere else in the code.
|[https://github.com/kartikrawool/expertiza/commit/63c62027d8cb3aebe281a6152113632aafc444ec Commit]
|-
|2
|Renamed <code>copy_participants</code> method test case
|Renamed the respective method of <code>copy_participants</code> to <code>copy_assignment_participants</code>in rspec file as well.
|[https://github.com/kartikrawool/expertiza/commit/5177376c0f9a4f1890f49879bbbbfee0b28b8263 Commit]<br>
|}
 
=== Changes to <code>spec/models/course_team_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Renamed <code>copy</code> to <code>copy_to_assignment_team</code>
|Renamed <code>copy</code> to <code>copy_to_assignment_team</code>in rspec file as well.
|[https://github.com/kartikrawool/expertiza/commit/372b8d44dfbc3b29c07f41500a5cc53d23cf577b#diff-8b753735f47ae84fdf88f7e9f15e08b8dd13ba1bc8fd16abad11148800aa8954 Commit]<br>
|}
 
=== Changes to <code>spec/models/course_team_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Import functionality changes</code>
|Test cases are added to rspec file as well for import functionality changes.
|[https://github.com/kartikrawool/expertiza/commit/972c66e47fbc22965490cc4da3ae69a56d076d6b Commit]<br>
|}
 
=== Changes to <code>spec/models/assignment_team_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Import functionality changes</code>
|Test cases are added to rspec file as well for import functionality changes.
|[https://github.com/kartikrawool/expertiza/commit/2bae1785a15caede251f81517ecf944302f80d89 Commit]<br>
|}
 
=== Changes to <code>spec/models/team_spec.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Import functionality changes</code>
|Test cases are added to rspec file as well for import functionality changes.
|[https://github.com/kartikrawool/expertiza/commit/53a86107644d3a0b0bef28a42d499a0b7b386ebb#diff-638390a8686ba4d2e18c1fc9dfbef22271c8a02bb5d56bf78ded42e428fcf8b1 Commit]<br>
|}
 
== CHANGES IN CONTROLLER FILES ==
 
=== Changes to <code>app/controllers/teams_controller.rb</code> ===
{| class="wikitable" style="width: 100%;
! &nbsp;#&nbsp; !! Change !! Rationale !! Commit Link
|-
|1
|Renamed <code>copy</code> to <code>copy_to_assignment_team</code>
|The change was directly related to changes of copy method in <code>course_team.rb</code>.
|[https://github.com/kartikrawool/expertiza/commit/372b8d44dfbc3b29c07f41500a5cc53d23cf577b Commit]
|-
|}
 
== Test Plan ==
 
Manual Testing
*''' Adding Course Team member'''
** Adding member student1922 to course CSC540
[[File:Before_adding_course_team_member.png|1000px|center|Add course member to team Radical]]
[[File:After_adding_course_team_member.png|1000px|center|Succesffully added member to the team]]
 
*''' Importing Course Teams'''
** Importing Course Teams
** Import Preview
[[File:Importpreview.png|1000px|center|]]
** Import Error: We have refactored the import functionality according to E1923. The ImportFileController calls the import methods of the models course_team, assignment_team, team, etc. To debug this error, we need to change other models, such as assignment_participant, assignment_team, course_participant, metareview_response_map, question, review_response_map, sign_up_sheet, sign_up_topic, and user so that importing of all models would be generalised and would be functional. Some UI changes would be needed accordingly.
[[File:Importerror.png|1000px|center|Import error]]
 
Automated Testing
All the suggested changes were made to <code>course.rb</code> and <code>course_team.rb</code>, all these models were amended and as a result we need to update the tests. We added or amended tests in their respective .spec files. The methods were tested for edge cases and positive and negative test cases were written. The refactored methods were updated in the test cases.
 
*<code>'''course_team'''</code>




===About Expertiza===
[[File:course_team_spec.jpg|1000px|center|Test passing for <code>course_team.rb</code>]]


[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.


===Problem Statement===
*<code>'''course'''</code>
The following tasks were accomplished in this project:


* Improved the clarity of code by improving the variable and parameter names.
* Long character strings were taken and given appropriate names.
* Handled pagination by a separate helper module, which can be used by multiple controllers.
* Implemented action_allowed for access_control to prevent unauthorized access of methods.
* Prevented displaying of all versions for all users and tables when a user views the index page.
* Added missing CRUD methods to Versions Controller
* Added RSPEC testcases for testing changes done in Versions Controller


===About Versions Controller===
[[File:course_spec.jpg|1000px|center|Test passing for <code>course_team.rb</code>]]
This class manages different versions of reviews. If a reviewer reviews a submission, and after that, the author revises the submission, the next time the reviewer does a review (s)he will create a new version. Sometimes it’s necessary to find the current version of a review; sometimes it’s necessary to find all versions. Similarly, a user may want to delete the current version of a review, or all versions of a review. Pagination of versions helps the user to view a subset of versions at a time. Considering the huge number of versions in the system, it is very useful to have a pagination mechanism and a filtering mechanism which can be applied on the whole set of versions. The idea is to display the versions in an ordered, comprehensible and logical manner. In Expertiza the gem ‘will_paginate’ is used to achieve pagination.


===Current Implementation===


*<code>'''assignment_team'''</code>


=====Functionality=====
* Any user irrespective of his/ her privileges can view all the versions.
::The versions which a particular user can view should be restricted based on the privileges of the user. For instance, only a user with Administrator privileges should be able to view all the versions in the system. However, that is not the case now. Every user can view all the versions irrespective of whether the user is a student or an administrator.
* Any user can delete any version
::The versions which a particular user can delete should be restricted based on the privileges of the user. For instance, a student should not be allowed to delete any version. According to the current implementation any user can delete any version in the system.
* Filtering of versions were restricted to the current user
::The filtering options on versions were restricted to the current user. Sometimes a user might want to view versions associated with other users. For instance, an instructor might want to view the list of versions created by a particular student. This is not possible with the current implementation.


=====Drawbacks and Solutions=====
[[File:assignment_team_spec.jpg|1000px|center|Test passing for <code>course_team.rb</code>]]
* '''Problem 1''': The method paginate_list is doing more than one thing.
::The method paginate_list was building a complex search criteria based on the input params, getting the list of versions from the Database matching this search criteria and then calling the Page API. All these tasks in a single method made it difficult to understand.
<pre>
# For filtering the versions list with proper search and pagination.
  def paginate_list(id, user_id, item_type, event, datetime)
    # Set up the search criteria
    criteria = ''
    criteria = criteria + "id = #{id} AND " if id && id.to_i > 0
    if current_user_role? == 'Super-Administrator'
      criteria = criteria + "whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
    end
    criteria = criteria + "whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
    criteria = criteria + "item_type = '#{item_type}' AND " if item_type && !(item_type.eql? 'Any')
    criteria = criteria + "event = '#{event}' AND " if event && !(event.eql? 'Any')
    criteria = criteria + "created_at >= '#{time_to_string(params[:start_time])}' AND "
    criteria = criteria + "created_at <= '#{time_to_string(params[:end_time])}' AND "


    if current_role == 'Instructor' || current_role == 'Administrator'


    end
*<code>'''team.rb'''</code>


    # Remove the last ' AND '
    criteria = criteria[0..-5]


    versions = Version.page(params[:page]).order('id').per_page(25).where(criteria)
[[File:team_spec_output.jpg|1000px|center|Test passing for <code>course_team.rb</code>]]
    versions
  end
</pre>
* '''Solution''': The implementation has been changed in such a way that the versions which a user is allowed to see depends on the privileges of the user. The approach we have taken is as follows:
**An administrator can see all the versions
**An instructor can see all the versions created by him and other users who are in his course or are participants in the assignments he creates.
**A TA can see all the versions created by him and other users who are in the course for which he/ she assists.
**A Student can see all the versions created by him/ her.
* '''Problem 2''': The search criteria created in the method paginate_list was difficult to comprehend.
::The code which builds the search criteria in the method paginate_list uses many string literals and conditions and is hardly intuitive. The programmer will have to spend some time to understand what the code is really doing.
* '''Solution''': The implementation has been changed. A student is not allowed to delete any versions now. Other types of users, for instance administrators, instructors and TAs are allowed to delete only the versions they are authorized to view.
* '''Problem 3''': The paginate method can be moved to a helper class.
::VersionsController is not the only component which require to paginate items. There are other components too. For instance, the UsersController has to paginate the list of users. Hence the Paginate method can be moved to a helper class which can be accessed by other components as well.
* '''Solution''': The filtering options has also been enhanced. The current user can now choose as part of the version search filter any user from a list of users if the current user is authorized to see the versions created by that user.


===New Implementation===
*The method paginate_list has been split into 2 methods now.
** BuildSearchCriteria – as the name suggests the sole purpose of this method is to build a search criteria based on the input search filters when the current user initiates a search in versions.
** paginate_list – this method will call the paginate API.
:First the search criteria is built, then the criteria is applied to versions in the database to get all versions which matches the criteria and then the retrieved versions are paginated.
<pre>
  # pagination.
  def paginate_list(versions)
    paginate(versions, VERSIONS_PER_PAGE);
  end


  def BuildSearchCriteria(id, user_id, item_type, event)
=== Coverage: ===
    # Set up the search criteria
'''Before refactoring'''
    search_criteria = ''
[[File:coverage_old.jpg|center|frame|Coverage of codebase before refactoring]]
    search_criteria = search_criteria + add_id_filter_if_valid(id).to_s
    if current_user_role? == 'Super-Administrator'
      search_criteria = search_criteria + add_user_filter_for_super_admin(user_id).to_s
    end
    search_criteria = search_criteria + add_user_filter
    search_criteria = search_criteria + add_version_type_filter(item_type).to_s
    search_criteria = search_criteria + add_event_filter(event).to_s
    search_criteria = search_criteria + add_date_time_filter
    search_criteria
  end
</pre>
* The string literals and conditions in the method paginate_list were replaced with methods with intuitive names so that the programmer can understand the code more easily. We also removed an empty if clause and a redundant statement.
<pre>
  def add_id_filter_if_valid (id)
    "id = #{id} AND " if id && id.to_i > 0
  end


  def add_user_filter_for_super_admin (user_id)
    "whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
  end


  def add_user_filter
[[File:course_team_coverage.jpg|center|frame|Coverage of <code>course_team.rb</code> before refactoring]]
    "whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
  end


  def add_event_filter (event)
    "event = '#{event}' AND " if event && !(event.eql? 'Any')
  end


  def add_date_time_filter
[[File:course_coverage.jpg|center|frame|Coverage of <code>course.rb</code> before refactoring]]
    "created_at >= '#{time_to_string(params[:start_time])}' AND " +
        "created_at <= '#{time_to_string(params[:end_time])}'"
  end


  def add_version_type_filter (version_type)
    "item_type = '#{version_type}' AND " if version_type && !(version_type.eql? 'Any')
  end
</pre>
* The paginate method has been moved to the helper class Pagination_Helper. This new method can be now reused by the different components like UsersController etc. The method receives two parameters, first the list to paginate and second the number of items to be displayed in a page.


<pre>
[[File:assignment_team_coverage.jpg|center|frame|Coverage of <code>assignment_team.rb</code> before refactoring]]
module PaginationHelper


  def paginate (items, number_of_items_per_page)
    items.page(params[:page]).per_page(number_of_items_per_page)
  end


end
[[File:team_coverage.jpg|center|frame|Coverage of <code>team.rb</code> before refactoring]]
</pre>


===Code improvements===
* Introduced a constant VERSIONS_PER_PAGE and assigned the value 25 to it. The pagination algorithm for VersionsController displays at most 25 versions in a page. The existing implementation uses the value 25 straight in the code and there are few problems associated with such an approach.
** It is not easy to understand what 25 is unless the programmer takes a close look at the code.
** In case if the value 25 is used at more than one places and in future a new requirement comes to show at most 30 versions in a page, all the values will have to be modified. It is not very DRY.
* The VersionsController was overriding AccessHelper - action_allowed? method to return true in all the cases. This was violating the whole purpose of the method action_allowed?. The purpose of this method is to determine whether the user who is triggering a CRUD operation is allowed to do so. So when the current user invokes a CRUD operation, the action_allowed? method is invoked first and if the method returns true the CRUD operation is triggered or else the user is intimated with a message and gracefully exited. Hence, when the action_allowed? method is overridden to return true always, it results in providing unauthorized access to certain users.


<pre>
'''After refactoring'''
def action_allowed?
[[File:coverage_refactor.jpg|center|frame|Coverage of codebase after refactoring]]
    true
  end
</pre>


:With the new implementation the AccessHelper - action_allowed? method has been modified in such a way that unauthorized access is prevented. As per the new algorithm, 'new', 'create', 'edit', 'update' cannot be invoked by any user. These operations can be accessed only by ‘papertrail’ gem. Only an ‘Administrator’ or ‘Super-Administrator’ can call 'destroy_all' method. All the other methods are accessible to ‘Administrator’,  ‘Super-Administrator’, ‘Instructor’, ‘Teaching Assistant’ and ‘Student’.


<pre>
[[File:course_team_coverage_refactor.jpg|center|frame|Coverage of <code>course_team.rb</code> after refactoring]]
  def action_allowed?
    case params[:action]
    when 'new', 'create', 'edit', 'update'
    #Modifications can only be done by papertrail
      return false
    when 'destroy_all'
      ['Super-Administrator',
      'Administrator'].include? current_role_name
    else
      #Allow all others
      ['Super-Administrator',
      'Administrator',
      'Instructor',
      'Teaching Assistant',
      'Student'].include? current_role_name
    end
  end
</pre>


===Automated Testing using RSPEC===
The current version of expertiza did not have any test for VersionsController. Using the test driven development(TDD) approach, we have added an exhaustive set of RSPEC tests for VersionsController, to test all the modifications we have done to the code of the controller class. The tests use double and stub features of rspec-rails gem, to fake the log in by different users - Administrator, Instructor, Student etc. The tests can be executed "rpec spec" command as shown below.
<pre>
user-expertiza $rspec spec
.
.
.
Finished in 5.39 seconds (files took 25.33 seconds to load)
66 examples, 0 failures


Randomized with seed 19254
[[File:course_coverage_refactor.jpg|center|frame|Coverage of <code>course.rb</code> after refactoring]]
.
.
</pre>


===Testing from UI===
Following are a few testcases with respectto our code changes that can be tried from UI:
1. To go to versions index page, type in the following url after logging in:
  http://152.46.16.81:3000/versions


2. After logging in as student/instructor or admin : Try accessing the  new, create, edit, update actions. These actions are not allowed to any of the users.
[[File: assignment_team_coverage_refactor.jpg|center|frame|Coverage of <code>assignment_team.rb</code> after refactoring]]
  http://152.46.16.81:3000/versions/new
  This calls the new action. In the current production version of expertiza, it is unhandled and application gives a default 404 page.


3. Another feature that can be tested from UI is Pagination. Try searching for a user's versions and see if the results are paginated or not. Search here:
  http://152.46.16.81:3000/versions/search


4. Visit the same URL as step 3, you should see only the students under that instructor in the users dropdown.
[[File:team_coverage_refactor.jpg|center|frame|Coverage of <code>team.rb</code> after refactoring]]


===References===
== Relevant Links ==
* '''Github Repository:''' https://github.com/kartikrawool/expertiza/tree/refactor_course
* '''Pull Request:''' https://github.com/expertiza/expertiza/pull/2548
* '''VCL Server:''' http://152.7.178.105:8080/


#[https://github.com/expertiza/expertiza Expertiza on GitHub]
== Contributors to this project ==
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
* Kartik Rawool, (unityID:khrawool, GitHub:kartikrawool)
#[http://expertiza.ncsu.edu/ The live Expertiza website]
* Ameya Vaichalkar, (unityID:agvaicha, GitHub:ameyagv)
#[http://bit.ly/myexpertiza  Demo link]
* Vikram Pande, (unityID:vspande, GitHub:vikrampande7)
#[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza project documentation wiki]
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

Latest revision as of 04:00, 28 March 2023


Team Contact

Team Members

  • Kartik Rawool, (unityID:khrawool, GitHub:kartikrawool)
  • Ameya Vaichalkar, (unityID:agvaicha, GitHub:ameyagv)
  • Vikram Pande, (unityID:vspande, GitHub:vikrampande7)

Mentor

  • Kartiki Bhandakkar, kbhanda3

Overview of Expertiza

Expertiza is an open-source software written using Ruby on Rails which functions as a learning management software system. It has many different functions and abilities including the ability to create assignments, quizzes, assignment groups, and topics, and also a complete mechanism for providing peer reviews and feedback for other groups and other teammates. Part of its functionality is a system containing information about the course and its associated teams respectively. The Course and CourseTeam which are the models primarily addressed in this project are both critical components in providing this functionality.

Description of Project

course model stores information about the instructor and institution and is associated with other models such as User, CourseParticipant, CourseTeam, Assignment, AssignmentParticipant, and Notification. The course model is responsible for completing a variety of tasks, including returning course teams, returning the submission directory for the course, viewing participants enrolled in the course, adding a participant to the course, copying the Assignment Participants to Course Participants, and checking whether a user is part of any CourseTeam. The problem description lists code smells that need to be fixed. We fixed some issues, such as deleting the unused methods and renaming methods to indicate their action. The renaming problem required us to change a few methods calls where the function to be renamed was used. Also, to reduce the cognitive complexity of the copy_assignment_participants method, we created a method to separate the raising error functionality. Comments were added to indicate the action of the methods.

The course_team is a subclass of the team class. CourseTeams are a type of team that an instructor can use throughout an entire semester, providing consistency in team membership over time. The course_team model is responsible for completing various tasks, including returning the course of the team, adding a participant to the CourseTeam, and copying the CourseTeam Participants to AssignmentTeam Participants. There are methods for importing and exporting data to/from CSV files for CourseTeam objects. The problem description lists code smells/issues that need to be fixed. The issues that we fixed were deleting unused methods, renaming methods, and changing method calls, changing rspec expectations, adding meaningful comments. To refactor methods, DRY principles were used. In addition, analogous changes were made in the assignment_team model.

Files Modified

CHANGES IN MODEL FILES

Changes to app/models/course.rb

 #  Change Rationale Commit Link
1 Checked and Removed get_participant method Checked the usage of Method through IDE whether the function is used, the only call was from course_spec.rb and not from anywhere else. Hence, removed from course.rb Commit
2 Renamed copy_participants method Renamed copy_participants to copy_assignment_participants which makes more sense and a clear idea of how method works and what is the purpose of method. Commit
3 Removed add_member method from course_team model Assignment_team and Course_team are subclasses of Team. Both should follow the similar logic of adding members of respective teams. As of for, add_participants, usage is different for course.rb and course_team.rb, hence it should be present in both. However, to make the code more consistent among classes, team, course_team, and assignment_team, the add_member method should only be present in the team class. Commit
4 Created method raise_errors To reduce Cognitive Complexity of method copy_participants Commit

Changes to app/models/course_team.rb

 #  Change Rationale Commit Link
1 Removed assignment_id CourseTeam and AssignmentTeam inherit from Team class. The teams' table has an attribute parent_id that helps determine whether the team is AssigmentTeam or CourseTeam. Thus, there is no need for assignment_id in the CourseTeam class and no need for course_id in AssignmentTeam class. It will be a bad coding practice to call the assignment_id method on the CourseTeam object, calling the course_id method on the AssignmentTeam object as it would be inappropriate. Commit
2 Renamed copy method as copy_to_assignment_team As this method is copying a CourseTeam to an AssignmentTeam, renamed it as copy_to_assignment_team. A suggestion for copy method in AssignmentTeam would be to rename it as copy_to_course_team. Commit
3 Added Import Functionality Refactored import method, added methods import_options, optional_import_fields, import_options which are being called in the ImportFileController. Commit

Changes to app/models/assignment_team.rb

 #  Change Rationale Commit Link
1 Renamed copy method for AssignmentTeam Renamed copy for copying current AssignmentTeam to CourseTeam as copy_to_course_team Commit
2 Added Import Functionality Refactored import method, added methods import_options, optional_import_fields, import_options which are being called in the ImportFileController to get required and optional fields from the respective model. Commit

Changes to app/models/team.rb

 #  Change Rationale Commit Link
1 Added Import Functionality Refactored import_team_members and added import_helper, that is used for importing CourseTeam and AssignmentTeam objects. Commit

CHANGES IN RSPEC FILES

Changes to spec/models/course_spec.rb

 #  Change Rationale Commit Link
1 Removed the test case for get_paprticipant method test case Removed the test case for get_participant method as the method was not used anywhere else in the code. Commit
2 Renamed copy_participants method test case Renamed the respective method of copy_participants to copy_assignment_participantsin rspec file as well. Commit

Changes to spec/models/course_team_spec.rb

 #  Change Rationale Commit Link
1 Renamed copy to copy_to_assignment_team Renamed copy to copy_to_assignment_teamin rspec file as well. Commit

Changes to spec/models/course_team_spec.rb

 #  Change Rationale Commit Link
1 Import functionality changes Test cases are added to rspec file as well for import functionality changes. Commit

Changes to spec/models/assignment_team_spec.rb

 #  Change Rationale Commit Link
1 Import functionality changes Test cases are added to rspec file as well for import functionality changes. Commit

Changes to spec/models/team_spec.rb

 #  Change Rationale Commit Link
1 Import functionality changes Test cases are added to rspec file as well for import functionality changes. Commit

CHANGES IN CONTROLLER FILES

Changes to app/controllers/teams_controller.rb

 #  Change Rationale Commit Link
1 Renamed copy to copy_to_assignment_team The change was directly related to changes of copy method in course_team.rb. Commit

Test Plan

Manual Testing

  • Adding Course Team member
    • Adding member student1922 to course CSC540
Add course member to team Radical
Add course member to team Radical
Succesffully added member to the team
Succesffully added member to the team
  • Importing Course Teams
    • Importing Course Teams
    • Import Preview
    • Import Error: We have refactored the import functionality according to E1923. The ImportFileController calls the import methods of the models course_team, assignment_team, team, etc. To debug this error, we need to change other models, such as assignment_participant, assignment_team, course_participant, metareview_response_map, question, review_response_map, sign_up_sheet, sign_up_topic, and user so that importing of all models would be generalised and would be functional. Some UI changes would be needed accordingly.
Import error
Import error

Automated Testing All the suggested changes were made to course.rb and course_team.rb, all these models were amended and as a result we need to update the tests. We added or amended tests in their respective .spec files. The methods were tested for edge cases and positive and negative test cases were written. The refactored methods were updated in the test cases.

  • course_team


Test passing for course_team.rb
Test passing for course_team.rb


  • course


Test passing for course_team.rb
Test passing for course_team.rb


  • assignment_team


Test passing for course_team.rb
Test passing for course_team.rb


  • team.rb


Test passing for course_team.rb
Test passing for course_team.rb


Coverage:

Before refactoring

Coverage of codebase before refactoring


Coverage of course_team.rb before refactoring


Coverage of course.rb before refactoring


Coverage of assignment_team.rb before refactoring


Coverage of team.rb before refactoring


After refactoring

Coverage of codebase after refactoring


Coverage of course_team.rb after refactoring


Coverage of course.rb after refactoring


Coverage of assignment_team.rb after refactoring


Coverage of team.rb after refactoring

Relevant Links

Contributors to this project

  • Kartik Rawool, (unityID:khrawool, GitHub:kartikrawool)
  • Ameya Vaichalkar, (unityID:agvaicha, GitHub:ameyagv)
  • Vikram Pande, (unityID:vspande, GitHub:vikrampande7)
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517/Spring_2023/E2308._Refactor_course.rb_and_course_team.rb_models&oldid=148673"