diff --git a/Gemfile b/Gemfile index 2d9b5b9102..7ed9ca7495 100644 --- a/Gemfile +++ b/Gemfile @@ -72,7 +72,7 @@ gem 'formtastic' gem 'wkhtmltopdf-binary' gem 'thin' gem 'wicked_pdf' -gem 'htmltoword' +gem 'htmltoword', '>= 0.7' gem 'feedjira' gem 'yaml_db', :git => 'https://github.com/vyruss/yaml_db.git' diff --git a/Gemfile.lock b/Gemfile.lock index 53bf3cf52e..bde99fd4ef 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -142,7 +142,7 @@ GEM activesupport (>= 4.1.0) hashdiff (0.3.0) hashie (3.4.6) - htmltoword (0.5.1) + htmltoword (0.7.0) actionpack nokogiri rubyzip (>= 1.0) @@ -282,7 +282,7 @@ GEM railties (>= 4.2.0, < 5.1) rolify (5.1.0) ruby-progressbar (1.8.1) - rubyzip (1.2.0) + rubyzip (1.2.1) safe_yaml (1.0.4) sass (3.4.22) sass-rails (5.0.6) @@ -368,7 +368,7 @@ DEPENDENCIES gettext (>= 3.0.2) gettext_i18n_rails (~> 1.8) gettext_i18n_rails_js (~> 1.2.0) - htmltoword + htmltoword (>= 0.7) i18n-js (>= 3.0.0.rc11) jbuilder jquery-rails diff --git a/ISSUE_TEMPLATE.md b/ISSUE_TEMPLATE.md new file mode 100644 index 0000000000..ed97d044ce --- /dev/null +++ b/ISSUE_TEMPLATE.md @@ -0,0 +1,7 @@ +Please complete the following fields as applicable: + +**Expected behaviour:** + +**Actual behaviour:** + +**Steps to reproduce:** diff --git a/PULL_REQUEST_TEMPLATE.md b/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 0000000000..460ec83ae6 --- /dev/null +++ b/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,4 @@ +Fixes # . + +Changes proposed in this PR: +- diff --git a/app/controllers/answers_controller.rb b/app/controllers/answers_controller.rb index 7d48ddf5b7..406d75fd6f 100644 --- a/app/controllers/answers_controller.rb +++ b/app/controllers/answers_controller.rb @@ -3,31 +3,33 @@ class AnswersController < ApplicationController respond_to :html # PUT/PATCH /answers/[:id] - def update + def update p_params = permitted_params() - @answer = Answer.find_by({plan_id: p_params[:plan_id], question_id: p_params[:question_id], }) - begin - if @answer - authorize @answer - @answer.update(p_params) - if p_params[:question_option_ids].present? - @answer.touch() # Saves the record with the updated_at set to the current time. Needed if only answer.question_options is updated + Answer.transaction do + @answer = Answer.find_by({plan_id: p_params[:plan_id], question_id: p_params[:question_id]}) + begin + if @answer.present? + authorize @answer + @answer.update(p_params) + if p_params[:question_option_ids].present? + @answer.touch() # Saves the record with the updated_at set to the current time. Needed if only answer.question_options is updated + end + else + @answer = Answer.new(p_params) + @answer.lock_version = 1 + authorize @answer + # NOTE: save! and destroy! must be used for transactions as they raise errors instead of returning false + @answer.save! end - else - @answer = Answer.new(p_params) - @answer.lock_version = 1 - authorize @answer - @answer.save() # NOTE, there is a chance to create multiple answer associated for a plan/question (IF any concurrent thread) INSERTS an answer after checking the existence of an answer (Line 8) - # In order to avoid that edge-case, it is recommended to create answers whenever a new plan is created (e.g. after_create callback) + rescue ActiveRecord::StaleObjectError + @stale_answer = @answer + @answer = Answer.find_by({plan_id: p_params[:plan_id], question_id: p_params[:question_id]}) end - rescue ActiveRecord::StaleObjectError - @stale_answer = @answer - @answer = Answer.find_by({plan_id: p_params[:plan_id], question_id: p_params[:question_id]}) end - + @plan = Plan.includes({ - sections: { - questions: [ + sections: { + questions: [ :answers, :question_format ] @@ -37,7 +39,7 @@ def update @section = @plan.get_section(@question.section_id) respond_to do |format| - format.js {} + format.js {} end end # End update diff --git a/app/controllers/notes_controller.rb b/app/controllers/notes_controller.rb index 474281a18e..42b2c6d621 100644 --- a/app/controllers/notes_controller.rb +++ b/app/controllers/notes_controller.rb @@ -8,22 +8,25 @@ def create @note = Note.new user_id = params[:new_note][:user_id] @note.user_id = user_id - answer_id = params[:new_note][:answer_id] question_id = params[:new_note][:question_id] plan_id = params[:new_note][:plan_id] # create answer if we dont already have one - if answer_id.present? - answer = Answer.find(answer_id) - else - answer = Answer.new - answer.plan_id = plan_id - answer.question_id = question_id - answer.user_id = user_id - answer.save! + answer=nil # if defined within transaction block, was not accessable after + # ensure user has access to plan BEFORE creating/finding an answer + raise Pundit::NotAuthorizedError unless Plan.find(plan_id).readable_by?(user_id) + Answer.transaction do + answer = Answer.find_by(question_id: question_id, plan_id: plan_id) + if answer.blank? + answer = Answer.new + answer.plan_id = plan_id + answer.question_id = question_id + answer.user_id = user_id + answer.save! + end end - @note.answer= answer + @note.answer = answer @note.text = params["#{question_id}new_note_text"] authorize @note diff --git a/app/controllers/plans_controller.rb b/app/controllers/plans_controller.rb index 2c4d8c63d3..3c713bb58c 100644 --- a/app/controllers/plans_controller.rb +++ b/app/controllers/plans_controller.rb @@ -66,7 +66,7 @@ def create published: true) if !ggs.blank? then @plan.guidance_groups << ggs end - default = Template.find_by(is_default: true) + default = Template.default msg = "#{_('Plan was successfully created.')} " @@ -404,7 +404,7 @@ def template_options(org_id, funder_id) # If no templates were available use the generic templates if @templates.empty? @msg = _("Using the generic Data Management Plan") - @templates << Template.where(is_default: true, published: true).first + @templates << Template.default end @templates = @templates.sort{|x,y| x.title <=> y.title } if @templates.count > 1 diff --git a/app/models/template.rb b/app/models/template.rb index b72d2169a5..f8a7e6a0f3 100644 --- a/app/models/template.rb +++ b/app/models/template.rb @@ -17,7 +17,7 @@ class Template < ActiveRecord::Base ## # Possibly needed for active_admin # -relies on protected_attributes gem as syntax depricated in rails 4.2 - attr_accessible :id, :org_id, :description, :published, :title, :locale, :customization_of, + attr_accessible :id, :org_id, :description, :published, :title, :locale, :customization_of, :is_default, :guidance_group_ids, :org, :plans, :phases, :dmptemplate_id, :migrated, :version, :visibility, :published, :as => [:default, :admin] @@ -33,16 +33,20 @@ def self.dmptemplate_ids Template.all.valid.distinct.pluck(:dmptemplate_id) end - # Retrieves the most recent version of the template for the specified Org and dmptemplate_id + # Retrieves the most recent version of the template for the specified Org and dmptemplate_id def self.current(dmptemplate_id) Template.where(dmptemplate_id: dmptemplate_id).order(version: :desc).valid.first end - - # Retrieves the current published version of the template for the specified Org and dmptemplate_id + + # Retrieves the current published version of the template for the specified Org and dmptemplate_id def self.live(dmptemplate_id) Template.where(dmptemplate_id: dmptemplate_id, published: true).valid.first end + def self.default + Template.valid.where(is_default: true, published: true).order(:version).last + end + ## # Retrieves the most current customization of the template for the # specified org and dmptemplate_id @@ -79,8 +83,8 @@ def self.deep_copy(template) ## # convert the given template to a hash and return with all it's associations - # to use, please pre-fetch org, phases, section, questions, annotations, - # question_options, question_formats, + # to use, please pre-fetch org, phases, section, questions, annotations, + # question_options, question_formats, # TODO: Themes & guidance? # # @return [hash] hash of template, phases, sections, questions, question_options, annotations @@ -145,7 +149,7 @@ def set_creation_defaults self.visibility = 1 self.is_default = false self.version = 0 if self.version.nil? - + # Generate a unique identifier for the dmptemplate_id if necessary if self.dmptemplate_id.nil? self.dmptemplate_id = loop do diff --git a/app/views/notes/_add.html.erb b/app/views/notes/_add.html.erb index 2817b90020..e335daaf93 100644 --- a/app/views/notes/_add.html.erb +++ b/app/views/notes/_add.html.erb @@ -12,7 +12,6 @@ id: "new_note_form_#{questionid}") do |f| %> <%= f.hidden_field :user_id, value: current_user.id %> <%= f.hidden_field :question_id, value: questionid %> - <%= f.hidden_field :answer_id, value: answer.id %> <%= f.hidden_field :plan_id, value: plan_id %>
diff --git a/db/seeds.rb b/db/seeds.rb index fabb9a8328..1c6d5532fb 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -368,10 +368,19 @@ org: Org.find_by(abbreviation: 'GA'), is_default: false, version: 0, - migrated:false, + migrated: false, dmptemplate_id: 3} ] -templates.map{ |t| Template.create!(t) if Template.find_by(title: t[:title]).nil? } +# Template creation calls defaults handler which sets is_default and +# published to false automatically, so update them after creation +templates.map do |t| + if Template.find_by(title: t[:title]).nil? + tmplt = Template.create!(t) + tmplt.published = t[:published] + tmplt.is_default = t[:is_default] + tmplt.save! + end +end # Create 2 phases for the funder's template and one for our generic template # ------------------------------------------------------- diff --git a/test/integration/template_selection_test.rb b/test/integration/template_selection_test.rb index 256795419d..eb95dc7b10 100644 --- a/test/integration/template_selection_test.rb +++ b/test/integration/template_selection_test.rb @@ -5,100 +5,102 @@ class TemplateSelectionTest < ActionDispatch::IntegrationTest setup do scaffold_template - @template.is_default = true - @template.published = true - @template.save! - + @template = Template.default + @researcher = User.last - + scaffold_org_admin(@template.org) - + @funder = Org.find_by(org_type: 2) - @funder_template = Template.create(title: 'Funder template', org: @funder, migrated: false) + @funder_template = @funder.templates.where(published: true).first #Template.create(title: 'Funder template', org: @funder, migrated: false) # Template can't be published on creation so do it afterward @funder_template.published = true @funder_template.save - + @org = @researcher.org @org_template = Template.create(title: 'Org template', org: @org, migrated: false) # Template can't be published on creation so do it afterward @org_template.published = true @org_template.save end - + # ---------------------------------------------------------- test 'plan gets publish versions of templates' do original_id = @template.id template = version_template(@template) - + sign_in @researcher - + post plans_path(format: :js), {plan: {org_id: @template.org.id}} assert_response :success assert @response.body.include?("$(\"#plan_template_id\").val(\"#{original_id}\");") assert_equal original_id, Template.live(@template.dmptemplate_id).id - + # Version the template again original_id = template.id template = version_template(template) - + # Make sure the published version is used post plans_path(format: :js), {plan: {org_id: @template.org.id}} assert_response :success assert @response.body.include?("$(\"#plan_template_id\").val(\"#{original_id}\");") assert_equal original_id, Template.live(@template.dmptemplate_id).id - + # Update the template and make sure the published version stayed the same sign_in @user put admin_update_template_path(template), {template: {title: "Blah blah blah"}} - + sign_in @researcher - + post plans_path(format: :js), {plan: {org_id: @template.org.id}} assert_response :success assert @response.body.include?("$(\"#plan_template_id\").val(\"#{original_id}\");") assert_equal original_id, Template.live(@template.dmptemplate_id).id end - + # ---------------------------------------------------------- test 'plan gets generic template when no funder or org' do - @template.is_default = true - @template.save! - + temp = Template.find_by(published: true, is_default: true) + if temp.blank? + @template.is_default = true + @template.save! + temp = @template + end + sign_in @researcher - + post plans_path(format: :js), {plan: {org_id: nil}} assert_response :success - assert @response.body.include?("$(\"#plan_template_id\").val(\"#{@template.id}\");"), @response.body + assert @response.body.include?("$(\"#plan_template_id\").val(\"#{temp.id}\");"), @response.body end - + # ---------------------------------------------------------- test 'plan gets org template when no funder' do sign_in @researcher - + post plans_path(format: :js), {plan: {org_id: @org.id, funder_id: nil}} assert_response :success assert @response.body.include?("$(\"#plan_template_id\").val(\"#{@org_template.id}\");"), @response.body end - + # ---------------------------------------------------------- test 'plan gets funder template when no org' do sign_in @researcher - + post plans_path(format: :js), {plan: {org_id: nil, funder_id: @funder.id}} assert_response :success assert @response.body.include?("$(\"#plan_template_id\").val(\"#{@funder_template.id}\");"), @response.body end - + # ---------------------------------------------------------- test 'plan gets funder template when org has no customization' do sign_in @researcher - + post plans_path(format: :js), {plan: {org_id: @org.id, funder_id: @funder.id}} assert_response :success assert @response.body.include?("$(\"#plan_template_id\").val(\"#{@funder_template.id}\");"), @response.body end - + # ---------------------------------------------------------- test 'plan gets customized version of funder template' do customization = Template.create(title: 'Customization', org: @org) @@ -106,9 +108,9 @@ class TemplateSelectionTest < ActionDispatch::IntegrationTest customization.published = true customization.customization_of = @funder_template.dmptemplate_id customization.save - + sign_in @researcher - + post plans_path(format: :js), {plan: {org_id: @org.id, funder_id: @funder.id}} assert_response :success assert @response.body.include?("$(\"#plan_template_id\").val(\"#{customization.id}\");"), @response.body @@ -120,17 +122,17 @@ class TemplateSelectionTest < ActionDispatch::IntegrationTest # Template can't be published on creation so do it afterward funder_template2.published = true funder_template2.save - + sign_in @researcher - + post plans_path(format: :js), {plan: {org_id: @org.id, funder_id: @funder.id}} assert_response :success assert_select "option", 3, "expected a dropdown with 2 templates and a 'please select' option" assert @response.body.include?("