Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

E1745. [TLD] Refactor response_controller.rb #1025

Merged
merged 20 commits into from Nov 11, 2017

Conversation

dipanshagupta
Copy link

Tasks completed:

  1. Refactored response_controller.rb (Made changes in 4 existing methods, and created 1 method)
  2. Added test cases in response_controller_spec.rb (Wrote 23 test cases)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 51.659% when pulling 78a8b37 on dipanshagupta:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 51.692% when pulling 78a8b37 on dipanshagupta:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 51.67% when pulling ed0dc13 on dipanshagupta:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 51.681% when pulling 1e4e5b3 on dipanshagupta:master into a26cf3a on expertiza:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 51.592% when pulling 6e05d26 on dipanshagupta:master into a26cf3a on expertiza:master.

@Winbobob Winbobob self-assigned this Oct 26, 2017
when 'edit' # If response has been submitted, no further editing allowed
response = user_id = nil
action = params[:action]
if %w(edit delete update view).include?(action)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good refactoring!

@Winbobob
Copy link
Member

Winbobob commented Oct 29, 2017

Hi team,

  • I think your refactoring is very good!
  • For your tests, you put all stubs in before(:each) block. I don't think it is a good idea.
    • In your case, every test will execute many unnecessary stubs, which may take more time to run tests.
    • It will be better if you put necessary stubs in each test and only put frequently-used code in before(:each) block.

Thanks,
Zhewei

@Winbobob Winbobob added the Merge label Oct 29, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 51.055% when pulling 6892b6d on dipanshagupta:master into a26cf3a on expertiza:master.

participant_type = item.where(user_id: session[:user].id)
next unless participant_type
participant_type.each do |p|
survey_deployment_type = participant_type == CourseParticipant ? AssignmentSurveyDeployment : CourseSurveyDeployment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the logic here is correct. participant_type is an array of participants, instead of the type of participant. I think this mistake is due to your misleading variable names. And you just refactor code without varifying the correctness.

@Winbobob Winbobob merged commit 6892b6d into expertiza:master Nov 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants