Commit c54beec92b48f79d26d75afff48e11040bfa2748
Exists in
master
and in
28 other branches
Merge branch 'ai3168' into 'stable'
Improve validation of submissions/answers http://noosfero.org/Development/ActionItem3168
Showing
5 changed files
with
49 additions
and
60 deletions
Show diff stats
plugins/custom_forms/controllers/custom_forms_plugin_profile_controller.rb
@@ -6,40 +6,26 @@ class CustomFormsPluginProfileController < ProfileController | @@ -6,40 +6,26 @@ class CustomFormsPluginProfileController < ProfileController | ||
6 | 6 | ||
7 | @form = CustomFormsPlugin::Form.find(params[:id]) | 7 | @form = CustomFormsPlugin::Form.find(params[:id]) |
8 | if user | 8 | if user |
9 | - @submission ||= CustomFormsPlugin::Submission.find_by_form_id_and_profile_id(@form.id,user.id) | 9 | + @submission = CustomFormsPlugin::Submission.find_by_form_id_and_profile_id(@form.id,user.id) |
10 | @submission ||= CustomFormsPlugin::Submission.new(:form_id => @form.id, :profile_id => user.id) | 10 | @submission ||= CustomFormsPlugin::Submission.new(:form_id => @form.id, :profile_id => user.id) |
11 | else | 11 | else |
12 | - @submission ||= CustomFormsPlugin::Submission.new(:form_id => @form.id) | 12 | + @submission = CustomFormsPlugin::Submission.new(:form_id => @form.id) |
13 | end | 13 | end |
14 | 14 | ||
15 | # build the answers | 15 | # build the answers |
16 | - @submission.answers.push(*(answers = build_answers(params[:submission], @form))) if params[:submission] | 16 | + @answers = if params[:submission] then @submission.build_answers params[:submission] else @submission.answers end |
17 | 17 | ||
18 | if request.post? | 18 | if request.post? |
19 | begin | 19 | begin |
20 | raise 'Submission already present!' if user.present? && CustomFormsPlugin::Submission.find_by_form_id_and_profile_id(@form.id,user.id) | 20 | raise 'Submission already present!' if user.present? && CustomFormsPlugin::Submission.find_by_form_id_and_profile_id(@form.id,user.id) |
21 | raise 'Form expired!' if @form.expired? | 21 | raise 'Form expired!' if @form.expired? |
22 | 22 | ||
23 | - # @submission.answers for some reason has the same answer twice | ||
24 | - failed_answers = answers.select {|answer| !answer.valid? } | ||
25 | - | ||
26 | - if failed_answers.empty? | ||
27 | - # Save the submission | ||
28 | - ActiveRecord::Base.transaction do | ||
29 | - if !user | ||
30 | - @submission.author_name = params[:author_name] | ||
31 | - @submission.author_email = params[:author_email] | ||
32 | - end | ||
33 | - @submission.save! | ||
34 | - end | ||
35 | - else | ||
36 | - @submission.errors.clear | ||
37 | - failed_answers.each do |answer| | ||
38 | - answer.valid? | ||
39 | - answer.errors.each do |attribute, msg| | ||
40 | - @submission.errors.add(answer.field.id.to_s.to_sym, msg) | ||
41 | - end | ||
42 | - end | 23 | + if !user |
24 | + @submission.author_name = params[:author_name] | ||
25 | + @submission.author_email = params[:author_email] | ||
26 | + end | ||
27 | + | ||
28 | + if not @submission.save | ||
43 | raise 'Submission failed: answers not valid' | 29 | raise 'Submission failed: answers not valid' |
44 | end | 30 | end |
45 | session[:notice] = _('Submission saved') | 31 | session[:notice] = _('Submission saved') |
plugins/custom_forms/lib/custom_forms_plugin/helper.rb
1 | module CustomFormsPlugin::Helper | 1 | module CustomFormsPlugin::Helper |
2 | + | ||
3 | + protected | ||
4 | + | ||
2 | def html_for_field(builder, association, klass) | 5 | def html_for_field(builder, association, klass) |
3 | new_object = klass.new | 6 | new_object = klass.new |
4 | builder.fields_for(association, new_object, :child_index => "new_#{association}") do |f| | 7 | builder.fields_for(association, new_object, :child_index => "new_#{association}") do |f| |
@@ -117,22 +120,4 @@ module CustomFormsPlugin::Helper | @@ -117,22 +120,4 @@ module CustomFormsPlugin::Helper | ||
117 | type_for_options(field.class) == 'select_field' && field.select_field_type == 'check_box' | 120 | type_for_options(field.class) == 'select_field' && field.select_field_type == 'check_box' |
118 | end | 121 | end |
119 | 122 | ||
120 | - def build_answers(submission, form) | ||
121 | - answers = [] | ||
122 | - form.fields.each do |field| | ||
123 | - final_value = '' | ||
124 | - if submission.has_key?(field.id.to_s) | ||
125 | - value = submission[field.id.to_s] | ||
126 | - if value.kind_of?(String) | ||
127 | - final_value = value | ||
128 | - elsif value.kind_of?(Array) | ||
129 | - final_value = value.join(',') | ||
130 | - elsif value.kind_of?(Hash) | ||
131 | - final_value = value.map {|option, present| present == '1' ? option : nil}.compact.join(',') | ||
132 | - end | ||
133 | - end | ||
134 | - answers << CustomFormsPlugin::Answer.new(:field => field, :value => final_value) | ||
135 | - end | ||
136 | - answers | ||
137 | - end | ||
138 | end | 123 | end |
plugins/custom_forms/lib/custom_forms_plugin/membership_survey.rb
@@ -11,23 +11,9 @@ class CustomFormsPlugin::MembershipSurvey < Task | @@ -11,23 +11,9 @@ class CustomFormsPlugin::MembershipSurvey < Task | ||
11 | form = CustomFormsPlugin::Form.find(form_id) | 11 | form = CustomFormsPlugin::Form.find(form_id) |
12 | raise 'Form expired' if form.expired? | 12 | raise 'Form expired' if form.expired? |
13 | 13 | ||
14 | - answers = build_answers(submission, form) | ||
15 | s = CustomFormsPlugin::Submission.create!(:form => form, :profile => target) | 14 | s = CustomFormsPlugin::Submission.create!(:form => form, :profile => target) |
16 | - s.answers.push(*answers) | ||
17 | - | ||
18 | - failed_answers = answers.select {|answer| !answer.valid? } | ||
19 | - if failed_answers.empty? | ||
20 | - s.save! | ||
21 | - else | ||
22 | - s.errors.clear | ||
23 | - answers.each do |answer| | ||
24 | - answer.valid? | ||
25 | - answer.errors.each do |attribute, msg| | ||
26 | - s.errors.add(answer.field.id.to_s.to_sym, msg) | ||
27 | - end | ||
28 | - end | ||
29 | - raise ActiveRecord::RecordInvalid, s | ||
30 | - end | 15 | + s.build_answers submission |
16 | + s.save! | ||
31 | end | 17 | end |
32 | 18 | ||
33 | def title | 19 | def title |
plugins/custom_forms/lib/custom_forms_plugin/submission.rb
@@ -2,12 +2,14 @@ class CustomFormsPlugin::Submission < Noosfero::Plugin::ActiveRecord | @@ -2,12 +2,14 @@ class CustomFormsPlugin::Submission < Noosfero::Plugin::ActiveRecord | ||
2 | belongs_to :form, :class_name => 'CustomFormsPlugin::Form' | 2 | belongs_to :form, :class_name => 'CustomFormsPlugin::Form' |
3 | belongs_to :profile | 3 | belongs_to :profile |
4 | 4 | ||
5 | - has_many :answers, :class_name => 'CustomFormsPlugin::Answer', :dependent => :destroy | 5 | + # validation is done manually, see below |
6 | + has_many :answers, :class_name => 'CustomFormsPlugin::Answer', :dependent => :destroy, :validate => false | ||
6 | 7 | ||
7 | validates_presence_of :form | 8 | validates_presence_of :form |
8 | validates_presence_of :author_name, :author_email, :if => lambda {|submission| submission.profile.nil?} | 9 | validates_presence_of :author_name, :author_email, :if => lambda {|submission| submission.profile.nil?} |
9 | validates_uniqueness_of :author_email, :scope => :form_id, :allow_nil => true | 10 | validates_uniqueness_of :author_email, :scope => :form_id, :allow_nil => true |
10 | validates_format_of :author_email, :with => Noosfero::Constants::EMAIL_FORMAT, :if => (lambda {|submission| !submission.author_email.blank?}) | 11 | validates_format_of :author_email, :with => Noosfero::Constants::EMAIL_FORMAT, :if => (lambda {|submission| !submission.author_email.blank?}) |
12 | + validate :check_answers | ||
11 | 13 | ||
12 | def self.human_attribute_name(attrib) | 14 | def self.human_attribute_name(attrib) |
13 | if /\d+/ =~ attrib and (f = CustomFormsPlugin::Field.find_by_id(attrib.to_i)) | 15 | if /\d+/ =~ attrib and (f = CustomFormsPlugin::Field.find_by_id(attrib.to_i)) |
@@ -17,5 +19,35 @@ class CustomFormsPlugin::Submission < Noosfero::Plugin::ActiveRecord | @@ -17,5 +19,35 @@ class CustomFormsPlugin::Submission < Noosfero::Plugin::ActiveRecord | ||
17 | end | 19 | end |
18 | end | 20 | end |
19 | 21 | ||
22 | + def build_answers submission | ||
23 | + self.form.fields.each do |field| | ||
24 | + next unless value = submission[field.id.to_s] | ||
25 | + | ||
26 | + final_value = '' | ||
27 | + if value.kind_of?(String) | ||
28 | + final_value = value | ||
29 | + elsif value.kind_of?(Array) | ||
30 | + final_value = value.join(',') | ||
31 | + elsif value.kind_of?(Hash) | ||
32 | + final_value = value.map {|option, present| present == '1' ? option : nil}.compact.join(',') | ||
33 | + end | ||
34 | + | ||
35 | + self.answers.build :field => field, :value => final_value | ||
36 | + end | ||
37 | + | ||
38 | + self.answers | ||
39 | + end | ||
40 | + | ||
41 | + protected | ||
42 | + | ||
43 | + def check_answers | ||
44 | + self.answers.each do |answer| | ||
45 | + answer.valid? | ||
46 | + answer.errors.each do |attribute, msg| | ||
47 | + self.errors.add answer.field.id.to_s.to_sym, msg | ||
48 | + end | ||
49 | + end | ||
50 | + end | ||
51 | + | ||
20 | end | 52 | end |
21 | 53 |
plugins/custom_forms/test/unit/custom_forms_plugin/submission_test.rb
@@ -36,8 +36,8 @@ class CustomFormsPlugin::SubmissionTest < ActiveSupport::TestCase | @@ -36,8 +36,8 @@ class CustomFormsPlugin::SubmissionTest < ActiveSupport::TestCase | ||
36 | form = CustomFormsPlugin::Form.create!(:name => 'Free Software', :profile => fast_create(Profile)) | 36 | form = CustomFormsPlugin::Form.create!(:name => 'Free Software', :profile => fast_create(Profile)) |
37 | field = CustomFormsPlugin::Field.create!(:name => 'License', :form => form) | 37 | field = CustomFormsPlugin::Field.create!(:name => 'License', :form => form) |
38 | submission = CustomFormsPlugin::Submission.create!(:form => form, :profile => fast_create(Profile)) | 38 | submission = CustomFormsPlugin::Submission.create!(:form => form, :profile => fast_create(Profile)) |
39 | - a1 = CustomFormsPlugin::Answer.create!(:field => field, :submission => submission) | ||
40 | - a2 = CustomFormsPlugin::Answer.create!(:field => field, :submission => submission) | 39 | + a1 = submission.answers.create!(:field => field, :submission => submission) |
40 | + a2 = submission.answers.create!(:field => field, :submission => submission) | ||
41 | 41 | ||
42 | assert_includes submission.answers, a1 | 42 | assert_includes submission.answers, a1 |
43 | assert_includes submission.answers, a2 | 43 | assert_includes submission.answers, a2 |