frab · erdgeist · Oct 29, 2019 · Aug 29, 2019 · Aug 26, 2019 · Aug 27, 2019
diff --git a/app/assets/stylesheets/frab.css b/app/assets/stylesheets/frab.css
@@ -60,6 +60,8 @@ div.rating {
   white-space: nowrap;
 }
 
+label.review-scores { width: auto; min-width: 40px; }
+
 table.room {
   width: 150px;
   margin: 10px;

diff --git a/app/controllers/conferences_controller.rb b/app/controllers/conferences_controller.rb
@@ -105,6 +105,13 @@ def edit_classifiers
     end
   end
 
+  def edit_review_metrics
+    authorize @conference, :orga?
+    respond_to do |format|
+      format.html
+    end
+  end
+
   def send_notification
     authorize @conference, :orga?
     SendBulkTicketJob.new.async.perform @conference, params[:notification]
@@ -172,7 +179,7 @@ def get_previous_nested_form(parameters)
       next unless attribs.positive?
 
       test = name.gsub('_attributes', '')
-      next unless %w(rooms days schedule notifications tracks classifiers ticket_server).include?(test)
+      next unless %w(rooms days schedule notifications tracks review_metrics classifiers ticket_server).include?(test)
       return "edit_#{test}"
     }
     'edit'
@@ -228,7 +235,8 @@ def existing_conference_params
 
     if @conference.main_conference? || policy(@conference.parent).manage?
       allowed += [
-	classifiers_attributes: %i(name description _destroy id),
+        classifiers_attributes: %i(name description _destroy id),
+        review_metrics_attributes: %i(name description _destroy id),
         rooms_attributes: %i(name size public rank _destroy id),
         tracks_attributes: %i(name color _destroy id)
       ]

diff --git a/app/controllers/event_ratings_controller.rb b/app/controllers/event_ratings_controller.rb
@@ -4,6 +4,12 @@ class EventRatingsController < BaseConferenceController
 
   def show
     @rating = @event.event_ratings.find_by(person_id: current_user.person.id) || EventRating.new
+
+    # Add any review_metrics missing from @rating
+    missing_ids = @conference.review_metrics.pluck(:id) - (@rating.review_scores.pluck(:review_metric_id))
+
+    @rating.review_scores_attributes = missing_ids.map{ |rmid| { review_metric_id: rmid, score: 0} }
+
     setup_batch_reviews_next_event
   end
 
@@ -67,6 +73,6 @@ def find_event
   end
 
   def event_rating_params
-    params.require(:event_rating).permit(:rating, :comment, :text)
+    params.require(:event_rating).permit(:rating, :comment, :text, review_scores_attributes: [:score, :review_metric_id, :id])
   end
 end
diff --git a/app/controllers/events_controller.rb b/app/controllers/events_controller.rb
@@ -84,7 +84,7 @@ def attachments
   def ratings
     authorize @conference, :read?
 
-    result = search @conference.events
+    result = search @conference.events_with_review_averages
     @events = result.paginate page: page_param
     clean_events_attributes
 
@@ -94,7 +94,7 @@ def ratings
     @events_no_review_total = @events_total - @events_reviewed_total
 
     # current_user rated:
-    @events_reviewed = @conference.events.joins(:event_ratings).where('event_ratings.person_id' => current_user.person.id).count
+    @events_reviewed = @conference.events.joins(:event_ratings).where('event_ratings.person_id' => current_user.person.id).where.not('event_ratings.rating' => [nil, 0]).count
     @events_no_review = @events_total - @events_reviewed
   end
 

diff --git a/app/models/average_review_score.rb b/app/models/average_review_score.rb
@@ -0,0 +1,8 @@
+class AverageReviewScore < ApplicationRecord
+  belongs_to :event
+  belongs_to :review_metric
+  has_one :conference, through: :event
+
+  validates :review_metric, presence: true, uniqueness: { scope: :event }
+  validates :event, presence: true
+end
diff --git a/app/models/conference.rb b/app/models/conference.rb
@@ -5,6 +5,7 @@ class Conference < ApplicationRecord
 
   has_many :availabilities, dependent: :destroy
   has_many :classifiers, dependent: :destroy
+  has_many :review_metrics, dependent: :destroy
   has_many :conference_users, dependent: :destroy
   has_many :days, dependent: :destroy
   has_many :events, dependent: :destroy
@@ -21,6 +22,7 @@ class Conference < ApplicationRecord
 
   accepts_nested_attributes_for :rooms, reject_if: proc { |r| r['name'].blank? }, allow_destroy: true
   accepts_nested_attributes_for :classifiers, reject_if: proc { |r| r['name'].blank? }, allow_destroy: true
+  accepts_nested_attributes_for :review_metrics, reject_if: proc { |r| r['name'].blank? }, allow_destroy: true
   accepts_nested_attributes_for :days, reject_if: :all_blank, allow_destroy: true
   accepts_nested_attributes_for :notifications, allow_destroy: true
   accepts_nested_attributes_for :tracks, reject_if: :all_blank, allow_destroy: true
@@ -210,6 +212,10 @@ def schedule_events
       .scheduled
   end
 
+  def events_with_review_averages
+    events.with_review_averages(self)
+  end
+
   def to_s
     "#{model_name.human}: #{title} (#{acronym})"
   end

diff --git a/app/models/event.rb b/app/models/event.rb
@@ -14,6 +14,8 @@ class Event < ApplicationRecord
   has_many :event_feedbacks, dependent: :destroy
   has_many :event_people, dependent: :destroy
   has_many :event_ratings, dependent: :destroy
+  has_many :review_scores, through: :event_ratings
+  has_many :average_review_scores, dependent: :destroy
   has_many :event_classifiers, dependent: :destroy
   has_many :links, as: :linkable, dependent: :destroy
   has_many :people, through: :event_people
@@ -31,6 +33,8 @@ class Event < ApplicationRecord
   accepts_nested_attributes_for :event_attachments, allow_destroy: true, reject_if: :all_blank
   accepts_nested_attributes_for :ticket, allow_destroy: true, reject_if: :all_blank
   accepts_nested_attributes_for :event_classifiers, allow_destroy: true
+  accepts_nested_attributes_for :event_ratings, allow_destroy: true
+  accepts_nested_attributes_for :average_review_scores, allow_destroy: true
 
   validates_attachment_content_type :logo, content_type: [/jpg/, /jpeg/, /png/, /gif/]
 
@@ -48,16 +52,36 @@ class Event < ApplicationRecord
   scope :with_speaker, -> { where('speaker_count > 0') }
   scope :with_more_than_one_speaker, -> { where('speaker_count > 1') }
 
+  scope :with_review_averages, ->(conference) {
+    e = select(column_names, conference.review_metrics.map{|rm| "#{rm.safe_name}.score AS #{rm.safe_name}"})
+    conference.review_metrics.each do |rm|
+      e = e.joins("LEFT OUTER JOIN average_review_scores #{rm.safe_name} ON #{rm.safe_name}.event_id=events.id AND #{rm.safe_name}.review_metric_id=#{rm.id}")
+    end
+    e
+  }
+
+  ReviewMetric.all.each do |rm|
+    ransacker rm.safe_name do
+      Arel.sql(rm.safe_name)
+    end
+  end
+
   has_paper_trail
   has_secure_token :invite_token
 
   def self.ids_by_least_reviewed(conference, reviewer)
-    already_reviewed = connection.select_rows("SELECT events.id FROM events JOIN event_ratings ON events.id = event_ratings.event_id WHERE events.conference_id = #{conference.id} AND event_ratings.person_id = #{reviewer.id}").flatten.map(&:to_i)
-    least_reviewed = connection.select_rows("SELECT events.id FROM events LEFT OUTER JOIN event_ratings ON events.id = event_ratings.event_id WHERE events.conference_id = #{conference.id} GROUP BY events.id ORDER BY COUNT(event_ratings.id) ASC, events.id ASC").flatten.map(&:to_i)
+    already_reviewed = connection.select_rows("SELECT events.id 
+                                               FROM events 
+                                               JOIN event_ratings ON events.id = event_ratings.event_id 
+                                               WHERE events.conference_id = #{conference.id}
+                                               AND   event_ratings.person_id = #{reviewer.id} 
+                                               AND   event_ratings.rating IS NOT NULL 
+                                               AND   event_ratings.rating <> 0").flatten.map(&:to_i)
+    least_reviewed = conference.events.order(event_ratings_count: :asc).pluck(:id)
     least_reviewed -= already_reviewed
     least_reviewed
   end
-  
+
   def localized_event_type(locale = nil)
     return '' unless event_type.present?
     I18n.t(event_type, scope: 'options', locale: locale, default: event_type)
@@ -70,7 +94,7 @@ def track_name
   def end_time
     start_time.since((time_slots * conference.timeslot_duration).minutes)
   end
-  
+
   def people_involved_or_reviewing
     ids = event_ratings.pluck(:person_id) + event_people.pluck(:person_id)
     Person.where(id: ids.uniq)
@@ -94,7 +118,14 @@ def recalculate_average_feedback!
   end
 
   def recalculate_average_rating!
-    update_attributes(average_rating: average(:event_ratings))
+    update_attributes(average_rating: average_of_nonzeros(event_ratings.pluck(:rating)), event_ratings_count: event_ratings.where.not(rating: [nil, 0]).count )
+  end
+
+  def recalculate_review_averages!
+    conference.review_metrics.each do |review_metric|
+      avg = average_of_nonzeros(review_scores.where(review_metric: review_metric).pluck(:score))
+      average_review_scores.find_or_create_by(review_metric: review_metric).update_attributes(score: avg)
+    end
   end
 
   def speakers
@@ -178,4 +209,11 @@ def average(rating_type)
     return nil if rating_count.zero?
     result.to_f / rating_count
   end
+
+  def average_of_nonzeros(list)
+    return nil unless list
+    list=list.select{ |x| x && x>0 }
+    return nil if list.empty?
+    list.reduce(:+).to_f / list.size
+  end
 end
diff --git a/app/models/event_rating.rb b/app/models/event_rating.rb
@@ -1,15 +1,16 @@
 class EventRating < ApplicationRecord
-  belongs_to :event, counter_cache: true
+  belongs_to :event
   has_one :conference, through: :event
+  has_many :review_scores, dependent: :destroy
   belongs_to :person
 
   after_save :update_average
   after_destroy :update_average
 
-  validates :rating, presence: true
-
   validates :event, presence: true
   validates :person, presence: true
+
+  accepts_nested_attributes_for :review_scores, allow_destroy: true
 
   protected
 

diff --git a/app/models/review_metric.rb b/app/models/review_metric.rb
@@ -0,0 +1,19 @@
+class ReviewMetric < ApplicationRecord
+  belongs_to :conference
+  has_many :review_score, dependent: :destroy
+  has_many :average_review_score, dependent: :destroy
+
+  has_paper_trail meta: { associated_id: :conference_id, associated_type: 'Conference' }
+
+  def to_s
+    "#{model_name.human}: #{name}"
+  end
+
+  def safe_name
+    # safe_name is used as an sql term, and also as a request parameter.
+    # So we try to have it similiar to the r
A377
eview metric name.
+    name.parameterize.gsub(%r{[^a-z0-9]}, '_').presence || "rm#{id}"
+  end
+
+  validates :name, presence: true, uniqueness: { scope: :conference }
+end
diff --git a/app/models/review_score.rb b/app/models/review_score.rb
@@ -0,0 +1,19 @@
+class ReviewScore < ApplicationRecord
+  belongs_to :event_rating
+  belongs_to :review_metric
+  has_one :event, through: :event_rating
+  has_one :conference, through: :event_rating
+
+  after_save :update_average
+  after_destroy :update_average
+
+  validates :score, inclusion: 0..5 # 0 is N/A
+  validates :review_metric, presence: true, uniqueness: { scope: :event_rating }
+  validates :event_rating, presence: true
+
+  protected
+
+  def update_average
+    event_rating.event.recalculate_review_averages!
+  end
+end
diff --git a/app/views/conferences/_form_review_metrics.html.haml b/app/views/conferences/_form_review_metrics.html.haml
@@ -0,0 +1,4 @@
+= simple_form_for(@conference, url: conference_path) do |f|
+  = dynamic_association :review_metrics, t(:review_metrics), f
+  .actions
+    = f.button :submit, class: 'primary'
diff --git a/app/views/conferences/_review_metric_fields.html.haml b/app/views/conferences/_review_metric_fields.html.haml
@@ -0,0 +1,5 @@
+.nested-fields
+  %fieldset.inputs
+    = f.input :name
+    = f.input :description
+    = remove_association_link :review_metric, f
diff --git a/app/views/conferences/_tabs.html.haml b/app/views/conferences/_tabs.html.haml
@@ -14,6 +14,8 @@
       = link_to t(:ticket_server), edit_ticket_server_conference_path
   - conference_tab(:classifiers, active) do
     = link_to t(:classifiers), edit_classifiers_conference_path
+  - conference_tab(:reviewing, active) do
+    = link_to t(:reviewing), edit_review_metrics_conference_path
   - if @conference.ticket_type == 'integrated' or @conference.bulk_notification_enabled
     - conference_tab(:notifications, active) do
       = link_to t('notifications'), edit_notifications_conference_path
diff --git a/app/views/conferences/edit_review_metrics.html.haml b/app/views/conferences/edit_review_metrics.html.haml
@@ -0,0 +1,26 @@
+%section
+  .page-header
+    %h1= t :edit_conference_review_metrics
+  = render partial: 'tabs', locals: { active: :reviewing }
+
+  - if @conference.sub_conference? && !policy(@conference).manage?
+    .row
+      .span16
+        .blank-slate
+          %p= raw(GitHub.render(t('reviewing_module.modify_review_metric_with_parent', {parent: @conference.parent.title})))
+          %p= t('reviewing_module.current_review_metrics')
+
+          %uls
+            - @conference.review_metrics.each do |review_metric|
+              %li
+                ="#{review_metric.name} (description: #{review_metric.description})"
+
+  - else
+    - if @conference.review_metrics.empty?
+      .row
+        .span16
+          .blank-slate
+            %p= t('reviewing_module.empty_review_metrics')
+    .row
+      .span16
+        = render 'form_review_metrics'
diff --git a/app/views/event_ratings/show.html.haml b/app/views/event_ratings/show.html.haml
@@ -75,6 +75,17 @@
         %fieldset.inputs
           = f.input :rating, as: :rating
           = f.input :comment, input_html: {rows: 3}
+          = f.simple_fields_for :review_scores do |ff|
+            = ff.input :score,
+              as: :radio_buttons,
+              collection: [[t('events_module.not_applicable'), '0'], ['1', '1'], ['2', '2'], ['3', '3'], ['4', '4'], ['5', '5']], 
+              label: ff.object.review_metric.name, 
+              hint: ff.object.review_metric.description,
+              item_label_class: 'review-scores'
+
+            = ff.input :review_metric_id, as: :hidden
+            = ff.input :id, as: :hidden
+
         .actions
           = f.button :submit,  class: 'primary'
           - if @rating.persisted?
@@ -98,6 +109,8 @@
                 %th
                 %th= t('user')
                 %th= t('rating')
+                - @conference.review_metrics.each do |review_metric|
+                  %th=review_metric.name
                 %th= t('comment')
             %tbody
               - @event_ratings.each do |event_rating|
@@ -108,4 +121,7 @@
                     %td=event_rating.person.full_name
                     %td
                       = raty_for("event_rating_#{event_rating.id}", event_rating.rating)
+                    - @conference.review_metrics.each do |review_metric|
+                      - score=event_rating.review_scores.find_by(review_metric: review_metric)&.score
+                      %td=score if score and score>0
                     %td=event_rating.comment