Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

E2451. Reimplement feedback_response_map.rb #144

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

E2451. Reimplement feedback_response_map.rb #144

wants to merge 32 commits into from

Conversation

TiptonG
Copy link

@TiptonG TiptonG commented Dec 4, 2024

Change Descriptions / Recap

Changes to the Model:

  • Feedback_response_report method with all implementation vs. the addition of the 3 private helper methods
    • Identified 3 chunks of the method that were complex enough to warrant their own method call, and spliced them out into private class methods (get_feedback_authors, varying_rubrics_report, static_rubrics_report).
    • From there, reworked varying_rubrics_report to use a more streamlined data structure (2D array instead of 3 separate arrays), then generalized this data structure to adapt to more than three rounds (now able to adapt to all numeric rounds).
    • Removed a redundant conditional from the end of the original method, merging the functionality into the original conditional.
  • Conformed inconsistent logic (next if vs unless in what is now self.static_rubrics_report) into a consistent variation for maintainability.
  • Changed some variable names because they were either too long, not descriptive enough, or both: temp_review_response_map_ids to review_response_map_ids, temp_review_responses to review_responses (in the helper method), and all_review_response_ids_round_one/two/three to all_review_response_ids_rounds
  • Rename get_title to title to adhere to rubocop guidelines
  • Added comments explaining every step of the way (even to methods that were unchanged).
  • Fixed all Rubocop issues with the feedback_reponse_map model

Changes to the Spec Test:

  • Commented out references to questionnaires as they are not implemented in reimplementation-back-end
  • Commented out test for displaying review HTML as that functionality does not exist in reimplementation-back-end
  • Added assignment_team.rb and assignment_team_analytic.rb as the existing spec tests depended on those 2 files. No changes were made to these files other than commenting out include Scoring in assignment_team.rb
  • Added factories for response, course, institution, and role
  • Fixed factory formatting to function properly
  • Fixed all Rubocop issues with the feedback_reponse_map spec test as well as both factory files

Other Changes:

  • Added type and calibrate_to fields to the response_maps schema since those existed in expertiza/expertiza but not reimplementation-back-end

Resources:

Wiki Link:

Test Explanation / Demo Video Link:

TiptonG and others added 30 commits November 16, 2024 19:59
@TiptonG
original model and model spec files
38baf8f 
Present for convenient review of old material, and for use in case of the spec file (our project likely will not include a test revamp)
@TiptonG
dos2unix + test debugging
d061378 
Trying to get existing tests to pass for a good baseline
@TiptonG
Test fix attempt
255e213 
Adding necessary factory methods
commenting out elements that do not exist in reimplementation back end yet.
@Grochocinski
added import for factory bot dependencies
db3b576
@TiptonG
Fixing the factory formatting
f38d3e6
@TiptonG
Modularize self.feedback_response_map
14c728c 
Separate self.feedback_response_map functionality across several private methods to reduce complexity.
NOTES:
* These changes are in line with the documentation, with the only exception being that self.get_feedback_authors() requires an argument I did not forsee
* These changes are untested
* The variable names might want to be changed in the future, for the time being I stuck largely with what was there already
* Some of the code might want to be displayed better in the future, the "next if" section on line 126 still vexes me.
@TiptonG
Fix naming issue
903cdbb
@TiptonG
Add Comments
bdccaaf 
Add header comments to all methods without them.
Add inner comments to the private methods used in self.feedback_response_report

NEXT STEPS:
* Remove unnecessary comments from self.feedback_response_report
* Rework self.varying_rubrics_report to have dynamic dictionary
@TiptonG
Attempted rework of self.varying_rubrics_report
7dbf99e 
The old self.varying_rubrics_report has been commented out, and changed to self.varying_rubrics_report_old

The new self.varying_rubrics_report starts with an empty dictionary, and has the keys added to it when a new one appears (the value is initialized to an empty array if the value is initially missing, that is what ||= does).
I added a note above line 141, since I'm not sure if it's interacting the way we want it to... but that WAS the logic of the existing function, so I don't want to touch it.

This change is untested, I have not ran the tests in a while, and do not want to go through the hassle of docker at the moment. PLEASE LET ME KNOW if these changes do not work.
@ssdaniel1824
Modified feedback_response_map.rb
95ed78f 
Removed colons on lines 116, 132. Also removed the return keyword in lines 128, 150, 180. Lastly, added a comment mentioning that a "next if" keyword should be changed to "next unless", in line 141.
@ssdaniel1824
Modified feedback_response_map.rb using RuboCop
0e1b78a 
Removed an extra colon in line 169, as well as the comment made in the previous commit. All changes were suggested by RuboCop.
There is one error that rubocop notes, "Lint/Syntax: unexpected token kEND" at line 151, for the "end" keyword. I am unsure of how to remove this rubocop offense.
Moved the @temp_response_map_ids into the 2 helper methods, varying_rubrics_report and static_rubrics_report
@Grochocinski
fixed invalid formatting for factory data
15d0ef3
@Grochocinski
added missing defs for private functions
02bc923
@Grochocinski
added missing fields to response_maps schema
531e608
@Grochocinski
Imported missing required files from expertiza/expertiza
4574843
@Grochocinski
Updated factories and tests to (mostly) run tests without errors
6253fce
@Grochocinski
fixed incorrect author variable reference
7d9b20d
@Grochocinski
adapted test cases to match reimplementation-back-end
a622b74
@Grochocinski
fixed invalid author return param, fixed calling order on a non-activ…
e3ba300 
…e record
@Grochocinski
added TODO comments for unimplemented functionality in tests
97e0a81
@TiptonG
Update comments in self.feedback_response_report
2295715
@ssdaniel1824
Rubocop offenses
85b09bb 
Commented out unneeded lines, fixed indent and spacing problems, and added a "next" to an "unless" statement at line 166.
@TiptonG
dos2unix + removing "next" before "unless"
58f376f 
The next before unless breaks everything, despite one of the code smell tools thinking that it is necessary.
@TiptonG
Swap to next if from unless
2d39d9c
@Grochocinski
fixed all spec test rubocop violations except for blocklength
9e7a594
@Grochocinski
disabled blocklength check for spec test block
3bd5afc
@Grochocinski
fixed rubocop violations in both factory files
e7d3e0a
@TiptonG
Fixing rubocop issues
5c6b386 
Remove old code comments, since I got the screenshots with them that I want.
Disable rubocop AbcSize metrics for the email method, since that is not our concern per the project description.
@TiptonG
Update feedback_response_report varying conditional
a736574 
Generalizes return case to return ALL values of key-value pairs in the dictionary IN KEY ORDER.
@TiptonG
Removing unnecessary comment
423dc2e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TiptonG @Grochocinski @ssdaniel1824