CSC/ECE 517 Fall 2014/oss E1508 MRS: Difference between revisions

From Expertiza_Wiki
Jump to navigation Jump to search
Line 50: Line 50:
| The old new_feedback method was not getting called due to ruby rules
| The old new_feedback method was not getting called due to ruby rules
|-
|-
| rowspan="3" valign="middle" |
| view
destroy
| Deleted the old view method
| Renamed the 'delete' method to 'destroy'
| The old view method was not getting called due to ruby rules
| This was done for better Rails 4 routing compatibility
|-
| Initialized the assignment_id parameter
| This is used further on and is not passed from the link URL
|-
| Fixed redirection after topic deletion to Topics tab
| Once a topic was successfully deleted, the view rendered was just a list of topics
|}
|}



Revision as of 02:53, 19 March 2015

E1508 Refactoring Response Controller

This page provides a description of the Expertiza based OSS project. This project aimed at refactoring the ResponseController.

Problem Statement

Classes involved:

response_controller.rb
response.rb
???????

What they do: Allows the user to create and edit responses to questionnaires … such as performing a review, rating a teammate, or giving feedback to a reviewer.

What's wrong with it:

  • It doesn’t do authorization properly.
  • It contains duplicated methods.
  • Functionality that should be in models is incorporated into the controller.

What needs to be done:

  1. latestresponseversion seems misnamed. It fetches all previous versions of a response (which is to say, all previous review versions by the current reviewer of the current author for the current assignment).
  2. get_scores gets all the scores assigned in a particular response. A “response” is created when someone submits a review, a partner evaluation, a survey, etc.
  3. The rereview method is 98 lines long. Several parts of it should be turned into methods. Sorting review versions is really not a controller responsibility; it would be better to do this in a model class (which class?) Ditto for determining whether a review is current (i.e., was done during the current assignment phase). This is a query that is made about a review (actually, about a response, which may be a review, author feedback, etc.). It should be placed in the appropriate model class.
  4. rereview contains special code to check whether an assignment is “Jen’s assignment”; this was the first assignment we ever created with a multipart rubric. It was hard-coded into the system, rather than working on a rubric that was created in the normal way. It is probably impossible to remove this code without breaking that assignment, but it should be done in a separate method, and commented appropriately as a kludge.
  5. Again in rereview, creating a new version of a review is a model responsibility, and should be moved out of the controller.
  6. There are two (consecutive) copies of the edit method. The second appears to be the newer one, and, according to the rules for method definition, is the one that is currently in use. The first should be removed. Ditto for new_feedback and view--the 2nd version of each appears to be newer & should be kept.
  7. Authorization to perform actions is not checked correctly. It is supposed to be done through the action_allowed? method at the beginning of the class definition. Different authorization should be required for different operations. For example, someone should be allowed to view a response if they wrote the response, or they are the person or on the team whose work the response applied to, or if they are an instructor or TA for the class. The person who wrote a response should be allowed to edit it, but not the person/team who was being reviewed, nor the instructor or TA for the class. Currently, authorization is denied by the redirect_when_disallowed method, which was an earlier, more error-prone way of controlling access. It should go away, now that the class has an action_allowed? method.

Changes Made

Response Controller

Method Name Changes Made Reason For Change
latestResponseVersion Changed name to previous_responses The name wasn't indicative of what the method did
edit Deleted the old edit method The old edit method was not getting called due to ruby rules
new_feedback Deleted the old new_feedback method The old new_feedback method was not getting called due to ruby rules
view Deleted the old view method The old view method was not getting called due to ruby rules

Views

View Name Changes Made Reason For Change

sign_up_sheet/_add_signup_topcis

Fixed title The page title displayed for an instructor was "Sign-up sheet for ... "
Commented out View/Manage bookmark links These links were broken, and according to existing code were not meant for the instructor view.
Added variable initialization for @sign_up_topics The variable was referenced further on in the view, which was causing an 'nil value' error
Fixed flash message rendering for new topic creation The flash message was rendering incorrectly with HTML tags visible on the web page
Added full path names to partial names The shared partials were also being used by views in the Assignments controller, so full paths were needed
sign_up_sheet/_add_topics Added separation between the Import Topics and Manage Bookmarks links The two links were clumped together and it was difficult to distinguish between them
sign_up_sheet/_actions Replaced rendering of the reserve_topic partial with the _all_actions partial The reserve_topic partial is no longer being used and is replaced by the newly created _all_actions partial
sign_up_sheet/_all_actions Created this new partial We created the _all_actions partial to conditionally render the "Actions" table field for both instructors and students. This now also includes working links through which instructors or administrators can edit or delete topics.
sign_up_sheet/_table_line Gave edit/delete rights to Super Admin The Super-Administrator user wasn't included in the list of users with these permissions
sign_up_sheet/edit Modified title and included @assignment_id variable to be passed as a parameter variable to the update action The view was initially trying to access the params[:assignment_id] variable which was not getting initialized
assignments/edit Changed the partial rendered to /sign_up_sheet/add_signup_topcis.html The assignments/edit/add_signup_topics partial (with duplicated code) was otherwise being rendered