deduction and interval should be required for penalty and penalty decay submission rules #648

Closed
hoboman313 opened this Issue Feb 11, 2012 · 13 comments

Comments

Projects
None yet
5 participants
@hoboman313

Steps to reproduce:

  • login as admin
  • create a new one or modify an existing assignment
  • under the "submission rules" tab select penalty formula or penalty decay formula
  • do not fill in any of the fields and submit
  • read the error message and notice how only the "hours" field is required to be filled in

It does not seem logical that only "hours" field is required for these two submission rules. For example, you could submit a penalty formula submission rule with only hours and a blank ( stored as null ) deduction field. I have not tested, but I assume that our code does not take into account and may break if we try to deduct "nil" every x hours.

Note that the hours/interval/deduction fields that I am referring to are the names of the fields in the db where these values are actually stored in the "periods" table. Currently only the hours field is enforced ( greater or equal to 0 ) in the periods model as that is the only field required for all three submission rules.

@Ayaya

This comment has been minimized.

Show comment
Hide comment
@Ayaya

Ayaya Feb 14, 2012

Contributor

I think I will pick this up.

It does make sense for deduction to be set to something that isn't nil just in case. Even if the deduction is 0, I think it would be better to require the user to enter some number into the field.

Contributor

Ayaya commented Feb 14, 2012

I think I will pick this up.

It does make sense for deduction to be set to something that isn't nil just in case. Even if the deduction is 0, I think it would be better to require the user to enter some number into the field.

@Ayaya

This comment has been minimized.

Show comment
Hide comment
@Ayaya

Ayaya Feb 14, 2012

Contributor

After digging around the code and trying to understand I'm also not quite sure how I can proceed with this issue, any ideas?

Contributor

Ayaya commented Feb 14, 2012

After digging around the code and trying to understand I'm also not quite sure how I can proceed with this issue, any ideas?

@Ayaya

This comment has been minimized.

Show comment
Hide comment
@Ayaya

Ayaya Feb 14, 2012

Contributor

I was thinking.. since you can't just add the enforcements to the periods table, perhaps there is some sort of conditional validation which can check :intervals and :deductions only if certain submission rules are selected.. not quite sure how I can do that..

Contributor

Ayaya commented Feb 14, 2012

I was thinking.. since you can't just add the enforcements to the periods table, perhaps there is some sort of conditional validation which can check :intervals and :deductions only if certain submission rules are selected.. not quite sure how I can do that..

@Ayaya

This comment has been minimized.

Show comment
Hide comment
@Ayaya

Ayaya Feb 25, 2012

Contributor

Hey, just coming back to this issue again, still not really sure how to use the conditional validation checks for the deduction and intervals.. would be cool if anyone had any ideas! :) thanks!

Contributor

Ayaya commented Feb 25, 2012

Hey, just coming back to this issue again, still not really sure how to use the conditional validation checks for the deduction and intervals.. would be cool if anyone had any ideas! :) thanks!

@jerboaa

This comment has been minimized.

Show comment
Hide comment
@jerboaa

jerboaa Feb 25, 2012

Member

@Ayaya what you'd want to do is to add an additonal validation in app/model/period.rb. We would need this validation to only trigger for penalty-decay and grace-period-submission rule. This can be done by adding a validation callback which takes a block (IIRC). Then use the passed in block to see if you want to make this save valid or not. I suggest to read up on Rails record validations :) HTH.

Member

jerboaa commented Feb 25, 2012

@Ayaya what you'd want to do is to add an additonal validation in app/model/period.rb. We would need this validation to only trigger for penalty-decay and grace-period-submission rule. This can be done by adding a validation callback which takes a block (IIRC). Then use the passed in block to see if you want to make this save valid or not. I suggest to read up on Rails record validations :) HTH.

@Ayaya

This comment has been minimized.

Show comment
Hide comment
@Ayaya

Ayaya Mar 18, 2012

Contributor

Hey @jerboaa , I spent quite some time looking into doing validation callbacks which takes a block but I still can't seem to figure out how to do it correctly. What I did spend time doing was having something like:

  • validate_presence_of :interval, :if => action, but the problem is, I don't really know what kind of variables I have in scope which I can use in the action to check if the submission rule is penalty-decay/grace-period.

Any advice that could point me in the right direction would be extremely helpful!

Thanks,
Aaron

Contributor

Ayaya commented Mar 18, 2012

Hey @jerboaa , I spent quite some time looking into doing validation callbacks which takes a block but I still can't seem to figure out how to do it correctly. What I did spend time doing was having something like:

  • validate_presence_of :interval, :if => action, but the problem is, I don't really know what kind of variables I have in scope which I can use in the action to check if the submission rule is penalty-decay/grace-period.

Any advice that could point me in the right direction would be extremely helpful!

Thanks,
Aaron

@jerboaa

This comment has been minimized.

Show comment
Hide comment
@jerboaa

jerboaa Mar 18, 2012

Member

@Ayaya You seem to be doing old validations. validate_presence_of is now :validates :attribute :presence => true. Anyhow, have you seen this? http://guides.rubyonrails.org/active_record_validations_callbacks.html#conditional-validation

Something along the lines of:

validates :interval, :presence => true, :if => :requires_interval_validation?

