Expertiza_Wiki - User contributions [en]

CSC/ECE 517 Spring 2023 - E2336. Reimplement response.rb

2023-04-25T23:16:41Z

Mdbrain2: /* Issue 6 */

==Project Overview==
This project is a continuation to E2312. The current project is to make design changes to the Response class, ReviewCommentMixin module, ScorableMixin module and add comments to test cases. We explain the issue with respect to the current implementation of each of these entities as subsections below.

==Note to the reviwer for final submission==
* This project is a refactoring of some files done in Project 3. We have made these changes in a separate repo so that the new changes can be seen clearly.

* We have added a new section in this documentation called '''Changes made / Solutions''' that has all the changes we have made and explanations.

'''PR for this project:''' https://github.com/expertiza/reimplementation-back-end/pull/38

'''The PR for the previous project (Project 3) on top of which these changes have been made:''' https://github.com/expertiza/reimplementation-back-end/pull/23

==Issues==

There are several issues we plan to address as part of this project.

* Refactor the get_all_review_comments method as an instance method in response.rb file
* Refactoring significant_difference
* Make the method in ReviewCommentMixin to be an instance method
* Simplify + Refactor methods in ScorableMixin
* Refactor the self.get_all_responses method as an instance method in response_map.rb file.
* Add more descriptive comments for the tests.

===Issue 1: Refactor the self.get_all_review_comments method===
Currently, the get_all_review_comments method is a class method that returns a data structure containing all the review comments for all responses. This violates the principles of good design as it creates a large data structure that the calling method needs to break apart, leading to poor performance and increased complexity.

[[File:get_all_review_comments.png|700px]]

To address this issue and come up with a much more elegant design, we plan to refactor as a new implementation that should convert the get_all_review_comments method into an instance method that takes a review object as an argument and returns the comments for that review only. This will allow the client class to use an Iterator pattern to fetch one review at a time from the Response class rather than iterate over that data structure that get_all_review_comments returned to it.

===Issue 2: Refactoring significant_difference?===
While the significant_difference method does not necessarily need to be refactored, it is the only method with a call for the avg_scores_and_count_for_prev_reviews method, which does need to be refactored it both is a class method and returns a data structure.

def self.avg_scores_and_count_for_prev_reviews(existing_responses, current_response)
scores_assigned = []
count = 0
existing_responses.each do |existing_response|
unless existing_response.id == current_response.id # the current_response is also in existing_responses array
count += 1
scores_assigned << existing_response.aggregate_questionnaire_score.to_f / existing_response.maximum_score
end
end
[scores_assigned.sum / scores_assigned.size.to_f, count]
end

Because of this we decided that we should simply refactor significant_difference instead and simply remove avg_scores_and_count_for_prev_reviews.

# compare the current response score with other scores on the same artifact, and test if the difference
# is significant enough to notify instructor.
# Precondition: the response object is associated with a ReviewResponseMap
### "map_class.assessments_for" method need to be refactored
def significant_difference?
map_class = map.class
existing_responses = map_class.assessments_for(map.reviewee)
average_score_on_same_artifact_from_others, count = Response.avg_scores_and_count_for_prev_reviews(existing_responses, self)
# if this response is the first on this artifact, there's no grade conflict
return false if count.zero?
# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score_on_same_artifact_from_others - score).abs * 100 > allowed_difference_percentage
end

===Issue 3: Make the method in ReviewCommentMixin to be an instance method ===
====Problems====
* The ReviewCommentMixin has one single method '''volume_of_review_comments''' and it is currently present as a class method. There is no need for it to be a class method and it should instead be an instance method.
* Volume of review comments is a metric, which belongs in metrics.rb, not in a mixin for responses.

==== Proposed Solution====
The change is quite straight forward. We should move this method to be an instance method in '''metrics.rb''' file. A side note is that, this method calls the '''get_all_review_comments''' method from '''response.rb'''. This method will be changed as part of this project (See above points). Any change to that function's method call must be reflected here too.

===Issue 4: Simplify + Refactor methods in ScorableMixin ===

This mixin has methods that are related to scoring. https://expertiza.csc.ncsu.edu/index.php/Scoring_%26_Grading_Methods_(Fall_%2721) is a good resource to understand how scoring and grading works.

====Problems====
* 1) Not DRY : The following piece of code is used in 2 functions - calculate_total_score and maximum_score

scores.each do |s|
question = Question.find(s.question_id)
< Perform a calculation >
end

Basically, we iterate through the score and calculate something. The common calculation between both these functions is total_weight calculation.

'''What problem does this cause?''' If the logic to calculate the total_weight changes in the future, we will have to make changes to both these places. A developer who is relatively new to the project might miss out one changing it in a location. It is always good to have a single point of change by 'DRY'ing our code.

* 2) Not optimized: Another change related to the above snippet has to do with how the queries are made and the number of queries.

scores.each do |s|
question = Question.find(s.question_id)
<Other lines irrelevant to this case >
end

'''What problem does this cause?''' This is done inside a loop, meaning we perform a select query to each time -> N queries for N objects. Our solution aims to optimize this.

* 3) Checking the class type of an object to perform some business logic

This issue is present in the questionnaire_by_answer method. The following snippet shows it:

assignment = if map.is_a? ReviewResponseMap
map.assignment
else
Participant.find(map.reviewer_id).assignment
end

==== Proposed Solution====
* 1) The common function would involve calculating the sum of weights.

def calculate_total_weight()
scores.each do |s|
question = Question.find(s.question_id)
total_weight += question.weight unless s.answer.nil? || !question.is_a?(ScoredQuestion)
end
And this can be called inside the calculate_total_score and maximum_score.

* 2) Instead of calling Question.find inside the loop, we can get the set of question_ids and use a where function.

question_ids = scores.map(&:question_id)
questions = Question.where(question_id: question_ids)
< Make the calculation here >
This way, we only make one database call instead of N calls.

* 3) To solve checking the object type, we can write a method named '''response_assignment''' in ResponseMap and ReviewResponseMap. This way, we can just call that method in the object and depending on what object it is, we would get the corresponding answer.
* In ReviewResponseMap, the method should return its assignment
* In ResponseMap, the method should return Participant.find(map.reviewer_id).assignment

A visual representation of this change would look like this. The snippet uses an 'OR' condition to pick the appropriate assignment. Our change would remove the need for this check.

[[File:ScorableMixin3.png|700px]]

===Issue 5: Refactor the self.get_all_responses method===

The self.get_all_responses method which is currently in the response.rb file would be much cleaner as an instance method in response_map.rb. We plan to refactor the self.get_all_responses method in response.rb to an instance method in response_map.rb.

[[File:Get all responses.png|700px]]

[[File:Response map class.png|700px]]

===Issue 6: Add more descriptive comments for the tests===

The existing tests need more descriptive comments. We plan to add more detailed comments for all the test cases.

[[File:Response tests 1.png|700px]]

[[File:Response tests 2.png|700px]]

[[File:Response tests 3.png|700px]]

==Changes made / Solutions==

===Issue 1===
The get_all_review_comments method has been moved to be an instance method of the assignment class and modified to only take the reviewee_id. However, the return value has not been modified. This is because upon further investigation the get_all_review_comments (previously called concatenate_all_review_comments) is only used within the volume_of_review_comments method and that modifying volume_of_review_comments to only get a single variable from get_all_review_comments proved to difficult. Unlike the avg_scores_and_count_for_prev_reviews, however, get_all_review_comments was not merged into get_all_review_comments due to its size and that doing so would make get_all_review_comments difficult to understand.

===Issue 2===
The avg_scores_and_count_for_prev_reviews method has been merged with the significant_difference? method.

def significant_difference?
map_class = map.class
# gets all responses made by a reviewee
existing_responses = map_class.assessments_for(map.reviewee)
#
count = 0
total = 0
# gets the sum total percentage scores of all responses that are not this response
existing_responses.each do |response|
unless id == response.id # the current_response is also in existing_responses array
count += 1
total += response.aggregate_questionnaire_score.to_f / response.maximum_score
end
end
#
# if this response is the only response by the reviewee, there's no grade conflict
return false if count.zero?
#
# calculates the average score of all other responses
average_score = total / count
#
# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
#
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
#
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score - score).abs * 100 > allowed_difference_percentage
end

===Issue 3===
We have made the volume_of_review_comments method to be an instance method and moved it to the assignment.rb since it used to take a 'assignment_id' as an input argument. In this way, we are able to make it more sense and removed the ReviewCommentMixin.

# frozen_string_literal: true
module MetricHelper
# Determine the average size of review comments in each round
# the first entry in the returned list is the overall average
# word count.
def volume_of_review_comments(assignment_id, reviewer_id)
comments, counter,
@comments_in_round, @counter_in_round = Response.get_all_review_comments(assignment_id, reviewer_id)
# Index 0 is a nil element that can be ignored in the round count
num_rounds = @comments_in_round.count - 1

overall_avg_vol = (Lingua::EN::Readability.new(comments).num_words / (counter.zero? ? 1 : counter)).round(0)
review_comments_volume = []
review_comments_volume.push(overall_avg_vol)
(1..num_rounds).each do |round|
num = Lingua::EN::Readability.new(@comments_in_round[round]).num_words
den = (@counter_in_round[round].zero? ? 1 : @counter_in_round[round])
avg_vol_in_round = (num / den).round(0)
review_comments_volume.push(avg_vol_in_round)
end
review_comments_volume
end
end

===Issue 4===
* 1) We have decided not to address this part of the issue (See the Issue 4) after discussing with the professor. The reason behind it is that the lines involved in this section is only 2 lines long and hence it does not have to be moved to a method.

* 2) We looked at a lot of solutions to make this. The issue is that using just a 'where' method will not ensure that the Active Record Association returned will be in the same order of that of the list of ids given.

For example:

ids = [4,1,2,7]
answers = Answer.where(id: ids)
return answers.map(&:id)

This example snippet would return 1,2,4,7

Hence, to ensure the order of the return values(thereby ensuring the correct working) and to improve the performance, we have used a small gem 'find_with_order'. The main reason why we decided to add this gem over a custom solution is portability / ease of modification. A custom solution would have required us to write SQL queries in where clauses and this would be tied to the mysql(current database used). This gem is compatible with popular databases and hence this would be automatically taken care of even if the database is changed.

Hence, our final solution is:

def calculate_total_score
# Only count the scorable questions, only when the answer is not nil (we accept nil as
# answer for scorable questions, and they will not be counted towards the total score)
sum = 0
question_ids = scores.map(&:question_id)
# We use find with order here to ensure that the list of questions we get is in the same order as that of question_ids
questions = Question.find_with_order(question_ids)
scores.each_with_index do |score, idx|
sum += score.answer * questions[idx].weight unless score.answer.nil? || !questions[idx].is_a?(ScoredQuestion)
end
sum
end

def maximum_score
# Only count the scorable questions, only when the answer is not nil (we accept nil as
# answer for scorable questions, and they will not be counted towards the total score)
total_weight = 0
question_ids = scores.map(&:question_id)
# We use find with order here to ensure that the list of questions we get is in the same order as that of question_ids
questions = Question.find_with_order(question_ids)
scores.each_with_index do |score, idx|
total_weight += questions[idx].weight unless score.answer.nil? || !questions[idx].is_a?(ScoredQuestion)
end
questionnaire = if scores.empty?
questionnaire_by_answer(nil)
else
questionnaire_by_answer(scores.first)
end
total_weight * questionnaire.max_question_score
end

* 3) Usage of visitor pattern to avoid checking class types: We solved this by adding a method '''response_assignment''' to ReviewResponseMap and ResponseMap classes.

In response_map.rb

# returns the assignment related to the response map
def response_assignment
return Participant.find(self.reviewer_id).assignment
end

In review_response_map.rb

# returns the assignment related to the review response map
def response_assignment
return self.assignment
end

Hence, the '''questionnaire_by_answer''' method in '''ScorableHelper''' will have the following snippet:

map = ResponseMap.find(map_id)
assignment = map.response_assignment

===Issue 5===
The get_all_responses method has been made an instance method of response_map and simplified as the function no longer need to find a response map.

def get_all_responses
Response.where(map_id: map_id).all
end

===Issue 6===
Additional comments have been made to the tests for the response class.

==Design Principles==
This section aims to act as a consolidation of the design principles that we will implement as part of this project
* DRY: We will DRY the code that we handle as part of this project. An example of this would be the change that we make as part of the ScorableMixin (see Issue 4).

* Visitor Pattern: Having to check the class of an object and making calls based on that is an example of a bad design pattern. An example of this is ScorableMixin. We will adopt a loose implementation of the Visitor pattern to overcome this by adding a method in ResponseMap and ReviewResponseMap and just call that method in the object we receive. See Issue 4 section for a detailed explanation.

==Test Plan==
As part of this section, we track the set of files that are to be changed for tests and scenarios that are to be tested.
===Files===
Since this project is tied to the Response class, all the test cases will be added to response_spec.rb

* Issue 1,2,4: Modify tests in response_spec.rb
* Issue 3: Write tests for '''volume_of_review_comments''' method
===Scenarios===
* Test the score calculation cases - maximum possible total score for all scores, average score across all of the instances and total score (weighted sum). Ideally average score calculation should cover all the scenarios, that is, if the maximum score is zero, it uses the total score(weighted).
* Test comments for review - We also want to test the scenario that involves seeing the comments for a review.

==Future Work==
The issues pointed out in this documentation will be implemented once the design gets approved. Further changes will be updated in this section as we make progress with the project.

==Mentors==
* Prof. Edward F. Gehringer

==Contributors==
* Arvind Srinivas Subramanian (asubram9)
* David Brain (mdbrain2)
* Dhanya Sri Dasari (ddasari)
* Zhihao Wang (zwang238@ncsu.edu)

CSC/ECE 517 Spring 2023 - E2336. Reimplement response.rb

2023-04-25T23:15:11Z

Mdbrain2: /* Issue 5 */

==Project Overview==
This project is a continuation to E2312. The current project is to make design changes to the Response class, ReviewCommentMixin module, ScorableMixin module and add comments to test cases. We explain the issue with respect to the current implementation of each of these entities as subsections below.

==Note to the reviwer for final submission==
* This project is a refactoring of some files done in Project 3. We have made these changes in a separate repo so that the new changes can be seen clearly.

* We have added a new section in this documentation called '''Changes made / Solutions''' that has all the changes we have made and explanations.

'''PR for this project:''' https://github.com/expertiza/reimplementation-back-end/pull/38

'''The PR for the previous project (Project 3) on top of which these changes have been made:''' https://github.com/expertiza/reimplementation-back-end/pull/23

==Issues==

There are several issues we plan to address as part of this project.

* Refactor the get_all_review_comments method as an instance method in response.rb file
* Refactoring significant_difference
* Make the method in ReviewCommentMixin to be an instance method
* Simplify + Refactor methods in ScorableMixin
* Refactor the self.get_all_responses method as an instance method in response_map.rb file.
* Add more descriptive comments for the tests.

===Issue 1: Refactor the self.get_all_review_comments method===
Currently, the get_all_review_comments method is a class method that returns a data structure containing all the review comments for all responses. This violates the principles of good design as it creates a large data structure that the calling method needs to break apart, leading to poor performance and increased complexity.

[[File:get_all_review_comments.png|700px]]

To address this issue and come up with a much more elegant design, we plan to refactor as a new implementation that should convert the get_all_review_comments method into an instance method that takes a review object as an argument and returns the comments for that review only. This will allow the client class to use an Iterator pattern to fetch one review at a time from the Response class rather than iterate over that data structure that get_all_review_comments returned to it.

===Issue 2: Refactoring significant_difference?===
While the significant_difference method does not necessarily need to be refactored, it is the only method with a call for the avg_scores_and_count_for_prev_reviews method, which does need to be refactored it both is a class method and returns a data structure.

def self.avg_scores_and_count_for_prev_reviews(existing_responses, current_response)
scores_assigned = []
count = 0
existing_responses.each do |existing_response|
unless existing_response.id == current_response.id # the current_response is also in existing_responses array
count += 1
scores_assigned << existing_response.aggregate_questionnaire_score.to_f / existing_response.maximum_score
end
end
[scores_assigned.sum / scores_assigned.size.to_f, count]
end

Because of this we decided that we should simply refactor significant_difference instead and simply remove avg_scores_and_count_for_prev_reviews.

# compare the current response score with other scores on the same artifact, and test if the difference
# is significant enough to notify instructor.
# Precondition: the response object is associated with a ReviewResponseMap
### "map_class.assessments_for" method need to be refactored
def significant_difference?
map_class = map.class
existing_responses = map_class.assessments_for(map.reviewee)
average_score_on_same_artifact_from_others, count = Response.avg_scores_and_count_for_prev_reviews(existing_responses, self)
# if this response is the first on this artifact, there's no grade conflict
return false if count.zero?
# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score_on_same_artifact_from_others - score).abs * 100 > allowed_difference_percentage
end

===Issue 3: Make the method in ReviewCommentMixin to be an instance method ===
====Problems====
* The ReviewCommentMixin has one single method '''volume_of_review_comments''' and it is currently present as a class method. There is no need for it to be a class method and it should instead be an instance method.
* Volume of review comments is a metric, which belongs in metrics.rb, not in a mixin for responses.

==== Proposed Solution====
The change is quite straight forward. We should move this method to be an instance method in '''metrics.rb''' file. A side note is that, this method calls the '''get_all_review_comments''' method from '''response.rb'''. This method will be changed as part of this project (See above points). Any change to that function's method call must be reflected here too.

===Issue 4: Simplify + Refactor methods in ScorableMixin ===

This mixin has methods that are related to scoring. https://expertiza.csc.ncsu.edu/index.php/Scoring_%26_Grading_Methods_(Fall_%2721) is a good resource to understand how scoring and grading works.

====Problems====
* 1) Not DRY : The following piece of code is used in 2 functions - calculate_total_score and maximum_score

scores.each do |s|
question = Question.find(s.question_id)
< Perform a calculation >
end

Basically, we iterate through the score and calculate something. The common calculation between both these functions is total_weight calculation.

'''What problem does this cause?''' If the logic to calculate the total_weight changes in the future, we will have to make changes to both these places. A developer who is relatively new to the project might miss out one changing it in a location. It is always good to have a single point of change by 'DRY'ing our code.

* 2) Not optimized: Another change related to the above snippet has to do with how the queries are made and the number of queries.

scores.each do |s|
question = Question.find(s.question_id)
<Other lines irrelevant to this case >
end

'''What problem does this cause?''' This is done inside a loop, meaning we perform a select query to each time -> N queries for N objects. Our solution aims to optimize this.

* 3) Checking the class type of an object to perform some business logic

This issue is present in the questionnaire_by_answer method. The following snippet shows it:

assignment = if map.is_a? ReviewResponseMap
map.assignment
else
Participant.find(map.reviewer_id).assignment
end

==== Proposed Solution====
* 1) The common function would involve calculating the sum of weights.

def calculate_total_weight()
scores.each do |s|
question = Question.find(s.question_id)
total_weight += question.weight unless s.answer.nil? || !question.is_a?(ScoredQuestion)
end
And this can be called inside the calculate_total_score and maximum_score.

* 2) Instead of calling Question.find inside the loop, we can get the set of question_ids and use a where function.

question_ids = scores.map(&:question_id)
questions = Question.where(question_id: question_ids)
< Make the calculation here >
This way, we only make one database call instead of N calls.

* 3) To solve checking the object type, we can write a method named '''response_assignment''' in ResponseMap and ReviewResponseMap. This way, we can just call that method in the object and depending on what object it is, we would get the corresponding answer.
* In ReviewResponseMap, the method should return its assignment
* In ResponseMap, the method should return Participant.find(map.reviewer_id).assignment

A visual representation of this change would look like this. The snippet uses an 'OR' condition to pick the appropriate assignment. Our change would remove the need for this check.

[[File:ScorableMixin3.png|700px]]

===Issue 5: Refactor the self.get_all_responses method===

The self.get_all_responses method which is currently in the response.rb file would be much cleaner as an instance method in response_map.rb. We plan to refactor the self.get_all_responses method in response.rb to an instance method in response_map.rb.

[[File:Get all responses.png|700px]]

[[File:Response map class.png|700px]]

===Issue 6: Add more descriptive comments for the tests===

The existing tests need more descriptive comments. We plan to add more detailed comments for all the test cases.

[[File:Response tests 1.png|700px]]

[[File:Response tests 2.png|700px]]

[[File:Response tests 3.png|700px]]

==Changes made / Solutions==

===Issue 1===
The get_all_review_comments method has been moved to be an instance method of the assignment class and modified to only take the reviewee_id. However, the return value has not been modified. This is because upon further investigation the get_all_review_comments (previously called concatenate_all_review_comments) is only used within the volume_of_review_comments method and that modifying volume_of_review_comments to only get a single variable from get_all_review_comments proved to difficult. Unlike the avg_scores_and_count_for_prev_reviews, however, get_all_review_comments was not merged into get_all_review_comments due to its size and that doing so would make get_all_review_comments difficult to understand.

===Issue 2===
The avg_scores_and_count_for_prev_reviews method has been merged with the significant_difference? method.

def significant_difference?
map_class = map.class
# gets all responses made by a reviewee
existing_responses = map_class.assessments_for(map.reviewee)
#
count = 0
total = 0
# gets the sum total percentage scores of all responses that are not this response
existing_responses.each do |response|
unless id == response.id # the current_response is also in existing_responses array
count += 1
total += response.aggregate_questionnaire_score.to_f / response.maximum_score
end
end
#
# if this response is the only response by the reviewee, there's no grade conflict
return false if count.zero?
#
# calculates the average score of all other responses
average_score = total / count
#
# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
#
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
#
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score - score).abs * 100 > allowed_difference_percentage
end

===Issue 3===
We have made the volume_of_review_comments method to be an instance method and moved it to the assignment.rb since it used to take a 'assignment_id' as an input argument. In this way, we are able to make it more sense and removed the ReviewCommentMixin.

# frozen_string_literal: true
module MetricHelper
# Determine the average size of review comments in each round
# the first entry in the returned list is the overall average
# word count.
def volume_of_review_comments(assignment_id, reviewer_id)
comments, counter,
@comments_in_round, @counter_in_round = Response.get_all_review_comments(assignment_id, reviewer_id)
# Index 0 is a nil element that can be ignored in the round count
num_rounds = @comments_in_round.count - 1

overall_avg_vol = (Lingua::EN::Readability.new(comments).num_words / (counter.zero? ? 1 : counter)).round(0)
review_comments_volume = []
review_comments_volume.push(overall_avg_vol)
(1..num_rounds).each do |round|
num = Lingua::EN::Readability.new(@comments_in_round[round]).num_words
den = (@counter_in_round[round].zero? ? 1 : @counter_in_round[round])
avg_vol_in_round = (num / den).round(0)
review_comments_volume.push(avg_vol_in_round)
end
review_comments_volume
end
end

===Issue 4===
* 1) We have decided not to address this part of the issue (See the Issue 4) after discussing with the professor. The reason behind it is that the lines involved in this section is only 2 lines long and hence it does not have to be moved to a method.

* 2) We looked at a lot of solutions to make this. The issue is that using just a 'where' method will not ensure that the Active Record Association returned will be in the same order of that of the list of ids given.

For example:

ids = [4,1,2,7]
answers = Answer.where(id: ids)
return answers.map(&:id)

This example snippet would return 1,2,4,7

Hence, to ensure the order of the return values(thereby ensuring the correct working) and to improve the performance, we have used a small gem 'find_with_order'. The main reason why we decided to add this gem over a custom solution is portability / ease of modification. A custom solution would have required us to write SQL queries in where clauses and this would be tied to the mysql(current database used). This gem is compatible with popular databases and hence this would be automatically taken care of even if the database is changed.

Hence, our final solution is:

def calculate_total_score
# Only count the scorable questions, only when the answer is not nil (we accept nil as
# answer for scorable questions, and they will not be counted towards the total score)
sum = 0
question_ids = scores.map(&:question_id)
# We use find with order here to ensure that the list of questions we get is in the same order as that of question_ids
questions = Question.find_with_order(question_ids)
scores.each_with_index do |score, idx|
sum += score.answer * questions[idx].weight unless score.answer.nil? || !questions[idx].is_a?(ScoredQuestion)
end
sum
end

def maximum_score
# Only count the scorable questions, only when the answer is not nil (we accept nil as
# answer for scorable questions, and they will not be counted towards the total score)
total_weight = 0
question_ids = scores.map(&:question_id)
# We use find with order here to ensure that the list of questions we get is in the same order as that of question_ids
questions = Question.find_with_order(question_ids)
scores.each_with_index do |score, idx|
total_weight += questions[idx].weight unless score.answer.nil? || !questions[idx].is_a?(ScoredQuestion)
end
questionnaire = if scores.empty?
questionnaire_by_answer(nil)
else
questionnaire_by_answer(scores.first)
end
total_weight * questionnaire.max_question_score
end

* 3) Usage of visitor pattern to avoid checking class types: We solved this by adding a method '''response_assignment''' to ReviewResponseMap and ResponseMap classes.

In response_map.rb

# returns the assignment related to the response map
def response_assignment
return Participant.find(self.reviewer_id).assignment
end

In review_response_map.rb

# returns the assignment related to the review response map
def response_assignment
return self.assignment
end

Hence, the '''questionnaire_by_answer''' method in '''ScorableHelper''' will have the following snippet:

map = ResponseMap.find(map_id)
assignment = map.response_assignment

===Issue 5===
The get_all_responses method has been made an instance method of response_map and simplified as the function no longer need to find a response map.

def get_all_responses
Response.where(map_id: map_id).all
end

==Design Principles==
This section aims to act as a consolidation of the design principles that we will implement as part of this project
* DRY: We will DRY the code that we handle as part of this project. An example of this would be the change that we make as part of the ScorableMixin (see Issue 4).

* Visitor Pattern: Having to check the class of an object and making calls based on that is an example of a bad design pattern. An example of this is ScorableMixin. We will adopt a loose implementation of the Visitor pattern to overcome this by adding a method in ResponseMap and ReviewResponseMap and just call that method in the object we receive. See Issue 4 section for a detailed explanation.

==Test Plan==
As part of this section, we track the set of files that are to be changed for tests and scenarios that are to be tested.
===Files===
Since this project is tied to the Response class, all the test cases will be added to response_spec.rb

