CSC/ECE 517 Fall 2022 - E2270. Improve suggestion controller

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:

Write meaningful comments for methods working amounting to more than the English version of the first two lines of code.
There are two methods named similarly: approve and approve_suggestion.

Their functionality is not clear at first glance. Rename them so that they more accurately reflect their purpose. Add appropriate comments to explain the changes.

Move the send_email method to the Mailer class in app/mailers/mailer.rb.
In views/suggestion/show.html.erb and views/suggestion/student_view.html.erb, there seems to be a DRY violation that needs to be fixed.

Merge both files into a single view file in order to fix the DRY problem.

Make the necessary changes to the tests so that they pass following the above changes. Increase the test coverage of the controller and functionality by adding more tests.
Change the names of methods as follows:

 def add_comment : Why can't comment be a field of a Suggestion object? (unfortunately, requires a data migration)
 def approve
 def approve_suggestion → approve (how is it different from previous method?)
 def create
 def list
 def new
 def notification → notify_suggester
 def reject_suggestion → reject
 def send_email
 def show
 def student_edit // Why do we need two non-CRUD methods?  Can’t we achieve
 def student_view // same functionality in another way?  Maybe edit and show?
 def submit
 def suggestion_params
 def update_suggestion → update

Problems and Solutions

Problem 1: Write meaningful comments for methods working amounting to more than the English version of the first two lines of code

For example for notification method

def notification
  """
    method content
  """
end

Solution: The approach we have taken is to add comment lines to all methods present in the suggestion_controller.rb file. These comments will explain the main purpose of the function and how it relates to other sections of the code. An example of this can be seen below.

##### notification #####
# Notification is called whenever a suggestion is approved by the function "approve_suggestion_and_notify", and
# when it's called it will check to see if the user is already in the team or if the topic hasn't been created.
# If either of these haven't been created, it'll create the team or topic respectively, which doesn't provide a
# notification through the Mailer in this portion of the code. Otherwise, it will queue an email through the Mailer 
# class to send an email with "notify_suggestion_approval".
def notification
  """
    method content
  """
end

Problem 2: There are two methods named similarly: approve and approve_suggestion.

Their functionality is not clear at first glance. Rename them so that they more accurately reflect their purpose. Add appropriate comments to explain the changes.

def approve
  """
    method content
  """
end

def approve_suggestion
  """
    method content
  """
end

Solution: Renamed approve method to approve_suggestion and approve_suggestion to approve_suggestion_and_notify

##### approve_suggestion #####
# approve_suggestion is triggered whenever a user is viewing suggestions on Expertiza. They'll be given the option
# to either "Approve suggestion" or "Reject suggestion", and in the case of approving a suggestion the function
# "approve_suggestion_and_notify" will be triggered in "submit". approve_suggestion handles the process of turning
# this suggestion into a topic that teams can choose from by calling the SignUpTopic class.
def approve_suggestion
  """
    method content
  """
end

##### approve_suggestion_and_notify #####
# approve_suggestion_and_notify is triggered whenever a user is viewing suggestions on Expertiza. They'll be given the option
# to either "Approve suggestion" or "Reject suggestion", and in the case of approving a suggestion this function will be triggered.
# This function's main purpose is to ensure that "approve_suggestion" is called, which will turn the suggestion into a SignUpTopic,
# and then to call the "notification" function which handles the mailing process to the appropriate teams if this topic is 
# being altered or created.
def approve_suggestion_and_notify
  """
    method content
  """
end

Problem 3: Move the send_email method from suggestion_controller to Mailer class in app/Mailer/mailer.rb file and change its name.

def send_email
    proposer = User.find_by(id: @user_id)
    if proposer
      teams_users = TeamsUser.where(team_id: @team_id)
      cc_mail_list = []
      teams_users.each do |teams_user|
        cc_mail_list << User.find(teams_user.user_id).email if teams_user.user_id != proposer.id
      end
      Mailer.suggested_topic_approved_message(
        to: proposer.email,
        cc: cc_mail_list,
        subject: "Suggested topic '#{@suggestion.title}' has been approved",
        body: {
          approved_topic_name: @suggestion.title,
          proposer: proposer.name
        }
      ).deliver_now!
    end
  end

