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.

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

make script_id required in teacher_feedbacks #38568

Merged
merged 8 commits into from
Jan 27, 2021

Conversation

davidsbailey
Copy link
Member

@davidsbailey davidsbailey commented Jan 14, 2021

Finishes PLAT-684. See migrating teacher feedback ids for more context.

Previous PRs:

This PR contains a migration which does the following:

  1. backfills data in non-production environments
  2. marks the script_id column as non-null
  3. adds validations to ensure script_id is present and equal to script_level.script_id
  4. fixes unit tests to meet new constraints

Testing story

Ran the migration forward and backward locally, which succeeds even when some TeacherFeedbacks have no script_id.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Privacy and Security impacts have been assessed
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

@davidsbailey davidsbailey changed the title Require feedback script backfill script_id and make it required in teacher_feedbacks Jan 14, 2021
@davidsbailey davidsbailey changed the base branch from staging to backfill-feedback-script-id January 14, 2021 20:50
@davidsbailey davidsbailey marked this pull request as ready for review January 14, 2021 20:54
@davidsbailey davidsbailey changed the title backfill script_id and make it required in teacher_feedbacks make script_id required in teacher_feedbacks Jan 15, 2021
@davidsbailey davidsbailey requested a review from a team as a code owner January 15, 2021 01:29
@davidsbailey davidsbailey changed the base branch from backfill-feedback-script-id to staging January 15, 2021 02:46
@davidsbailey davidsbailey changed the base branch from staging to backfill-feedback-script-id January 15, 2021 08:04
Base automatically changed from backfill-feedback-script-id to staging January 15, 2021 17:36
belongs_to :level
belongs_to :script_level
belongs_to :teacher, class_name: 'User'
validate :validate_script_and_script_level, on: :create
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only do this on create, because we want update to succeed at

def increment_visit_count
even if the script_level_id has become invalid.

end
end

change_column :teacher_feedbacks, :script_id, :integer, null: false
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anyone hits an error in this step in their local environment because some TeacherFeedback objects do not have script ids, they can resolve it by removing those TeacherFeedback objects.

@davidsbailey davidsbailey merged commit ce43bd8 into staging Jan 27, 2021
@davidsbailey davidsbailey deleted the require-feedback-script-id branch January 27, 2021 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants