Achievement condition validations #425

dariusf · 2015-07-14T07:53:45Z

Depends on #416.

Achievements page now shows requirements
Front- and back-end validations for achievement conditions, and related specs

houndci-bot · 2015-07-14T07:53:50Z

spec/features/course/achievement_management_spec.rb

+          click_link I18n.t('course.condition.achievements.new.header')
+          expect(page).not_to have_selector("#condition_achievement_achievement_id >" \
+            "option[value='#{other_achievement.id}']")
+          expect(page).not_to have_selector("#condition_achievement_achievement_id >" \


Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2015-07-14T08:01:13Z

spec/features/course/achievement_management_spec.rb

+          _, achievement, other_achievement = create_achievement_condition
+          visit edit_course_achievement_path(course, achievement)
+          click_link I18n.t('course.condition.achievements.new.header')
+          expect(page).not_to have_selector("#condition_achievement_achievement_id >" \


Prefer single-quoted strings when you don't need string interpolation or special symbols.

houndci-bot · 2015-07-14T10:08:09Z

spec/models/course/condition/achievement_spec.rb

+
+  let!(:instance) { create(:instance) }
+  with_tenant(:instance) do
+


Extra empty line detected at block body beginning.

dariusf · 2015-07-14T10:32:09Z

lib/extensions/conditional/active_record/base.rb

+    # A human-readable name for each condition; usually just wraps a title
+    # or name field. Meant to be used in a polymorphic manner for views.
+    def name
+      fail 'To be implemented by each condition.'


How do we test this line? All models which declare acts_as_condition and override this method are covered. Do we include this module in a dummy object?

I think I told @minhtule to remove that line because it's no different from not defining the method. but there needs to be a place to put this info.

Yes, the most straightforward way is to include this in a class and not override the method.

By the way -- should this raise a NotImplementedError?

lowjoel · 2015-07-15T00:30:21Z

I'll review this again after #416 is merged.

lowjoel · 2015-07-15T07:30:00Z

app/controllers/course/controller.rb

+  # @param [Course::Achievement] conditional The conditional for which to return conditions.
+  # @return [Array<Course::Achievement>]
+  def valid_achievement_conditions(conditional)
+    Course::Achievement.all -


course.achievements

houndci-bot · 2015-07-15T09:51:35Z

spec/features/course/achievement_management_spec.rb

@@ -69,33 +69,60 @@
      end

      scenario 'I can create an achievement condition' do
+


Extra empty line detected at block body beginning.

lowjoel · 2015-07-29T00:31:07Z

app/models/course/condition/achievement.rb

+    #   where { condition.conditional.id == achievement.id }.
+    #   map(&:achievement)
+
+    # Workaround, pending the squeel bugfix (activerecord-hackery/squeel#390) that will allow


awesome work!

lowjoel · 2015-07-29T01:34:58Z

Sorry, another batch... now that I've sat down and tried out the i18n framework myself.

Added missing table headers Published status was shown regardless of ability to manage achievements

For use in polymorphic contexts where all require is the human-readable anme of the condition

lowjoel · 2015-07-29T09:24:21Z

app/models/course/condition/achievement.rb

@@ -3,4 +3,46 @@ class Course::Condition::Achievement < ActiveRecord::Base
  belongs_to :achievement, class_name: Course::Achievement.name, inverse_of: false

  default_scope { includes(:achievement) }
+
+  validate :validate_achievement_condition, if: :achievement_id_changed?


so did you check whether achievement_changed? is better?

(Wrote a reply but it seems to have disappeared...)

In general it's better because it checks the validity of the association and not just the presence of the key, but in this case it's equivalent, because the id will always point to an existing achievement with a valid id; no achievement is being created.

Also, _changed? methods are only provided for actual attributes, so there is no achievement_changed? by default (in contrast, you can do stuff like validates :achievement, presence: true on the association).

okay, great.

This ensures that there cannot be duplicate or self-referential achievement conditions

Conditions must be associated with a conditional. Achievement conditions must be associated with an achievement.

It fails FactoryGirl linting because it doesn't declare the now non-null `conditional` association. Since it cannot be directly instantiated, and the factories for things which act_as conditions do declare `conditional`, it isn't needed.

…finements Achievement condition validations

houndci-bot reviewed Jul 14, 2015
View reviewed changes

dariusf force-pushed the dariusf/condition-refinements branch from b6ff773 to e237ff3 Compare July 14, 2015 08:01

houndci-bot reviewed Jul 14, 2015
View reviewed changes

dariusf force-pushed the dariusf/condition-refinements branch 3 times, most recently from 5618100 to 3be3d55 Compare July 14, 2015 10:07

houndci-bot reviewed Jul 14, 2015
View reviewed changes

dariusf force-pushed the dariusf/condition-refinements branch 3 times, most recently from 156181d to fd618c5 Compare July 14, 2015 10:18

dariusf reviewed Jul 14, 2015
View reviewed changes

dariusf force-pushed the dariusf/condition-refinements branch 2 times, most recently from 2779f6a to 92ed2de Compare July 15, 2015 07:23

lowjoel reviewed Jul 15, 2015
View reviewed changes

dariusf force-pushed the dariusf/condition-refinements branch from 92ed2de to 0e3ebe8 Compare July 15, 2015 09:51

houndci-bot reviewed Jul 15, 2015
View reviewed changes

dariusf force-pushed the dariusf/condition-refinements branch 2 times, most recently from 29078b1 to cf09820 Compare July 28, 2015 12:03

lowjoel reviewed Jul 29, 2015
View reviewed changes

dariusf added 3 commits July 29, 2015 16:07

Fixed bugs with achievement page

da4454b

Added missing table headers Published status was shown regardless of ability to manage achievements

Added title method to conditions

ffe952b

For use in polymorphic contexts where all require is the human-readable anme of the condition

Displayed achievement requirements in index

acd24a7

dariusf force-pushed the dariusf/condition-refinements branch from cf09820 to ec95034 Compare July 29, 2015 08:11

lowjoel reviewed Jul 29, 2015
View reviewed changes

dariusf added 6 commits July 29, 2015 17:41

Added validations for achievement conditions

05dbfd2

This ensures that there cannot be duplicate or self-referential achievement conditions

Added matcher for content tags

06fdcce

Added specs for validations and cleaned up old specs

28989ed

Added non-null constraints to condition fields

3046664

Conditions must be associated with a conditional. Achievement conditions must be associated with an achievement.

Removed course_condition factory

e6a0f0d

It fails FactoryGirl linting because it doesn't declare the now non-null `conditional` association. Since it cannot be directly instantiated, and the factories for things which act_as conditions do declare `conditional`, it isn't needed.

Restructured activerecord translations

61e751d

dariusf force-pushed the dariusf/condition-refinements branch from ec95034 to 61e751d Compare July 29, 2015 09:45

lowjoel added a commit that referenced this pull request Jul 29, 2015

Merge pull request #425 from coursemology-collab/dariusf/condition-re…

977cb77

…finements Achievement condition validations

lowjoel merged commit 977cb77 into Coursemology:master Jul 29, 2015

dariusf deleted the dariusf/condition-refinements branch October 20, 2015 12:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Achievement condition validations #425

Achievement condition validations #425

dariusf commented Jul 14, 2015

houndci-bot Jul 14, 2015

houndci-bot Jul 14, 2015

houndci-bot Jul 14, 2015

dariusf Jul 14, 2015

lowjoel Jul 14, 2015

lowjoel commented Jul 15, 2015

lowjoel Jul 15, 2015

houndci-bot Jul 15, 2015

lowjoel Jul 29, 2015

lowjoel commented Jul 29, 2015

lowjoel Jul 29, 2015

dariusf Jul 29, 2015

lowjoel Jul 29, 2015


		let!(:instance) { create(:instance) }
		with_tenant(:instance) do

		@@ -69,33 +69,60 @@
		end

		scenario 'I can create an achievement condition' do

Achievement condition validations #425

Achievement condition validations #425

Conversation

dariusf commented Jul 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowjoel commented Jul 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowjoel commented Jul 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment