Skip to content

addresses 1091 #1100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jollopre merged 2 commits into from
Feb 1, 2018
Merged

addresses 1091 #1100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27cc094
addresses 1091
xsrust Jan 30, 2018
1cae6f4
updates load_for_phase to handle non-existant plans/phases
xsrust Feb 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions app/controllers/phases_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,18 @@ 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])
# 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

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.
Expand Down Expand Up @@ -57,9 +62,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|
Expand Down
58 changes: 32 additions & 26 deletions app/models/plan.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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

Expand Down Expand Up @@ -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|
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
##
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -991,16 +993,20 @@ 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
# 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
break
end
return plan, phase


end


Expand Down Expand Up @@ -1050,12 +1056,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
Expand Down Expand Up @@ -1135,7 +1141,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
Expand Down