Skip to content

Commit

Permalink
E1936: Fixing issue expertiza#1475
Browse files Browse the repository at this point in the history 
  - Topics tab data gets overwritten by Rubrics tab due to design flaw
    that was discovered during debugging session
  - Required complete re-design of the project and re-implementation
  - New Design:
    * Two additional columns are added into the Assignment table that
      determines whether Rubrics varies by either Round or Topic with
      default values False
      - vary_by_round
      - vary by_topic
    * update_assignment_questionnaires method is re-implemented
      - Having extra column in the QA table topic_id, no need for
        deleting all the data and re-writing it again every single time
        in the DB
    * The only varying value is questionnaire_id, the rest values may
      not change from Topics or Rubrics tabs, but can be added
    * There are 4 (four) possible cases for saving and updating data:
      - used_in_round = null and topic_id = null
        * Indicates that update is received from Rubrics tab and Rubric
          does not vary by round. Update/enter the value for the type of
          questionnaire specified in the data. Example:
        id = 1 questionnaire_id = 1 used_in_round = null topic_id = null
      - used_in_round = integer and topic_id = null
        * Indicates that update is received from Rubrics tab and Rubric
          varies by specified rounds. Update/enter questionnaire_id for
          each round specified in the data. Example:
        id = 2 questionnaire_id = 1 used_in_round = 1 topic_id = null
        id = 3 questionnaire_id = 1 used_in_round = 2 topic_id = null
        id = 4 questionnaire_id = 1 used_in_round = 3 topic_id = null
      - used_in_round = null and topic_id = integer
        * Indicates that update is received from Topics tab, but Rubrics
          does not vary by round. Update/enter questionnaire_id for each
          corresponding task specified in the data. Example:
        id = 5 questionnaire_id = 1 used_in_round = null topic_id = 1
        id = 6 questionnaire_id = 1 used_in_round = null topic_id = 2
      - used_in_round = integer and topic_id = integer
        * Indicates that update id received from Topics tab and rubrics
          varies by round as well. Update/enter questionnaire_id for
          each corresponding task and round specified in data. Example:
        id = 7 questionnaire_id = 1 used_in_round = 1 topic_id = 1
        id = 8 questionnaire_id = 1 used_in_round = 2 topic_id = 1
        id = 9 questionnaire_id = 1 used_in_round = 1 topic_id = 2
        id = 10 questionnaire_id = 1 used_in_round = 2 topic_id = 2
  - Described design simplifies currently convoluted implementation and
    is a lot easier. It also helps to retrieve the data for Topics and
    Rubrics tabs.

TODO:
  - Fix all related Rspec Tests
  - All related Rspec Tests are Failing
  • Loading branch information
@nikolaygtitov
nikolaygtitov committed Jun 10, 2019
1 parent 5da9def commit 23ed2cc
Show file tree
Hide file tree
Showing 34 changed files with 312 additions and 400 deletions.
28 changes: 1 addition & 27 deletions app/controllers/assignments_controller.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ def edit
adjust_timezone_when_due_date_present(dd)
break if validate_due_date
end
check_assignment_questionnaires_usage
@due_date_all = update_nil_dd_deadline_name(@due_date_all)
@due_date_all = update_nil_dd_description_url(@due_date_all)
# only when instructor does not assign rubrics and in assignment edit page will show this error message.
Expand Down Expand Up @@ -284,8 +283,6 @@ def edit_params_setting

@assignment_questionnaires = AssignmentQuestionnaire.where(assignment_id: params[:id])
@due_date_all = AssignmentDueDate.where(parent_id: params[:id])
@review_vary_by_round_check = false
@review_vary_by_topic_check = false
@due_date_nameurl_not_empty = false
@due_date_nameurl_not_empty_checkbox = false
@metareview_allowed = false
Expand Down Expand Up @@ -328,21 +325,6 @@ def validate_due_date
(@metareview_allowed || @drop_topic_allowed || @signup_allowed || @team_formation_allowed)
end

def check_assignment_questionnaires_usage
@assignment_questionnaires.each do |aq|
unless aq.used_in_round.nil?
@review_vary_by_round_check = 1
break
end
end
@assignment_questionnaires.each do |aq|
unless aq.topic_id.nil?
@review_vary_by_topic_check = 1
break
end
end
end

