CSC/ECE 517 Fall 2015/oss E1556 CHM

E1556. Refactoring SuggestionController.rb

Optimization

Syntax

For the code to be coordinated with Rails 4 syntax, there is one major difference between Ruby 1.9 and 1.8 need to be change in the suggestion_controller.rb. The hash operator using the "hash rocket":

{ :key => 'value' }

Need to be changed into:

 { key :  'value' }

Refator

The approve_suggestion() method is quite long in the original code, and the send email function in the method is achieved twice, which doesn’t conform with DRY principle.

The logic of the approve_suggestion() method is described as follows. First, approve the suggestion by create a new record in the SignUpTopic model, set relative parameters, and save the new record. Then send notification to the team. In the notification part, if the student doesn’t have a team, a new team should be created and assigned to the suggested topic. Based on the logic, the approve_suggestion() method can be clearly divided into two parts: approve suggestion part, and notification part. In the notification part, send email function can be written as a single method in order not to repeat. Besides, creating a new team can also be written as a new method.

After refactor, there are four new methods: approve, notification, create_new_team, and send_email. Approve and notification are called within approve_suggestion, while create_new_team and send_email are called within notification.

  def create_new_team
    new_team = AssignmentTeam.create(name: 'Team' + @user_id.to_s + '_' + rand(1000).to_s, parent_id: @signuptopic.assignment_id, type: 'AssignmentTeam')
    t_user = TeamsUser.create(team_id: new_team.id, user_id: @user_id)
    SignedUpTeam.create(topic_id: @signuptopic.id, team_id: new_team.id, is_waitlisted: 0)
          parent = TeamNode.create(parent_id: @signuptopic.assignment_id, node_object_id: new_team.id)
          TeamUserNode.create(parent_id: parent.id, node_object_id: t_user.id)
  end

  def send_email
    proposer = User.find(@user_id)
    teams_users = TeamsUser.where(team_id: @team_id)
    cc_mail_list = Array.new
    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 already been approved",
          body: {
              approved_topic_name: @suggestion.title,
              proposer: proposer.name
          }
        }
    ).deliver
  end

  def approve
    @suggestion = Suggestion.find(params[:id])
    @user_id = User.where(name: @suggestion.unityID).first.id
    @team_id = TeamsUser.team_id(@suggestion.assignment_id, @user_id)
    @topic_id = SignedUpTeam.topic_id(@suggestion.assignment_id, @user_id)
    @signuptopic = SignUpTopic.new
    @signuptopic.topic_identifier = 'S' + Suggestion.where("assignment_id = ? and id <= ?", @suggestion.assignment_id, @suggestion.id).size.to_s
    @signuptopic.topic_name = @suggestion.title
    @signuptopic.assignment_id = @suggestion.assignment_id
    @signuptopic.max_choosers = 1;
    if @signuptopic.save && @suggestion.update_attribute('status', 'Approved')
      flash[:notice] = 'Successfully approved the suggestion.'
    else
      flash[:error] = 'Error when approving the suggestion.'
    end
  end

  def notification
    if @suggestion.signup_preference == 'Y'
      #if this user do not have team in this assignment, create one for him/her and assign this topic to this team.
      if @team_id.nil?
        create_new_team
      else #this user has a team in this assignment, check whether this team has topic or not
        if @topic_id.nil?
          #clean waitlists
          SignedUpTeam.where(team_id: @team_id, is_waitlisted: 1).destroy_all
          SignedUpTeam.create(topic_id: @signuptopic.id, team_id: @team_id, is_waitlisted: 0)
        else
          @signuptopic.private_to = @user_id
          @signuptopic.save
          #if this team has topic, Expertiza will send an email (suggested_topic_approved_message) to this team
          send_email
        end
      end
    else
      #if this team has topic, Expertiza will send an email (suggested_topic_approved_message) to this team
      send_email
    end
  end

  def approve_suggestion
    approve
    notification
    redirect_to action: 'show', id: @suggestion
  end

Test

Rspec

Rspec was used to conduct all the tests. RSpec is a Behaviour-Driven Development tool for Ruby programmers. BDD is an approach to software development that combines Test-Driven Development, Domain Driven Design, and Acceptance Test-Driven Planning. RSpec helps you do the TDD part of that equation, focusing on the documentation and design aspects of TDD.

Test1

In the first test, we are going to test the result of approving a student's suggestion topic if the student is in a waitlist. He will be removed from the waitlist and added to the new list, if he selected the signup_preference to be 'Yes'. We choose 'Writing Assignment 1a' of 'CSC/ECE 517, Spring 2015' as test assignment. I simulate creating a new suggestion with student5717, in team 'Writing Assignment 1a Team14'.

