-
Notifications
You must be signed in to change notification settings - Fork 121
Feature/Create 'tutor_times' table and 'TutorTime' Model #51
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
base: development
Are you sure you want to change the base?
Feature/Create 'tutor_times' table and 'TutorTime' Model #51
Conversation
samindiii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, great work on the migration and model :)
I have tested your code and can verify that it works as intended, based off the ticket. Additionally, I have reviewed your code and suggested some minor adjustments to enhance code quality.
Once you have reviewed my comments, please inform me so that I can approve the changes.
|
@samindiii - changes have now been applied 🙂 |
samindiii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Martin! I'm happy with the changes and will be approving this!
b0ink
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @martindolores
I was able to pull your changes and migrate successfully, and confirmed it worked by retrieving the column names of the new table - just one note regarding the reference to the tutor in your table
| class CreateTutorTimes < ActiveRecord::Migration[7.1] | ||
| def change | ||
| create_table :tutor_times do |t| | ||
| t.references :user, null: false, foreign_key: { on_delete: :cascade } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the "user" here a tutor (staff) that is assigned within a unit? You could probably reference the staff from the unit_roles table instead, implying that we're referencing staff here and not students
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @b0ink :), I believe this would be a tutor, I can push up a change shortly.
Im assuming I would also have to change the model then too right to include the following:
class TutorTime < ApplicationRecord
...
belongs_to :unit_role
has_one :user, through: :unit_role
....
Description
Type of change
How Has This Been Tested?
doubtfire-api- if you are indoubtfire-deploy, simply run the following:cd doubtfire-apiMigration File:

Checklist
If involving code
Folders and Files Added/Modified