def handle_rubrics_not_assigned_case
if !empty_rubrics_list.empty? && request.original_fullpath == "/assignments/#{@assignment_form.assignment.id}/edit"
rubrics_needed = needed_rubrics(empty_rubrics_list)
Expand Down Expand Up @@ -402,19 +384,11 @@ def handle_current_user_timezonepref_nil
end
end

def convert_to_boolean(expression)
expression == 'true'
end

def update_feedback_assignment_form_attributes
if params[:set_pressed][:bool] == 'false'
flash[:error] = "There has been some submissions for the rounds of reviews that you're trying to reduce. You can only increase the round of review."
else
vary_by_topic_desired = convert_to_boolean(params[:vary_by_topic])
# Update based on the attributes rec'd in the form
# This also updates assignment_questionnaire records, including adding / removing records
# as "vary by topic" selection changes
if @assignment_form.update_attributes(assignment_form_params, current_user, vary_by_topic_desired)
if @assignment_form.update_attributes(assignment_form_params, current_user)
flash[:note] = 'The assignment was successfully saved....'
else
flash[:error] = "Failed to save the assignment: #{@assignment_form.errors.get(:message)}"
Expand Down
8 changes: 4 additions & 4 deletions app/controllers/grades_controller.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def view
@assignment = Assignment.find(params[:id])
questionnaires = @assignment.questionnaires

if @assignment.varying_rubrics_by_round?
if @assignment.vary_by_round
@questions = retrieve_questions questionnaires, @assignment.id
else # if this assignment does not have "varying rubric by rounds" feature
@questions = {}
Expand Down Expand Up @@ -95,7 +95,7 @@ def view_team
counter_for_same_rubric = 0
questionnaires.each do |questionnaire|
@round = nil
if @assignment.varying_rubrics_by_round? && questionnaire.type == "ReviewQuestionnaire"
if @assignment.vary_by_round && questionnaire.type == "ReviewQuestionnaire"
questionnaires = AssignmentQuestionnaire.where(assignment_id: @assignment.id, questionnaire_id: questionnaire.id)
if questionnaires.count > 1
@round = questionnaires[counter_for_same_rubric].used_in_round
Expand All @@ -109,7 +109,7 @@ def view_team
vmquestions = questionnaire.questions
vm.add_questions(vmquestions)
vm.add_team_members(@team)
vm.add_reviews(@participant, @team, @assignment.varying_rubrics_by_round?)
vm.add_reviews(@participant, @team, @assignment.vary_by_round)
vm.number_of_comments_greater_than_10_words
@vmlist << vm
end
Expand Down Expand Up @@ -247,7 +247,7 @@ def make_chart
participant_score_types = %i[metareview feedback teammate]
if @pscore[:review]
scores = []
if @assignment.varying_rubrics_by_round?
if @assignment.vary_by_round
(1..@assignment.rounds_of_reviews).each do |round|
responses = @pscore[:review][:assessments].select {|response| response.round == round }
scores = scores.concat(get_scores_for_chart(responses, 'review' + round.to_s))
Expand Down
4 changes: 1 addition & 3 deletions app/controllers/popup_controller.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,7 @@ def view_review_scores_popup
@reviews = []

assignment = Assignment.find(@assignment_id)
if assignment.varying_rubrics_by_topic?
flash.now[:error] = "This report is not implemented for assignments where the rubric varies by topic."
end
flash.now[:error] = "This report is not implemented for assignments where the rubric varies by topic." if assignment.vary_by_topic

# Builds tone analysis report and heatmap when instructor/admin/superadmin clicks on the "View Review Report" Icon for an assignment.
build_tone_analysis_report
Expand Down
85 changes: 1 addition & 84 deletions app/helpers/assignment_helper.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def review_strategy_options
review_strategy_options
end

# retrive or create a due_date
# retrieve or create a due_date
# use in views/assignment/edit.html.erb
# Be careful it is a tricky method, for types other than "submission" and "review",
# the parameter "round" should always be 0; for "submission" and "review" if you want
Expand All @@ -72,89 +72,6 @@ def due_date(assignment, type, round = 0)
end
end

# Find a questionnaire for the given assignment
# Find by type if round number & topic id not given
# Find by round number alone if round number alone is given (fallback to find by assignment)
# Find by topic id alone if topic id alone is given
# Find by round number and topic id if both are given
# Create new questionnaire of given type if no luck with any of these attempts
def questionnaire(assignment, questionnaire_type, round_number = nil, topic_id = nil)
if round_number.nil? && topic_id.nil?
# Find by type
questionnaire = assignment.questionnaires.find_by(type: questionnaire_type)
elsif topic_id.nil?
# Find by round
aq = assignment.assignment_questionnaires.find_by(used_in_round: round_number)
# E1936 change comments:
# If "Vary by round" is checked and since it saves DB state while switching the Tabs,
# Questionnaire rubric (not --None--) and the weight (total of 0 or 100% for all rounds)
# must be placed by default so the user may switch the tabs and it can save DB successfully
# Otherwise, DB is not successfully saved and switching to Topics tab,
# it still determines assignment as non-vary by round
aq = assignment.assignment_questionnaires.find_by(assignment_id: assignment.id) if aq.nil?
elsif round_number.nil?
# Find by topic
aq = assignment.assignment_questionnaires.find_by(topic_id: topic_id)
else
# Find by round and topic
aq = assignment.assignment_questionnaires.where(used_in_round: round_number, topic_id: topic_id).first
end
# get the questionnaire from the assignment_questionnaire relationship
questionnaire = aq.nil? ? questionnaire : assignment.questionnaires.find_by(id: aq.questionnaire_id)
# couldn't find a questionnaire? create a questionnaire of the given type
questionnaire.nil? ? Object.const_get(questionnaire_type).new : questionnaire
end

# Find an assignment_questionnaire relationship for the given assignment
# Find by type if round number & topic id not given
# Create a new assignment_questionnaire if no luck with given type
# Otherwise
# Find by round number alone if round number alone is given
# Find by topic id alone if topic id alone is given
# Find by round number and topic id if both are given
# Find by type if no luck with given round / topic
def assignment_questionnaire(assignment, questionnaire_type, round_number = nil, topic_id = nil)
q_by_type = assignment.questionnaires.find_by(type: questionnaire_type)
if q_by_type.nil?
# Create a new assignment_questionnaire if no luck with given type
default_weight = {}
default_weight['ReviewQuestionnaire'] = 100
default_weight['MetareviewQuestionnaire'] = 0
default_weight['AuthorFeedbackQuestionnaire'] = 0
default_weight['TeammateReviewQuestionnaire'] = 0
default_weight['BookmarkRatingQuestionnaire'] = 0
default_aq = AssignmentQuestionnaire.where(user_id: assignment.instructor_id, assignment_id: nil, questionnaire_id: nil).first
default_limit = if default_aq.nil?
15
else
default_aq.notification_limit
end

aq = AssignmentQuestionnaire.new
aq.questionnaire_weight = default_weight[questionnaire_type]
aq.notification_limit = default_limit
aq.assignment = @assignment
aq
else
# No need to create a new assignment_questionnaire, should already have one
aq_by_type = assignment.assignment_questionnaires.find_by(questionnaire_id: q_by_type.id)
if round_number.nil? && topic_id.nil?
# Find by type
aq = aq_by_type
elsif topic_id.nil?
# Find by round
aq = assignment.assignment_questionnaires.find_by(used_in_round: round_number)
elsif round_number.nil?
# Find by topic
aq = assignment.assignment_questionnaires.find_by(topic_id: topic_id)
else
# Find by round and topic
aq = assignment.assignment_questionnaires.where(used_in_round: round_number, topic_id: topic_id).first
end
aq.nil? ? aq_by_type : aq
end
end

def get_data_for_list_submissions(team)
teams_users = TeamsUser.where(team_id: team.id)
topic = SignedUpTeam.where(team_id: team.id).first.try :topic
Expand Down
4 changes: 2 additions & 2 deletions app/helpers/grades_helper.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ def view_heatgrid(participant_id, type)
# loop through each questionnaire, and populate the view model for all data necessary
# to render the html tables.
questionnaires.each do |questionnaire|
@round = if @assignment.varying_rubrics_by_round? && questionnaire.type == "ReviewQuestionnaire"
@round = if @assignment.vary_by_round && questionnaire.type == "ReviewQuestionnaire"
AssignmentQuestionnaire.find_by(assignment_id: @assignment.id, questionnaire_id: questionnaire.id).used_in_round
else
nil
Expand All @@ -82,7 +82,7 @@ def view_heatgrid(participant_id, type)
questions = questionnaire.questions
vm.add_questions(questions)
vm.add_team_members(@team)
vm.add_reviews(@participant, @team, @assignment.varying_rubrics_by_round?)
vm.add_reviews(@participant, @team, @assignment.vary_by_round)
vm.number_of_comments_greater_than_10_words
@vmlist << vm
end
Expand Down
2 changes: 1 addition & 1 deletion app/helpers/report_formatter_helper.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def review_response_map(params, _session = nil)
def feedback_response_map(params, _session = nil)
assign_basics(params)
# If review report for feedback is required call feedback_response_report method in feedback_review_response_map model
if @assignment.varying_rubrics_by_round?
if @assignment.vary_by_round
@authors, @all_review_response_ids_round_one, @all_review_response_ids_round_two, @all_review_response_ids_round_three =
FeedbackResponseMap.feedback_response_report(@id, @type)
else
Expand Down
6 changes: 3 additions & 3 deletions app/helpers/summary_helper.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def summarize_reviews_by_criterion(assignment, summary_ws_url)
self.summary[round] = {}
self.avg_scores_by_criterion[round] = {}

questions_used_in_round = rubric[assignment.varying_rubrics_by_round? ? round : 0]
questions_used_in_round = rubric[assignment.vary_by_round ? round : 0]
# get answers of each question in the rubric
questions_used_in_round.each do |question|
next if question.type.eql?("SectionHeader")
Expand Down Expand Up @@ -126,7 +126,7 @@ def summarize_reviews_by_reviewees(assignment, summary_ws_url)

# iterate each round and get answers
# if use the same rubric, only use rubric[0]
rubric_questions_used = rubric[assignment.varying_rubrics_by_round? ? round : 0]
rubric_questions_used = rubric[assignment.vary_by_round ? round : 0]
rubric_questions_used.each do |q|
next if q.type.eql?("SectionHeader")
summary[reviewee.name][round][q.txt] = ""
Expand Down Expand Up @@ -202,7 +202,7 @@ def get_questions_by_assignment(assignment)
rubric = []
(0..assignment.rounds_of_reviews - 1).each do |round|
rubric[round] = nil
if assignment.varying_rubrics_by_round?
if assignment.vary_by_round
# get rubric id in each round
# E1936 team did not update this usage of review_questionnaire_id() to include topic,
# because this method does not seem to be used anywhere in Expertiza
Expand Down
31 changes: 2 additions & 29 deletions app/models/assignment.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ def scores(questions)
self.teams.each do |team|
scores[:teams][index.to_s.to_sym] = {}
scores[:teams][index.to_s.to_sym][:team] = team
if self.varying_rubrics_by_round?
if self.vary_by_round
grades_by_rounds = {}
total_score = 0
total_num_of_assessments = 0 # calculate grades for each rounds
Expand Down Expand Up @@ -342,33 +342,6 @@ def current_stage_name(topic_id = nil)
end
end

# check if this assignment has multiple review phases with different review rubrics
def varying_rubrics_by_round?
AssignmentQuestionnaire.where(assignment_id: self.id, used_in_round: 2).size >= 1
end

# Check if this assignment has rubrics which vary by topic
# returns false for an assignment which has no questionnaires
# returns false for an assignment if it has assignment-questionnaire(s) that have
# topic_id nil and none with topic_id non-nil
# returns true for an assignment if it has assignment-questionnaire(s) that have
# topic_id non-nil and none with topic_id nil
# throws exception for an assignment if it has assignment-questionnaire(s) that have
# topic_id non-nil and nil
def varying_rubrics_by_topic?
aq_no_topic_id = AssignmentQuestionnaire.where(assignment_id: self.id, topic_id: nil).size >= 1
aq_with_topic_id = AssignmentQuestionnaire.where(assignment_id: self.id).where.not(topic_id: nil).size >= 1
if !aq_no_topic_id && !aq_with_topic_id
return false
elsif aq_no_topic_id && !aq_with_topic_id
return false
elsif !aq_no_topic_id && aq_with_topic_id
return true
else
raise StandardError.new("Assignment with id " + self.id.to_s + " has a conflict about whether or not it varies by topic")
end
end

def link_for_current_stage(topic_id = nil)
if self.staggered_deadline?
return nil if topic_id.nil?
Expand Down Expand Up @@ -585,7 +558,7 @@ def self.export(csv, parent_id, options)
@questions = {}
questionnaires = @assignment.questionnaires
questionnaires.each do |questionnaire|
if @assignment.varying_rubrics_by_round?
if @assignment.vary_by_round
round = AssignmentQuestionnaire.find_by(assignment_id: @assignment.id, questionnaire_id: @questionnaire.id).used_in_round
questionnaire_symbol = if round.nil?
questionnaire.symbol
Expand Down

0 comments on commit 23ed2cc

Please sign in to comment.