use script id to find teacher feedback script level #38729
Conversation
davidsbailey
force-pushed
the
use-feedback-script-id
branch
from
ff6a4c4
to
4b0b0c0
Compare
davidsbailey
changed the base branch from
staging
to
require-feedback-script-id
| @@ -6,14 +6,23 @@ class TeacherFeedbacksController < ApplicationController | |||
| # Feedback from any verified teacher who has provided feedback to the current | |||
| # student on any level | |||
| def index | |||
| @feedbacks_as_student = @teacher_feedbacks.order(created_at: :desc).includes(script_level: {lesson: :script}).select do |feedback| | |||
| scope = { | |||
There was a problem hiding this comment.
@davidsbailey Can you point me to a good place to read about this?
There was a problem hiding this comment.
the effect of passing this to includes can be read about briefly here: https://api.rubyonrails.org/v6.1.1/classes/ActiveRecord/QueryMethods.html#method-i-includes the short version is that this syntax tells it which associated models to load, so that (in this case) when I call feedback.level.script_levels, no additional DB queries are needed. (the other nested fields are used by summary_for_feedback)
There are a few blog posts that go into way more detail about all the stuff that rails is doing behind the scenes when you use includes or other similar methods: https://bigbinary.com/blog/preload-vs-eager-load-vs-joins-vs-includes
FInishes PLAT-685. See migrating teacher feedback ids for more context.
It turns out the only place we were using TeacherFeedback.script_level was on the studio.code.org/feedback page. This PR updates that codepath to use script id instead of script level id, and adds a bit of test coverage.
Testing story
Future work
When viewing the script level page, we should show only feedback for that script. today, we show any feedback on that level in any script. This work is being tracked by learning platform (see slack).
Reviewer Checklist: