Merge pull request #660 from coursemology-collab/weiqingtoh/levels-re…

…factoring

Implement a default Level 0 for each course, and refactor levels_concern

app/models/components/course/levels_ability_component.rb

@@ -11,5 +11,7 @@ def define_permissions
 
   def allow_staff_manage_levels
     can :manage, Course::Level, course_staff_hash
+    # User cannot delete default level
+    cannot :destroy, Course::Level, experience_points_threshold: 0
   end
 end

app/models/concerns/course/levels_concern.rb

@@ -9,36 +9,29 @@ module Course::LevelsConcern
   # @return [Array<Course::Levels>] Array with numbered Course::Levels
   def numbered_levels
     levels.each_with_index.map do |level, index|
-      level.clone.tap { |l| l.level_number = index + 1 }
+      level.clone.tap { |l| l.level_number = index }
     end
   end
 
   # Returns Course::Level object corresponding to
   # the level that a course participant would have attained if
   # s/he had experience_points number of experience points.
   #
-  # The motivation for this method is to accommodate named levels.
-  #
-  # nil is returned if experience_points is insufficient to
-  # attain to any level, i.e. nil represents the zeroth level.
+  # If experience_points <= 0, the level is assumed to be the
+  # default level (the 0th level) with 0 experience_points threshold.
   #
   # @param [Fixnum] experience_points Number of Experience Points
-  # @return [Course::Level, nil] A Course::Level instance, or nil.
-  def compute_level(experience_points)
+  # @return [Course::Level] A Course::Level instance.
+  def level_for(experience_points)
     i = numbered_levels.rindex do |l|
       l.experience_points_threshold <= experience_points
     end
-    i ? numbered_levels[i] : nil
+    i ? numbered_levels[i] : numbered_levels.first
   end
 
-  # Returns the level number that a course participant would
-  # have attained if s/he had experience_points number of
-  # experience points.
-  #
-  # @param [Fixnum] experience_points Number of experience points
-  # @return [Fixnum] The level number for the given number of experience points
-  def compute_level_number(experience_points)
-    level = compute_level(experience_points)
-    level ? level.level_number : 0
+  # Test if the course has a default level.
+  # @return [Boolean] True if there is a default level, otherwise false.
+  def has_default_level?
+    levels.any?(&:default_level?)
   end
 end

app/models/course.rb

@@ -33,7 +33,7 @@ class Course < ActiveRecord::Base
   has_many :assessment_programming_evaluations,
            class_name: Course::Assessment::ProgrammingEvaluation.name, dependent: :destroy,
            inverse_of: :course
-  has_many :levels, dependent: :destroy
+  has_many :levels, dependent: :destroy, inverse_of: :course
   has_many :groups, dependent: :destroy, class_name: Course::Group.name
   has_many :lesson_plan_items, class_name: Course::LessonPlan::Item.name, dependent: :destroy
   has_many :lesson_plan_milestones, class_name: Course::LessonPlan::Milestone.name,

app/models/course/level.rb

@@ -1,9 +1,25 @@
 class Course::Level < ActiveRecord::Base
-  validates :experience_points_threshold, numericality: { greater_than: 0 }
+  include Course::ModelComponentHost::Component
+  validates :experience_points_threshold, numericality: { greater_than_or_equal_to: 0 }
+
   belongs_to :course, inverse_of: :levels
   default_scope { order(:experience_points_threshold) }
   attr_writer :level_number
 
+  def self.after_course_initialize(course)
+    return if course.persisted? || course.has_default_level?
+
+    course.levels.build(experience_points_threshold: 0)
+  end
+
+  # Returns true if level is a default level.
+  # Default level is currently implemented as a level with 0 threshold
+  #
+  # @return [Boolean]
+  def default_level?
+    experience_points_threshold == 0
+  end
+
   # Retrieves the level number of the current level,
   # relative to the other levels in the same course.
   # This number is set by Course::LevelsConcern#number_levels.

app/models/course_user.rb

@@ -63,7 +63,7 @@ def self.has_user?(user)
   #
   # @return [Fixnum] CourseUser level number
   def level_number
-    course.compute_level_number(experience_points)
+    course.level_for(experience_points).level_number
   end
 
   # Test whether this course_user is a staff (i.e. teaching_assistant, manager or owner)

db/migrate/20151214081508_add_unique_index_to_course_levels.rb

@@ -0,0 +1,8 @@
+class AddUniqueIndexToCourseLevels < ActiveRecord::Migration
+  def change
+    add_index :course_levels,
+              [:course_id, :experience_points_threshold],
+              unique: true,
+              name: 'index_experience_points_threshold_on_course_id'
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20151214080700) do
+ActiveRecord::Schema.define(version: 20151214081508) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -477,6 +477,7 @@
     t.datetime "created_at",                  null: false
     t.datetime "updated_at",                  null: false
   end
+  add_index "course_levels", ["course_id", "experience_points_threshold"], name: "index_experience_points_threshold_on_course_id", unique: true
 
   create_table "course_material_folders", force: :cascade do |t|
     t.integer  "parent_id",          index: {name: "fk__course_material_folders_parent_id"}, foreign_key: {references: "course_material_folders", name: "fk_course_material_folders_parent_id", on_update: :no_action, on_delete: :no_action}

spec/features/course/level_management_spec.rb

@@ -31,19 +31,16 @@
       scenario 'I can create a course level' do
         visit course_levels_path(course)
         find_link(nil, href: new_course_level_path(course)).click
-        fill_in 'level_experience_points_threshold', with: 100
+        fill_in 'level_experience_points_threshold', with: 400
 
-        expect do
-          click_button I18n.t('helpers.submit.level.create')
-        end.to change(course.levels, :count).by(1)
+        expect { click_button I18n.t('helpers.submit.level.create') }.
+          to change(course.levels, :count).by(1)
       end
 
       scenario 'I can delete a course level' do
         visit course_levels_path(course)
-
-        expect do
-          find_link(nil, href: course_level_path(course, levels[0])).click
-        end.to change(course.levels, :count).by(-1)
+        expect { find_link(nil, href: course_level_path(course, levels[0])).click }.
+          to change(course.levels, :count).by(-1)
       end
     end
   end

spec/models/course/level_ability_spec.rb

@@ -7,14 +7,16 @@
     subject { Ability.new(user) }
     let(:course) { create(:course) }
     let!(:level) { create(:course_level, course: course) }
+    let!(:default_level) { course.reload.levels.first }
 
     context 'when the user is a Course Staff' do
       let(:user) { create(:course_manager, :approved, course: course).user }
 
       it { is_expected.to be_able_to(:manage, level) }
+      it { is_expected.not_to be_able_to(:destroy, default_level) }
 
       it 'sees all levels' do
-        expect(course.levels.accessible_by(subject)).to contain_exactly(level)
+        expect(course.levels.accessible_by(subject)).to contain_exactly(*course.reload.levels)
       end
     end
   end

spec/models/course/level_spec.rb

@@ -2,11 +2,68 @@
 
 RSpec.describe Course::Level, type: :model do
   it { is_expected.to belong_to(:course).inverse_of(:levels) }
-  it { is_expected.to validate_numericality_of(:experience_points_threshold).is_greater_than(0) }
+  it 'ensures that experience points threshold is greater or equal to 0' do
+    expect(subject).
+      to validate_numericality_of(:experience_points_threshold).
+      is_greater_than_or_equal_to(0)
+  end
+
+  let!(:instance) { create(:instance) }
+  with_tenant(:instance) do
+    let!(:course) { build(:course) }
+
+    describe 'validations' do
+      describe 'uniqueness of experience points threshold' do
+        context 'when level have the same threshold as existing level' do
+          before { create(:course_level, course: course, experience_points_threshold: 100) }
+          subject { build(:course_level, course: course, experience_points_threshold: 100) }
+
+          it 'is invalid' do
+            expect(subject).not_to be_valid
+          end
+        end
+      end
+    end
+
+    context 'before level_number is set' do
+      it 'raises an IllegalStateError' do
+        expect { Course::Level.new.level_number }.to raise_error(IllegalStateError)
+      end
+    end
+
+    describe '.after_course_initialize' do
+      it 'builds one default level' do
+        expect(course.levels.size).to eq(1)
+      end
+
+      context 'when course is initialised again' do
+        it 'does not build another level' do
+          level = course.levels.first
+
+          # Call the callback one more time
+          Course::Level.after_course_initialize(course)
+          expect(course.levels.size).to eq(1)
+
+          course.save
+          expect(level).to be_persisted
+        end
+      end
+    end
+
+    describe '.default_level?' do
+      context 'when the level is a default level' do
+        it 'returns true' do
+          level = build(:course_level, experience_points_threshold: 0)
+          expect(level).to be_default_level
+        end
+      end
 
-  context 'before level_number is set' do
-    it 'raises' do
-      expect { Course::Level.new.level_number }.to raise_error(IllegalStateError)
+      context 'when the level is not a default level' do
+        it 'returns false' do
+          level = build(:course_level, experience_points_threshold: 1)
+          expect(level).not_to be_default_level
+        end
+      end
     end
   end
 end

spec/models/course_spec.rb

@@ -49,61 +49,40 @@
     describe 'levels' do
       let!(:user) { create(:administrator) }
       let!(:course) { create(:course) }
-      let!(:levels) do
-        create_list(:course_level, 5, course: course).map(&:experience_points_threshold)
-      end
+      let!(:levels) { create_list(:course_level, 5, course: course) }
+      before { course.reload }
 
       describe '.levels' do
         it 'returns levels is ascending order' do
-          course_level_numbers = course.levels.map(&:experience_points_threshold)
-          expect(course_level_numbers).to eq levels
-        end
-      end
-
-      describe '#compute_level' do
-        it 'returns nil when experience_points is 0' do
-          level = course.compute_level(0)
-          expect(level).to be_nil
+          level_thresholds = course.levels.map(&:experience_points_threshold)
+          expect(level_thresholds).to eq(level_thresholds.sort)
         end
       end
 
-      describe '#compute_level_number' do
-        context 'when experience_points is 0' do
-          it 'returns 0' do
-            level = course.compute_level_number(0)
-            expect(level).to eq 0
-          end
-        end
-
-        context 'when experience_points is between threshold' do
-          it 'returns the correct level number' do
-            experience_points = levels.max - 1
-            level = course.compute_level_number(experience_points)
-            expect(level).to eq levels.size - 1
-          end
-        end
-
-        context 'when experience_points coincides with a level threshold' do
-          it 'returns the correct level number' do
-            experience_points = levels[1]
-            level = course.compute_level_number(experience_points)
-            expect(level).to eq 2
+      describe '#level_for' do
+        context 'when experience_points is 0 or negative' do
+          it 'returns the first level' do
+            [0, -1].each do |experience_points|
+              expect(course.level_for(experience_points)).to be_default_level
+            end
           end
         end
 
-        context 'when experience_points exceeds all level thresholds' do
+        context 'when experience_points is a positive number' do
           it 'returns the correct level number' do
-            experience_points = levels.max + 1
-            level = course.compute_level_number(experience_points)
-            expect(level).to eq levels.size
+            course.numbered_levels.each do |level|
+              experience_points = level.experience_points_threshold
+              expect(course.level_for(experience_points)).to eq(level)
+              expect(course.level_for(experience_points + 1)).to eq(level)
+            end
           end
         end
       end
 
       describe '#numbered_levels' do
         it 'numbers levels' do
           numbering = course.numbered_levels.map(&:level_number)
-          expect(numbering).to eq((1..(levels.size)).to_a)
+          expect(numbering).to eq((0..(course.levels.count - 1)).to_a)
         end
       end
     end
@@ -241,5 +220,19 @@
         end
       end
     end
+
+    describe '#has_default_level?' do
+      let(:course) { build(:course) }
+      subject { course.has_default_level? }
+
+      context 'when course is a new record' do
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when course is persisted' do
+        before { course.save }
+        it { is_expected.to be_truthy }
+      end
+    end
   end
 end

spec/models/course_user_spec.rb

@@ -130,6 +130,7 @@
       before do
         create :course_level, course: course, experience_points_threshold: 100
         create :course_level, course: course, experience_points_threshold: 200
+        course.reload
       end
 
       context 'when student has no experience points' do
@@ -140,7 +141,9 @@
         before do
           create :course_experience_points_record, points_awarded: 150, course_user: student
         end
-        it { is_expected.to eq(1) }
+        it 'returns the correct level number' do
+          expect(subject).to eq(1)
+        end
       end
     end