Feedback: require script_level_id, and other similar db validations

[Erin007]

LP - 638

After backfilling all of the empty script_level_id fields for teacher feedbacks on production, staging, test and levelbuilder, it's time to make script_level_id a required field.

For production, where accuracy matters more, I used a series of strategies to backfill script_level_id (#29900, #29822, #29753, #29711, #29701, #29676).

There weren't any teacher feedbacks without a script_level_id on staging. And for test and levelbuilder I ran:

TeacherFeedback.where(script_level_id: nil).each do |fb| fb.update_attributes(script_level_id: fb.level.script_levels[0].id) end

to select the first ScriptLevel associated with the saved Level.

[davidsbailey]

I would recommend enforcing this via schema constraint at the DB layer instead: https://stackoverflow.com/questions/5966840/how-to-change-a-nullable-column-to-not-nullable-in-a-rails-migration Here is some rationale for this: https://thoughtbot.com/blog/validation-database-constraint-or-both

1. Are you trying to prevent bad data from being written to the database? If so, you must have a schema constraint.
2. Are you preventing errors that your application user can fix for themselves? If so, you should use a model validation.

a missing script level seems to me like a programming error (not a thing a user can fix), so once you've added the schema constraint I don't think you'd need this model validation.

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (staging@cffd6d7). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff             @@
##             staging   #30122   +/-   ##
==========================================
  Coverage           ?   76.56%           
==========================================
  Files              ?      674           
  Lines              ?    27537           
  Branches           ?        0           
==========================================
  Hits               ?    21084           
  Misses             ?     6453           
  Partials           ?        0

Impacted Files	Coverage Δ
dashboard/app/models/teacher_feedback.rb	96% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cffd6d7...21a6ed9. Read the comment docs.

[davidsbailey]

Thank you Erin! Looks good.

[Erin007]

Thanks for the info @davidsbailey! I updated based on your suggestions. student_id, level_id and teacher_id also seem like fields where we want to prevent bad/incomplete data from going into the database. Do they need to be changed as well or ok to leave as model validations?

[davidsbailey]

Ah, good questions! For the same reasons, it seems like the validations on level_id, student_id and teacher_id would all be better as schema constraints.

[maddiedierker]

+1 to Dave's suggestion that this column (and the others you mentioned) should be enforced at the schema/database level. However, I think we should also have validations at the model level because those allow Rails to provide human-readable error messages when model constraints aren't met (more info from the Thoughtbot article Dave shared). Otherwise, the user (or, more likely, Honeybadger) would get the error from the database, which is less user-friendly.

[davidsbailey]

Keeping these validations sounds fine to me! Good point about HB error readability, Maddie

[davidsbailey]

Looks great!

[maddiedierker]

🎉 yay, thank you for adding these validations!

[Erin007]

I learned that we don't want to enforce student_id to not be null in the database because when a student is deleted we disassociate them from any feedback that has been left for them.