Expertiza_Wiki - User contributions [en]

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

2023-04-26T03:48:58Z

Asubram9:

==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

* Check the beginning of the test plan section on how to test the functionality.

==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 belongs in assignment.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 '''assignment.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.

'''Update for final submission:''' We have decided not to do this since the repeated code was only of 2 lines and that is too small to be considered not DRY.

* 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.

'''Update for final submission:''' We have decided not to do this since the repeated code was only of 2 lines and that is too small to be considered not DRY.

* 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.

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

===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==
Our is a refactor project for models. Hence, to test the functionality, we have ensured to cover existing test cases and add new test cases for any new code we have added.

To run rspec for this project,

rspec spec/models/response_spec.rb

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-26T01:23:07Z

Asubram9:

==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

* Check the beginning of the test plan section on how to test the functionality.

==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 belongs in assignment.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 '''assignment.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.

'''Update for final submission:''' We have decided not to do this since the repeated code was only of 2 lines and that is too small to be considered not DRY.

* 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.

'''Update for final submission:''' We have decided not to do this since the repeated code was only of 2 lines and that is too small to be considered not DRY.

* 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==
Our is a refactor project for models. Hence, to test the functionality, we have ensured to cover existing test cases and add new test cases for any new code we have added.

To run rspec for this project,

rspec spec/models/response_spec.rb

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-26T01:16:28Z

Asubram9:

==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 belongs in assignment.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 '''assignment.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.

'''Update for final submission:''' We have decided not to do this since the repeated code was only of 2 lines and that is too small to be considered not DRY.

* 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.

'''Update for final submission:''' We have decided not to do this since the repeated code was only of 2 lines and that is too small to be considered not DRY.

* 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-25T18:40:25Z

Asubram9:

==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 3 solved===
We have made the volume_of_review_comments method to be an instance method. We have also created a module called MetricHelper to make 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

==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-25T18:39:34Z

Asubram9: Note

==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 3:===
We have made the volume_of_review_comments method to be an instance method. We have also created a module called MetricHelper to make 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

==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-25T18:34:34Z

Asubram9:

==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.

==Update for resubmission==
We have verified the design doc with the professor and made a few changes:
* Added function name for issue 4
* Fixed typos
* Verified the solutions and visual aid

==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 3:===
We have made the volume_of_review_comments method to be an instance method. We have also created a module called MetricHelper to make 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

==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-25T18:33:34Z

Asubram9: Issue 3 and 4 solutions updated

==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.

==Update for resubmission==
We have verified the design doc with the professor and made a few changes:
* Added function name for issue 4
* Fixed typos
* Verified the solutions and visual aid

==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 3:==
We have made the volume_of_review_comments method to be an instance method. We have also created a module called MetricHelper to make 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

==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-25T18:18:48Z

Asubram9:

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

2023-04-13T03:46:10Z

Asubram9:

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

2023-04-13T00:13:11Z

Asubram9: Update - Method name added

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

2023-04-08T01:30:18Z

Asubram9:

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

2023-04-08T01:26:54Z

Asubram9:

CSC/ECE 517 Spring 2023

2023-04-08T01:26:00Z

Asubram9:

== OSS Projects ==
[[CSC/ECE 517 Spring_2023 -E2306. Refactor user_controller.rb, user.rb and its child classes]]

[[CSC/ECE 517 Spring_2023 - E2320. Reimplement the Question hierarchy]]

[[CSC/ECE 517 Spring_2023 - E2312 + E2313. Reimplement response.rb and responses_controller.rb]]

[[CSC/ECE 517 Spring_2023 - NTNX-1. Support provisioning MongoDb via NDB Kubernetes Operator]]

[[CSC/ECE 517 Spring_2023 - E2316. Reimplement sign_up_sheet_controller.rb]]

[[CSC/ECE 517 Spring_2023 - E2315. Reimplement signed_up_team.rb, sign_up_topic.rb, sign_up_sheet.rb]]

[[CSC/ECE 517 Spring_2023 - E2340. Reimplement signed_up_team.rb, sign_up_topic.rb, sign_up_sheet.rb (Project 4)]]

[[CSC/ECE 517 Spring_2023 - E2323. Refactor DueDate functionality from assignment.rb]]

[[CSC/ECE 517 Spring 2023 - E2317: Reimplement participant.rb]]

[[CSC/ECE 517 Spring 2023 - E2314. Reimplement the response map hierarchy]]

[[CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws]]

[[CSC/ECE 517 Spring 2023 - E2321. Reimplement QuestionnairesController and QuestionsController]]

[[CSC/ECE 517 Spring 2023 - E2305. Grading audit trail]]

[[CSC/ECE 517 Spring 2023 - E2329. Grading audit trail(Project 4)]]

[[CSC/ECE 517 Spring 2023 - E2309. Refactor Node model and its subclasses]]

[[CSC/ECE 517 Spring 2023 - NTNX-2. Support provisioning mySQL databases via NDB Kubernetes Operator]]

[[CSC/ECE 517 Spring 2023 - E2300. Refactor E1858. Github metrics integration]]

[[CSC/ECE 517 Sping 2023 - E2322: Refactor Questionnaire View to display Bookmark Rating]]

[[CSC/ECE 517 Spring 2023 - NTNX-3. Refactor models to keep profiles (software, compute, network, etc) as optional and use default if not specified]]

[[CSC/ECE_517_Spring_2023_-_E2301._Refactor_review_maping_helper]]

[[CSC/ECE_517_Spring_2023_-_E2337._Reimplement_node_hierarchy]]

[[CSC/ECE_517_Spring_2023_-E2339._Reimplement signed_up_teams_controller and sign_up_topics_controller.rb]]

[[CSC/ECE_517_Spring_2023_-_E2320._Reimplement the Question Hierarchy]]

[[CSC/ECE_517_Spring_2023 - E2341. UI for Participants.rb]]

[[CSC/ECE_517_Spring_2023_-_E2336._Reimplement_response.rb]]

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

2023-04-08T01:23:12Z

Asubram9:

==Project Overview==
This project is a continuation to E2312. The current project is to make design changes to the Question class, 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.

==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
* 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]]

==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===
* 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:14:57Z

Asubram9:

==Project Overview==
This project is a continuation to E2312. The current project is to make design changes to the Question class, 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.

==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
* 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]]

==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===
* 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).

==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-08T00:50:51Z

Asubram9:

==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

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).

File:ScorableMixin4.png

2023-04-08T00:50:01Z

Asubram9:

File:ScorableMixin3.png

2023-04-08T00:47:02Z

Asubram9:

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

2023-04-08T00:46:50Z

Asubram9:

==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

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:32:02Z

Asubram9:

==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 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).

File:ScorableMixin2.png

2023-04-08T00:30:19Z

Asubram9:

File:ScorableMixin.png

2023-04-08T00:27:32Z

Asubram9:

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

2023-04-07T23:56:28Z

Asubram9:

==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 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

===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-07T23:53:03Z

Asubram9:

==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 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.
```
question = Question.find(s.question_id)
```

'''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

===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-07T23:47:18Z

Asubram9:

==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 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.
```
question = Question.find(s.question_id)
```

'''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

===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]]

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

2023-04-07T23:46:36Z

Asubram9: Mixin issues

==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 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.

====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:====
* 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.
```
question = Question.find(s.question_id)
```

'''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

===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]]

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

2023-04-07T00:15:48Z

Asubram9: Page creation

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-28T03:54:16Z

Asubram9:

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==

==Note to the reviewer==
Design Doc Link: https://docs.google.com/document/d/1Drglvj0b487qIACeIW1jYG83uRtJtl7fIchM6B1H9Bk/edit?usp=sharing
This is the document that we are actively using. All the details in the design document are already written here in a more clear way.

A major part of the logic can only be written after we get access to the keys of an EC2 instance that we can connect to the Nutanix Database Service. This differs from other NTNX teams since we need to provision the database on AWS. Hence, we cannot use the Test Nutanix endpoint to create clusterID or Secret's name. We have added a '''Note''' section in the Implementation section to inform the parts which fall under this category. We have also added this information collectively under the '''Future Implementation''' section.

'''UPDATE: We have now received access and have added as much code as possible within the stipulated time. We have added a 'Resubmission progress' section to inform you about the changes we have done now. We have also updated the Future Implementation section accordingly.'''

==Resubmission progress==
* We have received the access now, and have added a bit of code after that. We have analyzed the JSON request that is needed to be sent to provision a database and created a struct of that type in database_types.go.
* We have stuck to the DRY principle of re-using an existing variable's fields by inheriting from it. CloudDatabaseProvisionRequest inherits from DatabaseProvisionRequest.
* Added logic to create a CloudDatabaseProvisionRequest object if the remoteType is 'cloud' - This is because the request to this requires extra fields
* We have also now added the remoteType field to be passed as a variable for it to be available in the controller helper logic.
* We fixed the test cases after adding this logic.
* We have added a google drive link with a video of the tests running.
* Updated the implementation section with what is left.

==Problem Statement==
Nutanix Database Service is the only hybrid cloud database-as-a-service for Microsoft SQL
Server, Oracle Database, PostgreSQL, MongoDB, and MySQL. Currently, using the NDB operator, you can only provision a database on the Nutanix Cloud Infrastructure(on-prem).This project aims to extend the Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on cloud - specifically on AWS EC2. We aim to do this in a scalable way so that changing the cloud provider to say Azure or GCP would require changes only to the configuration file(CRD Manifest).

==Architecture==
The important components here are:
# '''Kubernetes Cluster'''
# '''Kubernetes Service''' that almost acts like a gateway to the provisioned database - The application pods that want to communicate with the database do so through the K8 service and endpoint that is created as part of the last step of provisioning. The workflow is explained in detail in the next section.
# '''Nutanix Database Service''' - The operator pod running in the K8 cluster communicates with this service to provision, de-provision, obtain status of the database etc.
# '''Cloud platform''' (AWS in our case)

[[File:Ntnx4arch.png|1000px|]]

==Workflow==
* Apply the instance CRD manifest file to create an instance of NDB resource in the K8 cluster

* The operator controller runs its logic to build a provisioning request and sends it to the Nutanix Database Service endpoint pertaining to provision in cloud(AWS in our case)

* The reconcile function loops to take actions depending on the status of provisioning - Empty, Provisioning and Ready

* Once the database is provisioned in AWS and the status is in Ready, we set up a Kubernetes networking service and an endpoint with the same name in the cluster. Any application pod in this cluster will communicate with this endpoint rather than communicating directly with the database.

[[File:Workflowntnx4.png|1000px|]]

==Design==
===Single Responsibility Principle(SRP)===
SRP is adopted here to separate the business logic of the operator from the reconciliation loop.

* The logic to handle '''remoteType''' : cloud is handled separately from the client (ndbClient) through helper methods - This ensures that the client is responsible for only making get/post requests to the specified endpoint without worrying about whether it is for on-prem or cloud.

* The reconciliation loop is responsible for ensuring that the state of the system matches the desired state declared by the operator. It listens to events from the Kubernetes API server and performs the necessary actions to reconcile the system to the desired state. The business logic of the operator, on the other hand, is responsible for implementing the higher-level functionality of the operator, such as managing the deployment of a complex application or managing a database cluster.

* Even within the reconciliation logic, there are clear indications of following SRP. We have a separate file utils/secret.go to implement GetDataFromSecret and GetAllDataFromSecret - logic to get data from the resource denoted by the name/namespace combination.

===Operator Pattern===
The operator pattern is based on the idea of declarative configuration management. Rather than writing imperative code to perform a series of steps to reach a desired state, an operator declares the desired state of the system and uses the Kubernetes API to perform the necessary actions to reconcile the system to that desired state.

We continue to follow this pattern for our implementation. We have a manifest file for a NDB custom resource responsible for provisioning and managing PostgreSQL in AWS and the logic for it is handled in the controller code (controllers/database_controllers.go)

EXAMPLE MANIFEST FILE
...

apiVersion: ndb.nutanix.com/v1alpha1
kind: Database
metadata:
name: db
spec:
ndb:
remoteType: "cloud"
clusterId: "Nutanix Cluster Id"
credentialSecret : ndb-secret-name
server: https://[NDB IP]:8443/era/v1.0
skipCertificateVerification: true
databaseInstance:
databaseInstanceName: "Database Instance Name"
databaseNames:
- database_one
- database_two
- database_three
credentialSecret: db-instance-secret-name
size: 10
timezone: "UTC"
type: postgres
...

The controller logic and related helper code will use the newly created ''''remoteType'''' field to select the NDS endpoint and construct provisionRequest appropriately. We have distinguished the configuration(Manifest file) and the logic(controller code) this way.

==Implementation related to this project==

This section will explain the changes that are made/to-be-made as part of this project.

'''NOTE: WE HAVE NOW RECEIVED ACCESS TO THE INSTANCE AND HAVE MADE AS MANY CHANGES AS POSSIBLE WITHIN THIS TIME. HAVE A LOOK AT THE RESUBMISSION PROGRESS SECTION FOR THE UPDATE'''
'''We will tell the changes that have not been made and they will be made in the future'''

===Generate Secrets manifest and set name field + credentials in it===
This secrets.yaml manifest file is to create a Secret resource instance that is will have the data regarding the NDB instance name and database credentials.

===Modify database_types.go to have allow remote types===
We have added a '''remoteType''' field to the NDB struct along with validation markers
...
// +kubebuilder:validation:Required;Enum=cloud;on-prem
RemoteType string `json:"remoteType"`

===Add helper method to figure out endpoint depending on remote type===
We currently have this logic as part of the ndbClient and should be moved to a separate helper method. The purpose of this function is to determine if the provisioning is for on-prem or cloud.
...
type NDBClient struct {
username string
password string
url string
remoteType string
client *http.Client
}
...
===Modify helper methods to generate request, provision database and setup connectivity===
Below are the functions that are core to the provisioning logic. They are written for provisioning a database in Nutanix cloud infrastructure(on-prem). These functions constitute the primary part of the provisioning workflow.

====GenerateProvisioningRequest(database_reconciler_helpers.go)====
This data structure should be modified/extended to include fields that are required to be part of the JSON request for the NDS endpoint that pertains to provisioning on cloud. Adding fields with appropriate data type is required here to serialize and deserialize requests properly.

'''UPDATE: This has now been implemented
'''

The new request structure is as follows:
type CloudDatabaseProvisionRequest struct {
DatabaseProvisionRequest
AwsKeyName string `json:"awsKeyName"`
}

The logic to build the appropriate request structure depending on the remoteType:
[[File:Cloudcode.png|500px|]]

====ProvisionDatabase(database_reconciler_helpers.go)====
This method takes care of posting a request to the NDS endpoint to provision database. Should be modified to allow the same on cloud. This method should take care of a helper method logic to determine the correct request data structure and endpoint to be used. We have currently implemented a part of this logic as part of the ndbClient.
'''Note: We have now received access and will implemented the previous steps. This will be the next step to do'''

====SetupConnectivity(database_reconciler_helpers.go)====
This function checks and creates a new service (without label selectors) if it does not exists and also sets up the database as the owner for the created service. This also checks and creates an endpoints object for the service if it does not already exists.

'''Note: This is the last part of the serial steps. The previous steps must be completed in order to write this logic.'''

==Test plan==
===Runing tests===
Pull the code, cd into the directory and run:

make test

This runs all the required tests as specified in the Makefile.

===Test Scenarios===
# Testing construction of provision request - All test cases for this that are currently written for on-prem must work for cloud '''remoteType''' too.
# Additional tests for fields that are mandatory for requests constructed for provisioning in cloud
# Verify that the correct API endpoints are being hit based on the remote type specified - On-Prem should hit 0.9 API and cloud should hit 1.0 API
# Test the connectivity between the K8 service and the deployed DB on EC2.

===Actions to be taken before Manual testing===
Manual testing would require you to get access to the Nutanix Database Service

# Obtain access to Nutanix Database Service SASS and access keys for the AWS EC2 instance.

# Add the appropriate credentials to the secrets.yaml file.

# Apply your manifest instance using :
kubectl apply -f <your manifest instance file>

You can verify the status of provisioning in your NDB dashboard.

==Future Implementation==
* Obtain EC2 keys and connect NDS to the EC2 instance (DONE)
* Construct provision request for provisioning DB in cloud (DONE)
* Add logic to select proper endpoint and request based on the '''remoteType''' - We are currently passing this value as function parameter. This will be refactored into a helper method later.

* Add logic to de-provision the database.

==Github==
Repo(Public): https://github.com/arvindsrinivas1/ndb-operator
Pull Request: https://github.com/nutanix-cloud-native/ndb-operator/pull/73
Test video link: https://drive.google.com/file/d/1eRMvXjzatbz9W8xYPcg3V72pZyZBfVmi/view?usp=sharing

==Mentors==
* Prof. Edward F. Gehringer
* Krunal Jhaveri
* Manav Rajvanshi
* Krishna Saurabh Vankadaru

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

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-28T03:53:55Z

Asubram9:

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==

==Note to the reviewer==
Design Doc Link: https://docs.google.com/document/d/1Drglvj0b487qIACeIW1jYG83uRtJtl7fIchM6B1H9Bk/edit?usp=sharing
This is the document that we are actively using. All the details in the design document are already written here in a more clear way.

A major part of the logic can only be written after we get access to the keys of an EC2 instance that we can connect to the Nutanix Database Service. This differs from other NTNX teams since we need to provision the database on AWS. Hence, we cannot use the Test Nutanix endpoint to create clusterID or Secret's name. We have added a '''Note''' section in the Implementation section to inform the parts which fall under this category. We have also added this information collectively under the '''Future Implementation''' section.

'''UPDATE: We have now received access and have added as much code as possible within the stipulated time. We have added a 'Resubmission progress' section to inform you about the changes we have done now. We have also updated the Future Implementation section accordingly.'''

==Resubmission progress==
* We have received the access now, and have added a bit of code after that. We have analyzed the JSON request that is needed to be sent to provision a database and created a struct of that type in database_types.go.
* We have stuck to the DRY principle of re-using an existing variable's fields by inheriting from it. CloudDatabaseProvisionRequest inherits from DatabaseProvisionRequest.
* Added logic to create a CloudDatabaseProvisionRequest object if the remoteType is 'cloud' - This is because the request to this requires extra fields
* We have also now added the remoteType field to be passed as a variable for it to be available in the controller helper logic.
* We fixed the test cases after adding this logic.
* We have added a google drive link with a video of the tests running.
* Updated the implementation section with what is left.

==Problem Statement==
Nutanix Database Service is the only hybrid cloud database-as-a-service for Microsoft SQL
Server, Oracle Database, PostgreSQL, MongoDB, and MySQL. Currently, using the NDB operator, you can only provision a database on the Nutanix Cloud Infrastructure(on-prem).This project aims to extend the Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on cloud - specifically on AWS EC2. We aim to do this in a scalable way so that changing the cloud provider to say Azure or GCP would require changes only to the configuration file(CRD Manifest).

==Architecture==
The important components here are:
# '''Kubernetes Cluster'''
# '''Kubernetes Service''' that almost acts like a gateway to the provisioned database - The application pods that want to communicate with the database do so through the K8 service and endpoint that is created as part of the last step of provisioning. The workflow is explained in detail in the next section.
# '''Nutanix Database Service''' - The operator pod running in the K8 cluster communicates with this service to provision, de-provision, obtain status of the database etc.
# '''Cloud platform''' (AWS in our case)

[[File:Ntnx4arch.png|1000px|]]

==Workflow==
* Apply the instance CRD manifest file to create an instance of NDB resource in the K8 cluster

* The operator controller runs its logic to build a provisioning request and sends it to the Nutanix Database Service endpoint pertaining to provision in cloud(AWS in our case)

* The reconcile function loops to take actions depending on the status of provisioning - Empty, Provisioning and Ready

* Once the database is provisioned in AWS and the status is in Ready, we set up a Kubernetes networking service and an endpoint with the same name in the cluster. Any application pod in this cluster will communicate with this endpoint rather than communicating directly with the database.

[[File:Workflowntnx4.png|1000px|]]

==Design==
===Single Responsibility Principle(SRP)===
SRP is adopted here to separate the business logic of the operator from the reconciliation loop.

* The logic to handle '''remoteType''' : cloud is handled separately from the client (ndbClient) through helper methods - This ensures that the client is responsible for only making get/post requests to the specified endpoint without worrying about whether it is for on-prem or cloud.

* The reconciliation loop is responsible for ensuring that the state of the system matches the desired state declared by the operator. It listens to events from the Kubernetes API server and performs the necessary actions to reconcile the system to the desired state. The business logic of the operator, on the other hand, is responsible for implementing the higher-level functionality of the operator, such as managing the deployment of a complex application or managing a database cluster.

* Even within the reconciliation logic, there are clear indications of following SRP. We have a separate file utils/secret.go to implement GetDataFromSecret and GetAllDataFromSecret - logic to get data from the resource denoted by the name/namespace combination.

===Operator Pattern===
The operator pattern is based on the idea of declarative configuration management. Rather than writing imperative code to perform a series of steps to reach a desired state, an operator declares the desired state of the system and uses the Kubernetes API to perform the necessary actions to reconcile the system to that desired state.

We continue to follow this pattern for our implementation. We have a manifest file for a NDB custom resource responsible for provisioning and managing PostgreSQL in AWS and the logic for it is handled in the controller code (controllers/database_controllers.go)

EXAMPLE MANIFEST FILE
...

apiVersion: ndb.nutanix.com/v1alpha1
kind: Database
metadata:
name: db
spec:
ndb:
remoteType: "cloud"
clusterId: "Nutanix Cluster Id"
credentialSecret : ndb-secret-name
server: https://[NDB IP]:8443/era/v1.0
skipCertificateVerification: true
databaseInstance:
databaseInstanceName: "Database Instance Name"
databaseNames:
- database_one
- database_two
- database_three
credentialSecret: db-instance-secret-name
size: 10
timezone: "UTC"
type: postgres
...

The controller logic and related helper code will use the newly created ''''remoteType'''' field to select the NDS endpoint and construct provisionRequest appropriately. We have distinguished the configuration(Manifest file) and the logic(controller code) this way.

==Implementation related to this project==

This section will explain the changes that are made/to-be-made as part of this project.

'''NOTE: WE HAVE NOW RECEIVED ACCESS TO THE INSTANCE AND HAVE MADE AS MANY CHANGES AS POSSIBLE WITHIN THIS TIME. HAVE A LOOK AT THE RESUBMISSION PROGRESS SECTION FOR THE UPDATE'''
'''We will tell the changes that have not been made and they will be made in the future'''

===Generate Secrets manifest and set name field + credentials in it===
This secrets.yaml manifest file is to create a Secret resource instance that is will have the data regarding the NDB instance name and database credentials.

===Modify database_types.go to have allow remote types===
We have added a '''remoteType''' field to the NDB struct along with validation markers
...
// +kubebuilder:validation:Required;Enum=cloud;on-prem
RemoteType string `json:"remoteType"`

===Add helper method to figure out endpoint depending on remote type===
We currently have this logic as part of the ndbClient and should be moved to a separate helper method. The purpose of this function is to determine if the provisioning is for on-prem or cloud.
...
type NDBClient struct {
username string
password string
url string
remoteType string
client *http.Client
}
...
===Modify helper methods to generate request, provision database and setup connectivity===
Below are the functions that are core to the provisioning logic. They are written for provisioning a database in Nutanix cloud infrastructure(on-prem). These functions constitute the primary part of the provisioning workflow.

====GenerateProvisioningRequest(database_reconciler_helpers.go)====
This data structure should be modified/extended to include fields that are required to be part of the JSON request for the NDS endpoint that pertains to provisioning on cloud. Adding fields with appropriate data type is required here to serialize and deserialize requests properly.

'''UPDATE: This has now been implemented
'''
The new request structure is as follows:
type CloudDatabaseProvisionRequest struct {
DatabaseProvisionRequest
AwsKeyName string `json:"awsKeyName"`
}

The logic to build the appropriate request structure depending on the remoteType:
[[File:Cloudcode.png|500px|]]

====ProvisionDatabase(database_reconciler_helpers.go)====
This method takes care of posting a request to the NDS endpoint to provision database. Should be modified to allow the same on cloud. This method should take care of a helper method logic to determine the correct request data structure and endpoint to be used. We have currently implemented a part of this logic as part of the ndbClient.
'''Note: We have now received access and will implemented the previous steps. This will be the next step to do'''

====SetupConnectivity(database_reconciler_helpers.go)====
This function checks and creates a new service (without label selectors) if it does not exists and also sets up the database as the owner for the created service. This also checks and creates an endpoints object for the service if it does not already exists.

'''Note: This is the last part of the serial steps. The previous steps must be completed in order to write this logic.'''

==Test plan==
===Runing tests===
Pull the code, cd into the directory and run:

make test

This runs all the required tests as specified in the Makefile.

===Test Scenarios===
# Testing construction of provision request - All test cases for this that are currently written for on-prem must work for cloud '''remoteType''' too.
# Additional tests for fields that are mandatory for requests constructed for provisioning in cloud
# Verify that the correct API endpoints are being hit based on the remote type specified - On-Prem should hit 0.9 API and cloud should hit 1.0 API
# Test the connectivity between the K8 service and the deployed DB on EC2.

===Actions to be taken before Manual testing===
Manual testing would require you to get access to the Nutanix Database Service

# Obtain access to Nutanix Database Service SASS and access keys for the AWS EC2 instance.

# Add the appropriate credentials to the secrets.yaml file.

# Apply your manifest instance using :
kubectl apply -f <your manifest instance file>

You can verify the status of provisioning in your NDB dashboard.

==Future Implementation==
* Obtain EC2 keys and connect NDS to the EC2 instance (DONE)
* Construct provision request for provisioning DB in cloud (DONE)
* Add logic to select proper endpoint and request based on the '''remoteType''' - We are currently passing this value as function parameter. This will be refactored into a helper method later.

* Add logic to de-provision the database.

==Github==
Repo(Public): https://github.com/arvindsrinivas1/ndb-operator
Pull Request: https://github.com/nutanix-cloud-native/ndb-operator/pull/73
Test video link: https://drive.google.com/file/d/1eRMvXjzatbz9W8xYPcg3V72pZyZBfVmi/view?usp=sharing

==Mentors==
* Prof. Edward F. Gehringer
* Krunal Jhaveri
* Manav Rajvanshi
* Krishna Saurabh Vankadaru

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

File:Cloudcode.png

2023-03-28T03:51:04Z

Asubram9:

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-28T03:48:24Z

Asubram9:

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==

==Note to the reviewer==
Design Doc Link: https://docs.google.com/document/d/1Drglvj0b487qIACeIW1jYG83uRtJtl7fIchM6B1H9Bk/edit?usp=sharing
This is the document that we are actively using. All the details in the design document are already written here in a more clear way.

A major part of the logic can only be written after we get access to the keys of an EC2 instance that we can connect to the Nutanix Database Service. This differs from other NTNX teams since we need to provision the database on AWS. Hence, we cannot use the Test Nutanix endpoint to create clusterID or Secret's name. We have added a '''Note''' section in the Implementation section to inform the parts which fall under this category. We have also added this information collectively under the '''Future Implementation''' section.

'''UPDATE: We have now received access and have added as much code as possible within the stipulated time. We have added a 'Resubmission progress' section to inform you about the changes we have done now. We have also updated the Future Implementation section accordingly.'''

==Resubmission progress==
* We have received the access now, and have added a bit of code after that. We have analyzed the JSON request that is needed to be sent to provision a database and created a struct of that type in database_types.go.
* We have stuck to the DRY principle of re-using an existing variable's fields by inheriting from it. CloudDatabaseProvisionRequest inherits from DatabaseProvisionRequest.
* Added logic to create a CloudDatabaseProvisionRequest object if the remoteType is 'cloud' - This is because the request to this requires extra fields
* We have also now added the remoteType field to be passed as a variable for it to be available in the controller helper logic.
* We fixed the test cases after adding this logic.
* We have added a google drive link with a video of the tests running.
* Updated the implementation section with what is left.

==Problem Statement==
Nutanix Database Service is the only hybrid cloud database-as-a-service for Microsoft SQL
Server, Oracle Database, PostgreSQL, MongoDB, and MySQL. Currently, using the NDB operator, you can only provision a database on the Nutanix Cloud Infrastructure(on-prem).This project aims to extend the Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on cloud - specifically on AWS EC2. We aim to do this in a scalable way so that changing the cloud provider to say Azure or GCP would require changes only to the configuration file(CRD Manifest).

==Architecture==
The important components here are:
# '''Kubernetes Cluster'''
# '''Kubernetes Service''' that almost acts like a gateway to the provisioned database - The application pods that want to communicate with the database do so through the K8 service and endpoint that is created as part of the last step of provisioning. The workflow is explained in detail in the next section.
# '''Nutanix Database Service''' - The operator pod running in the K8 cluster communicates with this service to provision, de-provision, obtain status of the database etc.
# '''Cloud platform''' (AWS in our case)

[[File:Ntnx4arch.png|1000px|]]

==Workflow==
* Apply the instance CRD manifest file to create an instance of NDB resource in the K8 cluster

* The operator controller runs its logic to build a provisioning request and sends it to the Nutanix Database Service endpoint pertaining to provision in cloud(AWS in our case)

* The reconcile function loops to take actions depending on the status of provisioning - Empty, Provisioning and Ready

* Once the database is provisioned in AWS and the status is in Ready, we set up a Kubernetes networking service and an endpoint with the same name in the cluster. Any application pod in this cluster will communicate with this endpoint rather than communicating directly with the database.

[[File:Workflowntnx4.png|1000px|]]

==Design==
===Single Responsibility Principle(SRP)===
SRP is adopted here to separate the business logic of the operator from the reconciliation loop.

* The logic to handle '''remoteType''' : cloud is handled separately from the client (ndbClient) through helper methods - This ensures that the client is responsible for only making get/post requests to the specified endpoint without worrying about whether it is for on-prem or cloud.

* The reconciliation loop is responsible for ensuring that the state of the system matches the desired state declared by the operator. It listens to events from the Kubernetes API server and performs the necessary actions to reconcile the system to the desired state. The business logic of the operator, on the other hand, is responsible for implementing the higher-level functionality of the operator, such as managing the deployment of a complex application or managing a database cluster.

* Even within the reconciliation logic, there are clear indications of following SRP. We have a separate file utils/secret.go to implement GetDataFromSecret and GetAllDataFromSecret - logic to get data from the resource denoted by the name/namespace combination.

===Operator Pattern===
The operator pattern is based on the idea of declarative configuration management. Rather than writing imperative code to perform a series of steps to reach a desired state, an operator declares the desired state of the system and uses the Kubernetes API to perform the necessary actions to reconcile the system to that desired state.

We continue to follow this pattern for our implementation. We have a manifest file for a NDB custom resource responsible for provisioning and managing PostgreSQL in AWS and the logic for it is handled in the controller code (controllers/database_controllers.go)

EXAMPLE MANIFEST FILE
...

apiVersion: ndb.nutanix.com/v1alpha1
kind: Database
metadata:
name: db
spec:
ndb:
remoteType: "cloud"
clusterId: "Nutanix Cluster Id"
credentialSecret : ndb-secret-name
server: https://[NDB IP]:8443/era/v1.0
skipCertificateVerification: true
databaseInstance:
databaseInstanceName: "Database Instance Name"
databaseNames:
- database_one
- database_two
- database_three
credentialSecret: db-instance-secret-name
size: 10
timezone: "UTC"
type: postgres
...

The controller logic and related helper code will use the newly created ''''remoteType'''' field to select the NDS endpoint and construct provisionRequest appropriately. We have distinguished the configuration(Manifest file) and the logic(controller code) this way.

==Implementation related to this project==

This section will explain the changes that are made/to-be-made as part of this project.

'''NOTE: WE HAVE NOW RECEIVED ACCESS TO THE INSTANCE AND HAVE MADE AS MANY CHANGES AS POSSIBLE WITHIN THIS TIME. HAVE A LOOK AT THE RESUBMISSION PROGRESS SECTION FOR THE UPDATE'''
'''We will tell the changes that have not been made and they will be made in the future'''

===Generate Secrets manifest and set name field + credentials in it===
This secrets.yaml manifest file is to create a Secret resource instance that is will have the data regarding the NDB instance name and database credentials.

===Modify database_types.go to have allow remote types===
We have added a '''remoteType''' field to the NDB struct along with validation markers
...
// +kubebuilder:validation:Required;Enum=cloud;on-prem
RemoteType string `json:"remoteType"`

===Add helper method to figure out endpoint depending on remote type===
We currently have this logic as part of the ndbClient and should be moved to a separate helper method. The purpose of this function is to determine if the provisioning is for on-prem or cloud.
...
type NDBClient struct {
username string
password string
url string
remoteType string
client *http.Client
}
...
===Modify helper methods to generate request, provision database and setup connectivity===
Below are the functions that are core to the provisioning logic. They are written for provisioning a database in Nutanix cloud infrastructure(on-prem). These functions constitute the primary part of the provisioning workflow.

====GenerateProvisioningRequest(database_reconciler_helpers.go)====
This data structure should be modified/extended to include fields that are required to be part of the JSON request for the NDS endpoint that pertains to provisioning on cloud. Adding fields with appropriate data type is required here to serialize and deserialize requests properly.

UPDATE: This has now been implemented

type CloudDatabaseProvisionRequest struct {
DatabaseProvisionRequest
AwsKeyName string `json:"awsKeyName"`
}

====ProvisionDatabase(database_reconciler_helpers.go)====
This method takes care of posting a request to the NDS endpoint to provision database. Should be modified to allow the same on cloud. This method should take care of a helper method logic to determine the correct request data structure and endpoint to be used. We have currently implemented a part of this logic as part of the ndbClient.
'''Note: This can only be implemented after obtaining the access. We have now received access and will implement it'''

====SetupConnectivity(database_reconciler_helpers.go)====
This function checks and creates a new service (without label selectors) if it does not exists and also sets up the database as the owner for the created service. This also checks and creates an endpoints object for the service if it does not already exists.

'''Note: This is the last part of the serial steps. The previous steps must be completed in order to write this logic.'''

==Test plan==
===Runing tests===
Pull the code, cd into the directory and run:

make test

This runs all the required tests as specified in the Makefile.

===Test Scenarios===
# Testing construction of provision request - All test cases for this that are currently written for on-prem must work for cloud '''remoteType''' too.
# Additional tests for fields that are mandatory for requests constructed for provisioning in cloud
# Verify that the correct API endpoints are being hit based on the remote type specified - On-Prem should hit 0.9 API and cloud should hit 1.0 API
# Test the connectivity between the K8 service and the deployed DB on EC2.

===Actions to be taken before Manual testing===
Manual testing would require you to get access to the Nutanix Database Service

# Obtain access to Nutanix Database Service SASS and access keys for the AWS EC2 instance.

# Add the appropriate credentials to the secrets.yaml file.

# Apply your manifest instance using :
kubectl apply -f <your manifest instance file>

You can verify the status of provisioning in your NDB dashboard.

==Future Implementation==
* Obtain EC2 keys and connect NDS to the EC2 instance (DONE)
* Construct provision request for provisioning DB in cloud (DONE)
* Add logic to select proper endpoint and request based on the '''remoteType''' - We are currently passing this value as function parameter. This will be refactored into a helper method later.

* Add logic to de-provision the database.

==Github==
Repo(Public): https://github.com/arvindsrinivas1/ndb-operator
Pull Request: https://github.com/nutanix-cloud-native/ndb-operator/pull/73
Test video link: https://drive.google.com/file/d/1eRMvXjzatbz9W8xYPcg3V72pZyZBfVmi/view?usp=sharing

==Mentors==
* Prof. Edward F. Gehringer
* Krunal Jhaveri
* Manav Rajvanshi
* Krishna Saurabh Vankadaru

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

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-28T03:32:59Z

Asubram9:

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==

==Note to the reviewer==
Design Doc Link: https://docs.google.com/document/d/1Drglvj0b487qIACeIW1jYG83uRtJtl7fIchM6B1H9Bk/edit?usp=sharing
This is the document that we are actively using. All the details in the design document are already written here in a more clear way.

A major part of the logic can only be written after we get access to the keys of an EC2 instance that we can connect to the Nutanix Database Service. This differs from other NTNX teams since we need to provision the database on AWS. Hence, we cannot use the Test Nutanix endpoint to create clusterID or Secret's name. We have added a '''Note''' section in the Implementation section to inform the parts which fall under this category. We have also added this information collectively under the '''Future Implementation''' section.

'''UPDATE: We have now received access and have added as much code as possible within the stipulated time. We have added a 'Resubmission progress' section to inform you about the changes we have done now. We have also updated the Future Implementation section accordingly.'''

==Resubmission progress==
* We have received the access now, and have added a bit of code after that. We have analyzed the JSON request that is needed to be sent to provision a database and created a struct of that type in database_types.go.
* We have stuck to the DRY principle of re-using an existing variable's fields by inheriting from it. CloudDatabaseProvisionRequest inherits from DatabaseProvisionRequest.
* We have also now added the remoteType field to be passed as a variable for it to be available in the controller helper logic.
* We fixed the test cases after adding this logic.
* We have added a google drive link with a video of the tests running.
* Updated the implementation section with what is left.

==Problem Statement==
Nutanix Database Service is the only hybrid cloud database-as-a-service for Microsoft SQL
Server, Oracle Database, PostgreSQL, MongoDB, and MySQL. Currently, using the NDB operator, you can only provision a database on the Nutanix Cloud Infrastructure(on-prem).This project aims to extend the Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on cloud - specifically on AWS EC2. We aim to do this in a scalable way so that changing the cloud provider to say Azure or GCP would require changes only to the configuration file(CRD Manifest).

==Architecture==
The important components here are:
# '''Kubernetes Cluster'''
# '''Kubernetes Service''' that almost acts like a gateway to the provisioned database - The application pods that want to communicate with the database do so through the K8 service and endpoint that is created as part of the last step of provisioning. The workflow is explained in detail in the next section.
# '''Nutanix Database Service''' - The operator pod running in the K8 cluster communicates with this service to provision, de-provision, obtain status of the database etc.
# '''Cloud platform''' (AWS in our case)

[[File:Ntnx4arch.png|1000px|]]

==Workflow==
* Apply the instance CRD manifest file to create an instance of NDB resource in the K8 cluster

* The operator controller runs its logic to build a provisioning request and sends it to the Nutanix Database Service endpoint pertaining to provision in cloud(AWS in our case)

* The reconcile function loops to take actions depending on the status of provisioning - Empty, Provisioning and Ready

* Once the database is provisioned in AWS and the status is in Ready, we set up a Kubernetes networking service and an endpoint with the same name in the cluster. Any application pod in this cluster will communicate with this endpoint rather than communicating directly with the database.

[[File:Workflowntnx4.png|1000px|]]

==Design==
===Single Responsibility Principle(SRP)===
SRP is adopted here to separate the business logic of the operator from the reconciliation loop.

* The logic to handle '''remoteType''' : cloud is handled separately from the client (ndbClient) through helper methods - This ensures that the client is responsible for only making get/post requests to the specified endpoint without worrying about whether it is for on-prem or cloud.

* The reconciliation loop is responsible for ensuring that the state of the system matches the desired state declared by the operator. It listens to events from the Kubernetes API server and performs the necessary actions to reconcile the system to the desired state. The business logic of the operator, on the other hand, is responsible for implementing the higher-level functionality of the operator, such as managing the deployment of a complex application or managing a database cluster.

* Even within the reconciliation logic, there are clear indications of following SRP. We have a separate file utils/secret.go to implement GetDataFromSecret and GetAllDataFromSecret - logic to get data from the resource denoted by the name/namespace combination.

===Operator Pattern===
The operator pattern is based on the idea of declarative configuration management. Rather than writing imperative code to perform a series of steps to reach a desired state, an operator declares the desired state of the system and uses the Kubernetes API to perform the necessary actions to reconcile the system to that desired state.

We continue to follow this pattern for our implementation. We have a manifest file for a NDB custom resource responsible for provisioning and managing PostgreSQL in AWS and the logic for it is handled in the controller code (controllers/database_controllers.go)

EXAMPLE MANIFEST FILE
...

apiVersion: ndb.nutanix.com/v1alpha1
kind: Database
metadata:
name: db
spec:
ndb:
remoteType: "cloud"
clusterId: "Nutanix Cluster Id"
credentialSecret : ndb-secret-name
server: https://[NDB IP]:8443/era/v1.0
skipCertificateVerification: true
databaseInstance:
databaseInstanceName: "Database Instance Name"
databaseNames:
- database_one
- database_two
- database_three
credentialSecret: db-instance-secret-name
size: 10
timezone: "UTC"
type: postgres
...

The controller logic and related helper code will use the newly created ''''remoteType'''' field to select the NDS endpoint and construct provisionRequest appropriately. We have distinguished the configuration(Manifest file) and the logic(controller code) this way.

==Implementation related to this project==

This section will explain the changes that are made/to-be-made as part of this project.

'''NOTE: WE HAVE NOW RECEIVED ACCESS TO THE INSTANCE AND HAVE MADE AS MANY CHANGES AS POSSIBLE WITHIN THIS TIME. HAVE A LOOK AT THE RESUBMISSION PROGRESS SECTION FOR THE UPDATE'''
'''We will tell the changes that have not been made and they will be made in the future'''

===Generate Secrets manifest and set name field + credentials in it===
This secrets.yaml manifest file is to create a Secret resource instance that is will have the data regarding the NDB instance name and database credentials.

===Modify database_types.go to have allow remote types===
We have added a '''remoteType''' field to the NDB struct along with validation markers
...
// +kubebuilder:validation:Required;Enum=cloud;on-prem
RemoteType string `json:"remoteType"`

===Add helper method to figure out endpoint depending on remote type===
We currently have this logic as part of the ndbClient and should be moved to a separate helper method. The purpose of this function is to determine if the provisioning is for on-prem or cloud.
...
type NDBClient struct {
username string
password string
url string
remoteType string
client *http.Client
}
...
===Modify helper methods to generate request, provision database and setup connectivity===
Below are the functions that are core to the provisioning logic. They are written for provisioning a database in Nutanix cloud infrastructure(on-prem). These functions constitute the primary part of the provisioning workflow.

====GenerateProvisioningRequest(database_reconciler_helpers.go)====
This data structure should be modified/extended to include fields that are required to be part of the JSON request for the NDS endpoint that pertains to provisioning on cloud. Adding fields with appropriate data type is required here to serialize and deserialize requests properly.

UPDATE: This has now been implemented

type CloudDatabaseProvisionRequest struct {
DatabaseProvisionRequest
AwsKeyName string `json:"awsKeyName"`
}

====ProvisionDatabase(database_reconciler_helpers.go)====
This method takes care of posting a request to the NDS endpoint to provision database. Should be modified to allow the same on cloud. This method should take care of a helper method logic to determine the correct request data structure and endpoint to be used. We have currently implemented a part of this logic as part of the ndbClient.
'''Note: This can only be implemented after obtaining the access. We have now received access and will implement it'''

====SetupConnectivity(database_reconciler_helpers.go)====
This function checks and creates a new service (without label selectors) if it does not exists and also sets up the database as the owner for the created service. This also checks and creates an endpoints object for the service if it does not already exists.

'''Note: This is the last part of the serial steps. The previous steps must be completed in order to write this logic.'''

==Test plan==
===Runing tests===
Pull the code, cd into the directory and run:

make test

This runs all the required tests as specified in the Makefile.

===Test Scenarios===
# Testing construction of provision request - All test cases for this that are currently written for on-prem must work for cloud '''remoteType''' too.
# Additional tests for fields that are mandatory for requests constructed for provisioning in cloud
# Verify that the correct API endpoints are being hit based on the remote type specified - On-Prem should hit 0.9 API and cloud should hit 1.0 API
# Test the connectivity between the K8 service and the deployed DB on EC2.

===Actions to be taken before Manual testing===
Manual testing would require you to get access to the Nutanix Database Service

# Obtain access to Nutanix Database Service SASS and access keys for the AWS EC2 instance.

# Add the appropriate credentials to the secrets.yaml file.

# Apply your manifest instance using :
kubectl apply -f <your manifest instance file>

You can verify the status of provisioning in your NDB dashboard.

==Future Implementation==
* Obtain EC2 keys and connect NDS to the EC2 instance (DONE)
* Construct provision request for provisioning DB in cloud (DONE)
* Add logic to select proper endpoint and request based on the '''remoteType''' - We are currently passing this value as function parameter. This will be refactored into a helper method later.

* Add logic to de-provision the database.

==Github==
Repo(Public): https://github.com/arvindsrinivas1/ndb-operator
Pull Request: https://github.com/nutanix-cloud-native/ndb-operator/pull/73
Test video link: https://drive.google.com/file/d/1eRMvXjzatbz9W8xYPcg3V72pZyZBfVmi/view?usp=sharing

==Mentors==
* Prof. Edward F. Gehringer
* Krunal Jhaveri
* Manav Rajvanshi
* Krishna Saurabh Vankadaru

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

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-28T03:30:03Z

Asubram9: Resubmission update changes

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==

==Note to the reviewer==
Design Doc Link: https://docs.google.com/document/d/1Drglvj0b487qIACeIW1jYG83uRtJtl7fIchM6B1H9Bk/edit?usp=sharing
This is the document that we are actively using. All the details in the design document are already written here in a more clear way.

A major part of the logic can only be written after we get access to the keys of an EC2 instance that we can connect to the Nutanix Database Service. This differs from other NTNX teams since we need to provision the database on AWS. Hence, we cannot use the Test Nutanix endpoint to create clusterID or Secret's name. We have added a '''Note''' section in the Implementation section to inform the parts which fall under this category. We have also added this information collectively under the '''Future Implementation''' section.

UPDATE: We have now received access and have added as much code as possible within the stipulated time. Following are the changes we have implemented regarding this project and the submission:

==Resubmission progress==
* We have received the access now, and have added a bit of code after that. We have analyzed the JSON request that is needed to be sent to provision a database and created a struct of that type in database_types.go.
* We have stuck to the DRY principle of re-using an existing variable's fields by inheriting from it. CloudDatabaseProvisionRequest inherits from DatabaseProvisionRequest.
* We have also now added the remoteType field to be passed as a variable for it to be available in the controller helper logic.
* We fixed the test cases after adding this logic.
* We have added a google drive link with a video of the tests running.
* Updated the implementation section with what is left.

==Problem Statement==
Nutanix Database Service is the only hybrid cloud database-as-a-service for Microsoft SQL
Server, Oracle Database, PostgreSQL, MongoDB, and MySQL. Currently, using the NDB operator, you can only provision a database on the Nutanix Cloud Infrastructure(on-prem).This project aims to extend the Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on cloud - specifically on AWS EC2. We aim to do this in a scalable way so that changing the cloud provider to say Azure or GCP would require changes only to the configuration file(CRD Manifest).

==Architecture==
The important components here are:
# '''Kubernetes Cluster'''
# '''Kubernetes Service''' that almost acts like a gateway to the provisioned database - The application pods that want to communicate with the database do so through the K8 service and endpoint that is created as part of the last step of provisioning. The workflow is explained in detail in the next section.
# '''Nutanix Database Service''' - The operator pod running in the K8 cluster communicates with this service to provision, de-provision, obtain status of the database etc.
# '''Cloud platform''' (AWS in our case)

[[File:Ntnx4arch.png|1000px|]]

==Workflow==
* Apply the instance CRD manifest file to create an instance of NDB resource in the K8 cluster

* The operator controller runs its logic to build a provisioning request and sends it to the Nutanix Database Service endpoint pertaining to provision in cloud(AWS in our case)

* The reconcile function loops to take actions depending on the status of provisioning - Empty, Provisioning and Ready

* Once the database is provisioned in AWS and the status is in Ready, we set up a Kubernetes networking service and an endpoint with the same name in the cluster. Any application pod in this cluster will communicate with this endpoint rather than communicating directly with the database.

[[File:Workflowntnx4.png|1000px|]]

==Design==
===Single Responsibility Principle(SRP)===
SRP is adopted here to separate the business logic of the operator from the reconciliation loop.

* The logic to handle '''remoteType''' : cloud is handled separately from the client (ndbClient) through helper methods - This ensures that the client is responsible for only making get/post requests to the specified endpoint without worrying about whether it is for on-prem or cloud.

* The reconciliation loop is responsible for ensuring that the state of the system matches the desired state declared by the operator. It listens to events from the Kubernetes API server and performs the necessary actions to reconcile the system to the desired state. The business logic of the operator, on the other hand, is responsible for implementing the higher-level functionality of the operator, such as managing the deployment of a complex application or managing a database cluster.

* Even within the reconciliation logic, there are clear indications of following SRP. We have a separate file utils/secret.go to implement GetDataFromSecret and GetAllDataFromSecret - logic to get data from the resource denoted by the name/namespace combination.

===Operator Pattern===
The operator pattern is based on the idea of declarative configuration management. Rather than writing imperative code to perform a series of steps to reach a desired state, an operator declares the desired state of the system and uses the Kubernetes API to perform the necessary actions to reconcile the system to that desired state.

We continue to follow this pattern for our implementation. We have a manifest file for a NDB custom resource responsible for provisioning and managing PostgreSQL in AWS and the logic for it is handled in the controller code (controllers/database_controllers.go)

EXAMPLE MANIFEST FILE
...

apiVersion: ndb.nutanix.com/v1alpha1
kind: Database
metadata:
name: db
spec:
ndb:
remoteType: "cloud"
clusterId: "Nutanix Cluster Id"
credentialSecret : ndb-secret-name
server: https://[NDB IP]:8443/era/v1.0
skipCertificateVerification: true
databaseInstance:
databaseInstanceName: "Database Instance Name"
databaseNames:
- database_one
- database_two
- database_three
credentialSecret: db-instance-secret-name
size: 10
timezone: "UTC"
type: postgres
...

The controller logic and related helper code will use the newly created ''''remoteType'''' field to select the NDS endpoint and construct provisionRequest appropriately. We have distinguished the configuration(Manifest file) and the logic(controller code) this way.

==Implementation related to this project==

This section will explain the changes that are made/to-be-made as part of this project.

'''NOTE: WE HAVE NOW RECEIVED ACCESS TO THE INSTANCE AND HAVE MADE AS MANY CHANGES AS POSSIBLE WITHIN THIS TIME. HAVE A LOOK AT THE RESUBMISSION PROGRESS SECTION FOR THE UPDATE'''
'''We will tell the changes that have not been made and they will be made in the future'''

===Generate Secrets manifest and set name field + credentials in it===
This secrets.yaml manifest file is to create a Secret resource instance that is will have the data regarding the NDB instance name and database credentials.

===Modify database_types.go to have allow remote types===
We have added a '''remoteType''' field to the NDB struct along with validation markers
...
// +kubebuilder:validation:Required;Enum=cloud;on-prem
RemoteType string `json:"remoteType"`

===Add helper method to figure out endpoint depending on remote type===
We currently have this logic as part of the ndbClient and should be moved to a separate helper method. The purpose of this function is to determine if the provisioning is for on-prem or cloud.
...
type NDBClient struct {
username string
password string
url string
remoteType string
client *http.Client
}
...
===Modify helper methods to generate request, provision database and setup connectivity===
Below are the functions that are core to the provisioning logic. They are written for provisioning a database in Nutanix cloud infrastructure(on-prem). These functions constitute the primary part of the provisioning workflow.

====GenerateProvisioningRequest(database_reconciler_helpers.go)====
This data structure should be modified/extended to include fields that are required to be part of the JSON request for the NDS endpoint that pertains to provisioning on cloud. Adding fields with appropriate data type is required here to serialize and deserialize requests properly.

UPDATE: This has now been implemented

type CloudDatabaseProvisionRequest struct {
DatabaseProvisionRequest
AwsKeyName string `json:"awsKeyName"`
}

====ProvisionDatabase(database_reconciler_helpers.go)====
This method takes care of posting a request to the NDS endpoint to provision database. Should be modified to allow the same on cloud. This method should take care of a helper method logic to determine the correct request data structure and endpoint to be used. We have currently implemented a part of this logic as part of the ndbClient.
'''Note: This can only be implemented after obtaining the access. We have now received access and will implement it'''

====SetupConnectivity(database_reconciler_helpers.go)====
This function checks and creates a new service (without label selectors) if it does not exists and also sets up the database as the owner for the created service. This also checks and creates an endpoints object for the service if it does not already exists.

'''Note: This is the last part of the serial steps. The previous steps must be completed in order to write this logic.'''

==Test plan==
===Runing tests===
Pull the code, cd into the directory and run:

make test

This runs all the required tests as specified in the Makefile.

===Test Scenarios===
# Testing construction of provision request - All test cases for this that are currently written for on-prem must work for cloud '''remoteType''' too.
# Additional tests for fields that are mandatory for requests constructed for provisioning in cloud
# Verify that the correct API endpoints are being hit based on the remote type specified - On-Prem should hit 0.9 API and cloud should hit 1.0 API
# Test the connectivity between the K8 service and the deployed DB on EC2.

===Actions to be taken before Manual testing===
Manual testing would require you to get access to the Nutanix Database Service

# Obtain access to Nutanix Database Service SASS and access keys for the AWS EC2 instance.

# Add the appropriate credentials to the secrets.yaml file.

# Apply your manifest instance using :
kubectl apply -f <your manifest instance file>

You can verify the status of provisioning in your NDB dashboard.

==Future Implementation==
* Obtain EC2 keys and connect NDS to the EC2 instance (DONE)
* Construct provision request for provisioning DB in cloud (DONE)
* Add logic to select proper endpoint and request based on the '''remoteType''' - We are currently passing this value as function parameter. This will be refactored into a helper method later.

* Add logic to de-provision the database.

==Github==
Repo(Public): https://github.com/arvindsrinivas1/ndb-operator
Pull Request: https://github.com/nutanix-cloud-native/ndb-operator/pull/73
Test video link: https://drive.google.com/file/d/1eRMvXjzatbz9W8xYPcg3V72pZyZBfVmi/view?usp=sharing

==Mentors==
* Prof. Edward F. Gehringer
* Krunal Jhaveri
* Manav Rajvanshi
* Krishna Saurabh Vankadaru

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

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-24T14:02:04Z

Asubram9:

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==

==Note to the reviewer==
Design Doc Link: https://docs.google.com/document/d/1Drglvj0b487qIACeIW1jYG83uRtJtl7fIchM6B1H9Bk/edit?usp=sharing
This is the document that we are actively using. All the details in the design document are already written here in a more clear way.

A major part of the logic can only be written after we get access to the keys of an EC2 instance that we can connect to the Nutanix Database Service. This differs from other NTNX teams since we need to provision the database on AWS. Hence, we cannot use the Test Nutanix endpoint to create clusterID or Secret's name. We have added a '''Note''' section in the Implementation section to inform the parts which fall under this category. We have also added this information collectively under the '''Future Implementation''' section.

==Problem Statement==
Nutanix Database Service is the only hybrid cloud database-as-a-service for Microsoft SQL
Server, Oracle Database, PostgreSQL, MongoDB, and MySQL. Currently, using the NDB operator, you can only provision a database on the Nutanix Cloud Infrastructure(on-prem).This project aims to extend the Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on cloud - specifically on AWS EC2. We aim to do this in a scalable way so that changing the cloud provider to say Azure or GCP would require changes only to the configuration file(CRD Manifest).

==Architecture==
The important components here are:
# '''Kubernetes Cluster'''
# '''Kubernetes Service''' that almost acts like a gateway to the provisioned database - The application pods that want to communicate with the database do so through the K8 service and endpoint that is created as part of the last step of provisioning. The workflow is explained in detail in the next section.
# '''Nutanix Database Service''' - The operator pod running in the K8 cluster communicates with this service to provision, de-provision, obtain status of the database etc.
# '''Cloud platform''' (AWS in our case)

[[File:Ntnx4arch.png|1000px|]]

==Workflow==
* Apply the instance CRD manifest file to create an instance of NDB resource in the K8 cluster

* The operator controller runs its logic to build a provisioning request and sends it to the Nutanix Database Service endpoint pertaining to provision in cloud(AWS in our case)

* The reconcile function loops to take actions depending on the status of provisioning - Empty, Provisioning and Ready

* Once the database is provisioned in AWS and the status is in Ready, we set up a Kubernetes networking service and an endpoint with the same name in the cluster. Any application pod in this cluster will communicate with this endpoint rather than communicating directly with the database.

[[File:Workflowntnx4.png|1000px|]]

==Design==
===Single Responsibility Principle(SRP)===
SRP is adopted here to separate the business logic of the operator from the reconciliation loop.

* The logic to handle '''remoteType''' : cloud is handled separately from the client (ndbClient) through helper methods - This ensures that the client is responsible for only making get/post requests to the specified endpoint without worrying about whether it is for on-prem or cloud.

* The reconciliation loop is responsible for ensuring that the state of the system matches the desired state declared by the operator. It listens to events from the Kubernetes API server and performs the necessary actions to reconcile the system to the desired state. The business logic of the operator, on the other hand, is responsible for implementing the higher-level functionality of the operator, such as managing the deployment of a complex application or managing a database cluster.

* Even within the reconciliation logic, there are clear indications of following SRP. We have a separate file utils/secret.go to implement GetDataFromSecret and GetAllDataFromSecret - logic to get data from the resource denoted by the name/namespace combination.

===Operator Pattern===
The operator pattern is based on the idea of declarative configuration management. Rather than writing imperative code to perform a series of steps to reach a desired state, an operator declares the desired state of the system and uses the Kubernetes API to perform the necessary actions to reconcile the system to that desired state.

We continue to follow this pattern for our implementation. We have a manifest file for a NDB custom resource responsible for provisioning and managing PostgreSQL in AWS and the logic for it is handled in the controller code (controllers/database_controllers.go)

EXAMPLE MANIFEST FILE
...

apiVersion: ndb.nutanix.com/v1alpha1
kind: Database
metadata:
name: db
spec:
ndb:
remoteType: "cloud"
clusterId: "Nutanix Cluster Id"
credentialSecret : ndb-secret-name
server: https://[NDB IP]:8443/era/v1.0
skipCertificateVerification: true
databaseInstance:
databaseInstanceName: "Database Instance Name"
databaseNames:
- database_one
- database_two
- database_three
credentialSecret: db-instance-secret-name
size: 10
timezone: "UTC"
type: postgres
...

The controller logic and related helper code will use the newly created ''''remoteType'''' field to select the NDS endpoint and construct provisionRequest appropriately. We have distinguished the configuration(Manifest file) and the logic(controller code) this way.

==Implementation related to this project==

This section will explain the changes that are made/to-be-made as part of this project.

'''NOTE: MOST OF THESE CHANGES HAVE NOT BEEN MADE NOW SINCE OUR TEAM HAS NOT OBTAINED ACCESS TO CONNECT THE SERVICE WITH EC2 INSTANCE FROM THE NUTANIX TEAM'''
'''We will tell the changes that have not been made and they will be made in the future'''

===Generate Secrets manifest and set name field + credentials in it===
This secrets.yaml manifest file is to create a Secret resource instance that is will have the data regarding the NDB instance name and database credentials.

'''Note: We have created this file, but currently have template values. After getting access, we will fill them accordingly'''

===Modify database_types.go to have allow remote types===
We have added a '''remoteType''' field to the NDB struct along with validation markers
...
// +kubebuilder:validation:Required;Enum=cloud;on-prem
RemoteType string `json:"remoteType"`

===Add helper method to figure out endpoint depending on remote type===
We currently have this logic as part of the ndbClient and should be moved to a separate helper method. The purpose of this function is to determine if the provisioning is for on-prem or cloud.
...
type NDBClient struct {
username string
password string
url string
remoteType string
client *http.Client
}
...
===Modify helper methods to generate request, provision database and setup connectivity===
Below are the functions that are core to the provisioning logic. They are written for provisioning a database in Nutanix cloud infrastructure(on-prem). These functions constitute the primary part of the provisioning workflow.

====GenerateProvisioningRequest(database_reconciler_helpers.go)====
This data structure should be modified/extended to include fields that are required to be part of the JSON request for the NDS endpoint that pertains to provisioning on cloud. Adding fields with appropriate data type is required here to serialize and deserialize requests properly.

====ProvisionDatabase(database_reconciler_helpers.go)====
This method takes care of posting a request to the NDS endpoint to provision database. Should be modified to allow the same on cloud. This method should take care of a helper method logic to determine the correct request data structure and endpoint to be used. We have currently implemented a part of this logic as part of the ndbClient.
'''Note: This can only be implemented after obtaining the access. We are still waiting on it and will implement it as soon as we obtain it.'''

====SetupConnectivity(database_reconciler_helpers.go)====
This function checks and creates a new service (without label selectors) if it does not exists and also sets up the database as the owner for the created service. This also checks and creates an endpoints object for the service if it does not already exists.

'''Note: This is the last part of the serial steps. The previous steps must be completed in order to write this logic.'''

==Test plan==
===Runing tests===
Pull the code, cd into the directory and run:

make test

This runs all the required tests as specified in the Makefile.

===Test Scenarios===
# Testing construction of provision request - All test cases for this that are currently written for on-prem must work for cloud '''remoteType''' too.
# Additional tests for fields that are mandatory for requests constructed for provisioning in cloud
# Verify that the correct API endpoints are being hit based on the remote type specified - On-Prem should hit 0.9 API and cloud should hit 1.0 API
# Test the connectivity between the K8 service and the deployed DB on EC2.

===Actions to be taken before Manual testing===
Manual testing would require you to get access to the Nutanix Database Service

# Obtain access to Nutanix Database Service SASS and access keys for the AWS EC2 instance.

# Add the appropriate credentials to the secrets.yaml file.

# Apply your manifest instance using :
kubectl apply -f <your manifest instance file>

You can verify the status of provisioning in your NDB dashboard.

==Future Implementation==
* Obtain EC2 keys and connect NDS to the EC2 instance
* Construct provision request for provisioning DB in cloud
* Add logic to select proper endpoint and request based on the '''remoteType'''
* Add logic to de-provision the database.

==Github==
Repo(Public): https://github.com/arvindsrinivas1/ndb-operator
Pull Request: https://github.com/nutanix-cloud-native/ndb-operator/pull/73

==Mentors==
* Prof. Edward F. Gehringer
* Krunal Jhaveri
* Manav Rajvanshi
* Krishna Saurabh Vankadaru

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

CSC/ECE 517 Spring 2023

2023-03-22T01:39:47Z

Asubram9: /* OSS Projects */

== OSS Projects ==

[[CSC/ECE 517 Spring_2023 - E2320. Reimplement the Question hierarchy]]

[[CSC/ECE 517 Spring_2023 - E2312 + E2313. Reimplement response.rb and responses_controller.rb]]

[[CSC/ECE 517 Spring_2023 - CSC517. Support provisioning MongoDb via NDB Kubernetes Operator]]

[[CSC/ECE 517 Spring_2023 - E2316. Reimplement sign_up_sheet_controller.rb]]

[[CSC/ECE 517 Spring_2023 - E2315. Reimplement signed_up_team.rb, sign_up_topic.rb, sign_up_sheet.rb]]

[[CSC/ECE 517 Spring_2023 - E2323. Refactor DueDate functionality from assignment.rb]]

[[CSC/ECE 517 Spring 2023 - E2314. Reimplement the response map hierarchy]]

[[CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws]]

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-22T01:37:33Z

Asubram9:

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==

==Note to the reviewer==
A major part of the logic can only be written after we get access to the keys of an EC2 instance that we can connect to the Nutanix Database Service. This differs from other NTNX teams since we need to provision the database on AWS. Hence, we cannot use the Test Nutanix endpoint to create clusterID or Secret's name. We have added a '''Note''' section in the Implementation section to inform the parts which fall under this category. We have also added this information collectively under the '''Future Implementation''' section.

==Problem Statement==
Nutanix Database Service is the only hybrid cloud database-as-a-service for Microsoft SQL
Server, Oracle Database, PostgreSQL, MongoDB, and MySQL. Currently, using the NDB operator, you can only provision a database on the Nutanix Cloud Infrastructure(on-prem).This project aims to extend the Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on cloud - specifically on AWS EC2. We aim to do this in a scalable way so that changing the cloud provider to say Azure or GCP would require changes only to the configuration file(CRD Manifest).

==Architecture==
The important components here are:
# '''Kubernetes Cluster'''
# '''Kubernetes Service''' that almost acts like a gateway to the provisioned database - The application pods that want to communicate with the database do so through the K8 service and endpoint that is created as part of the last step of provisioning. The workflow is explained in detail in the next section.
# '''Nutanix Database Service''' - The operator pod running in the K8 cluster communicates with this service to provision, de-provision, obtain status of the database etc.
# '''Cloud platform''' (AWS in our case)

[[File:Ntnx4arch.png|1000px|]]

==Workflow==
* Apply the instance CRD manifest file to create an instance of NDB resource in the K8 cluster

* The operator controller runs its logic to build a provisioning request and sends it to the Nutanix Database Service endpoint pertaining to provision in cloud(AWS in our case)

* The reconcile function loops to take actions depending on the status of provisioning - Empty, Provisioning and Ready

* Once the database is provisioned in AWS and the status is in Ready, we set up a Kubernetes networking service and an endpoint with the same name in the cluster. Any application pod in this cluster will communicate with this endpoint rather than communicating directly with the database.

[[File:Workflowntnx4.png|1000px|]]

==Design==
===Single Responsibility Principle(SRP)===
SRP is adopted here to separate the business logic of the operator from the reconciliation loop.

* The logic to handle '''remoteType''' : cloud is handled separately from the client (ndbClient) through helper methods - This ensures that the client is responsible for only making get/post requests to the specified endpoint without worrying about whether it is for on-prem or cloud.

* The reconciliation loop is responsible for ensuring that the state of the system matches the desired state declared by the operator. It listens to events from the Kubernetes API server and performs the necessary actions to reconcile the system to the desired state. The business logic of the operator, on the other hand, is responsible for implementing the higher-level functionality of the operator, such as managing the deployment of a complex application or managing a database cluster.

* Even within the reconciliation logic, there are clear indications of following SRP. We have a separate file utils/secret.go to implement GetDataFromSecret and GetAllDataFromSecret - logic to get data from the resource denoted by the name/namespace combination.

===Operator Pattern===
The operator pattern is based on the idea of declarative configuration management. Rather than writing imperative code to perform a series of steps to reach a desired state, an operator declares the desired state of the system and uses the Kubernetes API to perform the necessary actions to reconcile the system to that desired state.

We continue to follow this pattern for our implementation. We have a manifest file for a NDB custom resource responsible for provisioning and managing PostgreSQL in AWS and the logic for it is handled in the controller code (controllers/database_controllers.go)

EXAMPLE MANIFEST FILE
...

apiVersion: ndb.nutanix.com/v1alpha1
kind: Database
metadata:
name: db
spec:
ndb:
remoteType: "cloud"
clusterId: "Nutanix Cluster Id"
credentialSecret : ndb-secret-name
server: https://[NDB IP]:8443/era/v1.0
skipCertificateVerification: true
databaseInstance:
databaseInstanceName: "Database Instance Name"
databaseNames:
- database_one
- database_two
- database_three
credentialSecret: db-instance-secret-name
size: 10
timezone: "UTC"
type: postgres
...

The controller logic and related helper code will use the newly created ''''remoteType'''' field to select the NDS endpoint and construct provisionRequest appropriately. We have distinguished the configuration(Manifest file) and the logic(controller code) this way.

==Implementation related to this project==

This section will explain the changes that are made/to-be-made as part of this project.

'''NOTE: MOST OF THESE CHANGES HAVE NOT BEEN MADE NOW SINCE OUR TEAM HAS NOT OBTAINED ACCESS TO CONNECT THE SERVICE WITH EC2 INSTANCE FROM THE NUTANIX TEAM'''
'''We will tell the changes that have not been made and they will be made in the future'''

===Generate Secrets manifest and set name field + credentials in it===
This secrets.yaml manifest file is to create a Secret resource instance that is will have the data regarding the NDB instance name and database credentials.

'''Note: We have created this file, but currently have template values. After getting access, we will fill them accordingly'''

===Modify database_types.go to have allow remote types===
We have added a '''remoteType''' field to the NDB struct along with validation markers
...
// +kubebuilder:validation:Required;Enum=cloud;on-prem
RemoteType string `json:"remoteType"`

===Add helper method to figure out endpoint depending on remote type===
We currently have this logic as part of the ndbClient and should be moved to a separate helper method. The purpose of this function is to determine if the provisioning is for on-prem or cloud.
...
type NDBClient struct {
username string
password string
url string
remoteType string
client *http.Client
}
...
===Modify helper methods to generate request, provision database and setup connectivity===
Below are the functions that are core to the provisioning logic. They are written for provisioning a database in Nutanix cloud infrastructure(on-prem). These functions constitute the primary part of the provisioning workflow.

====GenerateProvisioningRequest(database_reconciler_helpers.go)====
This data structure should be modified/extended to include fields that are required to be part of the JSON request for the NDS endpoint that pertains to provisioning on cloud. Adding fields with appropriate data type is required here to serialize and deserialize requests properly.

====ProvisionDatabase(database_reconciler_helpers.go)====
This method takes care of posting a request to the NDS endpoint to provision database. Should be modified to allow the same on cloud. This method should take care of a helper method logic to determine the correct request data structure and endpoint to be used. We have currently implemented a part of this logic as part of the ndbClient.
'''Note: This can only be implemented after obtaining the access. We are still waiting on it and will implement it as soon as we obtain it.'''

====SetupConnectivity(database_reconciler_helpers.go)====
This function checks and creates a new service (without label selectors) if it does not exists and also sets up the database as the owner for the created service. This also checks and creates an endpoints object for the service if it does not already exists.

'''Note: This is the last part of the serial steps. The previous steps must be completed in order to write this logic.'''

==Test plan==
===Runing tests===
Pull the code, cd into the directory and run:

make test

This runs all the required tests as specified in the Makefile.

===Test Scenarios===
# Testing construction of provision request - All test cases for this that are currently written for on-prem must work for cloud '''remoteType''' too.
# Additional tests for fields that are mandatory for requests constructed for provisioning in cloud
# Verify that the correct API endpoints are being hit based on the remote type specified - On-Prem should hit 0.9 API and cloud should hit 1.0 API
# Test the connectivity between the K8 service and the deployed DB on EC2.

===Actions to be taken before Manual testing===
Manual testing would require you to get access to the Nutanix Database Service

# Obtain access to Nutanix Database Service SASS and access keys for the AWS EC2 instance.

# Add the appropriate credentials to the secrets.yaml file.

# Apply your manifest instance using :
kubectl apply -f <your manifest instance file>

You can verify the status of provisioning in your NDB dashboard.

==Future Implementation==
* Obtain EC2 keys and connect NDS to the EC2 instance
* Construct provision request for provisioning DB in cloud
* Add logic to select proper endpoint and request based on the '''remoteType'''
* Add logic to de-provision the database.

==Github==
Repo(Public): https://github.com/arvindsrinivas1/ndb-operator
Pull Request: https://github.com/nutanix-cloud-native/ndb-operator/pull/73

==Contributors==
Arvind Srinivas Subramanian (asubram9)

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-22T01:36:34Z

Asubram9: Architecture explanation

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==

==Note to the reviewer==
A major part of the logic can only be written after we get access to the keys of an EC2 instance that we can connect to the Nutanix Database Service. This differs from other NTNX teams since we need to provision the database on AWS. Hence, we cannot use the Test Nutanix endpoint to create clusterID or Secret's name. We have added a '''Note''' section in the Implementation section to inform the parts which fall under this category. We have also added this information collectively under the '''Future Implementation''' section.

==Problem Statement==
Nutanix Database Service is the only hybrid cloud database-as-a-service for Microsoft SQL
Server, Oracle Database, PostgreSQL, MongoDB, and MySQL. Currently, using the NDB operator, you can only provision a database on the Nutanix Cloud Infrastructure(on-prem).This project aims to extend the Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on cloud - specifically on AWS EC2. We aim to do this in a scalable way so that changing the cloud provider to say Azure or GCP would require changes only to the configuration file(CRD Manifest).

==Architecture==
The important components here are:
# Kubernetes Cluster
# Kubernetes Service that almost acts like a gateway to the provisioned database
The application pods that want to communicate with the database do so through the K8 service and endpoint that is created as part of the last step of provisioning. The workflow is explained in detail in the next section.
# Nutanix Database Service
The operator pod running in the K8 cluster communicates with this service to provision, de-provision, obtain status of the database etc.
# Any of the cloud platforms (AWS in our case)

[[File:Ntnx4arch.png|1000px|]]

==Workflow==
* Apply the instance CRD manifest file to create an instance of NDB resource in the K8 cluster

* The operator controller runs its logic to build a provisioning request and sends it to the Nutanix Database Service endpoint pertaining to provision in cloud(AWS in our case)

* The reconcile function loops to take actions depending on the status of provisioning - Empty, Provisioning and Ready

* Once the database is provisioned in AWS and the status is in Ready, we set up a Kubernetes networking service and an endpoint with the same name in the cluster. Any application pod in this cluster will communicate with this endpoint rather than communicating directly with the database.

[[File:Workflowntnx4.png|1000px|]]

==Design==
===Single Responsibility Principle(SRP)===
SRP is adopted here to separate the business logic of the operator from the reconciliation loop.

* The logic to handle '''remoteType''' : cloud is handled separately from the client (ndbClient) through helper methods - This ensures that the client is responsible for only making get/post requests to the specified endpoint without worrying about whether it is for on-prem or cloud.

* The reconciliation loop is responsible for ensuring that the state of the system matches the desired state declared by the operator. It listens to events from the Kubernetes API server and performs the necessary actions to reconcile the system to the desired state. The business logic of the operator, on the other hand, is responsible for implementing the higher-level functionality of the operator, such as managing the deployment of a complex application or managing a database cluster.

* Even within the reconciliation logic, there are clear indications of following SRP. We have a separate file utils/secret.go to implement GetDataFromSecret and GetAllDataFromSecret - logic to get data from the resource denoted by the name/namespace combination.

===Operator Pattern===
The operator pattern is based on the idea of declarative configuration management. Rather than writing imperative code to perform a series of steps to reach a desired state, an operator declares the desired state of the system and uses the Kubernetes API to perform the necessary actions to reconcile the system to that desired state.

We continue to follow this pattern for our implementation. We have a manifest file for a NDB custom resource responsible for provisioning and managing PostgreSQL in AWS and the logic for it is handled in the controller code (controllers/database_controllers.go)

EXAMPLE MANIFEST FILE
...

apiVersion: ndb.nutanix.com/v1alpha1
kind: Database
metadata:
name: db
spec:
ndb:
remoteType: "cloud"
clusterId: "Nutanix Cluster Id"
credentialSecret : ndb-secret-name
server: https://[NDB IP]:8443/era/v1.0
skipCertificateVerification: true
databaseInstance:
databaseInstanceName: "Database Instance Name"
databaseNames:
- database_one
- database_two
- database_three
credentialSecret: db-instance-secret-name
size: 10
timezone: "UTC"
type: postgres
...

The controller logic and related helper code will use the newly created ''''remoteType'''' field to select the NDS endpoint and construct provisionRequest appropriately. We have distinguished the configuration(Manifest file) and the logic(controller code) this way.

==Implementation related to this project==

This section will explain the changes that are made/to-be-made as part of this project.

'''NOTE: MOST OF THESE CHANGES HAVE NOT BEEN MADE NOW SINCE OUR TEAM HAS NOT OBTAINED ACCESS TO CONNECT THE SERVICE WITH EC2 INSTANCE FROM THE NUTANIX TEAM'''
'''We will tell the changes that have not been made and they will be made in the future'''

===Generate Secrets manifest and set name field + credentials in it===
This secrets.yaml manifest file is to create a Secret resource instance that is will have the data regarding the NDB instance name and database credentials.

'''Note: We have created this file, but currently have template values. After getting access, we will fill them accordingly'''

===Modify database_types.go to have allow remote types===
We have added a '''remoteType''' field to the NDB struct along with validation markers
...
// +kubebuilder:validation:Required;Enum=cloud;on-prem
RemoteType string `json:"remoteType"`

===Add helper method to figure out endpoint depending on remote type===
We currently have this logic as part of the ndbClient and should be moved to a separate helper method. The purpose of this function is to determine if the provisioning is for on-prem or cloud.
...
type NDBClient struct {
username string
password string
url string
remoteType string
client *http.Client
}
...
===Modify helper methods to generate request, provision database and setup connectivity===
Below are the functions that are core to the provisioning logic. They are written for provisioning a database in Nutanix cloud infrastructure(on-prem). These functions constitute the primary part of the provisioning workflow.

====GenerateProvisioningRequest(database_reconciler_helpers.go)====
This data structure should be modified/extended to include fields that are required to be part of the JSON request for the NDS endpoint that pertains to provisioning on cloud. Adding fields with appropriate data type is required here to serialize and deserialize requests properly.

====ProvisionDatabase(database_reconciler_helpers.go)====
This method takes care of posting a request to the NDS endpoint to provision database. Should be modified to allow the same on cloud. This method should take care of a helper method logic to determine the correct request data structure and endpoint to be used. We have currently implemented a part of this logic as part of the ndbClient.
'''Note: This can only be implemented after obtaining the access. We are still waiting on it and will implement it as soon as we obtain it.'''

====SetupConnectivity(database_reconciler_helpers.go)====
This function checks and creates a new service (without label selectors) if it does not exists and also sets up the database as the owner for the created service. This also checks and creates an endpoints object for the service if it does not already exists.

'''Note: This is the last part of the serial steps. The previous steps must be completed in order to write this logic.'''

==Test plan==
===Runing tests===
Pull the code, cd into the directory and run:

make test

This runs all the required tests as specified in the Makefile.

===Test Scenarios===
# Testing construction of provision request - All test cases for this that are currently written for on-prem must work for cloud '''remoteType''' too.
# Additional tests for fields that are mandatory for requests constructed for provisioning in cloud
# Verify that the correct API endpoints are being hit based on the remote type specified - On-Prem should hit 0.9 API and cloud should hit 1.0 API
# Test the connectivity between the K8 service and the deployed DB on EC2.

===Actions to be taken before Manual testing===
Manual testing would require you to get access to the Nutanix Database Service

# Obtain access to Nutanix Database Service SASS and access keys for the AWS EC2 instance.

# Add the appropriate credentials to the secrets.yaml file.

# Apply your manifest instance using :
kubectl apply -f <your manifest instance file>

You can verify the status of provisioning in your NDB dashboard.

==Future Implementation==
* Obtain EC2 keys and connect NDS to the EC2 instance
* Construct provision request for provisioning DB in cloud
* Add logic to select proper endpoint and request based on the '''remoteType'''
* Add logic to de-provision the database.

==Github==
Repo(Public): https://github.com/arvindsrinivas1/ndb-operator
Pull Request: https://github.com/nutanix-cloud-native/ndb-operator/pull/73

==Contributors==
Arvind Srinivas Subramanian (asubram9)

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-22T01:34:53Z

Asubram9:

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==

==Note to the reviewer==
A major part of the logic can only be written after we get access to the keys of an EC2 instance that we can connect to the Nutanix Database Service. This differs from other NTNX teams since we need to provision the database on AWS. Hence, we cannot use the Test Nutanix endpoint to create clusterID or Secret's name. We have added a '''Note''' section in the Implementation section to inform the parts which fall under this category. We have also added this information collectively under the '''Future Implementation''' section.

==Problem Statement==
Nutanix Database Service is the only hybrid cloud database-as-a-service for Microsoft SQL
Server, Oracle Database, PostgreSQL, MongoDB, and MySQL. Currently, using the NDB operator, you can only provision a database on the Nutanix Cloud Infrastructure(on-prem).This project aims to extend the Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on cloud - specifically on AWS EC2. We aim to do this in a scalable way so that changing the cloud provider to say Azure or GCP would require changes only to the configuration file(CRD Manifest).

==Architecture==
The important components here are:
# Kubernetes Cluster
# Kubernetes Service that almost acts like a gateway to the provisioned database.
# Any of the cloud platforms (AWS in our case)

The application pods that want to communicate with the database do so through the K8 service and endpoint that is created as part of the last step of provisioning. The workflow is explained in detail in the next section.

[[File:Ntnx4arch.png|1000px|]]

==Workflow==
* Apply the instance CRD manifest file to create an instance of NDB resource in the K8 cluster

* The operator controller runs its logic to build a provisioning request and sends it to the Nutanix Database Service endpoint pertaining to provision in cloud(AWS in our case)

* The reconcile function loops to take actions depending on the status of provisioning - Empty, Provisioning and Ready

* Once the database is provisioned in AWS and the status is in Ready, we set up a Kubernetes networking service and an endpoint with the same name in the cluster. Any application pod in this cluster will communicate with this endpoint rather than communicating directly with the database.

[[File:Workflowntnx4.png|1000px|]]

==Design==
===Single Responsibility Principle(SRP)===
SRP is adopted here to separate the business logic of the operator from the reconciliation loop.

* The logic to handle '''remoteType''' : cloud is handled separately from the client (ndbClient) through helper methods - This ensures that the client is responsible for only making get/post requests to the specified endpoint without worrying about whether it is for on-prem or cloud.

* The reconciliation loop is responsible for ensuring that the state of the system matches the desired state declared by the operator. It listens to events from the Kubernetes API server and performs the necessary actions to reconcile the system to the desired state. The business logic of the operator, on the other hand, is responsible for implementing the higher-level functionality of the operator, such as managing the deployment of a complex application or managing a database cluster.

* Even within the reconciliation logic, there are clear indications of following SRP. We have a separate file utils/secret.go to implement GetDataFromSecret and GetAllDataFromSecret - logic to get data from the resource denoted by the name/namespace combination.

===Operator Pattern===
The operator pattern is based on the idea of declarative configuration management. Rather than writing imperative code to perform a series of steps to reach a desired state, an operator declares the desired state of the system and uses the Kubernetes API to perform the necessary actions to reconcile the system to that desired state.

We continue to follow this pattern for our implementation. We have a manifest file for a NDB custom resource responsible for provisioning and managing PostgreSQL in AWS and the logic for it is handled in the controller code (controllers/database_controllers.go)

EXAMPLE MANIFEST FILE
...

apiVersion: ndb.nutanix.com/v1alpha1
kind: Database
metadata:
name: db
spec:
ndb:
remoteType: "cloud"
clusterId: "Nutanix Cluster Id"
credentialSecret : ndb-secret-name
server: https://[NDB IP]:8443/era/v1.0
skipCertificateVerification: true
databaseInstance:
databaseInstanceName: "Database Instance Name"
databaseNames:
- database_one
- database_two
- database_three
credentialSecret: db-instance-secret-name
size: 10
timezone: "UTC"
type: postgres
...

The controller logic and related helper code will use the newly created ''''remoteType'''' field to select the NDS endpoint and construct provisionRequest appropriately. We have distinguished the configuration(Manifest file) and the logic(controller code) this way.

==Implementation related to this project==

This section will explain the changes that are made/to-be-made as part of this project.

'''NOTE: MOST OF THESE CHANGES HAVE NOT BEEN MADE NOW SINCE OUR TEAM HAS NOT OBTAINED ACCESS TO CONNECT THE SERVICE WITH EC2 INSTANCE FROM THE NUTANIX TEAM'''
'''We will tell the changes that have not been made and they will be made in the future'''

===Generate Secrets manifest and set name field + credentials in it===
This secrets.yaml manifest file is to create a Secret resource instance that is will have the data regarding the NDB instance name and database credentials.

'''Note: We have created this file, but currently have template values. After getting access, we will fill them accordingly'''

===Modify database_types.go to have allow remote types===
We have added a '''remoteType''' field to the NDB struct along with validation markers
...
// +kubebuilder:validation:Required;Enum=cloud;on-prem
RemoteType string `json:"remoteType"`

===Add helper method to figure out endpoint depending on remote type===
We currently have this logic as part of the ndbClient and should be moved to a separate helper method. The purpose of this function is to determine if the provisioning is for on-prem or cloud.
...
type NDBClient struct {
username string
password string
url string
remoteType string
client *http.Client
}
...
===Modify helper methods to generate request, provision database and setup connectivity===
Below are the functions that are core to the provisioning logic. They are written for provisioning a database in Nutanix cloud infrastructure(on-prem). These functions constitute the primary part of the provisioning workflow.

====GenerateProvisioningRequest(database_reconciler_helpers.go)====
This data structure should be modified/extended to include fields that are required to be part of the JSON request for the NDS endpoint that pertains to provisioning on cloud. Adding fields with appropriate data type is required here to serialize and deserialize requests properly.

====ProvisionDatabase(database_reconciler_helpers.go)====
This method takes care of posting a request to the NDS endpoint to provision database. Should be modified to allow the same on cloud. This method should take care of a helper method logic to determine the correct request data structure and endpoint to be used. We have currently implemented a part of this logic as part of the ndbClient.
'''Note: This can only be implemented after obtaining the access. We are still waiting on it and will implement it as soon as we obtain it.'''

====SetupConnectivity(database_reconciler_helpers.go)====
This function checks and creates a new service (without label selectors) if it does not exists and also sets up the database as the owner for the created service. This also checks and creates an endpoints object for the service if it does not already exists.

'''Note: This is the last part of the serial steps. The previous steps must be completed in order to write this logic.'''

==Test plan==
===Runing tests===
Pull the code, cd into the directory and run:

make test

This runs all the required tests as specified in the Makefile.

===Test Scenarios===
# Testing construction of provision request - All test cases for this that are currently written for on-prem must work for cloud '''remoteType''' too.
# Additional tests for fields that are mandatory for requests constructed for provisioning in cloud
# Verify that the correct API endpoints are being hit based on the remote type specified - On-Prem should hit 0.9 API and cloud should hit 1.0 API
# Test the connectivity between the K8 service and the deployed DB on EC2.

===Actions to be taken before Manual testing===
Manual testing would require you to get access to the Nutanix Database Service

# Obtain access to Nutanix Database Service SASS and access keys for the AWS EC2 instance.

# Add the appropriate credentials to the secrets.yaml file.

# Apply your manifest instance using :
kubectl apply -f <your manifest instance file>

You can verify the status of provisioning in your NDB dashboard.

==Future Implementation==
* Obtain EC2 keys and connect NDS to the EC2 instance
* Construct provision request for provisioning DB in cloud
* Add logic to select proper endpoint and request based on the '''remoteType'''
* Add logic to de-provision the database.

==Github==
Repo(Public): https://github.com/arvindsrinivas1/ndb-operator
Pull Request: https://github.com/nutanix-cloud-native/ndb-operator/pull/73

==Contributors==
Arvind Srinivas Subramanian (asubram9)

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-22T01:21:37Z

Asubram9: Second Draft

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==

==Problem Statement==
Nutanix Database Service is the only hybrid cloud database-as-a-service for Microsoft SQL
Server, Oracle Database, PostgreSQL, MongoDB, and MySQL. Efficiently manage hundreds to
thousands of databases. We need to extend the Nutanix Database Service Kubernetes operator to provision Postgres Single
Instance databases on AWS EC2.

==Architecture==
[[File:Ntnx4arch.png|1000px|]]

==Workflow==
[[File:Workflowntnx4.png|1000px|]]

* Apply the instance CRD manifest file to create an instance of NDB resource in the K8 cluster
* The operator controller runs its logic to build a provisioning request and sends it to the Nutanix Database Service endpoint pertaining to provision in cloud(AWS in our case)
* The reconcile function loops to take actions depending on the status of provisioning - Empty, Provisioning and Ready
* Once the database is provisioned in AWS and the status is in Ready, we set up a Kubernetes networking service and an endpoint with the same name in the cluster. Any application pod in this cluster will communicate with this endpoint rather than communicating directly with the database.

==Design==
===Single Responsibility Principle(SRP)===
SRP is adopted here to separate the business logic of the operator from the reconciliation loop.

* The logic to handle '''remoteType''' : cloud is handled separately from the client (ndbClient) through helper methods - This ensures that the client is responsible for only making get/post requests to the specified endpoint without worrying about whether it is for on-prem or cloud.

* The reconciliation loop is responsible for ensuring that the state of the system matches the desired state declared by the operator. It listens to events from the Kubernetes API server and performs the necessary actions to reconcile the system to the desired state. The business logic of the operator, on the other hand, is responsible for implementing the higher-level functionality of the operator, such as managing the deployment of a complex application or managing a database cluster.

* Even within the reconciliation logic, there are clear indications of following SRP. We have a separate file utils/secret.go to implement GetDataFromSecret and GetAllDataFromSecret - logic to get data from the resource denoted by the name/namespace combination.

===Operator Pattern===
The operator pattern is based on the idea of declarative configuration management. Rather than writing imperative code to perform a series of steps to reach a desired state, an operator declares the desired state of the system and uses the Kubernetes API to perform the necessary actions to reconcile the system to that desired state.

We continue to follow this pattern for our implementation. We have a manifest file for a NDB custom resource responsible for provisioning and managing PostgreSQL in AWS and the logic for it is handled in the controller code (controllers/database_controllers.go)

EXAMPLE MANIFEST FILE
...

apiVersion: ndb.nutanix.com/v1alpha1
kind: Database
metadata:
name: db
spec:
ndb:
remoteType: "cloud"
clusterId: "Nutanix Cluster Id"
credentialSecret : ndb-secret-name
server: https://[NDB IP]:8443/era/v1.0
skipCertificateVerification: true
databaseInstance:
databaseInstanceName: "Database Instance Name"
databaseNames:
- database_one
- database_two
- database_three
credentialSecret: db-instance-secret-name
size: 10
timezone: "UTC"
type: postgres
...

The controller logic and related helper code will use the newly created ''''remoteType'''' field to select the NDS endpoint and construct provisionRequest appropriately. We have distinguished the configuration(Manifest file) and the logic(controller code) this way.

==Implementation related to this project==

This section will explain the changes that are made/to-be-made as part of this project.

'''NOTE: MOST OF THESE CHANGES HAVE NOT BEEN MADE NOW SINCE OUR TEAM HAS NOT OBTAINED ACCESS TO CONNECT THE SERVICE WITH EC2 INSTANCE FROM THE NUTANIX TEAM'''
'''We will tell the changes that have not been made and they will be made in the future'''

===Generate Secrets manifest and set name field + credentials in it===
This secrets.yaml manifest file is to create a Secret resource instance that is will have the data regarding the NDB instance name and database credentials.

'''Note: We have created this file, but currently have template values. After getting access, we will fill them accordingly'''

===Modify database_types.go to have allow remote types===
We have added a '''remoteType''' field to the NDB struct along with validation markers
...
// +kubebuilder:validation:Required;Enum=cloud;on-prem
RemoteType string `json:"remoteType"`

===Add helper method to figure out endpoint depending on remote type===
We currently have this logic as part of the ndbClient and should be moved to a separate helper method. The purpose of this function is to determine if the provisioning is for on-prem or cloud.
...
type NDBClient struct {
username string
password string
url string
remoteType string
client *http.Client
}
...
===Modify helper methods to generate request, provision database and setup connectivity===
Below are the functions that are core to the provisioning logic. They are written for provisioning a database in Nutanix cloud infrastructure(on-prem). These functions constitute the primary part of the provisioning workflow.

====GenerateProvisioningRequest(database_reconciler_helpers.go)====
This data structure should be modified/extended to include fields that are required to be part of the JSON request for the NDS endpoint that pertains to provisioning on cloud. Adding fields with appropriate data type is required here to serialize and deserialize requests properly.

====ProvisionDatabase(database_reconciler_helpers.go)====
This method takes care of posting a request to the NDS endpoint to provision database. Should be modified to allow the same on cloud. This method should take care of a helper method logic to determine the correct request data structure and endpoint to be used. We have currently implemented a part of this logic as part of the ndbClient.

====SetupConnectivity(database_reconciler_helpers.go)====
This function checks and creates a new service (without label selectors) if it does not exists and also sets up the database as the owner for the created service. This also checks and creates an endpoints object for the service if it does not already exists.

==Test plan==
===Runing tests===
Pull the code, cd into the directory and run:

make test

This runs all the required tests as specified in the Makefile.

===Test Scenarios===
# Testing construction of provision request - All test cases for this that are currently written for on-prem must work for cloud '''remoteType''' too.
# Additional tests for fields that are mandatory for requests constructed for provisioning in cloud
# Verify that the correct API endpoints are being hit based on the remote type specified - On-Prem should hit 0.9 API and cloud should hit 1.0 API
# Test the connectivity between the K8 service and the deployed DB on EC2.

===Actions to be taken before Manual testing===
Manual testing would require you to get access to the Nutanix Database Service

# Obtain access to Nutanix Database Service SASS and access keys for the AWS EC2 instance.

# Add the appropriate credentials to the secrets.yaml file.

# Apply your manifest instance using :
kubectl apply -f <your manifest instance file>

You can verify the status of provisioning in your NDB dashboard.

==Future implementation==
* Obtain EC2 keys and connect NDS to the EC2 instance
* Construct provision request for provisioning DB in cloud
* Add logic to select proper endpoint and request based on the '''remoteType'''
* Add logic to de-provision the database.

==Github==
Repo(Public): https://github.com/arvindsrinivas1/ndb-operator
Pull Request: https://github.com/nutanix-cloud-native/ndb-operator/pull/73

==Contributors==
Arvind Srinivas Subramanian (asubram9)

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-22T01:15:01Z

Asubram9: First draft

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==

==Problem Statement==
Nutanix Database Service is the only hybrid cloud database-as-a-service for Microsoft SQL
Server, Oracle Database, PostgreSQL, MongoDB, and MySQL. Efficiently manage hundreds to
thousands of databases. We need to extend the Nutanix Database Service Kubernetes operator to provision Postgres Single
Instance databases on AWS EC2.

==Architecture==
[[File:Ntnx4arch.png|1000px|]]

==Workflow==
[[File:Workflowntnx4.png|1000px|]]

* Apply the instance CRD manifest file to create an instance of NDB resource in the K8 cluster
* The operator controller runs its logic to build a provisioning request and sends it to the Nutanix Database Service endpoint pertaining to provision in cloud(AWS in our case)
* The reconcile function loops to take actions depending on the status of provisioning - Empty, Provisioning and Ready
* Once the database is provisioned in AWS and the status is in Ready, we set up a Kubernetes networking service and an endpoint with the same name in the cluster. Any application pod in this cluster will communicate with this endpoint rather than communicating directly with the database.

==Design==
===Single Responsibility Principle(SRP)===
SRP is adopted here to separate the business logic of the operator from the reconciliation loop.

* The logic to handle remoteType : cloud is handled separately from the client (ndbClient) through helper methods - This ensures that the client is responsible for only making get/post requests to the specified endpoint without worrying about whether it is for on-prem or cloud.

* The reconciliation loop is responsible for ensuring that the state of the system matches the desired state declared by the operator. It listens to events from the Kubernetes API server and performs the necessary actions to reconcile the system to the desired state. The business logic of the operator, on the other hand, is responsible for implementing the higher-level functionality of the operator, such as managing the deployment of a complex application or managing a database cluster.

* Even within the reconciliation logic, there are clear indications of following SRP. We have a separate file utils/secret.go to implement GetDataFromSecret and GetAllDataFromSecret - logic to get data from the resource denoted by the name/namespace combination.

===Operator Pattern===
The operator pattern is based on the idea of declarative configuration management. Rather than writing imperative code to perform a series of steps to reach a desired state, an operator declares the desired state of the system and uses the Kubernetes API to perform the necessary actions to reconcile the system to that desired state.

We continue to follow this pattern for our implementation. We have a manifest file for a NDB custom resource responsible for provisioning and managing PostgreSQL in AWS and the logic for it is handled in the controller code (controllers/database_controllers.go)

EXAMPLE MANIFEST FILE
...

apiVersion: ndb.nutanix.com/v1alpha1
kind: Database
metadata:
name: db
spec:
ndb:
remoteType: "cloud"
clusterId: "Nutanix Cluster Id"
credentialSecret : ndb-secret-name
server: https://[NDB IP]:8443/era/v1.0
skipCertificateVerification: true
databaseInstance:
databaseInstanceName: "Database Instance Name"
databaseNames:
- database_one
- database_two
- database_three
credentialSecret: db-instance-secret-name
size: 10
timezone: "UTC"
type: postgres
...

The controller logic and related helper code will use the newly created 'remoteType' field to select the NDS endpoint and construct provisionRequest appropriately. We have distinguished the configuration(Manifest file) and the logic(controller code) this way.

==Implementation related to this project==

This section will explain the changes that are made/to-be-made as part of this project.

'''NOTE: MOST OF THESE CHANGES HAVE NOT BEEN MADE NOW SINCE OUR TEAM HAS NOT OBTAINED ACCESS TO CONNECT THE SERVICE WITH EC2 INSTANCE FROM THE NUTANIX TEAM'''
'''We will tell the changes that have not been made and they will be made in the future'''

===Generate Secrets manifest and set name field + credentials in it===
This secrets.yaml manifest file is to create a Secret resource instance that is will have the data regarding the NDB instance name and database credentials.

'''Note: We have created this file, but currently have template values. After getting access, we will fill them accordingly'''

===Modify database_types.go to have allow remote types===
We have added a '''remoteType''' field to the NDB struct along with validation markers
...
// +kubebuilder:validation:Required;Enum=cloud;on-prem
RemoteType string `json:"remoteType"`

===Add helper method to figure out endpoint depending on remote type===
We currently have this logic as part of the ndbClient and should be moved to a separate helper method. The purpose of this function is to determine if the provisioning is for on-prem or cloud.

===Modify helper methods to generate request, provision database and setup connectivity===
Below are the functions that are core to the provisioning logic. They are written for provisioning a database in Nutanix cloud infrastructure(on-prem)
====GenerateProvisioningRequest(database_reconciler_helpers.go)====
This data structure should be modified/extended to include fields that are required to be part of the JSON request for the NDS endpoint that pertains to provisioning on cloud.
====ProvisionDatabase(database_reconciler_helpers.go)====
This method takes care of posting a request to the NDS endpoint to provision database. Should be modified to allow the same on cloud.
====SetupConnectivity(database_reconciler_helpers.go)====
This function checks and creates a new service (without label selectors) if it does not exists and also sets up the database as the owner for the created service. This also checks and creates an endpoints object for the service if it does not already exists.

==Test plan==
===Runing tests===
Pull the code, cd into the directory and run:

make test

This runs all the required tests as specified in the Makefile.

===Test Scenarios===
# Testing construction of provision request - All test cases for this that are currently written for on-prem must work for cloud remoteType too.
# Additional tests for fields that are mandatory for requests constructed for provisioning in cloud
# Verify that the correct API endpoints are being hit based on the remote type specified - On-Prem should hit 0.9 API and cloud should hit 1.0 API
# Test the connectivity between the K8 service and the deployed DB on EC2.

===Actions to be taken before Manual testing===
Manual testing would require you to get access to the Nutanix Database Service

# Obtain access to Nutanix Database Service SASS and access keys for the AWS EC2 instance.

# Add the appropriate credentials to the secrets.yaml file.

# Apply your manifest instance using :
kubectl apply -f <your manifest instance file>

You can verify the status of provisioning in your NDB dashboard.

==Future implementation==
* Obtain EC2 keys and connect NDS to the EC2 instance
* Construct provision request for provisioning DB in cloud
* Add logic to select proper endpoint and request based on the remoteType
* Add logic to de-provision the database.

==Github==
Repo(Public): https://github.com/arvindsrinivas1/ndb-operator
Pull Request: https://github.com/nutanix-cloud-native/ndb-operator/pull/73

==Contributors==
Arvind Srinivas Subramanian (asubram9)

File:Workflowntnx4.png

2023-03-21T23:48:26Z

Asubram9: Workflow of how NDB is used to provision database

Workflow of how NDB is used to provision database

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-21T23:47:45Z

Asubram9: Architecture

File:Ntnx4arch.png

2023-03-21T23:43:31Z

Asubram9: Architecture for the ntnx4 project

Architecture for the ntnx4 project

File:Architecture.png

2023-03-21T23:35:58Z

Asubram9: Architecture of the entire system, mentioning all the components involved in its working

Architecture of the entire system, mentioning all the components involved in its working

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-21T23:35:06Z

Asubram9: Problem Statement

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-21T21:38:07Z

Asubram9: Topics

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==

==Problem Statement==

==Goal of this Topic==

==Architecture==

==Workflow==
==Implementation==

==Test Plan==

===Actions to be taken before testing===

===Things to test===

==Future implementation==

==Github==

==Contributors==

CSC/ECE 517 Spring 2023- NTNX-4. Extend NDB operator provision postregresql aws

2023-03-21T19:30:29Z

Asubram9: Created page with "==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2=="

==NTNX-4. Extend Nutanix Database Service Kubernetes operator to provision Postgres Single Instance databases on AWS EC2==