anastasiaalt / quiz

Anastasia's Ruby on Rails Project
1 stars 0 forks source link

Yikes... Did I forget to make a Cohort Model/table? #2

Closed anastasiaalt closed 8 years ago

anastasiaalt commented 8 years ago

Hi. Was reviewing all my user stories and read this line again:

When an instructor creates a new cohort through the app, they should be able to enroll students by email address. Each student should receive an email with a link back to the app for them to either sign up or login, unless they've already been registered for a previous class.

My first thought was to add an email field for instructor and student (easy). My next thought was that Cohort ought to be it's own Model. However, before I F with my amazing seed file that is totally working and amazing or otherwise mess with my good vibes, thought I'd layout this game plan and get a quick "yay" or "nay" before I officially kick off the journey of views/controllers/etc.

I updated the ERD to show what I think the Cohort/Student/Instructor relationship would be.

https://docs.google.com/presentation/d/1HzwgKg0xh0RhoiE93FZ10NVQLtj4EVdiWVDYCVZr0tw/edit?usp=sharing

And think the Cohort model would look like this

class Cohort < ActiveRecord::Base
  belongs_to :instructor

  has_many :students
end

and the Student and Instructor models would need to updated as below:

class Student < ActiveRecord::Base
  has_secure_password

  has_many :submissions
  has_many :chosen_options, through: :submissions, source: :option

  has_one :cohort
end

Not sure exactly if this should be has_one versus belongs_to in this case... The internet isn't super clear either (http://stackoverflow.com/questions/3808926/whats-the-difference-between-belongs-to-and-has-one)

class Instructor < ActiveRecord::Base
  has_secure_password

  has_many :quizzes
  has_many :cohorts
end

Lastly, the migrations would need to be made and updated as follows:

1) Create a cohort table / migration like the below

class CreateCohorts < ActiveRecord::Migration
  def change
    create_table :cohorts do |t|
      t.string :name, :null => false

      t.references :instructor
    end
  end
end

2) Update student migration

class CreateStudents < ActiveRecord::Migration
  def change
    create_table :students do |t|
      t.string :first_name, :null => false
      t.string :last_name, :null => false

      t.string :username, :null => false
      t.string :password_digest, :null => false

      t.references :cohort
    end
  end
end
Avizacherman commented 8 years ago

Anastasia,

I can clearly see your thought process which is great. Think of SQL this way, try to minimize the amount of relationships that exist. The more you add the more complexity there is. Do you really need a table that assigns students to an instructor or can that just be done naturally through a student - instructor relationship? Does that make sense?

anastasiaalt commented 8 years ago

Hi. I agree with wanting to minimize tables but I would think that based on what the prompt asks, you'd need a unique Cohort ID for each student to effectively group and organize them this way. The only other possibility I could imagine would be to have a :cohort column in both instructor and student with a string in it that I would match but this wouldn't really be assigning a student to a cohort in the strictest sense... Thoughts on how to proceed?

Avizacherman commented 8 years ago

I looked at your ERD and think of it this way. Your students are currently not connected to the instructor at all. Connecting them through a table which may contain only one column non-reference column expensive, especially when you scale an application up. It will still work and for the scale of this application probably won't matter, but never be afraid to think bigger. Another way to approach this is tohave them linked directly (an instructor has students, a student has an instructor). If you want to have a cohort name it could just be a column on one of those tables. Does that make sense?

anastasiaalt commented 8 years ago

This does make sense that you'd want to reduce complexity and avoid the additional table. I also understand how establishing a relationship directly between student and instructor is one possible solution in this case. However, an instructor could have students in different cohorts and (so there is no unique identifier between them) and there is a user story to create a password for the students upon joining using the cohort name. So in order to use the Cohort information easily, I am working to implement the Cohort model now. I will come back if this proves problematic but hopefully my thinking and outline above prevents this.

anastasiaalt commented 8 years ago

This was successful. The one change to the above I needed to make was to make the relationship belongs_to versus has_one for the Student model. If you could clarify/explain why that is (I mention in my note the Stack Overflow articles and online resources are not clear), that would be great

Avizacherman commented 8 years ago

Anastasia,

Part of setting up relationships in SQL and ActiveRecord in turn, is that there exists a parent - child relationships between tables. That is, one table OWNs another table. So an instructor table OWNs the cohort table. This means that the instructor should have the cohort, and the cohort belongs to the instructor. Same thing where the cohort table OWNs the students. It just determines which table has the foreign key or reference that points to the other table. In this instance, since the student belongs_to the cohort, the foreign key linking them is on the student.

Does this make sense?

short-matthew-f commented 8 years ago

This seems resolved, yes?

anastasiaalt commented 8 years ago

Yes. I made the Model though :)

Thanks.

Anastasia

Anastasia Alt cell: (917) 232-4106 e-mail: anastasia.alt@gmail.com

"Yesterday I was clever, so I wanted to change the world. Today I am wise, so I am changing myself." - Rumi

On Thu, Feb 11, 2016 at 10:19 AM, Matthew Short notifications@github.com wrote:

This seems resolved, yes?

— Reply to this email directly or view it on GitHub https://github.com/anastasiaalt/quiz/issues/2#issuecomment-182911175.