From 27cc094c44d248c0f0dc3849c2fdfac29e4f6719 Mon Sep 17 00:00:00 2001 From: xsrust Date: Tue, 30 Jan 2018 12:51:16 +0000 Subject: [PATCH 1/2] addresses 1091 --- app/controllers/phases_controller.rb | 8 ++--- app/models/plan.rb | 54 ++++++++++++++-------------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/app/controllers/phases_controller.rb b/app/controllers/phases_controller.rb index 82633c7b13..a6341e37ff 100644 --- a/app/controllers/phases_controller.rb +++ b/app/controllers/phases_controller.rb @@ -6,13 +6,11 @@ class PhasesController < ApplicationController # GET /plans/:plan_id/phases/:id/edit def edit - - @plan = Plan.load_for_phase(params[:plan_id], params[:id]) + @plan, @phase = Plan.load_for_phase(params[:plan_id], params[:id]) # authorization done on plan so found in plan_policy authorize @plan phase_id = params[:id].to_i - @phase = @plan.template.phases.first @readonly = !@plan.editable_by?(current_user.id) # Now we need to get all the themed guidance for the plan. @@ -57,9 +55,9 @@ def edit # Puts in question_guidance (key/value) entries where key is the question id and value is a hash. # Each question id hash has (key/value) entries where key is a theme and value is an Array of {text, org} objects # Example hash - # question_guidance = { question.id => + # question_guidance = { question.id => # { theme => [ {text: "......", org: "....."} ] } - # } + # } questions.each do |question| qg = {} question.themes.each do |t| diff --git a/app/models/plan.rb b/app/models/plan.rb index d47cb82814..5107cd19ff 100644 --- a/app/models/plan.rb +++ b/app/models/plan.rb @@ -28,7 +28,7 @@ class Plan < ActiveRecord::Base ## # Possibly needed for active_admin # -relies on protected_attributes gem as syntax depricated in rails 4.2 - attr_accessible :locked, :project_id, :version_id, :version, :plan_sections, + attr_accessible :locked, :project_id, :version_id, :version, :plan_sections, :exported_plans, :project, :title, :template, :grant_number, :identifier, :principal_investigator, :principal_investigator_identifier, :description, :data_contact, :funder_name, :visibility, :exported_plans, @@ -38,7 +38,7 @@ class Plan < ActiveRecord::Base # public is a Ruby keyword so using publicly enum visibility: [:organisationally_visible, :publicly_visible, :is_test, :privately_visible] - #TODO: work out why this messes up plan creation : + #TODO: work out why this messes up plan creation : # briley: Removed reliance on :users, its really on :roles (shouldn't have a plan without at least a creator right?) It should be ok like this though now # validates :template, :title, presence: true @@ -210,7 +210,7 @@ def guidance_for_question(question) end end end - + # Get guidance by theme from any guidance groups currently selected self.guidance_groups.each do |group| group.guidances.each do |guidance| @@ -369,7 +369,7 @@ def status format = rec.qformat answer = nil - if qa_map.has_key?(qid) + if qa_map.has_key?(qid) answer = qa_map[qid] end @@ -423,7 +423,7 @@ def status opt_hash[aid] << optid end - status["questions"].each_key do |questionid| + status["questions"].each_key do |questionid| answerid = status["questions"][questionid]["answer_id"] status["questions"][questionid]["answer_option_ids"] = opt_hash[answerid] end @@ -636,10 +636,10 @@ def assign_creator(user_id) user_id = user_id.id if user_id.is_a?(User) add_user(user_id, true, true, true) end - -# TODO: commenting these out because they are overriden by private methods below, so this + +# TODO: commenting these out because they are overriden by private methods below, so this # is unreachable =begin ## @@ -696,7 +696,7 @@ def height_of_text(text, font_size_inc = 0, vertical_margin = 0) end =end -# TODO: What are these used for? Should just be using self.org and self.org.funder? +# TODO: What are these used for? Should just be using self.org and self.org.funder? =begin ## # sets a new funder for the project @@ -950,12 +950,14 @@ def template_owner self.dmptemplate.try(:organisation).try(:abbreviation) end =end - + # Returns the number of answered questions from the entire plan def num_answered_questions n = 0 - self.sections.each do |s| - n+= s.num_answered_questions(self.id) + self.template.phases.each do |p| + p.sections.each do |s| + n+= s.num_answered_questions(self.id) + end end return n end @@ -991,16 +993,16 @@ def self.eager_load(id) end def self.load_for_phase(id, phase_id) -# Plan.includes( -# [template: [ -# {phases: {sections: {questions: [{answers: :notes}, :annotations, :question_format, :themes]}}}, -# {customizations: :org}, -# :org -# ], -# plans_guidance_groups: {guidance_group: {guidances: :themes}} -# ]).where(id: id, phases: { id: phase_id }).first - - Plan.joins(:phases).where('plans.id = ? AND phases.id = ?', id, phase_id).includes(:template, :sections, :questions, :answers, :notes).first + plan = Plan.includes(template: {phases: {sections: {questions: :answers}}}).joins(template: {phases: {sections: {questions: :answers}}}).where("phases.id = #{phase_id}").distinct.merge( Plan.where("phases.id=#{phase_id}").joins(:phases).includes({answers: :notes})).find_by(id: id) + phase = nil + plan.template.phases.each do |p| + next unless p.id = phase_id + phase = p + break + end + return plan, phase + + end @@ -1050,12 +1052,12 @@ def add_user(user_id, is_editor = false, is_administrator = false, is_creator = end role.save - - # This is necessary because we're creating the associated record but not assigning it + + # This is necessary because we're creating the associated record but not assigning it # to roles. Auto-saving like this may be confusing when coding upstream in a controller, - # view or api. Should probably change this to: + # view or api. Should probably change this to: # self.roles << role - # and then let the save be called manually via: + # and then let the save be called manually via: # plan.save! #self.reload end @@ -1135,7 +1137,7 @@ def height_of_text(text, font_size_inc = 0, vertical_margin = 0) # -------------------------------------------------------- def set_creation_defaults # Only run this before_validation because rails fires this before save/create - if self.id.nil? + if self.id.nil? self.title = "My plan (#{self.template.title})" if self.title.nil? && !self.template.nil? self.visibility = 3 end From 1cae6f47b2945956e311833e5654de226621abe5 Mon Sep 17 00:00:00 2001 From: xsrust Date: Thu, 1 Feb 2018 11:56:32 +0000 Subject: [PATCH 2/2] updates load_for_phase to handle non-existant plans/phases --- app/controllers/phases_controller.rb | 7 +++++++ app/models/plan.rb | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/app/controllers/phases_controller.rb b/app/controllers/phases_controller.rb index a6341e37ff..aae87b7e2a 100644 --- a/app/controllers/phases_controller.rb +++ b/app/controllers/phases_controller.rb @@ -7,6 +7,13 @@ class PhasesController < ApplicationController # GET /plans/:plan_id/phases/:id/edit def edit @plan, @phase = Plan.load_for_phase(params[:plan_id], params[:id]) + # check if plan exists first + if @plan.nil? + raise Pundit::NotAuthorizedError, "Must have access to plan" + end + if @phase.nil? + raise Pundit::NotAuthorizedError, "Phase must belong to plan" + end # authorization done on plan so found in plan_policy authorize @plan diff --git a/app/models/plan.rb b/app/models/plan.rb index 5107cd19ff..d095217e30 100644 --- a/app/models/plan.rb +++ b/app/models/plan.rb @@ -995,6 +995,10 @@ def self.eager_load(id) def self.load_for_phase(id, phase_id) plan = Plan.includes(template: {phases: {sections: {questions: :answers}}}).joins(template: {phases: {sections: {questions: :answers}}}).where("phases.id = #{phase_id}").distinct.merge( Plan.where("phases.id=#{phase_id}").joins(:phases).includes({answers: :notes})).find_by(id: id) phase = nil + # return nil for both if plan does not exist + if plan.blank? + return nil, nil + end plan.template.phases.each do |p| next unless p.id = phase_id phase = p