CSC/CSC 517 Fall 2019/oss E1970: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
No edit summary
 
(15 intermediate revisions by the same user not shown)
Line 25: Line 25:
** The option should be saved in database and reflected on the review page.
** The option should be saved in database and reflected on the review page.


None of the bug fixes requires design patterns


===[https://github.com/expertiza/expertiza/issues/1467 Issue #1467: Missing Header]===
===[https://github.com/expertiza/expertiza/issues/1467 Issue #1467: Missing Header]===
Line 30: Line 31:




===Before===
====Before====
[[File:Bug1467.png]]  
[[File:Bug1467.png]]  


Line 37: Line 38:


=====Solutions=====
=====Solutions=====
* '''Problem 1''': The method paginate_list is doing more than one thing.
* View
::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.
:: The view shows what's presented to user of this page, if we would like to have additional information displayed on the page, we would need to find the corresponding location to add it first:
<pre>
<pre>
# For filtering the versions list with proper search and pagination.
<% headers = {} %>
   def paginate_list(id, user_id, item_type, event, datetime)
<% headers["Topic_name"] = "16%" if @assignment.topics? %>
    # Set up the search criteria
<% if @assignment.max_team_size > 1 %>
    criteria = ''
  <% headers["Team name"] = "14%" %>
    criteria = criteria + "id = #{id} AND " if id && id.to_i > 0
   <% headers["Team member(s)"] = "18%" %>
     if current_user_role? == 'Super-Administrator'
<% else %>
       criteria = criteria + "whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
  <% headers["Participant name"] = "18%" %>
    end
<% end %>                             
    criteria = criteria + "whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
<% headers["Submitted item(s)"] = nil %>
    criteria = criteria + "item_type = '#{item_type}' AND " if item_type && !(item_type.eql? 'Any')
 
    criteria = criteria + "event = '#{event}' AND " if event && !(event.eql? 'Any')
<!--E1877: table id changed -->
    criteria = criteria + "created_at >= '#{time_to_string(params[:start_time])}' AND "
<table id ="submissionsTable" class="table table-striped" style="margin-top: 50px">
    criteria = criteria + "created_at <= '#{time_to_string(params[:end_time])}' AND "
    <thead>
     <!--E1877: class="sorter-true" added to sort all attributes-->
       <tr>
        <% if @assignment.topics? %>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Topic name</th>
        <% end %>
        <% if @assignment.max_team_size > 1 %>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Team name</th>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Team member(s)</th>
        <% else %>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Participant name</th>
        <% end %> 
        <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Links</th>
        <th class="sorter-false" style="font-weight: bold; font-size: 15px;"></th>
      </tr>                   
    </thead>
</pre>
In this case, the blank space above team E1877's change should be where the red circle stands.


    if current_role == 'Instructor' || current_role == 'Administrator'
====After====
[[File:fixed1467.png]]
* View change
:: Since this page already uses `@assignment` in a lot of places, and we found it in the corresponding controller, so directly using it wouldn't pose additional problems as the query of the assignment is already done in the back-end.


    end
<pre>
<% headers = {} %>
<% headers["Topic_name"] = "16%" if @assignment.topics? %>
<% if @assignment.max_team_size > 1 %>
  <% headers["Team name"] = "14%" %>
  <% headers["Team member(s)"] = "18%" %>
<% else %>
  <% headers["Participant name"] = "18%" %>
<% end %>                             
<% headers["Submitted item(s)"] = nil %>


    # Remove the last ' AND '
<!--E1970: Added missing header (assignment name) -->
    criteria = criteria[0..-5]
<h1><%= @assignment.name %></h1>


     versions = Version.page(params[:page]).order('id').per_page(25).where(criteria)
<!--E1877: table id changed -->
    versions
<table id ="submissionsTable" class="table table-striped" style="margin-top: 50px">
  end
    <thead>
     <!--E1877: class="sorter-true" added to sort all attributes-->
      <tr>
        <% if @assignment.topics? %>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Topic name</th>
        <% end %>
        <% if @assignment.max_team_size > 1 %>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Team name</th>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Team member(s)</th>
        <% else %>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Participant name</th>
        <% end %> 
        <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Links</th>
        <th class="sorter-false" style="font-weight: bold; font-size: 15px;"></th>
      </tr>                   
    </thead>
</pre>
</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===
===[http://github.com/expertiza/expertiza/issues/1212 Issue #1212: Spurious HTML tags in new review page]===
*The method paginate_list has been split into 2 methods now.  
When reviewing other people's work, the text box for review entering always contains a spurious HTML tag
** 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.
====Before====
: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.
[[File:Bug1212.png]]
 
=====Functionality=====
* This is a problem of the tinyMCE plugin which is used for text editing purposes for Expertiza, changing configuration of the plugin may solve the problem
 
=====Solutions=====
* Configuration file
:: Someone had similar issue on some other settings, refer to this [https://stackoverflow.com/questions/2628187/tinymce-hide-the-bar/22389390 StackOverflow post], we could also edit the `tinymce.yml` file accordingly
<pre>
<pre>
   # pagination.
menubar: false
   def paginate_list(versions)
toolbar:
    paginate(versions, VERSIONS_PER_PAGE);
   - link | media | codesample | undo redo |  formatselect | bold italic backcolor  | alignleft aligncenter alignright alignjustify | bullist numlist outdent indent | removeformat | help
   end
plugins:
   - link
  - media
   - codesample
height: 150
width: 70%
media_live_embeds: true
browser_spellcheck: true
</pre>
 
====After====
[[File:Fixed1212.png]]
* Configuration change
:: When we remove this p, the status bar becomes empty, and renders no use, so we removed it from the display


  def BuildSearchCriteria(id, user_id, item_type, event)
<pre>
    # Set up the search criteria
menubar: false
    search_criteria = ''
toolbar:
    search_criteria = search_criteria + add_id_filter_if_valid(id).to_s
  - link | media | codesample | undo redo |  formatselect | bold italic backcolor  | alignleft aligncenter alignright alignjustify | bullist numlist outdent indent | removeformat | help
    if current_user_role? == 'Super-Administrator'
plugins:
      search_criteria = search_criteria + add_user_filter_for_super_admin(user_id).to_s
  - link
    end
  - media
    search_criteria = search_criteria + add_user_filter
  - codesample
    search_criteria = search_criteria + add_version_type_filter(item_type).to_s
height: 150
    search_criteria = search_criteria + add_event_filter(event).to_s
width: 70%
    search_criteria = search_criteria + add_date_time_filter
media_live_embeds: true
    search_criteria
browser_spellcheck: true
  end
# E1970 Removing the display of 'p' under comment input box
statusbar: false
</pre>
</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)
===[http://github.com/expertiza/expertiza/issues/1352 Issue #131(5)2: Assignments with “Use dropdown instead” cannot be saved]===
    "whodunnit = #{user_id} AND " if user_id && user_id.to_i > 0
(This should be 1352, there must be a typo in the project google doc)
  end
When an assignment is being created with a rubric that uses the “Use dropdown instead” option, it is indicated the save was successful. However, the save is not persisted when we review the assignment rubric. The option “Use dropdown” is not selected.
 
====Before====
[[File:Bug1312 0.png]]


  def add_user_filter
=====Functionality=====
    "whodunnit = #{current_user.try(:id)} AND " if current_user.try(:id) && current_user.try(:id).to_i > 0
* the "dropdown" checkbox status appears to be not saved, or not displayed correctly if it is saved.
  end


  def add_event_filter (event)
=====Solutions=====
    "event = '#{event}' AND " if event && !(event.eql? 'Any')
* Isolate problem
  end
:: We checked the database schema and found the "dropdown" attribute in there actually represents the dropdown menu next to checkbox, this needs to be refactored in the future when there are more than 2 values in the dropdown (currently saved as boolean)
[[File:Bug1312 1.png]]
* DB:migrate to solve the issue we have to first create the attribute inside of database
* We found that the view could only either pass a value or not pass a value instead of passing true or false when checkbox is checked or unchecked; whatever value entered in the `value=` field would get passed to DB
<pre>
html += '<td align="center"><input type="checkbox" name="dropdown" id="dropdown" value="true"></td>';
</pre>
* To fix this issue, we have to setup another DB:Migration to have a default false value in this attribute
* A mysteries function of `copy assignment questionnaire` (why copying it? who uses this function?) need to have this new attribute in there in order to pass data to DB
* the view needs to be modified in order to reflect the database


  def add_date_time_filter
====After====
    "created_at >= '#{time_to_string(params[:start_time])}' AND " +
[[File:Fixed1312.png]]
        "created_at <= '#{time_to_string(params[:end_time])}'"
* DB change
  end
:: There's a new attribute `use_dropdown_instead` in `assignment_questionnaires` table now, it's default value is false


   def add_version_type_filter (version_type)
<pre>
    "item_type = '#{version_type}' AND " if version_type && !(version_type.eql? 'Any')
class AddUseDropdownInsteadToAssignmentQuestionnaires < ActiveRecord::Migration
   def change
  add_column :assignment_questionnaires, :use_dropdown_instead, :boolean, default: false
   end
   end
end
</pre>
</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.
* Controller change
 
:: The attribute `use_dropdown_instead` is being passed by all functions relating to rubric editing now
<pre>
<pre>
module PaginationHelper
   def self.copy_assignment_questionnaire(old_assign, new_assign, user)
 
     old_assign.assignment_questionnaires.each do |aq|
   def paginate (items, number_of_items_per_page)
      AssignmentQuestionnaire.create(
     items.page(params[:page]).per_page(number_of_items_per_page)
        assignment_id: new_assign.id,
        questionnaire_id: aq.questionnaire_id,
        user_id: user.id,
        notification_limit: aq.notification_limit,
        questionnaire_weight: aq.questionnaire_weight,
        used_in_round: aq.used_in_round,
        dropdown: aq.dropdown,
        # E1970 Fix issue #1213
        use_dropdown_instead: aq.use_dropdown_instead
      )
    end
   end
   end
 
</pre>
end
* View change
:: The view reflects values stored in use_dropdown_instead now
<pre>
        // E1970 fix issue #1312
        if (assignment_questionnaire.use_dropdown_instead){
          html += '<td align="center"><input type="checkbox" name="assignment_form[assignment_questionnaire][][use_dropdown_instead]" id="dropdown" value="true" checked ></td>';
        }
        else{
          html += '<td align="center"><input type="checkbox" name="assignment_form[assignment_questionnaire][][use_dropdown_instead]" id="dropdown" value="true"></td>';
        }
</pre>
</pre>


===Code improvements===
===Automated Testing===
* 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.
1. Test submission page header, use `view_submission_header_spec.rb`
** 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>
<pre>
def action_allowed?
describe "Check if view submission page has header" do
     true
  before(:each) do
     create(:assignment)
   end
   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’.
  describe "check header has assignment name" do


<pre>
     it "is able to add a member to an assignment team" do
  def action_allowed?
       login_as('instructor6')
     case params[:action]
       assignment = Assignment.first
    when 'new', 'create', 'edit', 'update'
       visit("/assignments/list_submissions?id=#{assignment.id}")
    #Modifications can only be done by papertrail
       expect(page).to have_content("#{assignment.name}")
       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
   end
   end
end
</pre>
</pre>


===Automated Testing using RSPEC===
2. This is a configuration change of a plugin, cannot be tested with 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.
 
3. This involves another bug much bigger than the scale of this project:
 
in `./app/helpers/assignment_helper.rb:63`
<pre>
<pre>
user-expertiza $rspec spec
due_date.deadline_type_id = DeadlineType.find_by(name: type).id
.
.
.
Finished in 5.39 seconds (files took 25.33 seconds to load)
66 examples, 0 failures
 
Randomized with seed 19254
.
.
</pre>
</pre>
The `DeadlineType.find_by(name: type)` cannot find anything, which stops us from visiting assignment creation or edit page, see `create_assignmenet_save_checkbox_spec.rb` for details.


===Testing from UI===
===Testing from UI===
Following are a few testcases with respectto our code changes that can be tried 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.
[https://youtu.be/8cbMswDfF8o Video of testing script]
  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.
1. To see homework title related to submission, go to "Manage > Assignment > View submissions" after logging in with a instructor account
 
2. To review text box for reviews:
 
** Login as a instructor
** Create an assignment set submission time to be an hour later
** Log in as student A
** Do assignment, then submit
** Log in as student B
** Do assignment, then submit
** Login as a instructor
** Set submission deadline to be an hour earlier
** Log in as student A
** Go to view other's work, and ask for a new review
** Begin review, user should find the textbox without the P


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:
3. To see the checkboxes reviewing what's saved:
  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.
** Login as a instructor
** Create an assignment, set checkboxes in rubric items
** The landing page (edit assignment) should review the checkboxes checked in previous step


===References===
===References===
Line 209: Line 287:
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[https://github.com/WintersLt/expertiza GitHub Project Repository Fork]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
#[http://expertiza.ncsu.edu/ The live Expertiza website]
#[http://bit.ly/myexpertiza Demo link]  
#[http://52.87.248.149:3000/  Demo link]  
#[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza project documentation wiki]
#[http://wikis.lib.ncsu.edu/index.php/Expertiza Expertiza project documentation wiki]
#[https://relishapp.com/rspec Rspec Documentation]
#[https://relishapp.com/rspec Rspec Documentation]
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin
#Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin

Latest revision as of 02:04, 7 November 2019

E1970. OSS project Oogie Boogie: User Experience & Confidence

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



About Expertiza

Expertiza is an open source project based on 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

The following tasks were accomplished in this project:

None of the bug fixes requires design patterns

Issue #1467: Missing Header

The ‘view submissions’ page of an assignment does not have an appropriate header. The header should be added to reflect text such as “Submissions for <assignment name>


Before

Functionality
  • The view submission page currently has no title representing which homework submission the viewer is viewing, a title needs to be added into where the red circle is.
Solutions
  • View
The view shows what's presented to user of this page, if we would like to have additional information displayed on the page, we would need to find the corresponding location to add it first:
<% headers = {} %>
<% headers["Topic_name"] = "16%" if @assignment.topics? %>
<% if @assignment.max_team_size > 1 %>
  <% headers["Team name"] = "14%" %>
  <% headers["Team member(s)"] = "18%" %>
<% else %>
  <% headers["Participant name"] = "18%" %>
<% end %>                              
<% headers["Submitted item(s)"] = nil %>

<!--E1877: table id changed -->
<table id ="submissionsTable" class="table table-striped" style="margin-top: 50px">
    <thead>
    <!--E1877: class="sorter-true" added to sort all attributes-->
      <tr>
        <% if @assignment.topics? %>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Topic name</th>
        <% end %>
        <% if @assignment.max_team_size > 1 %>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Team name</th>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Team member(s)</th>
        <% else %>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Participant name</th>
        <% end %>  
        <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Links</th> 
        <th class="sorter-false" style="font-weight: bold; font-size: 15px;"></th>
      </tr>                    
    </thead>

In this case, the blank space above team E1877's change should be where the red circle stands.

After

  • View change
Since this page already uses `@assignment` in a lot of places, and we found it in the corresponding controller, so directly using it wouldn't pose additional problems as the query of the assignment is already done in the back-end.
<% headers = {} %>
<% headers["Topic_name"] = "16%" if @assignment.topics? %>
<% if @assignment.max_team_size > 1 %>
  <% headers["Team name"] = "14%" %>
  <% headers["Team member(s)"] = "18%" %>
<% else %>
  <% headers["Participant name"] = "18%" %>
<% end %>                              
<% headers["Submitted item(s)"] = nil %>

<!--E1970: Added missing header (assignment name) -->
<h1><%= @assignment.name %></h1>

<!--E1877: table id changed -->
<table id ="submissionsTable" class="table table-striped" style="margin-top: 50px">
    <thead>
    <!--E1877: class="sorter-true" added to sort all attributes-->
      <tr>
        <% if @assignment.topics? %>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Topic name</th>
        <% end %>
        <% if @assignment.max_team_size > 1 %>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Team name</th>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Team member(s)</th>
        <% else %>
          <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Participant name</th>
        <% end %>  
        <th class="sorter-true" style="font-weight: bold; font-size: 15px;">Links</th> 
        <th class="sorter-false" style="font-weight: bold; font-size: 15px;"></th>
      </tr>                    
    </thead>

Issue #1212: Spurious HTML tags in new review page

When reviewing other people's work, the text box for review entering always contains a spurious HTML tag

Before

Functionality
  • This is a problem of the tinyMCE plugin which is used for text editing purposes for Expertiza, changing configuration of the plugin may solve the problem
Solutions
  • Configuration file
Someone had similar issue on some other settings, refer to this StackOverflow post, we could also edit the `tinymce.yml` file accordingly
menubar: false
toolbar:
  - link | media | codesample | undo redo |  formatselect | bold italic backcolor  | alignleft aligncenter alignright alignjustify | bullist numlist outdent indent | removeformat | help
plugins:
  - link
  - media
  - codesample
height: 150
width: 70%
media_live_embeds: true
browser_spellcheck: true

After

  • Configuration change
When we remove this p, the status bar becomes empty, and renders no use, so we removed it from the display
menubar: false
toolbar:
  - link | media | codesample | undo redo |  formatselect | bold italic backcolor  | alignleft aligncenter alignright alignjustify | bullist numlist outdent indent | removeformat | help
plugins:
  - link
  - media
  - codesample
height: 150
width: 70%
media_live_embeds: true
browser_spellcheck: true
# E1970 Removing the display of 'p' under comment input box
statusbar: false

Issue #131(5)2: Assignments with “Use dropdown instead” cannot be saved

(This should be 1352, there must be a typo in the project google doc) When an assignment is being created with a rubric that uses the “Use dropdown instead” option, it is indicated the save was successful. However, the save is not persisted when we review the assignment rubric. The option “Use dropdown” is not selected.

Before

Functionality
  • the "dropdown" checkbox status appears to be not saved, or not displayed correctly if it is saved.
Solutions
  • Isolate problem
We checked the database schema and found the "dropdown" attribute in there actually represents the dropdown menu next to checkbox, this needs to be refactored in the future when there are more than 2 values in the dropdown (currently saved as boolean)

  • DB:migrate to solve the issue we have to first create the attribute inside of database
  • We found that the view could only either pass a value or not pass a value instead of passing true or false when checkbox is checked or unchecked; whatever value entered in the `value=` field would get passed to DB
html += '<td align="center"><input type="checkbox" name="dropdown" id="dropdown" value="true"></td>';
  • To fix this issue, we have to setup another DB:Migration to have a default false value in this attribute
  • A mysteries function of `copy assignment questionnaire` (why copying it? who uses this function?) need to have this new attribute in there in order to pass data to DB
  • the view needs to be modified in order to reflect the database

After

  • DB change
There's a new attribute `use_dropdown_instead` in `assignment_questionnaires` table now, it's default value is false
class AddUseDropdownInsteadToAssignmentQuestionnaires < ActiveRecord::Migration
  def change
  	add_column :assignment_questionnaires, :use_dropdown_instead, :boolean, default: false
  end
end
  • Controller change
The attribute `use_dropdown_instead` is being passed by all functions relating to rubric editing now
  def self.copy_assignment_questionnaire(old_assign, new_assign, user)
    old_assign.assignment_questionnaires.each do |aq|
      AssignmentQuestionnaire.create(
        assignment_id: new_assign.id,
        questionnaire_id: aq.questionnaire_id,
        user_id: user.id,
        notification_limit: aq.notification_limit,
        questionnaire_weight: aq.questionnaire_weight,
        used_in_round: aq.used_in_round,
        dropdown: aq.dropdown,
        # E1970 Fix issue #1213
        use_dropdown_instead: aq.use_dropdown_instead
      )
    end
  end
  • View change
The view reflects values stored in use_dropdown_instead now
        // E1970 fix issue #1312
        if (assignment_questionnaire.use_dropdown_instead){
          html += '<td align="center"><input type="checkbox" name="assignment_form[assignment_questionnaire][][use_dropdown_instead]" id="dropdown" value="true" checked ></td>';
        }
        else{
          html += '<td align="center"><input type="checkbox" name="assignment_form[assignment_questionnaire][][use_dropdown_instead]" id="dropdown" value="true"></td>';
        }

Automated Testing

1. Test submission page header, use `view_submission_header_spec.rb` 

describe "Check if view submission page has header" do
  before(:each) do
    create(:assignment)
  end

  describe "check header has assignment name" do

    it "is able to add a member to an assignment team" do
      login_as('instructor6')
      assignment = Assignment.first
      visit("/assignments/list_submissions?id=#{assignment.id}")
      expect(page).to have_content("#{assignment.name}")
    end
  end
end

2. This is a configuration change of a plugin, cannot be tested with Rspec

3. This involves another bug much bigger than the scale of this project:

in `./app/helpers/assignment_helper.rb:63` 

due_date.deadline_type_id = DeadlineType.find_by(name: type).id

The `DeadlineType.find_by(name: type)` cannot find anything, which stops us from visiting assignment creation or edit page, see `create_assignmenet_save_checkbox_spec.rb` for details.

Testing from UI

Following are a few testcases with respectto our code changes that can be tried from UI:

Video of testing script

1. To see homework title related to submission, go to "Manage > Assignment > View submissions" after logging in with a instructor account

2. To review text box for reviews:

    • Login as a instructor
    • Create an assignment set submission time to be an hour later
    • Log in as student A
    • Do assignment, then submit
    • Log in as student B
    • Do assignment, then submit
    • Login as a instructor
    • Set submission deadline to be an hour earlier
    • Log in as student A
    • Go to view other's work, and ask for a new review
    • Begin review, user should find the textbox without the P

3. To see the checkboxes reviewing what's saved:

    • Login as a instructor
    • Create an assignment, set checkboxes in rubric items
    • The landing page (edit assignment) should review the checkboxes checked in previous step

References

  1. Expertiza on GitHub
  2. GitHub Project Repository Fork
  3. The live Expertiza website
  4. Demo link
  5. Expertiza project documentation wiki
  6. Rspec Documentation
  7. Clean Code: A handbook of agile software craftsmanship. Author: Robert C Martin
Retrieved from "https://wiki.expertiza.ncsu.edu/index.php?title=CSC/CSC_517_Fall_2019/oss_E1970&oldid=127774"