* Issue 1,2,4: Modify tests in response_spec.rb
* Issue 3: Write tests for '''volume_of_review_comments''' method
===Scenarios===
* Test the score calculation cases - maximum possible total score for all scores, average score across all of the instances and total score (weighted sum). Ideally average score calculation should cover all the scenarios, that is, if the maximum score is zero, it uses the total score(weighted).
* Test comments for review - We also want to test the scenario that involves seeing the comments for a review.

==Future Work==
The issues pointed out in this documentation will be implemented once the design gets approved. Further changes will be updated in this section as we make progress with the project.

==Mentors==
* Prof. Edward F. Gehringer

==Contributors==
* Arvind Srinivas Subramanian (asubram9)
* David Brain (mdbrain2)
* Dhanya Sri Dasari (ddasari)
* Zhihao Wang (zwang238@ncsu.edu)

CSC/ECE 517 Spring 2023 - E2336. Reimplement response.rb

2023-04-25T23:12:47Z

Mdbrain2: /* Issue 3 solved */

==Project Overview==
This project is a continuation to E2312. The current project is to make design changes to the Response class, ReviewCommentMixin module, ScorableMixin module and add comments to test cases. We explain the issue with respect to the current implementation of each of these entities as subsections below.

==Note to the reviwer for final submission==
* This project is a refactoring of some files done in Project 3. We have made these changes in a separate repo so that the new changes can be seen clearly.

* We have added a new section in this documentation called '''Changes made / Solutions''' that has all the changes we have made and explanations.

'''PR for this project:''' https://github.com/expertiza/reimplementation-back-end/pull/38

'''The PR for the previous project (Project 3) on top of which these changes have been made:''' https://github.com/expertiza/reimplementation-back-end/pull/23

==Issues==

There are several issues we plan to address as part of this project.

* Refactor the get_all_review_comments method as an instance method in response.rb file
* Refactoring significant_difference
* Make the method in ReviewCommentMixin to be an instance method
* Simplify + Refactor methods in ScorableMixin
* Refactor the self.get_all_responses method as an instance method in response_map.rb file.
* Add more descriptive comments for the tests.

===Issue 1: Refactor the self.get_all_review_comments method===
Currently, the get_all_review_comments method is a class method that returns a data structure containing all the review comments for all responses. This violates the principles of good design as it creates a large data structure that the calling method needs to break apart, leading to poor performance and increased complexity.

[[File:get_all_review_comments.png|700px]]

To address this issue and come up with a much more elegant design, we plan to refactor as a new implementation that should convert the get_all_review_comments method into an instance method that takes a review object as an argument and returns the comments for that review only. This will allow the client class to use an Iterator pattern to fetch one review at a time from the Response class rather than iterate over that data structure that get_all_review_comments returned to it.

===Issue 2: Refactoring significant_difference?===
While the significant_difference method does not necessarily need to be refactored, it is the only method with a call for the avg_scores_and_count_for_prev_reviews method, which does need to be refactored it both is a class method and returns a data structure.

def self.avg_scores_and_count_for_prev_reviews(existing_responses, current_response)
scores_assigned = []
count = 0
existing_responses.each do |existing_response|
unless existing_response.id == current_response.id # the current_response is also in existing_responses array
count += 1
scores_assigned << existing_response.aggregate_questionnaire_score.to_f / existing_response.maximum_score
end
end
[scores_assigned.sum / scores_assigned.size.to_f, count]
end

Because of this we decided that we should simply refactor significant_difference instead and simply remove avg_scores_and_count_for_prev_reviews.

# compare the current response score with other scores on the same artifact, and test if the difference
# is significant enough to notify instructor.
# Precondition: the response object is associated with a ReviewResponseMap
### "map_class.assessments_for" method need to be refactored
def significant_difference?
map_class = map.class
existing_responses = map_class.assessments_for(map.reviewee)
average_score_on_same_artifact_from_others, count = Response.avg_scores_and_count_for_prev_reviews(existing_responses, self)
# if this response is the first on this artifact, there's no grade conflict
return false if count.zero?
# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score_on_same_artifact_from_others - score).abs * 100 > allowed_difference_percentage
end

===Issue 3: Make the method in ReviewCommentMixin to be an instance method ===
====Problems====
* The ReviewCommentMixin has one single method '''volume_of_review_comments''' and it is currently present as a class method. There is no need for it to be a class method and it should instead be an instance method.
* Volume of review comments is a metric, which belongs in metrics.rb, not in a mixin for responses.

==== Proposed Solution====
The change is quite straight forward. We should move this method to be an instance method in '''metrics.rb''' file. A side note is that, this method calls the '''get_all_review_comments''' method from '''response.rb'''. This method will be changed as part of this project (See above points). Any change to that function's method call must be reflected here too.

===Issue 4: Simplify + Refactor methods in ScorableMixin ===

This mixin has methods that are related to scoring. https://expertiza.csc.ncsu.edu/index.php/Scoring_%26_Grading_Methods_(Fall_%2721) is a good resource to understand how scoring and grading works.

====Problems====
* 1) Not DRY : The following piece of code is used in 2 functions - calculate_total_score and maximum_score

scores.each do |s|
question = Question.find(s.question_id)
< Perform a calculation >
end

Basically, we iterate through the score and calculate something. The common calculation between both these functions is total_weight calculation.

'''What problem does this cause?''' If the logic to calculate the total_weight changes in the future, we will have to make changes to both these places. A developer who is relatively new to the project might miss out one changing it in a location. It is always good to have a single point of change by 'DRY'ing our code.

* 2) Not optimized: Another change related to the above snippet has to do with how the queries are made and the number of queries.

scores.each do |s|
question = Question.find(s.question_id)
<Other lines irrelevant to this case >
end

'''What problem does this cause?''' This is done inside a loop, meaning we perform a select query to each time -> N queries for N objects. Our solution aims to optimize this.

* 3) Checking the class type of an object to perform some business logic

This issue is present in the questionnaire_by_answer method. The following snippet shows it:

assignment = if map.is_a? ReviewResponseMap
map.assignment
else
Participant.find(map.reviewer_id).assignment
end

==== Proposed Solution====
* 1) The common function would involve calculating the sum of weights.

def calculate_total_weight()
scores.each do |s|
question = Question.find(s.question_id)
total_weight += question.weight unless s.answer.nil? || !question.is_a?(ScoredQuestion)
end
And this can be called inside the calculate_total_score and maximum_score.

* 2) Instead of calling Question.find inside the loop, we can get the set of question_ids and use a where function.

question_ids = scores.map(&:question_id)
questions = Question.where(question_id: question_ids)
< Make the calculation here >
This way, we only make one database call instead of N calls.

* 3) To solve checking the object type, we can write a method named '''response_assignment''' in ResponseMap and ReviewResponseMap. This way, we can just call that method in the object and depending on what object it is, we would get the corresponding answer.
* In ReviewResponseMap, the method should return its assignment
* In ResponseMap, the method should return Participant.find(map.reviewer_id).assignment

A visual representation of this change would look like this. The snippet uses an 'OR' condition to pick the appropriate assignment. Our change would remove the need for this check.

[[File:ScorableMixin3.png|700px]]

===Issue 5: Refactor the self.get_all_responses method===

The self.get_all_responses method which is currently in the response.rb file would be much cleaner as an instance method in response_map.rb. We plan to refactor the self.get_all_responses method in response.rb to an instance method in response_map.rb.

[[File:Get all responses.png|700px]]

[[File:Response map class.png|700px]]

===Issue 6: Add more descriptive comments for the tests===

The existing tests need more descriptive comments. We plan to add more detailed comments for all the test cases.

[[File:Response tests 1.png|700px]]

[[File:Response tests 2.png|700px]]

[[File:Response tests 3.png|700px]]

==Changes made / Solutions==

===Issue 1===
The get_all_review_comments method has been moved to be an instance method of the assignment class and modified to only take the reviewee_id. However, the return value has not been modified. This is because upon further investigation the get_all_review_comments (previously called concatenate_all_review_comments) is only used within the volume_of_review_comments method and that modifying volume_of_review_comments to only get a single variable from get_all_review_comments proved to difficult. Unlike the avg_scores_and_count_for_prev_reviews, however, get_all_review_comments was not merged into get_all_review_comments due to its size and that doing so would make get_all_review_comments difficult to understand.

===Issue 2===
The avg_scores_and_count_for_prev_reviews method has been merged with the significant_difference? method.

def significant_difference?
map_class = map.class
# gets all responses made by a reviewee
existing_responses = map_class.assessments_for(map.reviewee)
#
count = 0
total = 0
# gets the sum total percentage scores of all responses that are not this response
existing_responses.each do |response|
unless id == response.id # the current_response is also in existing_responses array
count += 1
total += response.aggregate_questionnaire_score.to_f / response.maximum_score
end
end
#
# if this response is the only response by the reviewee, there's no grade conflict
return false if count.zero?
#
# calculates the average score of all other responses
average_score = total / count
#
# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
#
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
#
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score - score).abs * 100 > allowed_difference_percentage
end

===Issue 3===
We have made the volume_of_review_comments method to be an instance method and moved it to the assignment.rb since it used to take a 'assignment_id' as an input argument. In this way, we are able to make it more sense and removed the ReviewCommentMixin.

# frozen_string_literal: true
module MetricHelper
# Determine the average size of review comments in each round
# the first entry in the returned list is the overall average
# word count.
def volume_of_review_comments(assignment_id, reviewer_id)
comments, counter,
@comments_in_round, @counter_in_round = Response.get_all_review_comments(assignment_id, reviewer_id)
# Index 0 is a nil element that can be ignored in the round count
num_rounds = @comments_in_round.count - 1

overall_avg_vol = (Lingua::EN::Readability.new(comments).num_words / (counter.zero? ? 1 : counter)).round(0)
review_comments_volume = []
review_comments_volume.push(overall_avg_vol)
(1..num_rounds).each do |round|
num = Lingua::EN::Readability.new(@comments_in_round[round]).num_words
den = (@counter_in_round[round].zero? ? 1 : @counter_in_round[round])
avg_vol_in_round = (num / den).round(0)
review_comments_volume.push(avg_vol_in_round)
end
review_comments_volume
end
end

===Issue 4===
* 1) We have decided not to address this part of the issue (See the Issue 4) after discussing with the professor. The reason behind it is that the lines involved in this section is only 2 lines long and hence it does not have to be moved to a method.

* 2) We looked at a lot of solutions to make this. The issue is that using just a 'where' method will not ensure that the Active Record Association returned will be in the same order of that of the list of ids given.

For example:

ids = [4,1,2,7]
answers = Answer.where(id: ids)
return answers.map(&:id)

This example snippet would return 1,2,4,7

Hence, to ensure the order of the return values(thereby ensuring the correct working) and to improve the performance, we have used a small gem 'find_with_order'. The main reason why we decided to add this gem over a custom solution is portability / ease of modification. A custom solution would have required us to write SQL queries in where clauses and this would be tied to the mysql(current database used). This gem is compatible with popular databases and hence this would be automatically taken care of even if the database is changed.

Hence, our final solution is:

def calculate_total_score
# Only count the scorable questions, only when the answer is not nil (we accept nil as
# answer for scorable questions, and they will not be counted towards the total score)
sum = 0
question_ids = scores.map(&:question_id)
# We use find with order here to ensure that the list of questions we get is in the same order as that of question_ids
questions = Question.find_with_order(question_ids)
scores.each_with_index do |score, idx|
sum += score.answer * questions[idx].weight unless score.answer.nil? || !questions[idx].is_a?(ScoredQuestion)
end
sum
end

def maximum_score
# Only count the scorable questions, only when the answer is not nil (we accept nil as
# answer for scorable questions, and they will not be counted towards the total score)
total_weight = 0
question_ids = scores.map(&:question_id)
# We use find with order here to ensure that the list of questions we get is in the same order as that of question_ids
questions = Question.find_with_order(question_ids)
scores.each_with_index do |score, idx|
total_weight += questions[idx].weight unless score.answer.nil? || !questions[idx].is_a?(ScoredQuestion)
end
questionnaire = if scores.empty?
questionnaire_by_answer(nil)
else
questionnaire_by_answer(scores.first)
end
total_weight * questionnaire.max_question_score
end

* 3) Usage of visitor pattern to avoid checking class types: We solved this by adding a method '''response_assignment''' to ReviewResponseMap and ResponseMap classes.

In response_map.rb

# returns the assignment related to the response map
def response_assignment
return Participant.find(self.reviewer_id).assignment
end

In review_response_map.rb

# returns the assignment related to the review response map
def response_assignment
return self.assignment
end

Hence, the '''questionnaire_by_answer''' method in '''ScorableHelper''' will have the following snippet:

map = ResponseMap.find(map_id)
assignment = map.response_assignment

===Issue 5===
The get_all_responses method

==Design Principles==
This section aims to act as a consolidation of the design principles that we will implement as part of this project
* DRY: We will DRY the code that we handle as part of this project. An example of this would be the change that we make as part of the ScorableMixin (see Issue 4).

* Visitor Pattern: Having to check the class of an object and making calls based on that is an example of a bad design pattern. An example of this is ScorableMixin. We will adopt a loose implementation of the Visitor pattern to overcome this by adding a method in ResponseMap and ReviewResponseMap and just call that method in the object we receive. See Issue 4 section for a detailed explanation.

==Test Plan==
As part of this section, we track the set of files that are to be changed for tests and scenarios that are to be tested.
===Files===
Since this project is tied to the Response class, all the test cases will be added to response_spec.rb

* Issue 1,2,4: Modify tests in response_spec.rb
* Issue 3: Write tests for '''volume_of_review_comments''' method
===Scenarios===
* Test the score calculation cases - maximum possible total score for all scores, average score across all of the instances and total score (weighted sum). Ideally average score calculation should cover all the scenarios, that is, if the maximum score is zero, it uses the total score(weighted).
* Test comments for review - We also want to test the scenario that involves seeing the comments for a review.

==Future Work==
The issues pointed out in this documentation will be implemented once the design gets approved. Further changes will be updated in this section as we make progress with the project.

==Mentors==
* Prof. Edward F. Gehringer

==Contributors==
* Arvind Srinivas Subramanian (asubram9)
* David Brain (mdbrain2)
* Dhanya Sri Dasari (ddasari)
* Zhihao Wang (zwang238@ncsu.edu)

CSC/ECE 517 Spring 2023 - E2336. Reimplement response.rb

2023-04-25T23:12:39Z

Mdbrain2: /* Issue 4 solved */

==Project Overview==
This project is a continuation to E2312. The current project is to make design changes to the Response class, ReviewCommentMixin module, ScorableMixin module and add comments to test cases. We explain the issue with respect to the current implementation of each of these entities as subsections below.

==Note to the reviwer for final submission==
* This project is a refactoring of some files done in Project 3. We have made these changes in a separate repo so that the new changes can be seen clearly.

* We have added a new section in this documentation called '''Changes made / Solutions''' that has all the changes we have made and explanations.

'''PR for this project:''' https://github.com/expertiza/reimplementation-back-end/pull/38

'''The PR for the previous project (Project 3) on top of which these changes have been made:''' https://github.com/expertiza/reimplementation-back-end/pull/23

==Issues==

There are several issues we plan to address as part of this project.

* Refactor the get_all_review_comments method as an instance method in response.rb file
* Refactoring significant_difference
* Make the method in ReviewCommentMixin to be an instance method
* Simplify + Refactor methods in ScorableMixin
* Refactor the self.get_all_responses method as an instance method in response_map.rb file.
* Add more descriptive comments for the tests.

===Issue 1: Refactor the self.get_all_review_comments method===
Currently, the get_all_review_comments method is a class method that returns a data structure containing all the review comments for all responses. This violates the principles of good design as it creates a large data structure that the calling method needs to break apart, leading to poor performance and increased complexity.

[[File:get_all_review_comments.png|700px]]

To address this issue and come up with a much more elegant design, we plan to refactor as a new implementation that should convert the get_all_review_comments method into an instance method that takes a review object as an argument and returns the comments for that review only. This will allow the client class to use an Iterator pattern to fetch one review at a time from the Response class rather than iterate over that data structure that get_all_review_comments returned to it.

===Issue 2: Refactoring significant_difference?===
While the significant_difference method does not necessarily need to be refactored, it is the only method with a call for the avg_scores_and_count_for_prev_reviews method, which does need to be refactored it both is a class method and returns a data structure.

def self.avg_scores_and_count_for_prev_reviews(existing_responses, current_response)
scores_assigned = []
count = 0
existing_responses.each do |existing_response|
unless existing_response.id == current_response.id # the current_response is also in existing_responses array
count += 1
scores_assigned << existing_response.aggregate_questionnaire_score.to_f / existing_response.maximum_score
end
end
[scores_assigned.sum / scores_assigned.size.to_f, count]
end

Because of this we decided that we should simply refactor significant_difference instead and simply remove avg_scores_and_count_for_prev_reviews.

# compare the current response score with other scores on the same artifact, and test if the difference
# is significant enough to notify instructor.
# Precondition: the response object is associated with a ReviewResponseMap
### "map_class.assessments_for" method need to be refactored
def significant_difference?
map_class = map.class
existing_responses = map_class.assessments_for(map.reviewee)
average_score_on_same_artifact_from_others, count = Response.avg_scores_and_count_for_prev_reviews(existing_responses, self)
# if this response is the first on this artifact, there's no grade conflict
return false if count.zero?
# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score_on_same_artifact_from_others - score).abs * 100 > allowed_difference_percentage
end

===Issue 3: Make the method in ReviewCommentMixin to be an instance method ===
====Problems====
* The ReviewCommentMixin has one single method '''volume_of_review_comments''' and it is currently present as a class method. There is no need for it to be a class method and it should instead be an instance method.
* Volume of review comments is a metric, which belongs in metrics.rb, not in a mixin for responses.

==== Proposed Solution====
The change is quite straight forward. We should move this method to be an instance method in '''metrics.rb''' file. A side note is that, this method calls the '''get_all_review_comments''' method from '''response.rb'''. This method will be changed as part of this project (See above points). Any change to that function's method call must be reflected here too.

===Issue 4: Simplify + Refactor methods in ScorableMixin ===

This mixin has methods that are related to scoring. https://expertiza.csc.ncsu.edu/index.php/Scoring_%26_Grading_Methods_(Fall_%2721) is a good resource to understand how scoring and grading works.

====Problems====
* 1) Not DRY : The following piece of code is used in 2 functions - calculate_total_score and maximum_score

scores.each do |s|
question = Question.find(s.question_id)
< Perform a calculation >
end

Basically, we iterate through the score and calculate something. The common calculation between both these functions is total_weight calculation.

'''What problem does this cause?''' If the logic to calculate the total_weight changes in the future, we will have to make changes to both these places. A developer who is relatively new to the project might miss out one changing it in a location. It is always good to have a single point of change by 'DRY'ing our code.

* 2) Not optimized: Another change related to the above snippet has to do with how the queries are made and the number of queries.

scores.each do |s|
question = Question.find(s.question_id)
<Other lines irrelevant to this case >
end

'''What problem does this cause?''' This is done inside a loop, meaning we perform a select query to each time -> N queries for N objects. Our solution aims to optimize this.

* 3) Checking the class type of an object to perform some business logic

This issue is present in the questionnaire_by_answer method. The following snippet shows it:

assignment = if map.is_a? ReviewResponseMap
map.assignment
else
Participant.find(map.reviewer_id).assignment
end

==== Proposed Solution====
* 1) The common function would involve calculating the sum of weights.

def calculate_total_weight()
scores.each do |s|
question = Question.find(s.question_id)
total_weight += question.weight unless s.answer.nil? || !question.is_a?(ScoredQuestion)
end
And this can be called inside the calculate_total_score and maximum_score.

* 2) Instead of calling Question.find inside the loop, we can get the set of question_ids and use a where function.

question_ids = scores.map(&:question_id)
questions = Question.where(question_id: question_ids)
< Make the calculation here >
This way, we only make one database call instead of N calls.

* 3) To solve checking the object type, we can write a method named '''response_assignment''' in ResponseMap and ReviewResponseMap. This way, we can just call that method in the object and depending on what object it is, we would get the corresponding answer.
* In ReviewResponseMap, the method should return its assignment
* In ResponseMap, the method should return Participant.find(map.reviewer_id).assignment

A visual representation of this change would look like this. The snippet uses an 'OR' condition to pick the appropriate assignment. Our change would remove the need for this check.

[[File:ScorableMixin3.png|700px]]

===Issue 5: Refactor the self.get_all_responses method===

The self.get_all_responses method which is currently in the response.rb file would be much cleaner as an instance method in response_map.rb. We plan to refactor the self.get_all_responses method in response.rb to an instance method in response_map.rb.

[[File:Get all responses.png|700px]]

[[File:Response map class.png|700px]]

===Issue 6: Add more descriptive comments for the tests===

The existing tests need more descriptive comments. We plan to add more detailed comments for all the test cases.

[[File:Response tests 1.png|700px]]

[[File:Response tests 2.png|700px]]

[[File:Response tests 3.png|700px]]

==Changes made / Solutions==

===Issue 1===
The get_all_review_comments method has been moved to be an instance method of the assignment class and modified to only take the reviewee_id. However, the return value has not been modified. This is because upon further investigation the get_all_review_comments (previously called concatenate_all_review_comments) is only used within the volume_of_review_comments method and that modifying volume_of_review_comments to only get a single variable from get_all_review_comments proved to difficult. Unlike the avg_scores_and_count_for_prev_reviews, however, get_all_review_comments was not merged into get_all_review_comments due to its size and that doing so would make get_all_review_comments difficult to understand.

===Issue 2===
The avg_scores_and_count_for_prev_reviews method has been merged with the significant_difference? method.

def significant_difference?
map_class = map.class
# gets all responses made by a reviewee
existing_responses = map_class.assessments_for(map.reviewee)
#
count = 0
total = 0
# gets the sum total percentage scores of all responses that are not this response
existing_responses.each do |response|
unless id == response.id # the current_response is also in existing_responses array
count += 1
total += response.aggregate_questionnaire_score.to_f / response.maximum_score
end
end
#
# if this response is the only response by the reviewee, there's no grade conflict
return false if count.zero?
#
# calculates the average score of all other responses
average_score = total / count
#
# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
#
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
#
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score - score).abs * 100 > allowed_difference_percentage
end

===Issue 3 solved===
We have made the volume_of_review_comments method to be an instance method and moved it to the assignment.rb since it used to take a 'assignment_id' as an input argument. In this way, we are able to make it more sense and removed the ReviewCommentMixin.

# frozen_string_literal: true
module MetricHelper
# Determine the average size of review comments in each round
# the first entry in the returned list is the overall average
# word count.
def volume_of_review_comments(assignment_id, reviewer_id)
comments, counter,
@comments_in_round, @counter_in_round = Response.get_all_review_comments(assignment_id, reviewer_id)
# Index 0 is a nil element that can be ignored in the round count
num_rounds = @comments_in_round.count - 1

overall_avg_vol = (Lingua::EN::Readability.new(comments).num_words / (counter.zero? ? 1 : counter)).round(0)
review_comments_volume = []
review_comments_volume.push(overall_avg_vol)
(1..num_rounds).each do |round|
num = Lingua::EN::Readability.new(@comments_in_round[round]).num_words
den = (@counter_in_round[round].zero? ? 1 : @counter_in_round[round])
avg_vol_in_round = (num / den).round(0)
review_comments_volume.push(avg_vol_in_round)
end
review_comments_volume
end
end

===Issue 4===
* 1) We have decided not to address this part of the issue (See the Issue 4) after discussing with the professor. The reason behind it is that the lines involved in this section is only 2 lines long and hence it does not have to be moved to a method.

* 2) We looked at a lot of solutions to make this. The issue is that using just a 'where' method will not ensure that the Active Record Association returned will be in the same order of that of the list of ids given.

For example:

ids = [4,1,2,7]
answers = Answer.where(id: ids)
return answers.map(&:id)

This example snippet would return 1,2,4,7

Hence, to ensure the order of the return values(thereby ensuring the correct working) and to improve the performance, we have used a small gem 'find_with_order'. The main reason why we decided to add this gem over a custom solution is portability / ease of modification. A custom solution would have required us to write SQL queries in where clauses and this would be tied to the mysql(current database used). This gem is compatible with popular databases and hence this would be automatically taken care of even if the database is changed.

Hence, our final solution is:

def calculate_total_score
# Only count the scorable questions, only when the answer is not nil (we accept nil as
# answer for scorable questions, and they will not be counted towards the total score)
sum = 0
question_ids = scores.map(&:question_id)
# We use find with order here to ensure that the list of questions we get is in the same order as that of question_ids
questions = Question.find_with_order(question_ids)
scores.each_with_index do |score, idx|
sum += score.answer * questions[idx].weight unless score.answer.nil? || !questions[idx].is_a?(ScoredQuestion)
end
sum
end

def maximum_score
# Only count the scorable questions, only when the answer is not nil (we accept nil as
# answer for scorable questions, and they will not be counted towards the total score)
total_weight = 0
question_ids = scores.map(&:question_id)
# We use find with order here to ensure that the list of questions we get is in the same order as that of question_ids
questions = Question.find_with_order(question_ids)
scores.each_with_index do |score, idx|
total_weight += questions[idx].weight unless score.answer.nil? || !questions[idx].is_a?(ScoredQuestion)
end
questionnaire = if scores.empty?
questionnaire_by_answer(nil)
else
questionnaire_by_answer(scores.first)
end
total_weight * questionnaire.max_question_score
end

* 3) Usage of visitor pattern to avoid checking class types: We solved this by adding a method '''response_assignment''' to ReviewResponseMap and ResponseMap classes.

In response_map.rb

# returns the assignment related to the response map
def response_assignment
return Participant.find(self.reviewer_id).assignment
end

In review_response_map.rb

# returns the assignment related to the review response map
def response_assignment
return self.assignment
end

Hence, the '''questionnaire_by_answer''' method in '''ScorableHelper''' will have the following snippet:

map = ResponseMap.find(map_id)
assignment = map.response_assignment

===Issue 5===
The get_all_responses method

==Design Principles==
This section aims to act as a consolidation of the design principles that we will implement as part of this project
* DRY: We will DRY the code that we handle as part of this project. An example of this would be the change that we make as part of the ScorableMixin (see Issue 4).

* Visitor Pattern: Having to check the class of an object and making calls based on that is an example of a bad design pattern. An example of this is ScorableMixin. We will adopt a loose implementation of the Visitor pattern to overcome this by adding a method in ResponseMap and ReviewResponseMap and just call that method in the object we receive. See Issue 4 section for a detailed explanation.

==Test Plan==
As part of this section, we track the set of files that are to be changed for tests and scenarios that are to be tested.
===Files===
Since this project is tied to the Response class, all the test cases will be added to response_spec.rb

* Issue 1,2,4: Modify tests in response_spec.rb
* Issue 3: Write tests for '''volume_of_review_comments''' method
===Scenarios===
* Test the score calculation cases - maximum possible total score for all scores, average score across all of the instances and total score (weighted sum). Ideally average score calculation should cover all the scenarios, that is, if the maximum score is zero, it uses the total score(weighted).
* Test comments for review - We also want to test the scenario that involves seeing the comments for a review.

==Future Work==
The issues pointed out in this documentation will be implemented once the design gets approved. Further changes will be updated in this section as we make progress with the project.

==Mentors==
* Prof. Edward F. Gehringer

==Contributors==
* Arvind Srinivas Subramanian (asubram9)
* David Brain (mdbrain2)
* Dhanya Sri Dasari (ddasari)
* Zhihao Wang (zwang238@ncsu.edu)

CSC/ECE 517 Spring 2023 - E2336. Reimplement response.rb

2023-04-25T23:12:28Z

Mdbrain2: /* Changes made / Solutions */

==Project Overview==
This project is a continuation to E2312. The current project is to make design changes to the Response class, ReviewCommentMixin module, ScorableMixin module and add comments to test cases. We explain the issue with respect to the current implementation of each of these entities as subsections below.

==Note to the reviwer for final submission==
* This project is a refactoring of some files done in Project 3. We have made these changes in a separate repo so that the new changes can be seen clearly.

* We have added a new section in this documentation called '''Changes made / Solutions''' that has all the changes we have made and explanations.

'''PR for this project:''' https://github.com/expertiza/reimplementation-back-end/pull/38

'''The PR for the previous project (Project 3) on top of which these changes have been made:''' https://github.com/expertiza/reimplementation-back-end/pull/23

==Issues==

There are several issues we plan to address as part of this project.

* Refactor the get_all_review_comments method as an instance method in response.rb file
* Refactoring significant_difference
* Make the method in ReviewCommentMixin to be an instance method
* Simplify + Refactor methods in ScorableMixin
* Refactor the self.get_all_responses method as an instance method in response_map.rb file.
* Add more descriptive comments for the tests.

===Issue 1: Refactor the self.get_all_review_comments method===
Currently, the get_all_review_comments method is a class method that returns a data structure containing all the review comments for all responses. This violates the principles of good design as it creates a large data structure that the calling method needs to break apart, leading to poor performance and increased complexity.

[[File:get_all_review_comments.png|700px]]

To address this issue and come up with a much more elegant design, we plan to refactor as a new implementation that should convert the get_all_review_comments method into an instance method that takes a review object as an argument and returns the comments for that review only. This will allow the client class to use an Iterator pattern to fetch one review at a time from the Response class rather than iterate over that data structure that get_all_review_comments returned to it.

===Issue 2: Refactoring significant_difference?===
While the significant_difference method does not necessarily need to be refactored, it is the only method with a call for the avg_scores_and_count_for_prev_reviews method, which does need to be refactored it both is a class method and returns a data structure.

def self.avg_scores_and_count_for_prev_reviews(existing_responses, current_response)
scores_assigned = []
count = 0
existing_responses.each do |existing_response|
unless existing_response.id == current_response.id # the current_response is also in existing_responses array
count += 1
scores_assigned << existing_response.aggregate_questionnaire_score.to_f / existing_response.maximum_score
end
end
[scores_assigned.sum / scores_assigned.size.to_f, count]
end

Because of this we decided that we should simply refactor significant_difference instead and simply remove avg_scores_and_count_for_prev_reviews.

# compare the current response score with other scores on the same artifact, and test if the difference
# is significant enough to notify instructor.
# Precondition: the response object is associated with a ReviewResponseMap
### "map_class.assessments_for" method need to be refactored
def significant_difference?
map_class = map.class
existing_responses = map_class.assessments_for(map.reviewee)
average_score_on_same_artifact_from_others, count = Response.avg_scores_and_count_for_prev_reviews(existing_responses, self)
# if this response is the first on this artifact, there's no grade conflict
return false if count.zero?
# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score_on_same_artifact_from_others - score).abs * 100 > allowed_difference_percentage
end

===Issue 3: Make the method in ReviewCommentMixin to be an instance method ===
====Problems====
* The ReviewCommentMixin has one single method '''volume_of_review_comments''' and it is currently present as a class method. There is no need for it to be a class method and it should instead be an instance method.
* Volume of review comments is a metric, which belongs in metrics.rb, not in a mixin for responses.

==== Proposed Solution====
The change is quite straight forward. We should move this method to be an instance method in '''metrics.rb''' file. A side note is that, this method calls the '''get_all_review_comments''' method from '''response.rb'''. This method will be changed as part of this project (See above points). Any change to that function's method call must be reflected here too.

===Issue 4: Simplify + Refactor methods in ScorableMixin ===

This mixin has methods that are related to scoring. https://expertiza.csc.ncsu.edu/index.php/Scoring_%26_Grading_Methods_(Fall_%2721) is a good resource to understand how scoring and grading works.

====Problems====
* 1) Not DRY : The following piece of code is used in 2 functions - calculate_total_score and maximum_score

scores.each do |s|
question = Question.find(s.question_id)
< Perform a calculation >
end

Basically, we iterate through the score and calculate something. The common calculation between both these functions is total_weight calculation.

'''What problem does this cause?''' If the logic to calculate the total_weight changes in the future, we will have to make changes to both these places. A developer who is relatively new to the project might miss out one changing it in a location. It is always good to have a single point of change by 'DRY'ing our code.

* 2) Not optimized: Another change related to the above snippet has to do with how the queries are made and the number of queries.

scores.each do |s|
question = Question.find(s.question_id)
<Other lines irrelevant to this case >
end

'''What problem does this cause?''' This is done inside a loop, meaning we perform a select query to each time -> N queries for N objects. Our solution aims to optimize this.

* 3) Checking the class type of an object to perform some business logic

This issue is present in the questionnaire_by_answer method. The following snippet shows it:

assignment = if map.is_a? ReviewResponseMap
map.assignment
else
Participant.find(map.reviewer_id).assignment
end

==== Proposed Solution====
* 1) The common function would involve calculating the sum of weights.

def calculate_total_weight()
scores.each do |s|
question = Question.find(s.question_id)
total_weight += question.weight unless s.answer.nil? || !question.is_a?(ScoredQuestion)
end
And this can be called inside the calculate_total_score and maximum_score.

* 2) Instead of calling Question.find inside the loop, we can get the set of question_ids and use a where function.

question_ids = scores.map(&:question_id)
questions = Question.where(question_id: question_ids)
< Make the calculation here >
This way, we only make one database call instead of N calls.

* 3) To solve checking the object type, we can write a method named '''response_assignment''' in ResponseMap and ReviewResponseMap. This way, we can just call that method in the object and depending on what object it is, we would get the corresponding answer.
* In ReviewResponseMap, the method should return its assignment
* In ResponseMap, the method should return Participant.find(map.reviewer_id).assignment

A visual representation of this change would look like this. The snippet uses an 'OR' condition to pick the appropriate assignment. Our change would remove the need for this check.

[[File:ScorableMixin3.png|700px]]

===Issue 5: Refactor the self.get_all_responses method===

The self.get_all_responses method which is currently in the response.rb file would be much cleaner as an instance method in response_map.rb. We plan to refactor the self.get_all_responses method in response.rb to an instance method in response_map.rb.

[[File:Get all responses.png|700px]]

[[File:Response map class.png|700px]]

===Issue 6: Add more descriptive comments for the tests===

The existing tests need more descriptive comments. We plan to add more detailed comments for all the test cases.

[[File:Response tests 1.png|700px]]

[[File:Response tests 2.png|700px]]

[[File:Response tests 3.png|700px]]

==Changes made / Solutions==

===Issue 1===
The get_all_review_comments method has been moved to be an instance method of the assignment class and modified to only take the reviewee_id. However, the return value has not been modified. This is because upon further investigation the get_all_review_comments (previously called concatenate_all_review_comments) is only used within the volume_of_review_comments method and that modifying volume_of_review_comments to only get a single variable from get_all_review_comments proved to difficult. Unlike the avg_scores_and_count_for_prev_reviews, however, get_all_review_comments was not merged into get_all_review_comments due to its size and that doing so would make get_all_review_comments difficult to understand.

===Issue 2===
The avg_scores_and_count_for_prev_reviews method has been merged with the significant_difference? method.

def significant_difference?
map_class = map.class
# gets all responses made by a reviewee
existing_responses = map_class.assessments_for(map.reviewee)
#
count = 0
total = 0
# gets the sum total percentage scores of all responses that are not this response
existing_responses.each do |response|
unless id == response.id # the current_response is also in existing_responses array
count += 1
total += response.aggregate_questionnaire_score.to_f / response.maximum_score
end
end
#
# if this response is the only response by the reviewee, there's no grade conflict
return false if count.zero?
#
# calculates the average score of all other responses
average_score = total / count
#
# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
#
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
#
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score - score).abs * 100 > allowed_difference_percentage
end

===Issue 3 solved===
We have made the volume_of_review_comments method to be an instance method and moved it to the assignment.rb since it used to take a 'assignment_id' as an input argument. In this way, we are able to make it more sense and removed the ReviewCommentMixin.

# frozen_string_literal: true
module MetricHelper
# Determine the average size of review comments in each round
# the first entry in the returned list is the overall average
# word count.
def volume_of_review_comments(assignment_id, reviewer_id)
comments, counter,
@comments_in_round, @counter_in_round = Response.get_all_review_comments(assignment_id, reviewer_id)
# Index 0 is a nil element that can be ignored in the round count
num_rounds = @comments_in_round.count - 1

overall_avg_vol = (Lingua::EN::Readability.new(comments).num_words / (counter.zero? ? 1 : counter)).round(0)
review_comments_volume = []
review_comments_volume.push(overall_avg_vol)
(1..num_rounds).each do |round|
num = Lingua::EN::Readability.new(@comments_in_round[round]).num_words
den = (@counter_in_round[round].zero? ? 1 : @counter_in_round[round])
avg_vol_in_round = (num / den).round(0)
review_comments_volume.push(avg_vol_in_round)
end
review_comments_volume
end
end

===Issue 4 solved===
* 1) We have decided not to address this part of the issue (See the Issue 4) after discussing with the professor. The reason behind it is that the lines involved in this section is only 2 lines long and hence it does not have to be moved to a method.

* 2) We looked at a lot of solutions to make this. The issue is that using just a 'where' method will not ensure that the Active Record Association returned will be in the same order of that of the list of ids given.

For example:

ids = [4,1,2,7]
answers = Answer.where(id: ids)
return answers.map(&:id)

This example snippet would return 1,2,4,7

Hence, to ensure the order of the return values(thereby ensuring the correct working) and to improve the performance, we have used a small gem 'find_with_order'. The main reason why we decided to add this gem over a custom solution is portability / ease of modification. A custom solution would have required us to write SQL queries in where clauses and this would be tied to the mysql(current database used). This gem is compatible with popular databases and hence this would be automatically taken care of even if the database is changed.

Hence, our final solution is:

def calculate_total_score
# Only count the scorable questions, only when the answer is not nil (we accept nil as
# answer for scorable questions, and they will not be counted towards the total score)
sum = 0
question_ids = scores.map(&:question_id)
# We use find with order here to ensure that the list of questions we get is in the same order as that of question_ids
questions = Question.find_with_order(question_ids)
scores.each_with_index do |score, idx|
sum += score.answer * questions[idx].weight unless score.answer.nil? || !questions[idx].is_a?(ScoredQuestion)
end
sum
end

def maximum_score
# Only count the scorable questions, only when the answer is not nil (we accept nil as
# answer for scorable questions, and they will not be counted towards the total score)
total_weight = 0
question_ids = scores.map(&:question_id)
# We use find with order here to ensure that the list of questions we get is in the same order as that of question_ids
questions = Question.find_with_order(question_ids)
scores.each_with_index do |score, idx|
total_weight += questions[idx].weight unless score.answer.nil? || !questions[idx].is_a?(ScoredQuestion)
end
questionnaire = if scores.empty?
questionnaire_by_answer(nil)
else
questionnaire_by_answer(scores.first)
end
total_weight * questionnaire.max_question_score
end

* 3) Usage of visitor pattern to avoid checking class types: We solved this by adding a method '''response_assignment''' to ReviewResponseMap and ResponseMap classes.

In response_map.rb

# returns the assignment related to the response map
def response_assignment
return Participant.find(self.reviewer_id).assignment
end

In review_response_map.rb

# returns the assignment related to the review response map
def response_assignment
return self.assignment
end

Hence, the '''questionnaire_by_answer''' method in '''ScorableHelper''' will have the following snippet:

map = ResponseMap.find(map_id)
assignment = map.response_assignment

===Issue 5===
The get_all_responses method

==Design Principles==
This section aims to act as a consolidation of the design principles that we will implement as part of this project
* DRY: We will DRY the code that we handle as part of this project. An example of this would be the change that we make as part of the ScorableMixin (see Issue 4).

* Visitor Pattern: Having to check the class of an object and making calls based on that is an example of a bad design pattern. An example of this is ScorableMixin. We will adopt a loose implementation of the Visitor pattern to overcome this by adding a method in ResponseMap and ReviewResponseMap and just call that method in the object we receive. See Issue 4 section for a detailed explanation.

==Test Plan==
As part of this section, we track the set of files that are to be changed for tests and scenarios that are to be tested.
===Files===
Since this project is tied to the Response class, all the test cases will be added to response_spec.rb

* Issue 1,2,4: Modify tests in response_spec.rb
* Issue 3: Write tests for '''volume_of_review_comments''' method
===Scenarios===
* Test the score calculation cases - maximum possible total score for all scores, average score across all of the instances and total score (weighted sum). Ideally average score calculation should cover all the scenarios, that is, if the maximum score is zero, it uses the total score(weighted).
* Test comments for review - We also want to test the scenario that involves seeing the comments for a review.

==Future Work==
The issues pointed out in this documentation will be implemented once the design gets approved. Further changes will be updated in this section as we make progress with the project.

==Mentors==
* Prof. Edward F. Gehringer

==Contributors==
* Arvind Srinivas Subramanian (asubram9)
* David Brain (mdbrain2)
* Dhanya Sri Dasari (ddasari)
* Zhihao Wang (zwang238@ncsu.edu)

CSC/ECE 517 Spring 2023 - E2336. Reimplement response.rb

2023-04-08T01:00:53Z

Mdbrain2:

==Project Overview==
There are several issues we plan to address as part of this project.

* Refactor the get_all_review_comments method as an instance method in response.rb file
* Refactor the self.get_all_responses method as an instance method in response_map.rb file.
* Add more descriptive comments for the tests.

===Issue 1: Refactor the self.get_all_review_comments method===
Currently, the get_all_review_comments method is a class method that returns a data structure containing all the review comments for all responses. This violates the principles of good design as it creates a large data structure that the calling method needs to break apart, leading to poor performance and increased complexity.

[[File:get_all_review_comments.png|700px]]

To address this issue and come up with a much more elegant design, we plan to refactor as a new implementation that should convert the get_all_review_comments method into an instance method that takes a review object as an argument and returns the comments for that review only.

===Issue 2: Refactoring significant_difference?===
While the significant_difference method does not necessarily need to be refactored, it is the only method with a call for the avg_scores_and_count_for_prev_reviews method, which does need to be refactored it both is a class method and returns a data structure.

def self.avg_scores_and_count_for_prev_reviews(existing_responses, current_response)
scores_assigned = []
count = 0
existing_responses.each do |existing_response|
unless existing_response.id == current_response.id # the current_response is also in existing_responses array
count += 1
scores_assigned << existing_response.aggregate_questionnaire_score.to_f / existing_response.maximum_score
end
end
[scores_assigned.sum / scores_assigned.size.to_f, count]
end

Because of this we decided that we should simply refactor significant_difference instead and simply remove avg_scores_and_count_for_prev_reviews.

# compare the current response score with other scores on the same artifact, and test if the difference
# is significant enough to notify instructor.
# Precondition: the response object is associated with a ReviewResponseMap
### "map_class.assessments_for" method need to be refactored
def significant_difference?
map_class = map.class
existing_responses = map_class.assessments_for(map.reviewee)
average_score_on_same_artifact_from_others, count = Response.avg_scores_and_count_for_prev_reviews(existing_responses, self)
# if this response is the first on this artifact, there's no grade conflict
return false if count.zero?
# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score_on_same_artifact_from_others - score).abs * 100 > allowed_difference_percentage
end

===Issue 3: Make the method in ReviewCommentMixin to be an instance method ===
====Problems====
* The ReviewCommentMixin has one single method '''volume_of_review_comments''' and it is currently present as a class method. There is no need for it to be a class method and it should instead be an instance method.
* Volume of review comments is a metric, which belongs in metrics.rb, not in a mixin for responses.

====Solutions====
The change is quite straight forward. We should move this method to be an instance method in '''metrics.rb''' file. A side note is that, this method calls the '''get_all_review_comments''' method from '''response.rb'''. This method will be changed as part of this project (See above points). Any change to that function's method call must be reflected here too.

===Issue 4: Simplify + Refactor methods in ScorableMixin ===

This mixin has methods that are related to scoring. https://expertiza.csc.ncsu.edu/index.php/Scoring_%26_Grading_Methods_(Fall_%2721) is a good resource to understand how scoring and grading works.

====Problems====
* Not DRY : The following piece of code is used in 2 functions - calculate_total_score and maximum_score

scores.each do |s|
question = Question.find(s.question_id)
< Perform a calculation >
end

Basically, we iterate through the score and calculate something. The common calculation between both these functions is total_weight calculation.

'''What problem does this cause?''' If the logic to calculate the total_weight changes in the future, we will have to make changes to both these places. A developer who is relatively new to the project might miss out one changing it in a location. It is always good to have a single point of change by 'DRY'ing our code.

* Not optimized: Another change related to the above snippet has to do with how the queries are made and the number of queries.

scores.each do |s|
question = Question.find(s.question_id)
<Other lines irrelevant to this case >
end

'''What problem does this cause?''' This is done inside a loop, meaning we perform a select query to each time -> N queries for N objects. Our solution aims to optimize this.

* Checking the class type of an object to perform some business logic

This issue is present in the questionnaire_by_answer method. The following snippet shows it:

assignment = if map.is_a? ReviewResponseMap
map.assignment
else
Participant.find(map.reviewer_id).assignment
end

====Solutions====
* The common function would involve calculating the sum of weight.

def calculate_total_weight()
scores.each do |s|
question = Question.find(s.question_id)
total_weight += question.weight unless s.answer.nil? || !question.is_a?(ScoredQuestion)
end
And this can be called inside the calculate_total_score and maximum_score.

* Instead of calling Question.find inside the loop, we can get the set of question_ids and use a where function.

question_ids = scores.map(&:question_id)
questions = Question.where(question_id: question_ids)
< Make the calculation here >
This way, we only make one database call instead of N calls.

* To solve checking the object type, we can write a method in ResponseMap and ReviewResponseMap. This way, we can just call that method in the object and depending on what object it is, we would get the corresponding answer.
* In ReviewResponseMap, the method should return it's assignment
* In ResponseMap, the method should return Participant.find(map.reviewer_id).assignment

A visual representation of this change would look like this. The snippet uses an 'OR' condition to pick the appropriate assignment. Our change would remove the need for this check.

[[File:ScorableMixin3.png|700px]]

===Issue 5: Refactor the self.get_all_responses method===

The self.get_all_responses method which is currently in the response.rb file would be much cleaner as an instance method in response_map.rb. We plan to refactor the self.get_all_responses method in response.rb to an instance method in response_map.rb.

[[File:Get all responses.png|700px]]

[[File:Response map class.png|700px]]

===Issue 6: Add more descriptive comments for the tests===

The existing tests need more descriptive comments. We plan to add more detailed comments for all the test cases.

[[File:Response tests 1.png|700px]]

[[File:Response tests 2.png|700px]]

[[File:Response tests 3.png|700px]]

==Test Plan==
As part of this section, we track the set of files that are to be changed for tests and scenarios that are to be tested.
===Files===
* Issue 3: Write tests for '''volume_of_review_comments''' method
* Issue 4: Modify tests in response_spec.rb
===Scenarios===
* Test the score calculation cases - maximum possible total score for all scores, average score across all of the instances and total score (weighted sum). Ideally average score calculation should cover all the scenarios, that is, if the maximum score is zero, it uses the total score(weighted).

CSC/ECE 517 Spring 2023 - E2336. Reimplement response.rb

2023-04-08T00:55:33Z

Mdbrain2:

==Project Overview==
There are several issues we plan to address as part of this project.

* Refactor the get_all_review_comments method as an instance method in response.rb file
* Refactor the self.get_all_responses method as an instance method in response_map.rb file.
* Add more descriptive comments for the tests.

===Issue 1: Refactor the self.get_all_review_comments method===
Currently, the get_all_review_comments method is a class method that returns a data structure containing all the review comments for all responses. This violates the principles of good design as it creates a large data structure that the calling method needs to break apart, leading to poor performance and increased complexity.

[[File:get_all_review_comments.png|700px]]

To address this issue and come up with a much more elegant design, we plan to refactor as a new implementation that should convert the get_all_review_comments method into an instance method that takes a review object as an argument and returns the comments for that review only.

===Issue 2: Refactoring significant_difference?===
While the significant_difference method does not necessarily need to be refactored, it is the only method with a call for the avg_scores_and_count_for_prev_reviews method, which does need to be refactored it both is a class method and returns a data structure.

def self.avg_scores_and_count_for_prev_reviews(existing_responses, current_response)
scores_assigned = []
count = 0
existing_responses.each do |existing_response|
unless existing_response.id == current_response.id # the current_response is also in existing_responses array
count += 1
scores_assigned << existing_response.aggregate_questionnaire_score.to_f / existing_response.maximum_score
end
end
[scores_assigned.sum / scores_assigned.size.to_f, count]
end

Because of this we decided that we should simply refactor significant_difference instead and simply remove avg_scores_and_count_for_prev_reviews.

# compare the current response score with other scores on the same artifact, and test if the difference
# is significant enough to notify instructor.
# Precondition: the response object is associated with a ReviewResponseMap
### "map_class.assessments_for" method need to be refactored
def significant_difference?
map_class = map.class
existing_responses = map_class.assessments_for(map.reviewee)
average_score_on_same_artifact_from_others, count = Response.avg_scores_and_count_for_prev_reviews(existing_responses, self)
# if this response is the first on this artifact, there's no grade conflict
return false if count.zero?
# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score_on_same_artifact_from_others - score).abs * 100 > allowed_difference_percentage
end

===Issue 3: Make the method in ReviewCommentMixin to be an instance method ===
====Problems====
* The ReviewCommentMixin has one single method '''volume_of_review_comments''' and it is currently present as a class method. There is no need for it to be a class method and it should instead be an instance method.
* Volume of review comments is a metric, which belongs in metrics.rb, not in a mixin for responses.

====Solutions====
The change is quite straight forward. We should move this method to be an instance method in '''metrics.rb''' file. A side note is that, this method calls the '''get_all_review_comments''' method from '''response.rb'''. This method will be changed as part of this project (See above points). Any change to that function's method call must be reflected here too.

===Issue 4: Simplify + Refactor methods in ScorableMixin ===

This mixin has methods that are related to scoring. https://expertiza.csc.ncsu.edu/index.php/Scoring_%26_Grading_Methods_(Fall_%2721) is a good resource to understand how scoring and grading works.

====Problems====
* Not DRY : The following piece of code is used in 2 functions - calculate_total_score and maximum_score

```
scores.each do |s|
question = Question.find(s.question_id)
< Perform a calculation >
end
```
Basically, we iterate through the score and calculate something. The common calculation between both these functions is total_weight calculation.

'''What problem does this cause?''' If the logic to calculate the total_weight changes in the future, we will have to make changes to both these places. A developer who is relatively new to the project might miss out one changing it in a location. It is always good to have a single point of change by 'DRY'ing our code.

* Not optimized: Another change related to the above snippet has to do with how the queries are made and the number of queries.
```
scores.each do |s|
question = Question.find(s.question_id)
<Other lines irrelevant to this case >
end
```

'''What problem does this cause?''' This is done inside a loop, meaning we perform a select query to each time -> N queries for N objects. Our solution aims to optimize this.

* Checking the class type of an object to perform some business logic

This issue is present in the questionnaire_by_answer method. The following snippet shows it:

```
assignment = if map.is_a? ReviewResponseMap
map.assignment
else
Participant.find(map.reviewer_id).assignment
end
```

====Solutions====
* The common function would involve calculating the sum of weight.

```
def calculate_total_weight()
scores.each do |s|
question = Question.find(s.question_id)
total_weight += question.weight unless s.answer.nil? || !question.is_a?(ScoredQuestion)
end
```
And this can be called inside the calculate_total_score and maximum_score.

* Instead of calling Question.find inside the loop, we can get the set of question_ids and use a where function.

```
question_ids = scores.map(&:question_id)
questions = Question.where(question_id: question_ids)
< Make the calculation here >
```
This way, we only make one database call instead of N calls.

* To solve checking the object type, we can write a method in ResponseMap and ReviewResponseMap. This way, we can just call that method in the object and depending on what object it is, we would get the corresponding answer.
* In ReviewResponseMap, the method should return it's assignment
* In ResponseMap, the method should return Participant.find(map.reviewer_id).assignment

A visual representation of this change would look like this. The snippet uses an 'OR' condition to pick the appropriate assignment. Our change would remove the need for this check.

[[File:ScorableMixin3.png|700px]]

===Issue 5: Refactor the self.get_all_responses method===