First, log in as Student5717 and suggest a new topic, and choose 'Yes' in signup_preference.

     # Login with student5717 account
      visit 'content_pages/view'
      expect(page).to have_content('Welcome')
      fill_in "User Name", with: "student5717"
      fill_in "Password", with: "password"
      click_button "SIGN IN"
    
      # suggest a new suggestion
      visit '/suggestion/new?id=711'
      fill_in 'suggestion_title',  with: 'RSpect'
      fill_in 'suggestion_description',  with: 'RSpect is a ROR test framework. It focus on function test'
      # select 'suggestion_signup_preference', with: 'Y'
      expect{click_button "Submit"}.to change(Suggestion, :count).by(1)

Then, log out Student5717, and log in with 'instructor6' account, who is the manager of this course. Then approve the suggest.

      # Logout current account student5717
      visit '/suggestion/new?id=711'
      click_link "Logout"
      expect(current_path).to eq("/")
      
      # Login with account instructor6
      visit 'content_pages/view'
      expect(page).to have_content('Welcome.')
      fill_in "User Name", with: 'instructor6'
      fill_in "Password", with: 'password'
      click_button "SIGN IN"
      
      # approve the suggestion
      expect(page).to have_content('Manage content')
      visit '/suggestion/list?id=711&type=Assignment'
      expect(page).to have_content('Suggested topics for Writing assignment 1a')
      expect(page).to have_content('RSpect')
      
      num = Suggestion.last.id
      visit "/suggestion/"+num.to_s
      expect(page).to have_content('Suggestion')
      expect(page).to have_content('Title:	RSpect')
      click_button "Approve suggestion"
      visit "/suggestion/"+num.to_s
      expect(page).to have_content('status:	Approved')

Finally, check if suggestion approved successfully. I need to check topic list with 'instructor6' account logged in and check the selected topic in student5717 account.

      # check if is not in waitlist
      visit "/assignments/711/edit#tabs-2"
      expect(page).to have_no_content("<br/><b>Writing assignment 1a_Team14</b><br/>student5717 <font color='red'>(waitlisted)</font>")
      
      # Logout current account instructor6
      visit '/suggestion/new?id=711'
      click_link "Logout"
      current_path.should == "/"
      
      # Login with student5717 account
      visit 'content_pages/view'
      expect(page).to have_content('Welcome')
      fill_in "User Name", with: "student5717"
      fill_in "Password", with: "password"
      click_button "SIGN IN"
      
      # Check if you select the topic successfully
      visit '/sign_up_sheet/list?assignment_id=711'
      expect(page).to have_content('Your topic(s): RSpect')

Test2

For the second test, Writing Assignment 1a team1, whose team id is 23781, was chosen to perform a serial of action. Team no.23781 is holding a topic: Amazon S3 and Rails. And Writing Assignment 1a team5, whose team id is 23800, is in the waiting list of this topic. First, sign in as Student 5404 from team1, send a suggestion for new topic and indicate they want to choose their suggested topic.

      @newtopic = 'Violet and Zoe'

      #sign in as student5404:
      visit 'content_pages/view'
      fill_in "User Name", with: 'student5404'
      fill_in "Password", with: 'password'
      click_button "SIGN IN"
      expect(page).to have_content('Assignments')

      #suggest a topic:
      # signup_preference default to be Y
      visit "/student_task/view?id=28634"
      expect(page).to have_content('Submit or Review work')
      visit "/suggestion/new?id=711"
      expect(page).to have_content('New suggestion')
      fill_in 'Title',with: @newtopic
      expect{click_button "Submit"}.to change(Suggestion, :count).by(1)

      #logout
      click_link "Logout"
      expect(current_path).to eq("/")
      visit '/suggestion/new?id=711'
      expect(page).to have_content('This is not allowed')
      expect(page).to have_content('Welcome')
      expect(page).to have_no_content('User: student5404')

Then sign in as instructor6 and approve the suggested topic.

 #sign in as instructor6
      visit 'content_pages/view'
      expect(page).to have_content('Welcome.')
      fill_in "User Name", with: 'instructor6'
      fill_in "Password", with: 'password'
      click_button "SIGN IN"
      expect(page).to have_content('Manage content')

      #approve the suggestion
      visit '/suggestion/list?id=711&type=Assignment'
      expect(page).to have_content('Suggested topics for Writing assignment 1a')
      num = Suggestion.last.id.to_s
      visit "/suggestion/"+num
      expect(page).to have_content('Suggestion')
      expect(page).to have_content('Title: '+@newtopic)
      click_button "Approve suggestion"
      visit "/suggestion/"+num.to_s
      expect(page).to have_content('status:	Approved')

      #logout as instructor6
      click_link "Logout"
      expect(current_path).to eq("/")
      visit '/suggestion/new?id=711'
      expect(page).to have_content('This is not allowed')
      expect(page).to have_content('Welcome')

Finally, to check the results. On the one hand, sign in as student5404 again and see if her/his team is holding the new topic. On the other hand sign in as instructor6 and check if team no.23800 is holding the old topic: Amazon S3 and Rails.

      #sign in as student5404:
      visit 'content_pages/view'
      fill_in "User Name", with: 'student5404'
      fill_in "Password", with: 'password'
      click_button "SIGN IN"
      expect(page).to have_content('Assignments')

      #check the approved suggestion in topics list
      visit "/sign_up_sheet/list?assignment_id=711"
      expect(page).to have_content("Your approved suggested topic")

      # switch to the new topic
      num2 = SignUpTopic.last.id.to_s
      visit "/sign_up_sheet/switch_original_topic_to_approved_suggested_topic/"+num2+"?assignment_id=711"
      expect(page).to have_content("Your topic(s): "+@newtopic)
      
      #logout student5404
      click_link "Logout"
      expect(current_path).to eq("/")
      visit '/suggestion/new?id=711'
      expect(page).to have_content('This is not allowed')
      expect(page).to have_content('Welcome')
      expect(page).to have_no_content('User: student5404')

      #sign in as instructor6
      visit 'content_pages/view'
      expect(page).to have_content('Welcome.')
      fill_in "User Name", with: 'instructor6'
      fill_in "Password", with: 'password'
      click_button "SIGN IN"
      expect(page).to have_content('Manage content')
      
      # check if team1 is has not enrolled
      visit "/assignments/711/edit#tabs-2"
      expect(page).to have_no_content("<br/><b>Writing assignment 1a_Team1</b><br/>student5404 student5731 <br/>")
      expect(page).to have_content("Writing assignment 1a_Team5 student5740 student5704")
      expect(page).to have_content("S1 Violet and Zoe Writing assignment 1a_Team1 student5404 student5731")

Test3

The third test is similar to the second one. Team no.23781 and assignment no.711 are chosen for this test again. First, student no.5404 login to the system, visit the assignment page, and make a topic suggestion. In the suggestion, instead of choosing “yes” in signup preference, the student chooses “no” in order not to use the suggested topic.

visit 'content_pages/view'
fill_in "User Name", with: "student5404"
fill_in "Password", with: "password"
click_button "SIGN IN"
  
visit '/suggestion/new?id=711'
fill_in 'suggestion_title',  with: 'test title'
fill_in 'suggestion_description',  with: 'test description'
select 'No', from: "suggestion_signup_preference"
click_button "Submit"
  
click_link "Logout"

After the suggestion is made, login as an instructor, find the assignment, and approve the suggestion.

visit 'content_pages/view'
fill_in "User Name", with: 'instructor6'
fill_in "Password", with: 'password'
click_button "SIGN IN"

num = Suggestion.last.id
path = "/suggestion/" + num.to_s
visit path
click_button "Approve suggestion"

click_link "Logout"

In the final step, we check if the new topic is shown in the topic list, then login as student no.5401 again, check if they still hold their old topic.

visit 'content_pages/view'
fill_in "User Name", with: 'student5404'
fill_in "Password", with: 'password'
click_button "SIGN IN"
  
visit '/sign_up_sheet/list?assignment_id=711'
expect(page).to have_content('Your topic(s): Amazon S3 and Rails')
click_link "Logout"

visit 'content_pages/view'
fill_in "User Name", with: 'instructor6'
fill_in "Password", with: 'password'
click_button "SIGN IN"

visit "/assignments/711/edit#tabs-2"
expect(page).to have_content("Amazon S3 and Rails")
expect(page).to have_content("test title")

Result

Here is the result of our tests. All test cases passed.

ChendeMacBook-Pro:expertiza chen$ rspec spec/controllers/suggestion_controller_spec.rb 
[Coveralls] Set up the SimpleCov formatter.
[Coveralls] Using SimpleCov's 'rails' settings.

Randomized with seed 8804
/Users/chen/RubymineProjects/expertiza/app/models/suggestion.rb:5: warning: circular argument reference - assignment_id
......

Deprecation Warnings:

Using `should` from rspec-expectations' old `:should` syntax without explicitly enabling the syntax is deprecated. Use the new `:expect` syntax or explicitly enable `:should` with `config.expect_with(:rspec) { |c| c.syntax = :should }` instead. Called from /Users/chen/RubymineProjects/expertiza/spec/controllers/suggestion_controller_spec.rb:67:in `block (3 levels) in <top (required)>'.


If you need more of the backtrace for any of these deprecations to
identify where to make the necessary changes, you can configure
`config.raise_errors_for_deprecations!`, and it will turn the
deprecation warnings into errors, giving you the full backtrace.

1 deprecation warning total

Finished in 1 minute 6.74 seconds (files took 5.3 seconds to load)
6 examples, 0 failures

Randomized with seed 8804

Coverage report generated for RSpec to /Users/chen/RubymineProjects/expertiza/coverage. 1638 / 5233 LOC (31.3%) covered.
[Coveralls] Outside the CI environment, not sending data.