def requires_interval_validation?
  if self.instance_of?(PenaltyPeriod) || self.instance_of?(...)
     return true
  end
  return false
end

As for the variables problem, you should have access to all attributes of the record (see db/schema.rb for which attributes are defined for period records). Let me know if you have more questions.

Member

jerboaa commented Mar 18, 2012

@Ayaya You seem to be doing old validations. validate_presence_of is now :validates :attribute :presence => true. Anyhow, have you seen this? http://guides.rubyonrails.org/active_record_validations_callbacks.html#conditional-validation

Something along the lines of:

validates :interval, :presence => true, :if => :requires_interval_validation?

def requires_interval_validation?
  if self.instance_of?(PenaltyPeriod) || self.instance_of?(...)
     return true
  end
  return false
end

As for the variables problem, you should have access to all attributes of the record (see db/schema.rb for which attributes are defined for period records). Let me know if you have more questions.

@Ayaya

This comment has been minimized.

Show comment
Hide comment
@Ayaya

Ayaya Mar 19, 2012

Contributor

Thanks @jerboaa , I had something similar to this, but I didn't have an idea on which methods I could use (like instance_of). I'll give this a try.

Contributor

Ayaya commented Mar 19, 2012

Thanks @jerboaa , I had something similar to this, but I didn't have an idea on which methods I could use (like instance_of). I'll give this a try.

@Ayaya

This comment has been minimized.

Show comment
Hide comment
@Ayaya

Ayaya Mar 19, 2012

Contributor

@jerboaa , it seems that they are all instances of "Period".. I don't think there are separate instances of PenaltyPeriod or Decay period so I can't use the class to determine if I need to validate... Looking at the attributes for Period, I don't think there is one which tells you which type of rule it goes under. Is there a way I can access params from the form?

Contributor

Ayaya commented Mar 19, 2012

@jerboaa , it seems that they are all instances of "Period".. I don't think there are separate instances of PenaltyPeriod or Decay period so I can't use the class to determine if I need to validate... Looking at the attributes for Period, I don't think there is one which tells you which type of rule it goes under. Is there a way I can access params from the form?

@Ayaya

This comment has been minimized.

Show comment
Hide comment
@Ayaya

Ayaya Mar 20, 2012

Contributor

I've done a bit of research and I think the only viable solution I have found is adding a virtual attribute to the model and then setting that attribute via the form.

Contributor

Ayaya commented Mar 20, 2012

I've done a bit of research and I think the only viable solution I have found is adding a virtual attribute to the model and then setting that attribute via the form.

@Ayaya

This comment has been minimized.

Show comment
Hide comment
@Ayaya

Ayaya Mar 21, 2012

Contributor

Hi @jerboaa , I was able to come up with a working validation solution, you can find the snippet here: http://pastie.org/3639423

The validation is performed within the SubmissionRule model because its the only location where I was able to access all the attributes needed to perform the validation (couldn't find a way to do it inside periods.rb).

I just wanted to get a green light on this before I move forward since this new validation rule caused a few errors in rake test:

  1. Error:
    test: A section with penalty_decay_period_submission rules. should add 30% penalty to submission. (PenaltyDecayPeriodSubmissionRuleTest):
    NoMethodError: undefined method interval' for nil:NilClass app/models/submission_rule.rb:12:inrequires_interval_validation'
    app/models/assignment.rb:660:in replace_submission_rule' test/unit/penalty_decay_period_submission_rule_test.rb:23:in__bind_1332303246_355544'

Thanks,
Aaron

Contributor

Ayaya commented Mar 21, 2012

Hi @jerboaa , I was able to come up with a working validation solution, you can find the snippet here: http://pastie.org/3639423

The validation is performed within the SubmissionRule model because its the only location where I was able to access all the attributes needed to perform the validation (couldn't find a way to do it inside periods.rb).

I just wanted to get a green light on this before I move forward since this new validation rule caused a few errors in rake test:

  1. Error:
    test: A section with penalty_decay_period_submission rules. should add 30% penalty to submission. (PenaltyDecayPeriodSubmissionRuleTest):
    NoMethodError: undefined method interval' for nil:NilClass app/models/submission_rule.rb:12:inrequires_interval_validation'
    app/models/assignment.rb:660:in replace_submission_rule' test/unit/penalty_decay_period_submission_rule_test.rb:23:in__bind_1332303246_355544'

Thanks,
Aaron

@jerboaa

This comment has been minimized.

Show comment
Hide comment
@jerboaa

jerboaa Mar 21, 2012

Member

@Ayaya Cool! Throw it up on review board and we'll discuss it there.

Member

jerboaa commented Mar 21, 2012

@Ayaya Cool! Throw it up on review board and we'll discuss it there.

@mina mina referenced this issue Aug 18, 2012

Merged

fix #648 #832

@mina

This comment has been minimized.

Show comment
Hide comment
@mina

mina Aug 18, 2012

Contributor

This was much harder than it looks... the period model didn't have access to what kind of submission it is. I added a virtual attr for that

Contributor

mina commented Aug 18, 2012

This was much harder than it looks... the period model didn't have access to what kind of submission it is. I added a virtual attr for that

mina added a commit to mina/Markus that referenced this issue Aug 31, 2012

benjaminvialle added a commit that referenced this issue Sep 2, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment