From 5aa45442a5d6d8de6f2e6cdf21e304c7c1d79791 Mon Sep 17 00:00:00 2001
From: Minh-Tu Le <minhtule05@gmail.com>
Date: Fri, 18 Dec 2015 14:06:19 +0800
Subject: [PATCH] Fix the query to retrieve achievement conditions for a
 particular conditional

---
 app/models/course/condition/achievement.rb    | 15 ++++----
 .../course_condition_achievements.rb          | 23 -------------
 .../course/condition/achievement_spec.rb      | 34 +++++++++++++++----
 3 files changed, 37 insertions(+), 35 deletions(-)

diff --git a/app/models/course/condition/achievement.rb b/app/models/course/condition/achievement.rb
index 369cefefb0a..ee28a0783fc 100644
--- a/app/models/course/condition/achievement.rb
+++ b/app/models/course/condition/achievement.rb
@@ -22,8 +22,8 @@ def satisfied_by?(course_user)
 
   # Given a conditional object, returns all achievements that it requires.
   #
-  # @param [Object] conditional The object that is declared as acts_as_conditional and for which
-  #   returned achievements are required.
+  # @param [#conditions] conditional The object that is declared as acts_as_conditional and for
+  #   which returned achievements are required.
   # @return [Array<Course::Achievement>]
   def required_achievements_for(conditional)
     # Course::Condition::Achievement.
@@ -33,13 +33,16 @@ def required_achievements_for(conditional)
 
     # Workaround, pending the squeel bugfix (activerecord-hackery/squeel#390) that will allow
     # allow the above query to work without #reload
-    Course::Achievement.joins(
-      "INNER JOIN
+    Course::Achievement.joins(<<-SQL)
+      INNER JOIN
         (SELECT cca.achievement_id
           FROM course_condition_achievements cca INNER JOIN course_conditions cc
             ON cc.actable_type = 'Course::Condition::Achievement' AND cc.actable_id = cca.id
-            WHERE cc.conditional_id = #{conditional.id}) ids
-    ON ids.achievement_id = course_achievements.id")
+            WHERE cc.conditional_id = #{conditional.id}
+              AND cc.conditional_type = #{ActiveRecord::Base.sanitize(conditional.class.name)}
+        ) ids
+      ON ids.achievement_id = course_achievements.id
+    SQL
   end
 
   def validate_achievement_condition
diff --git a/spec/factories/course_condition_achievements.rb b/spec/factories/course_condition_achievements.rb
index f7d8ba1b57e..8e86a452cef 100644
--- a/spec/factories/course_condition_achievements.rb
+++ b/spec/factories/course_condition_achievements.rb
@@ -4,28 +4,5 @@
     course
     achievement
     association :conditional, factory: :course_achievement
-
-    after(:stub) do |achievement_condition, evaluator|
-      achievement_condition.achievement = build_stubbed(:course_achievement,
-                                                        course: evaluator.course)
-      achievement_condition.conditional = build_stubbed(:course_achievement,
-                                                        course: evaluator.course)
-    end
-
-    trait :self_referential do
-      after(:stub) do |achievement_condition, _|
-        achievement_condition.achievement = achievement_condition.conditional
-      end
-    end
-
-    trait :duplicate_child do
-      after(:build) do |achievement_condition, evaluator|
-        condition = build(:course_condition_achievement,
-                          course: evaluator.course,
-                          achievement: evaluator.achievement)
-        achievement_condition.conditional.conditions << condition.condition
-      end
-      to_create { |instance| instance.save(validate: false) }
-    end
   end
 end
diff --git a/spec/models/course/condition/achievement_spec.rb b/spec/models/course/condition/achievement_spec.rb
index 708aec8de5b..3c4047ecb17 100644
--- a/spec/models/course/condition/achievement_spec.rb
+++ b/spec/models/course/condition/achievement_spec.rb
@@ -10,9 +10,11 @@
     describe 'validations' do
       context 'when an achievement is its own condition' do
         subject do
-          build_stubbed(:course_condition_achievement, :self_referential,
-                        course: course).tap do |cca|
-            allow(cca).to receive(:achievement_id_changed?).and_return(true)
+          achievement = create(:achievement, course: course)
+          build_stubbed(:achievement_condition,
+                        course: course, achievement: achievement, conditional: achievement).
+            tap do |achievement_condition|
+            allow(achievement_condition).to receive(:achievement_id_changed?).and_return(true)
           end
         end
         it { is_expected.to_not be_valid }
@@ -20,11 +22,31 @@
 
       context "when an achievement is already included in its conditional's conditions" do
         subject do
-          create(:course_condition_achievement, :duplicate_child, course: course).tap do |cca|
-            allow(cca).to receive(:achievement_id_changed?).and_return(true)
+          existing_achievement_condition = create(:achievement_condition, course: course)
+          build(:achievement_condition,
+                course: course, conditional: existing_achievement_condition.conditional,
+                achievement: existing_achievement_condition.achievement)
           end
         end
-        it { is_expected.to_not be_valid }
+
+        it 'is not valid' do
+          expect(subject).to_not be_valid
+          expect(subject.errors[:achievement]).to_not be_blank
+        end
+      end
+
+      context 'when an achievement is required by another conditional with the same id' do
+        subject do
+          id = Time.now.to_i
+          assessment = create(:assessment, course: course, id: id)
+          achievement = create(:achievement, course: course, id: id)
+          required_achievement = create(:achievement, course: course)
+          create(:achievement_condition,
+                 course: course, achievement: required_achievement, conditional: assessment)
+          build_stubbed(:achievement_condition,
+                        course: course, achievement: required_achievement, conditional: achievement)
+        end
+        it { is_expected.to be_valid }
       end
     end