Download as
Browse Code »

Commit eaa33fc40d8149e9a271e3f7675a2276c2c83014

Authored by Heitor
Committed by Rafael Manzo
1 parent e1253609
Exists in colab and in 4 other branches master, spb-master, spb-stable, stable

Refactor MetricConfiguration controllers 

Move as much code as possible to BaseMetricConfigurationsController
instead of repeating it ad-hoc in the concrete classes. Most of the
concrete code has gone away except for some small overrides:

- Hotspot's does not have a 'new' action, and therefore can't redirect
to it. It also does not accept any MetricConfiguration parameters.
- Metric's only accepts 'weight' and 'aggregation_form'
MetricConfiguration parameters. It also needs to fetch ReadingGroups to
display in the 'choose_metric' action.
- Compound's needs to force the Metric type when creating the metric,
and must accept parameters for Metric ('name', 'description', 'script',
'scope', 'code') in addition to MetricConfiguration's 'weight'.

Additionally:
- Update cache clearing to clear caches for Hotspot and Tree
configurations
- Verify the existence of the KalibroConfiguration and use the object
that was fetched whenever necessary, instead of only using the id.
- Ensure the KalibroConfiguration and ReadingGroup ids match between the
values inside and outside the 'metric_configuration' parameter.
Previously it was possible to send one in the URL and update another,
which might even have been a security issue.
- Allow sending either 'metric_code' or 'metric_name' as a parameter.
Both are used in different places in the code, but making them uniform
is a job for a later time.

Signed off by: Daniel Miranda <danielkza2@gmail.com>
Showing 4 changed files with 123 additions and 187 deletions   Show diff stats
app/controllers/base_metric_configurations_controller.rb
  Diff comments   View file @eaa33fc
1 -include OwnershipAuthentication  
2 -include MetricConfigurationsConcern  
3 -  
4 class BaseMetricConfigurationsController < ApplicationController 1 class BaseMetricConfigurationsController < ApplicationController
  2 + include OwnershipAuthentication
  3 + include MetricConfigurationsConcern
  4 +
5 before_action :authenticate_user!, except: [:show, :index] 5 before_action :authenticate_user!, except: [:show, :index]
6 before_action :metric_configuration_owner?, only: [:edit, :update, :destroy] 6 before_action :metric_configuration_owner?, only: [:edit, :update, :destroy]
7 before_action :kalibro_configuration_owner?, only: [:new, :create, :choose_metric] 7 before_action :kalibro_configuration_owner?, only: [:new, :create, :choose_metric]
8 - before_action :set_metric_configuration, only: [:show, :edit, :update, :destroy] 8 + before_action :set_kalibro_configuration!
  9 + before_action :find_metric_configuration!, only: [:show, :edit, :update, :destroy]
  10 + before_action :new_metric_configuration!, only: [:create]
  11 + before_action :set_metric!, only: [:create, :update]
  12 +
  13 + def show
  14 + @reading_group = ReadingGroup.find(@metric_configuration.reading_group_id)
  15 + @kalibro_ranges = @metric_configuration.kalibro_ranges
  16 + end
9 17
10 def new 18 def new
11 - update_metric_configuration(MetricConfiguration.new) 19 + @metric_configuration = MetricConfiguration.new
12 end 20 end
13 21
14 - def show  
15 - if metric_configuration  
16 - @reading_group = ReadingGroup.find(metric_configuration.reading_group_id)  
17 - @kalibro_ranges = metric_configuration.kalibro_ranges  
18 - else  
19 - raise NotImplementedError 22 + def create
  23 + respond_to { |format| save_and_redir format }
  24 + end
  25 +
  26 + def update
  27 + respond_to do |format|
  28 + save_and_redir(format) do |metric_configuration|
  29 + metric_configuration.reading_group_id = params[:reading_group_id].to_i
  30 + metric_configuration.update(metric_configuration_params)
  31 + end
20 end 32 end
21 end 33 end
22 34
23 - def create  
24 - update_metric_configuration(MetricConfiguration.new(metric_configuration_params)) 35 + def destroy
  36 + @metric_configuration.destroy
  37 + clear_caches
  38 +
  39 + respond_to do |format|
  40 + format.html { redirect_to kalibro_configuration_path(@kalibro_configuration.id) }
  41 + format.json { head :no_content }
  42 + end
25 end 43 end
26 44
27 protected 45 protected
28 46
29 - def metric_configuration  
30 - raise NotImplementedError 47 + def save_and_redir(format)
  48 + new_record = @metric_configuration.id.nil?
  49 + result = block_given? ? (yield @metric_configuration) : @metric_configuration.save
  50 +
  51 + if result
  52 + clear_caches
  53 +
  54 + format.html do
  55 + redirect_to kalibro_configuration_path(@kalibro_configuration.id),
  56 + notice: t(new_record ? 'successfully_created' : 'successfully_updated',
  57 + record: t(@metric_configuration.class))
  58 + end
  59 + format.json { render json: @metric_configuration, status: new_record ? :created : :ok }
  60 + else
  61 + failed_action(format, new_record ? :new : :edit)
  62 + end
31 end 63 end
32 64
33 - def update_metric_configuration (new_metric_configuration)  
34 - raise NotImplementedError 65 + # FIXME: This action should render with an error message on rendering the html
  66 + def failed_action(format, action)
  67 + format.html { render action }
  68 + format.json { render json: @metric_configuration.kalibro_errors, status: :unprocessable_entity }
  69 + end
  70 +
  71 + def clear_caches
  72 + Rails.cache.delete("#{@kalibro_configuration.id}_tree_metric_configurations")
  73 + Rails.cache.delete("#{@kalibro_configuration.id}_hotspot_metric_configurations")
35 end 74 end
36 75
37 - # Never trust parameters from the scary internet, only allow the white list through.  
38 - # TODO: this should be refactored to the concern metric configuration  
39 def metric_configuration_params 76 def metric_configuration_params
40 - params[:metric_configuration] 77 + raise NotImplementedError
  78 + end
  79 +
  80 + def set_kalibro_configuration!
  81 + @kalibro_configuration = KalibroConfiguration.find params[:kalibro_configuration_id].to_i
  82 + end
  83 +
  84 + def new_metric_configuration!
  85 + @metric_configuration = MetricConfiguration.new metric_configuration_params
  86 + @metric_configuration.kalibro_configuration_id = @kalibro_configuration.id
  87 + @metric_configuration.reading_group_id = params[:reading_group_id].to_i
  88 + end
  89 +
  90 + def find_metric_configuration!
  91 + @metric_configuration = MetricConfiguration.find params[:id].to_i
  92 +
  93 + # Make sure the metric configuration is really from the kalibro configuration we're being told it is
  94 + if @metric_configuration.kalibro_configuration_id != @kalibro_configuration.id
  95 + raise KalibroClient::Errors::RecordNotFound
  96 + end
  97 + end
  98 +
  99 + def set_metric!
  100 + collector = KalibroClient::Entities::Processor::MetricCollectorDetails.find_by_name(params[:metric_collector_name])
  101 + unless collector.nil?
  102 + # FIXME: Some view pass metric code as a parameter instead of metric name
  103 + if params.key?(:metric_code)
  104 + metric = collector.find_metric_by_code(params[:metric_code])
  105 + else
  106 + metric = collector.find_metric_by_name(params[:metric_name])
  107 + end
  108 +
  109 + if !metric.nil? && metric.type == self.class::METRIC_TYPE
  110 + @metric_configuration.metric = metric
  111 + return
  112 + end
  113 + end
  114 + respond_to do |format|
  115 + format.html { redirect_to kalibro_configurations_path(@kalibro_configuration.id) }
  116 + format.json { render json: { errors: 'Invalid metric type' }, status: :unprocessable_entity }
  117 + end
41 end 118 end
42 end 119 end
app/controllers/compound_metric_configurations_controller.rb
  Diff comments   View file @eaa33fc
1 class CompoundMetricConfigurationsController < BaseMetricConfigurationsController 1 class CompoundMetricConfigurationsController < BaseMetricConfigurationsController
2 - before_action :set_metric_configurations, only: [:new, :edit]  
3 -  
4 - def create  
5 - super  
6 - @compound_metric_configuration.metric.type = "CompoundMetricSnapshot"  
7 - respond_to do |format|  
8 - create_and_redir(format)  
9 - end  
10 - Rails.cache.delete("#{params[:kalibro_configuration_id].to_i}_tree_metric_configurations")  
11 - end  
12 -  
13 - def show  
14 - update_metric_configuration(@metric_configuration)  
15 - super  
16 - end  
17 -  
18 - def edit  
19 - @compound_metric_configuration = @metric_configuration  
20 - @kalibro_configuration_id = params[:kalibro_configuration_id].to_i  
21 - @compound_metric_configuration.kalibro_configuration_id = @kalibro_configuration_id  
22 - end 2 + METRIC_TYPE = 'CompoundMetricSnapshot'
23 3
24 - def update  
25 - respond_to do |format|  
26 - edit  
27 - if @compound_metric_configuration.update(metric_configuration_params)  
28 - format.html { redirect_to kalibro_configuration_path(@compound_metric_configuration.kalibro_configuration_id), notice: t('successfully_updated', :record => t('compound') + " " + @compound_metric_configuration.class.model_name.human) }  
29 - format.json { head :no_content }  
30 - else  
31 - failed_action(format, 'edit')  
32 - end  
33 - Rails.cache.delete("#{@compound_metric_configuration.kalibro_configuration_id}_tree_metric_configurations")  
34 - end  
35 - end 4 + before_action :set_metric_configurations, only: [:new, :edit]
36 5
37 protected 6 protected
38 7
39 - def metric_configuration  
40 - @compound_metric_configuration  
41 - end  
42 -  
43 - def update_metric_configuration (new_metric_configuration)  
44 - @kalibro_configuration_id = params[:kalibro_configuration_id]  
45 - @compound_metric_configuration = new_metric_configuration  
46 - end  
47 -  
48 - private  
49 -  
50 - # Duplicated code on create and update actions extracted here  
51 - def failed_action(format, destiny_action)  
52 - @kalibro_configuration_id = params[:kalibro_configuration_id]  
53 -  
54 - set_metric_configurations  
55 - format.html { render action: destiny_action }  
56 - format.json { render json: @compound_metric_configuration.kalibro_errors, status: :unprocessable_entity } 8 + def set_metric!
  9 + @metric_configuration.metric.type = 'CompoundMetricSnapshot'
57 end 10 end
58 11
59 - #Code extracted from create action  
60 - def create_and_redir(format)  
61 - if @compound_metric_configuration.save  
62 - format.html { redirect_to kalibro_configuration_path(@compound_metric_configuration.kalibro_configuration_id), notice: t('successfully_created', :record => t('compound') + " " + @compound_metric_configuration.class.model_name.human) }  
63 - else  
64 - failed_action(format, 'new')  
65 - end 12 + def metric_configuration_params
  13 + params.require(:metric_configuration).permit(:reading_group_id, :weight, :metric => [:name, :description, :script, :scope, :code])
66 end 14 end
67 15
68 def set_metric_configurations 16 def set_metric_configurations
69 - @metric_configurations = MetricConfiguration.metric_configurations_of(params[:kalibro_configuration_id].to_i) 17 + @metric_configurations = MetricConfiguration.metric_configurations_of(@kalibro_configuration.id)
70 end 18 end
71 -  
72 end 19 end
app/controllers/hotspot_metric_configurations_controller.rb
  Diff comments   View file @eaa33fc
1 class HotspotMetricConfigurationsController < BaseMetricConfigurationsController 1 class HotspotMetricConfigurationsController < BaseMetricConfigurationsController
2 - def create  
3 - super  
4 - @metric_configuration.metric = KalibroClient::Entities::Processor::MetricCollectorDetails.find_by_name(params[:metric_collector_name]).find_metric_by_code params[:metric_code]  
5 - respond_to do |format|  
6 - if @metric_configuration.save  
7 - format.html do  
8 - redirect_to kalibro_configuration_path(@metric_configuration.kalibro_configuration_id),  
9 - notice: t('successfully_created', record: t(@metric_configuration.class))  
10 - end  
11 - else  
12 - failed_action(format)  
13 - end  
14 - end  
15 - clear_caches  
16 - end  
17 -  
18 - protected  
19 -  
20 - def update_metric_configuration(new_metric_configuration)  
21 - @kalibro_configuration_id = params[:kalibro_configuration_id]  
22 - @metric_configuration = new_metric_configuration  
23 - end 2 + METRIC_TYPE = 'HotspotMetricSnapshot'
24 3
25 def metric_configuration_params 4 def metric_configuration_params
26 - { kalibro_configuration_id: params[:kalibro_configuration_id] } 5 + # Hotspot metric configurations don't accept any parameters on creation
  6 + # But we must make that explicit as the method isn't implemented in the parent class
  7 + params.permit
27 end 8 end
28 9
29 - private  
30 -  
31 - def failed_action(format)  
32 - @kalibro_configuration_id = params[:kalibro_configuration_id]  
33 - format.html { redirect_to kalibro_configuration_choose_metric_path } 10 + # FIXME: This action should render with an error message on rendering the html
  11 + def failed_action(format, _)
  12 + format.html { redirect_to kalibro_configurations_path(@kalibro_configuration.id) }
34 format.json { render json: @metric_configuration.kalibro_errors, status: :unprocessable_entity } 13 format.json { render json: @metric_configuration.kalibro_errors, status: :unprocessable_entity }
35 end 14 end
36 -  
37 - def clear_caches  
38 - Rails.cache.delete("#{params[:kalibro_configuration_id]}_tree_metric_configurations")  
39 - Rails.cache.delete("#{params[:kalibro_configuration_id]}_hotspot_metric_configurations")  
40 - end  
41 end 15 end
app/controllers/metric_configurations_controller.rb
  Diff comments   View file @eaa33fc
