Fix validation for achievement conditions

[minhtule]

• Fix the query to retrieve achievement conditions of a particular conditional object
• Improve the naming
• Remove complicated trait in achievement condition factory

[minhtule]

@allenwq does this reflect correctly the behavior of the function?

[allenwq]

@MinhTu Yep, think it can be generalised.

And can we have a spec for it ? to test that it works for any conditionals.

[houndci-bot]

Useless assignment to variable - existing_achievement_condition.

[lowjoel]

I'm not entirely sure what's being fixed?

[lowjoel]

You should specify the module that all such classes implement.

[minhtule]

acts_as_conditional is defined under

module Extensions::Conditional::ActiveRecord::Base
  module ClassMethods
    # Functions from conditional-and-condition framework.
    # Declare this function in the conditional model that requires conditions.
    def acts_as_conditional
        ...
    end
  end
end

Any class subclassing ActiveRecord::Base can add acts_as_conditional. What module should I specify? ActiveRecord::Base? And how should I specify it in the documentation part?

[lowjoel]

Extensions::Conditiona...ConditionalInstanceMethods OR use duck typing. What are the methods each of these classes need to implement, and declare those methods. See the yard documentation.

[minhtule]

The main fix is in the SQL query.

[lowjoel]

Put this in a SQL heredoc block.

[lowjoel]

@minhtule do you know what heredoc is?

[minhtule]

Is it this one?

[lowjoel]

Yep. If you use <<-SQL RubyMine will syntax highlight SQL.

[minhtule]

@lowjoel please review!

[lowjoel]

@minhtule I stared at this for a while, I think finally get what you're trying to do.

Achievement conditions store a reference to their conditional, and the conditional is a polymorphic relation. So you want to make sure that two different polymorphic types with the same ID can exist. Is that what you're testing?

If that's what you're testing, what you should do is make the conditional_type and conditional_id columns a unique index, then this scenario doesn't really need to be tested.

[minhtule]

Making (conditional_type, conditional_id) a unique index in the general condition table means the conditional appears only once in the table, which mean a conditional can only have 1 condition?

[lowjoel]

Oh, then (conditional_type, conditional_id, condition_type, condition_id) as the unique key? Is that what you're trying to do?

[lowjoel]

Wait, hang on, is there a general condition table? I remember only specific conditional condition tables?

[lowjoel]

Oh, course_conditions -- it's an actable table, so the unique key should be (actable_id, actable_type, conditional_id, conditional_type). Does that make sense?

[minhtule]

Yes, we have a general condition table and it has 2 polymorphic relations: conditionals and specific conditions. Sure, it's definitely a better solution.

[minhtule]

Actually I just recall that this test is to catch the mistake in the SQL query fixed in this PR here. Please see the comment over there.

Adding the index will le us remove the validation of duplicated specific conditions though.

[lowjoel]

Okay, I see the problem.

Mark this extra test as a TODO, removing when activerecord-hackery/squeel#390 is fixed, just like the SQL query. Typically you'd be using squeel and not writing raw SQL. Do the same for the other PR that does something like this.

[minhtule]

@lowjoel This query gives false validation when 2 different conditionals with the same id have the same condition e.g. when achievement 2 requires achievement 1 and assessment 2 also requires achievement 1. This validation fails (while it's not supposed to). The test catches this error.

[minhtule]

@lowjoel added TODO in the spec