Solution: Moved the send_email method from suggestion_controller to Mailer class in app/Mailer/mailer.rb file. Also changed the name of the method to notify_suggestion_approval.

def notify_suggestion_approval(used_id, team_id, suggestion_title)
    proposer = User.find_by(id: user_id)
    if proposer
      teams_users = TeamsUser.where(team_id: team_id)
      cc_mail_list = []
      teams_users.each do |teams_user|
        cc_mail_list << User.find(teams_user.user_id).email if teams_user.user_id != proposer.id
      end
      suggested_topic_approved_message(
        to: proposer.email,
        cc: cc_mail_list,
        subject: "Suggested topic '#{suggestion_title}' has been approved",
        body: {
          approved_topic_name: suggestion_title,
          proposer: proposer.name
        }
      ).deliver_now!
    end
 end

Problem 4: In views/suggestion/show.html.erb and views/suggestion/student_view.html.erb, there seems to be a DRY violation that needs to be fixed.

Merge both files into a single view file in order to fix the DRY problem.

Solution: Merge views/suggestion/show.html.erb and views/suggestion/student_view.html.erb into one file to fix the DRY problem.

<% if current_user_has_student_privileges? or session[:flip_user] %>
<!--If user is a student or non-student user in Student view-->
.....
else
.....
<%end%>

Problem 5: Make the necessary changes to the tests so that they pass following the above changes. Increase the test coverage of the controller and functionality by adding more tests.

Solution: Increase the test coverage of the suggestion controller by adjusting current test for above problems and adding 3 more tests: 'adds a participant is not successfull', 'rejecting a suggestion is not successfull', 'accept a suggestion is not successfull'

it 'adds a participant is not successfull' do
      allow_any_instance_of(SuggestionComment).to receive(:save).and_return(false)
      request_params = { id: 1, suggestion_comment: { vote: 'Y', comments: 'comments' } }
      user_session = { user: instructor }
      get :add_comment, params: request_params, session: user_session, xhr: true
      expect(flash[:error]).to eq 'There was an error in adding your comment.'
end

it 'rejecting a suggestion is not successfull' do
        allow_any_instance_of(Suggestion).to receive(:update_attribute).and_return(false)
        request_params = { id: 1, reject_suggestion: true }
        user_session = { user: instructor }
        get :submit, params: request_params, session: user_session, xhr: true
        expect(flash[:error]).to eq 'An error occurred when rejecting the suggestion.'
end

it 'accept a suggestion is not successfull' do
        allow(SignUpTopic).to receive(:new_topic_from_suggestion).and_return('failed')
        allow_any_instance_of(SuggestionController).to receive(:notification).and_return(true)
        request_params = { id: 1, approve_suggestion: true }
        user_session = { user: instructor }
        get :submit, params: request_params, session: user_session, xhr: true
        expect(flash[:error]).to eq 'An error occurred when approving the suggestion.'
end

Problem 6: Change the names of methods:

Solution: The names are changed to be simple and clear to understand.

def approve_suggestion will become -> approve 
def notification will become -> notify_suggester
def reject_suggestion will become -> reject
def update_suggestion will become -> update

Automated Testing using RSPEC

Go to the expertiza source code root path and use following command to run test:

$ rspec spec/contollers/suggestion_controller_spec.rb

Here is the operation video link:

https://youtu.be/qzkdNmTnAIc

Test Plan

Travis CI

Also , our project passed the Travis CI build test.

Coverage

Increased coverage at least by 10% by adding 3 more tests that are:

-'adds a participant is not successfull'

-'rejecting a suggestion is not successfull'

-'accept a suggestion is not successfull'

Before Changes

After Changes

Increase coverage at least by 10% to 66.25%

Testing from UI

Here is the VCL link:

http://152.7.176.119:8080/

For users intending to view the deployed Expertiza associated with this assignment, the credentials are below:

Instructor login: username -> instructor6, password -> password (Use this account for Testing)
Student login: username -> student4340, password -> password
Student login: username -> student4405, password -> password

1. After logging in as Instructor, click "Manage instructor content" -> "Assignment":