Add Course::Group and Course::GroupUser #161
Conversation
| course | ||
| creator | ||
| updater | ||
| name "group 1" |
There was a problem hiding this comment.
Prefer single-quoted strings when you don't need string interpolation or special symbols.
|
Dependency #140 |
allenwq
force-pushed
the
hanlin/groups-model
branch
3 times, most recently
from
7f29d55
to
7d2e023
Compare
| stampable | ||
| belongs_to :course, inverse_of: :groups | ||
| has_many :group_users, inverse_of: :group, dependent: :destroy, | ||
| class_name: Course::GroupUser.name, foreign_key: :course_group_id |
There was a problem hiding this comment.
Align the elements of a hash literal if they span more than one line.
allenwq
force-pushed
the
hanlin/groups-model
branch
3 times, most recently
from
463f0a7
to
d4042ea
Compare
| @@ -13,6 +13,8 @@ class Course < ActiveRecord::Base | |||
| has_many :announcements, inverse_of: :course, dependent: :destroy | |||
| has_many :achievements, inverse_of: :course, dependent: :destroy | |||
| has_many :levels, inverse_of: :course, dependent: :destroy | |||
| has_many :groups, inverse_of: :course, dependent: :destroy, class_name: Course::Group.name | |||
| has_many :group_users, through: :groups | |||
There was a problem hiding this comment.
I don't think this association makes sense.
allenwq
force-pushed
the
hanlin/groups-model
branch
from
d4042ea
to
bc8ec5f
Compare
| private | ||
|
|
||
| def user_and_group_in_same_course #:nodoc: | ||
| unless user.courses.include?(group.course) |
There was a problem hiding this comment.
Use a guard clause instead of wrapping the code inside a conditional expression.
allenwq
force-pushed
the
hanlin/groups-model
branch
2 times, most recently
from
c2ef135
to
c31a709
Compare
| def change | ||
| create_table :course_groups do |t| | ||
| t.string :name, null: false, default: '' | ||
| t.belongs_to :course, null: false |
allenwq
force-pushed
the
hanlin/groups-model
branch
from
c31a709
to
1226b97
Compare
|
@lowjoel Can you review this again ? thanks ! |
| class Course::Group < ActiveRecord::Base | ||
| stampable | ||
| belongs_to :course, inverse_of: :groups | ||
| has_many :group_users, inverse_of: :group, dependent: :destroy, |
There was a problem hiding this comment.
do we need the foreign_key key?
There was a problem hiding this comment.
@lowjoel Thanks ! It does not need the foreign_key here and I've removed it. However, we need to specify the foreign_key in group_users side.
| @@ -11,6 +11,9 @@ class User < ActiveRecord::Base | |||
| has_many :instances, through: :instance_users | |||
| has_many :course_users, inverse_of: :user, dependent: :destroy | |||
| has_many :courses, through: :course_users | |||
| has_many :course_group_users, inverse_of: :user, dependent: :destroy, | |||
| class_name: Course::GroupUser.name | |||
| has_many :groups, through: :course_group_users | |||
There was a problem hiding this comment.
This should be :course_groups, with an alias on Course::GroupUser
allenwq
force-pushed
the
hanlin/groups-model
branch
from
64eb1fb
to
c15ccd5
Compare
| create_table :course_group_users do |t| | ||
| t.belongs_to :course_group, null: false | ||
| t.belongs_to :user, null: false | ||
| t.integer :role, null: false, default: Course::GroupUser.roles[:normal] |
There was a problem hiding this comment.
By right this should be a constant 0, but okay.
There was a problem hiding this comment.
set defaults in the model, then.
allenwq
force-pushed
the
hanlin/groups-model
branch
2 times, most recently
from
992cfcf
to
e6d5c26
Compare
allenwq
force-pushed
the
hanlin/groups-model
branch
from
e6d5c26
to
1982920
Compare
| belongs_to :course, inverse_of: :groups | ||
| has_many :group_users, inverse_of: :course_group, dependent: :destroy, | ||
| class_name: Course::GroupUser.name | ||
| has_many :users, through: :group_users |
There was a problem hiding this comment.
empty name validation, no default owner?
allenwq
force-pushed
the
hanlin/groups-model
branch
from
1982920
to
636cb80
Compare
allenwq
force-pushed
the
hanlin/groups-model
branch
3 times, most recently
from
460cc62
to
301405d
Compare
|
@lowjoel I've updated here, Added a new unique index on group_name and course_id |
| class Course::Group < ActiveRecord::Base | ||
| stampable | ||
|
|
||
| before_validation :set_defaults, if: :new_record? |
There was a problem hiding this comment.
after_initialize and before_validate, and allow it to run twice. Sometimes not all the arguments are given at initialise.
allenwq
force-pushed
the
hanlin/groups-model
branch
from
301405d
to
4ee7baa
Compare
|
@lowjoel Can this be merged ? |
Add Course::Group and Course::GroupUser
No description provided.