1 class MetricConfigurationsController < BaseMetricConfigurationsController 1 class MetricConfigurationsController < BaseMetricConfigurationsController
2 - def choose_metric  
3 - @kalibro_configuration = KalibroConfiguration.find(params[:kalibro_configuration_id].to_i)  
4 - @metric_configuration_id = params[:metric_configuration_id].to_i  
5 - @metric_collectors_names = KalibroClient::Entities::Processor::MetricCollectorDetails.all_names  
6 - end 2 + METRIC_TYPE = 'NativeMetricSnapshot'
7 3
8 - def new  
9 - super  
10 - # FIXME: find_by_name throws an exception instead of returning nil, unlike ActiveRecord's API  
11 - metric_configuration.metric = KalibroClient::Entities::Processor::MetricCollectorDetails.find_by_name(params[:metric_collector_name]).find_metric_by_code params[:metric_code]  
12 - @reading_groups = ReadingGroup.public_or_owned_by_user(current_user).map { |reading_group|  
13 - [reading_group.name, reading_group.id]  
14 - }  
15 - end 4 + before_action :set_reading_groups!, only: [:new, :edit]
16 5
17 - def create 6 + def new
18 super 7 super
19 - @metric_configuration.metric = KalibroClient::Entities::Processor::MetricCollectorDetails.find_by_name(params[:metric_collector_name]).find_metric_by_name params[:metric_name]  
20 - respond_to do |format|  
21 - create_and_redir(format)  
22 - end  
23 - clear_caches  
24 - end  
25 -  
26 - def edit  
27 - # FIXME: set the configuration id just once!  
28 - @kalibro_configuration_id = params[:kalibro_configuration_id]  
29 - @metric_configuration.kalibro_configuration_id = @kalibro_configuration_id  
30 - @reading_groups = ReadingGroup.public_or_owned_by_user(current_user).map { |reading_group|  
31 - [reading_group.name, reading_group.id]  
32 - }  
33 - end  
34 -  
35 - def update  
36 - respond_to do |format|  
37 - @metric_configuration.kalibro_configuration_id = params[:kalibro_configuration_id]  
38 - if @metric_configuration.update(metric_configuration_params)  
39 - format.html { redirect_to(kalibro_configuration_path(@metric_configuration.kalibro_configuration_id), notice: t('successfully_updated', :record => t(metric_configuration.class))) }  
40 - format.json { head :no_content }  
41 - clear_caches  
42 - else  
43 - failed_action(format, 'edit')  
44 - end  
45 - end 8 + # The metric parameter comes from the choose metric form
  9 + set_metric!
46 end 10 end
47 11
48 - def destroy  
49 - @metric_configuration.destroy  
50 - respond_to do |format|  
51 - format.html { redirect_to kalibro_configuration_path(params[:kalibro_configuration_id]) }  
52 - format.json { head :no_content }  
53 - end  
54 - clear_caches 12 + def choose_metric
  13 + @metric_collectors_names = KalibroClient::Entities::Processor::MetricCollectorDetails.all_names
55 end 14 end
56 15
57 protected 16 protected
58 17
59 - def metric_configuration  
60 - @metric_configuration  
61 - end  
62 -  
63 - def update_metric_configuration (new_metric_configuration)  
64 - @kalibro_configuration_id = params[:kalibro_configuration_id]  
65 - @metric_configuration = new_metric_configuration 18 + def metric_configuration_params
  19 + params.require(:metric_configuration).permit(:reading_group_id, :weight, :aggregation_form)
66 end 20 end
67 21
68 private 22 private
69 23
70 - def clear_caches  
71 - Rails.cache.delete("#{params[:kalibro_configuration_id]}_tree_metric_configurations")  
72 - Rails.cache.delete("#{params[:kalibro_configuration_id]}_hotspot_metric_configurations")  
73 - end  
74 -  
75 - # FIXME: Duplicated code on create and update actions extracted here  
76 - def failed_action(format, destiny_action)  
77 - @kalibro_configuration_id = params[:kalibro_configuration_id]  
78 -  
79 - format.html { render action: destiny_action }  
80 - format.json { render json: @metric_configuration.kalibro_errors, status: :unprocessable_entity }  
81 - end  
82 -  
83 - # Code extracted from create action  
84 - def create_and_redir(format)  
85 - if @metric_configuration.save  
86 - format.html { redirect_to kalibro_configuration_path(@metric_configuration.kalibro_configuration_id), notice: t('successfully_created', :record => t(metric_configuration.class)) }  
87 - else  
88 - failed_action(format, 'new') 24 + def set_reading_groups!
  25 + @reading_groups = ReadingGroup.public_or_owned_by_user(current_user).map do |reading_group|
  26 + [reading_group.name, reading_group.id]
89 end 27 end
90 end 28 end
91 end 29 end