The self.get_all_responses method which is currently in the response.rb file would be much cleaner as an instance method in response_map.rb. We plan to refactor the self.get_all_responses method in response.rb to an instance method in response_map.rb.

[[File:Get all responses.png|700px]]

[[File:Response map class.png|700px]]

===Issue 6: Add more descriptive comments for the tests===

The existing tests need more descriptive comments. We plan to add more detailed comments for all the test cases.

[[File:Response tests 1.png|700px]]

[[File:Response tests 2.png|700px]]

[[File:Response tests 3.png|700px]]

==Test Plan==
As part of this section, we track the set of files that are to be changed for tests and scenarios that are to be tested.
===Files===
* Issue 3: Write tests for '''volume_of_review_comments''' method
* Issue 4: Modify tests in response_spec.rb
===Scenarios===
* Test the score calculation cases - maximum possible total score for all scores, average score across all of the instances and total score (weighted sum). Ideally average score calculation should cover all the scenarios, that is, if the maximum score is zero, it uses the total score(weighted).

CSC/ECE 517 Spring 2023 - E2336. Reimplement response.rb

2023-04-08T00:55:06Z

Mdbrain2:

==Project Overview==
There are several issues we plan to address as part of this project.

* Refactor the get_all_review_comments method as an instance method in response.rb file
* Refactor the self.get_all_responses method as an instance method in response_map.rb file.
* Add more descriptive comments for the tests.

===Issue 1: Refactor the self.get_all_review_comments method===
Currently, the get_all_review_comments method is a class method that returns a data structure containing all the review comments for all responses. This violates the principles of good design as it creates a large data structure that the calling method needs to break apart, leading to poor performance and increased complexity.

[[File:get_all_review_comments.png|700px]]

To address this issue and come up with a much more elegant design, we plan to refactor as a new implementation that should convert the get_all_review_comments method into an instance method that takes a review object as an argument and returns the comments for that review only.

===Issue 2: Refactoring significant_difference?===
While the significant_difference method does not necessarily need to be refactored, it is the only method with a call for the avg_scores_and_count_for_prev_reviews method, which does need to be refactored it both is a class method and returns a data structure.

def self.avg_scores_and_count_for_prev_reviews(existing_responses, current_response)
scores_assigned = []
count = 0
existing_responses.each do |existing_response|
unless existing_response.id == current_response.id # the current_response is also in existing_responses array
count += 1
scores_assigned << existing_response.aggregate_questionnaire_score.to_f / existing_response.maximum_score
end
end
[scores_assigned.sum / scores_assigned.size.to_f, count]
end

Because of this we decided that we should simply refactor significant_difference instead and simply remove avg_scores_and_count_for_prev_reviews.

# compare the current response score with other scores on the same artifact, and test if the difference
# is significant enough to notify instructor.
# Precondition: the response object is associated with a ReviewResponseMap
### "map_class.assessments_for" method need to be refactored
def significant_difference?
map_class = map.class
existing_responses = map_class.assessments_for(map.reviewee)
average_score_on_same_artifact_from_others, count = Response.avg_scores_and_count_for_prev_reviews(existing_responses, self)
# if this response is the first on this artifact, there's no grade conflict
return false if count.zero?

# This score has already skipped the unfilled scorable question(s)
score = aggregate_questionnaire_score.to_f / maximum_score
questionnaire = questionnaire_by_answer(scores.first)
assignment = map.assignment
assignment_questionnaire = AssignmentQuestionnaire.find_by(assignment_id: assignment.id, questionnaire_id: questionnaire.id)
# notification_limit can be specified on 'Rubrics' tab on assignment edit page.
allowed_difference_percentage = assignment_questionnaire.notification_limit.to_f
# the range of average_score_on_same_artifact_from_others and score is [0,1]
# the range of allowed_difference_percentage is [0, 100]
(average_score_on_same_artifact_from_others - score).abs * 100 > allowed_difference_percentage
end

===Issue 3: Make the method in ReviewCommentMixin to be an instance method ===
====Problems====
* The ReviewCommentMixin has one single method '''volume_of_review_comments''' and it is currently present as a class method. There is no need for it to be a class method and it should instead be an instance method.
* Volume of review comments is a metric, which belongs in metrics.rb, not in a mixin for responses.

====Solutions====
The change is quite straight forward. We should move this method to be an instance method in '''metrics.rb''' file. A side note is that, this method calls the '''get_all_review_comments''' method from '''response.rb'''. This method will be changed as part of this project (See above points). Any change to that function's method call must be reflected here too.

===Issue 4: Simplify + Refactor methods in ScorableMixin ===

This mixin has methods that are related to scoring. https://expertiza.csc.ncsu.edu/index.php/Scoring_%26_Grading_Methods_(Fall_%2721) is a good resource to understand how scoring and grading works.

====Problems====
* Not DRY : The following piece of code is used in 2 functions - calculate_total_score and maximum_score

```
scores.each do |s|
question = Question.find(s.question_id)
< Perform a calculation >
end
```
Basically, we iterate through the score and calculate something. The common calculation between both these functions is total_weight calculation.

'''What problem does this cause?''' If the logic to calculate the total_weight changes in the future, we will have to make changes to both these places. A developer who is relatively new to the project might miss out one changing it in a location. It is always good to have a single point of change by 'DRY'ing our code.

* Not optimized: Another change related to the above snippet has to do with how the queries are made and the number of queries.
```
scores.each do |s|
question = Question.find(s.question_id)
<Other lines irrelevant to this case >
end
```

'''What problem does this cause?''' This is done inside a loop, meaning we perform a select query to each time -> N queries for N objects. Our solution aims to optimize this.

* Checking the class type of an object to perform some business logic

This issue is present in the questionnaire_by_answer method. The following snippet shows it:

```
assignment = if map.is_a? ReviewResponseMap
map.assignment
else
Participant.find(map.reviewer_id).assignment
end
```

====Solutions====
* The common function would involve calculating the sum of weight.

```
def calculate_total_weight()
scores.each do |s|
question = Question.find(s.question_id)
total_weight += question.weight unless s.answer.nil? || !question.is_a?(ScoredQuestion)
end
```
And this can be called inside the calculate_total_score and maximum_score.

* Instead of calling Question.find inside the loop, we can get the set of question_ids and use a where function.

```
question_ids = scores.map(&:question_id)
questions = Question.where(question_id: question_ids)
< Make the calculation here >
```
This way, we only make one database call instead of N calls.

* To solve checking the object type, we can write a method in ResponseMap and ReviewResponseMap. This way, we can just call that method in the object and depending on what object it is, we would get the corresponding answer.
* In ReviewResponseMap, the method should return it's assignment
* In ResponseMap, the method should return Participant.find(map.reviewer_id).assignment

A visual representation of this change would look like this. The snippet uses an 'OR' condition to pick the appropriate assignment. Our change would remove the need for this check.

[[File:ScorableMixin3.png|700px]]

===Issue 5: Refactor the self.get_all_responses method===

The self.get_all_responses method which is currently in the response.rb file would be much cleaner as an instance method in response_map.rb. We plan to refactor the self.get_all_responses method in response.rb to an instance method in response_map.rb.

[[File:Get all responses.png|700px]]

[[File:Response map class.png|700px]]

===Issue 6: Add more descriptive comments for the tests===

The existing tests need more descriptive comments. We plan to add more detailed comments for all the test cases.

[[File:Response tests 1.png|700px]]

[[File:Response tests 2.png|700px]]

[[File:Response tests 3.png|700px]]

==Test Plan==
As part of this section, we track the set of files that are to be changed for tests and scenarios that are to be tested.
===Files===
* Issue 3: Write tests for '''volume_of_review_comments''' method
* Issue 4: Modify tests in response_spec.rb
===Scenarios===
* Test the score calculation cases - maximum possible total score for all scores, average score across all of the instances and total score (weighted sum). Ideally average score calculation should cover all the scenarios, that is, if the maximum score is zero, it uses the total score(weighted).

CSC/ECE 517 Spring 2023 - E2336. Reimplement response.rb

2023-04-08T00:46:10Z

Mdbrain2:

==Project Overview==
There are several issues we plan to address as part of this project.

* Refactor the get_all_review_comments method as an instance method in response.rb file
* Refactor the self.get_all_responses method as an instance method in response_map.rb file.
* Add more descriptive comments for the tests.

===Issue 1: Refactor the self.get_all_review_comments method===
Currently, the get_all_review_comments method is a class method that returns a data structure containing all the review comments for all responses. This violates the principles of good design as it creates a large data structure that the calling method needs to break apart, leading to poor performance and increased complexity.

[[File:get_all_review_comments.png|700px]]

To address this issue and come up with a much more elegant design, we plan to refactor as a new implementation that should convert the get_all_review_comments method into an instance method that takes a review object as an argument and returns the comments for that review only.

===Issue 2: Refactoring significant_difference?===
While the significant_difference method does not necessarily need to be refactored, it is the only method with a call for the avg_scores_and_count_for_prev_reviews method, which does need to be refactored it both is a class method and returns a data structure.

Because of this we decided that we should simply refactor significant_difference instead and simply remove avg_scores_and_count_for_prev_reviews.

===Issue 3: Make the method in ReviewCommentMixin to be an instance method ===
====Problems====
* The ReviewCommentMixin has one single method '''volume_of_review_comments''' and it is currently present as a class method. There is no need for it to be a class method and it should instead be an instance method.
* Volume of review comments is a metric, which belongs in metrics.rb, not in a mixin for responses.

====Solutions====
The change is quite straight forward. We should move this method to be an instance method in '''metrics.rb''' file. A side note is that, this method calls the '''get_all_review_comments''' method from '''response.rb'''. This method will be changed as part of this project (See above points). Any change to that function's method call must be reflected here too.

===Issue 4: Simplify + Refactor methods in ScorableMixin ===

This mixin has methods that are related to scoring. https://expertiza.csc.ncsu.edu/index.php/Scoring_%26_Grading_Methods_(Fall_%2721) is a good resource to understand how scoring and grading works.

====Problems====
* Not DRY : The following piece of code is used in 2 functions - calculate_total_score and maximum_score

```
scores.each do |s|
question = Question.find(s.question_id)
< Perform a calculation >
end
```
Basically, we iterate through the score and calculate something. The common calculation between both these functions is total_weight calculation.

'''What problem does this cause?''' If the logic to calculate the total_weight changes in the future, we will have to make changes to both these places. A developer who is relatively new to the project might miss out one changing it in a location. It is always good to have a single point of change by 'DRY'ing our code.

* Not optimized: Another change related to the above snippet has to do with how the queries are made and the number of queries.
```
scores.each do |s|
question = Question.find(s.question_id)
<Other lines irrelevant to this case >
end
```

'''What problem does this cause?''' This is done inside a loop, meaning we perform a select query to each time -> N queries for N objects. Our solution aims to optimize this.

* Checking the class type of an object to perform some business logic

This issue is present in the questionnaire_by_answer method. The following snippet shows it:

```
assignment = if map.is_a? ReviewResponseMap
map.assignment
else
Participant.find(map.reviewer_id).assignment
end
```

====Solutions====
* The common function would involve calculating the sum of weight.

```
def calculate_total_weight()
scores.each do |s|
question = Question.find(s.question_id)
total_weight += question.weight unless s.answer.nil? || !question.is_a?(ScoredQuestion)
end
```
And this can be called inside the calculate_total_score and maximum_score.

* Instead of calling Question.find inside the loop, we can get the set of question_ids and use a where function.

```
question_ids = scores.map(&:question_id)
questions = Question.where(question_id: question_ids)
< Make the calculation here >
```
This way, we only make one database call instead of N calls.

* To solve checking the object type, we can write a method in ResponseMap and ReviewResponseMap. This way, we can just call that method in the object and depending on what object it is, we would get the corresponding answer.
* In ReviewResponseMap, the method should return it's assignment
* In ResponseMap, the method should return Participant.find(map.reviewer_id).assignment

[[File:ScorableMixin2.png|700px]]

===Issue 5: Refactor the self.get_all_responses method===

The self.get_all_responses method which is currently in the response.rb file would be much cleaner as an instance method in response_map.rb. We plan to refactor the self.get_all_responses method in response.rb to an instance method in response_map.rb.

[[File:Get all responses.png|700px]]

[[File:Response map class.png|700px]]

===Issue 6: Add more descriptive comments for the tests===

The existing tests need more descriptive comments. We plan to add more detailed comments for all the test cases.

[[File:Response tests 1.png|700px]]

[[File:Response tests 2.png|700px]]

[[File:Response tests 3.png|700px]]

==Test Plan==
As part of this section, we track the set of files that are to be changed for tests and scenarios that are to be tested.
===Files===
* Issue 3: Write tests for '''volume_of_review_comments''' method
* Issue 4: Modify tests in response_spec.rb
===Scenarios===
* Test the score calculation cases - maximum possible total score for all scores, average score across all of the instances and total score (weighted sum). Ideally average score calculation should cover all the scenarios, that is, if the maximum score is zero, it uses the total score(weighted).

CSC/ECE 517 Spring 2023 - E2312 + E2313. Reimplement response.rb and responses controller.rb

2023-03-28T03:06:44Z

Mdbrain2:

In order to accommodate issues with another team, our team picked up another member and did both projects. The summaries of the projects are combined here to reflect that. Note that the requirements on the original projects are being treated more loosely to account for the additional work.

= E2312 Reimplement Response =

== Summary of Changes ==

The previous version of response.rb was described as "a mess." The goal of E2312 was to reimplement it to follow better coding standards.

* Made method names clearer, many method names were opaque and did not properly convey their functionality. For example, add_table_rows:
[[File:add_table_rows.png]]
* Moved many functionalities to mixins or helper classes to not violate the Single-Responsibility Principle (also many comments with no useful information)
[[File:scores_response.png]]
* Split the avg_scores_and_count_for_prev_reviews method into two separate methods
* Implemented additional functionally

== Functionality Moved ==

This section covers the functionality that was moved out of the response model.

=== Score Calculation ===

Score calculation is not inherently tied to the idea of a response.
Any object with multiple scores could take advantage of a mixin that provided methods to do calculations on those scores. This created multiple responsibilities for the response object and encouraged non-DRY code.
Functionality for calculating total, average, and maximum scores were all moved to a new Scorable mixin which can be leveraged by other models moving forward.

Previously that functionality was implemented directly on the response class:

[[File:aggregate_score.png]]

Now it exists in a mixin, as demonstrated with this snippet:

[[File:scorable_mixin.png]]

=== Emails ===

The response model provided a method for creating emails. This was moved to a new MailMixin, which can be expanded to provide email functionality to more models in the future. This removed additional responsibilities to help maintain the Single Responsibility principle and helps move email-based code toward a more DRY implementation in the future.

[[File:mail_mixin.png]]

=== Metrics ===

The response model provided a method for getting the volume of review comments on a response. This was moved to a ReviewCommentMixin as it is not necessarily specific to response models. This helps maintain the Single-Responsibility Principle for the response model.

[[File:review_comment.png]]

== Improved Naming ==

Fix instances of bad and opaque naming, like aggregate_question_score(), add_table_row(), and concatenate_all_review_comments() (which does more than the name implies).

== Method Split ==

The method avg_scores_and_count_for_prev_reviews was split into two separate functions, prev_reviews_count and prev_reviews_avg_scores.

== Additional Functionality ==

Two methods were added, get_latest_response and get_all_responses. These two methods both get responses made by a particular reviewer for a particular reviewee for an assignment. Get_latest_response returns the latest of the responses and get_all_responses returns all of the responses.

== Test Plan ==

Many other parts of the software were stubbed out in the implementation project in order to run unit tests. The existing test suite was run against the implementation and passed successfully. Due to the fact that this project has no front end, the professor asked us to record a video of tests being run to demonstrate functionality, which is linked in the project submission.

Test cases:

* Total score of a review
* Average scores across a review when the maximum score is 0
* Average scores across a review when the maximum score is not 0
* Maximum scores for a response when there are no points available
* Maximum scores for a response standard case
* Volume of review comments
* Concatenating all review comments

= E2313 Reimplement Response Controller =

== Summary of Changes ==

The previous version of response_controller.rb was too long. It included too many functions besides the basic CRUD methods. The goal of E2313 was to reimplement it to follow better coding standards.

* Made method name to follow plural convention not singular convention.
* Moved many functionalities to mixins or helper classes

== Functionality Moved ==

This section covers the functionality that was moved out of the responses controller.

=== Authentication and Authorization ===

There were authentication and authorization methods in the response controller. These were moved to authorization helper.

=== Redirect ===

There was a redirect function that helps redirect by params. It is moved to response helper.

=== Sort Reviews ===

There was functionality to sort reviews. We made a new function named sortReviews to perform this functionality and placed it in the response model

== Singular convention to Plural convention ==

There were some function and classes that followed singular conventions, but were changed to follow plural convention.

[[File:plural.png]]

[[File:authorization_helper.png]]

== Reducing code ==

There were multiple method invocations to set parameters (assign_action_parameters, set_content). We used the macro, before_action, so that we reduced the number of calls for these methods.

[[File:macro.png]]

== Testing ==

1. Response Deletion

- Verify that if the response is enabled for team reviewing, but lock is not existent, proceed to lock response
- Verify that if the response is not team review enabled, proceed to delete the response and redirect the user

2. Response Edit

- Verify preparing already existent response for editing purposes
- Verify creating new response if non-existent for editing purposes

3. Response Update

- Verify submitted response contains the new updated fields upon submission

4. Response New Feedback

- Redirect user to the new feedback page with relevant Response, Participant, and FeedbackResponseMap data
- If a review is not found, redirect user to the root path

5. Response Create

- If Response is found by query parameter, update with additional comments and instantiate answers
- If Response is not found by query parameter, create new Response instance with additional comments and instantiate answers

6. Response Save

- With the found ResponseMap, save with existing query parameters

7. Initialize Answers

- Verify answers are created from a passed-in list of questions

8. Questionnaire from Response Map

- Verify if current map type is ReviewResponseMap, set current round and questionnaire accordingly
- Verify if current map type is SelfReviewResponseMap, set current round and questionnaire accordingly
- Verify if current map type is MetareviewResponseMap, set questionnaire based on if assignment is duty based
- Verify if current map type is TeammateReviewResponseMap, set questionnaire based on if assignment is duty based
- Verify if current map type is FeedbackResponseMap, set questionnaire based on if assignment is duty based
- Verify if current map type is CourseSurveyResponseMap, set questionnaire based on if assignment is duty based
- Verify if current map type is AssignmentSurveyResponseMap, set questionnaire based on if assignment is duty based
- Verify if current map type is GlobalSurveyResponseMap, set questionnaire based on if assignment is duty based
- Verify if current map type is BookmarkRatingResponseMap, set questionnaire based on if assignment is duty based

9. Assign Action Parameters

- Verify when action is edit, header and next_action are assigned as "Edit" and "update" respectively
- Verify when action is new, header and next_action are assigned as "New" and "create" respectively

10. Setting Response

- Verify response and map instances are set based on the query parameter passed in

11. Toggling permission

- Verify a given response is found and its visibility attribute is toggled to its corresponding opposite value

12. Show Calibration Results for Student

- Verify with the given query parameters: assignment_id, calibration_response_map_id, review_response_map_id, that the assignment, calibration response, review response and questions instances are set correctly

13. Previous Review Count

- Verify that when given a list of reviews and the current review that the number of non current reviews is accurate

14. Previous Review Average Score

- Verify that when given a list of reviews and the current review that the average score of all non current reviews is accurate

15. Gets Latest Response by a Reviewer

- Verify that the latest response by a reviewer given the assignment, reviewer, and reviewee is accurate

16. Gets All Responses by a Reviewer

- Verify that all responses by a reviewer given the assignment, reviewer, and reviewee is accurate

[[File:Screenshot 2023-03-27 at 10.40.26 PM.png]]

[[File:Answers.png]]

CSC/ECE 517 Spring 2023 - E2312 + E2313. Reimplement response.rb and responses controller.rb

2023-03-28T02:57:47Z

Mdbrain2:

In order to accommodate issues with another team, our team picked up another member and did both projects. The summaries of the projects are combined here to reflect that. Note that the requirements on the original projects are being treated more loosely to account for the additional work.

= E2312 Reimplement Response =

== Summary of Changes ==

The previous version of response.rb was described as "a mess." The goal of E2312 was to reimplement it to follow better coding standards.

* Made method names clearer, many method names were opaque and did not properly convey their functionality. For example, add_table_rows:
[[File:add_table_rows.png]]
* Moved many functionalities to mixins or helper classes to not violate the Single-Responsibility Principle (also many comments with no useful information)
[[File:scores_response.png]]
* Reduced usage of class methods

== Functionality Moved ==

This section covers the functionality that was moved out of the response model.

=== Score Calculation ===

Score calculation is not inherently tied to the idea of a response.
Any object with multiple scores could take advantage of a mixin that provided methods to do calculations on those scores. This created multiple responsibilities for the response object and encouraged non-DRY code.
Functionality for calculating total, average, and maximum scores were all moved to a new Scorable mixin which can be leveraged by other models moving forward.

Previously that functionality was implemented directly on the response class:

[[File:aggregate_score.png]]

Now it exists in a mixin, as demonstrated with this snippet:

[[File:scorable_mixin.png]]

=== Emails ===

The response model provided a method for creating emails. This was moved to a new MailMixin, which can be expanded to provide email functionality to more models in the future. This removed additional responsibilities to help maintain the Single Responsibility principle and helps move email-based code toward a more DRY implementation in the future.

[[File:mail_mixin.png]]

=== Metrics ===

The response model provided a method for getting the volume of review comments on a response. This was moved to a ReviewCommentMixin as it is not necessarily specific to response models. This helps maintain the Single-Responsibility Principle for the response model.

[[File:review_comment.png]]

== Improved Naming ==

Fix instances of bad and opaque naming, like aggregate_question_score(), add_table_row(), and concatenate_all_review_comments() (which does more than the name implies).

== Test Plan ==

Many other parts of the software were stubbed out in the implementation project in order to run unit tests. The existing test suite was run against the implementation and passed successfully. Due to the fact that this project has no front end, the professor asked us to record a video of tests being run to demonstrate functionality, which is linked in the project submission.

Test cases:

* Total score of a review
* Average scores across a review when the maximum score is 0
* Average scores across a review when the maximum score is not 0
* Maximum scores for a response when there are no points available
* Maximum scores for a response standard case
* Volume of review comments
* Concatenating all review comments

= E2313 Reimplement Response Controller =

== Summary of Changes ==

The previous version of response_controller.rb was too long. It included too many functions besides the basic CRUD methods. The goal of E2313 was to reimplement it to follow better coding standards.

* Made method name to follow plural convention not singular convention.
* Moved many functionalities to mixins or helper classes

== Functionality Moved ==

This section covers the functionality that was moved out of the responses controller.

=== Authentication and Authorization ===

There were authentication and authorization methods in the response controller. These were moved to authorization helper.

=== Redirect ===

There was a redirect function that helps redirect by params. It is moved to response helper.

=== Sort Reviews ===

There was functionality to sort reviews. We made a new function named sortReviews to perform this functionality and placed it in the response model

== Singular convention to Plural convention ==

There were some function and classes that followed singular conventions, but were changed to follow plural convention.

[[File:plural.png]]

[[File:authorization_helper.png]]

== Reducing code ==

There were multiple method invocations to set parameters (assign_action_parameters, set_content). We used the macro, before_action, so that we reduced the number of calls for these methods.

[[File:macro.png]]

== Testing ==

1. Response Deletion

- Verify that if the response is enabled for team reviewing, but lock is not existent, proceed to lock response
- Verify that if the response is not team review enabled, proceed to delete the response and redirect the user

2. Response Edit

- Verify preparing already existent response for editing purposes
- Verify creating new response if non-existent for editing purposes

3. Response Update

- Verify submitted response contains the new updated fields upon submission

4. Response New Feedback

- Redirect user to the new feedback page with relevant Response, Participant, and FeedbackResponseMap data
- If a review is not found, redirect user to the root path

5. Response Create

- If Response is found by query parameter, update with additional comments and instantiate answers
- If Response is not found by query parameter, create new Response instance with additional comments and instantiate answers

6. Response Save

- With the found ResponseMap, save with existing query parameters

7. Initialize Answers

- Verify answers are created from a passed-in list of questions

8. Questionnaire from Response Map

- Verify if current map type is ReviewResponseMap, set current round and questionnaire accordingly
- Verify if current map type is SelfReviewResponseMap, set current round and questionnaire accordingly
- Verify if current map type is MetareviewResponseMap, set questionnaire based on if assignment is duty based
- Verify if current map type is TeammateReviewResponseMap, set questionnaire based on if assignment is duty based
- Verify if current map type is FeedbackResponseMap, set questionnaire based on if assignment is duty based
- Verify if current map type is CourseSurveyResponseMap, set questionnaire based on if assignment is duty based
- Verify if current map type is AssignmentSurveyResponseMap, set questionnaire based on if assignment is duty based
- Verify if current map type is GlobalSurveyResponseMap, set questionnaire based on if assignment is duty based
- Verify if current map type is BookmarkRatingResponseMap, set questionnaire based on if assignment is duty based

9. Assign Action Parameters

- Verify when action is edit, header and next_action are assigned as "Edit" and "update" respectively
- Verify when action is new, header and next_action are assigned as "New" and "create" respectively

10. Setting Response

- Verify response and map instances are set based on the query parameter passed in

11. Toggling permission

- Verify a given response is found and its visibility attribute is toggled to its corresponding opposite value

12. Show Calibration Results for Student

- Verify with the given query parameters: assignment_id, calibration_response_map_id, review_response_map_id, that the assignment, calibration response, review response and questions instances are set correctly

13. Previous Review Count

- Verify that when given a list of reviews and the current review that the number of non current reviews is accurate

14. Previous Review Average Score

- Verify that when given a list of reviews and the current review that the average score of all non current reviews is accurate

15. Gets Latest Response by a Reviewer

- Verify that the latest response by a reviewer given the assignment, reviewer, and reviewee is accurate

16. Gets All Responses by a Reviewer

- Verify that all responses by a reviewer given the assignment, reviewer, and reviewee is accurate

[[File:Screenshot 2023-03-27 at 10.40.26 PM.png]]

[[File:Answers.png]]