Use serializer for formatting cohorts data

8 files changed, 168 insertions, 67 deletions

diff --git a/app/controllers/admin/cohorts_controller.rb b/app/controllers/admin/cohorts_controller.rb
index 947afe3a028..9b77c554908 100644
--- a/app/controllers/admin/cohorts_controller.rb
+++ b/app/controllers/admin/cohorts_controller.rb
@@ -1,9 +1,11 @@
 class Admin::CohortsController < Admin::ApplicationController
   def index
-    if ApplicationSetting.current.usage_ping_enabled
-      @cohorts = Rails.cache.fetch('cohorts', expires_in: 1.day) do
+    if current_application_settings.usage_ping_enabled
+      cohorts_results = Rails.cache.fetch('cohorts', expires_in: 1.day) do
         CohortsService.new.execute
       end
+
+      @cohorts = CohortsSerializer.new.represent(cohorts_results)
     end
   end
 end
diff --git a/app/serializers/cohort_activity_month_entity.rb b/app/serializers/cohort_activity_month_entity.rb
new file mode 100644
index 00000000000..e6788a8b596
--- /dev/null
+++ b/app/serializers/cohort_activity_month_entity.rb
@@ -0,0 +1,11 @@
+class CohortActivityMonthEntity < Grape::Entity
+  include ActionView::Helpers::NumberHelper
+
+  expose :total do |cohort_activity_month|
+    number_with_delimiter(cohort_activity_month[:total])
+  end
+
+  expose :percentage do |cohort_activity_month|
+    number_to_percentage(cohort_activity_month[:percentage], precision: 0)
+  end
+end
diff --git a/app/serializers/cohort_entity.rb b/app/serializers/cohort_entity.rb
new file mode 100644
index 00000000000..7cdba5b0484
--- /dev/null
+++ b/app/serializers/cohort_entity.rb
@@ -0,0 +1,17 @@
+class CohortEntity < Grape::Entity
+  include ActionView::Helpers::NumberHelper
+
+  expose :registration_month do |cohort|
+    cohort[:registration_month].strftime('%b %Y')
+  end
+
+  expose :total do |cohort|
+    number_with_delimiter(cohort[:total])
+  end
+
+  expose :inactive do |cohort|
+    number_with_delimiter(cohort[:inactive])
+  end
+
+  expose :activity_months, using: CohortActivityMonthEntity
+end
diff --git a/app/serializers/cohorts_entity.rb b/app/serializers/cohorts_entity.rb
new file mode 100644
index 00000000000..98f5995ba6f
--- /dev/null
+++ b/app/serializers/cohorts_entity.rb
@@ -0,0 +1,4 @@
+class CohortsEntity < Grape::Entity
+  expose :months_included
+  expose :cohorts, using: CohortEntity
+end
diff --git a/app/serializers/cohorts_serializer.rb b/app/serializers/cohorts_serializer.rb
new file mode 100644
index 00000000000..fe9367b13d8
--- /dev/null
+++ b/app/serializers/cohorts_serializer.rb
@@ -0,0 +1,3 @@
+class CohortsSerializer < AnalyticsGenericSerializer
+  entity CohortsEntity
+end
diff --git a/app/services/cohorts_service.rb b/app/services/cohorts_service.rb
index e7f8a50605f..a7963f01176 100644
--- a/app/services/cohorts_service.rb
+++ b/app/services/cohorts_service.rb
@@ -1,11 +1,19 @@
 class CohortsService
   MONTHS_INCLUDED = 12
 
-  # Get a hash that looks like:
+  def execute
+    {
+      months_included: MONTHS_INCLUDED,
+      cohorts: cohorts
+    }
+  end
+
+  # Get an array of hashes that looks like:
   #
-  #     {
-  #       month => {
-  #         months: [3, 2, 1],
+  #     [
+  #       {
+  #         registration_month: Date.new(2017, 3),
+  #         activity_months: [3, 2, 1],
   #         total: 3
   #         inactive: 0
   #      },
@@ -13,29 +21,26 @@ class CohortsService
   #
   # The `months` array is always from oldest to newest, so it's always
   # non-strictly decreasing from left to right.
-  #
-  def execute
-    cohorts = {}
+  def cohorts
     months = Array.new(MONTHS_INCLUDED) { |i| i.months.ago.beginning_of_month.to_date }
 
-    MONTHS_INCLUDED.times do
-      created_at_month = months.last
-      activity_months = running_totals(months, created_at_month)
+    Array.new(MONTHS_INCLUDED) do
+      registration_month = months.last
+      activity_months = running_totals(months, registration_month)
 
       # Even if no users registered in this month, we always want to have a
       # value to fill in the table.
-      inactive = counts_by_month[[created_at_month, nil]].to_i
+      inactive = counts_by_month[[registration_month, nil]].to_i
+
+      months.pop
 
-      cohorts[created_at_month] = {
-        months: activity_months,
-        total: activity_months.first,
+      {
+        registration_month: registration_month,
+        activity_months: activity_months,
+        total: activity_months.first[:total],
         inactive: inactive
       }
-
-      months.pop
     end
-
-    cohorts
   end
 
   private
@@ -44,11 +49,20 @@ class CohortsService
   # count as active in this month, too. Start with the most recent month first,
   # for calculating the running totals, and then reverse for displaying in the
   # table.
-  def running_totals(all_months, created_at_month)
-    all_months
-      .map { |activity_month| counts_by_month[[created_at_month, activity_month]] }
-      .reduce([]) { |result, total| result << result.last.to_i + total.to_i }
-      .reverse
+  #
+  # Each month has a total, and a percentage of the overall total, as keys.
+  def running_totals(all_months, registration_month)
+    month_totals =
+      all_months
+        .map { |activity_month| counts_by_month[[registration_month, activity_month]] }
+        .reduce([]) { |result, total| result << result.last.to_i + total.to_i }
+        .reverse
+
+    overall_total = month_totals.first
+
+    month_totals.map do |total|
+      { total: total, percentage: total.zero? ? 0 : 100 * total / overall_total }
+    end
   end
 
   # Get a hash that looks like:
@@ -60,9 +74,8 @@ class CohortsService
   #     }
   #
   # created_at_month can never be nil, but current_sign_in_at_month can (when a
-  # user has never logged in, just been created). This covers the last twelve
-  # months.
-  #
+  # user has never logged in, just been created). This covers the last
+  # MONTHS_INCLUDED months.
   def counts_by_month
     @counts_by_month ||=
       begin
@@ -80,7 +93,7 @@ class CohortsService
   def column_to_date(column)
     if Gitlab::Database.postgresql?
       "CAST(DATE_TRUNC('month', #{column}) AS date)"
-    elsif Gitlab::Database.mysql?
+    else
       "STR_TO_DATE(DATE_FORMAT(#{column}, '%Y-%m-01'), '%Y-%m-%d')"
     end
   end
diff --git a/app/views/admin/cohorts/_cohorts_table.html.haml b/app/views/admin/cohorts/_cohorts_table.html.haml
index 38795583a8c..c3b37bcf8ec 100644
--- a/app/views/admin/cohorts/_cohorts_table.html.haml
+++ b/app/views/admin/cohorts/_cohorts_table.html.haml
@@ -1,8 +1,8 @@
 .bs-callout.clearfix
   %p
-    User cohorts are shown for the last twelve months. Only users with
-    activity are counted in the cohort total; inactive users are counted
-    separately.
+    User cohorts are shown for the last #{@cohorts[:months_included]}
+    months. Only users with activity are counted in the cohort total; inactive
+    users are counted separately.
     = link_to icon('question-circle'), help_page_path('administration/usage_ping_and_cohorts', anchor: 'cohorts'), title: 'About this feature', target: '_blank'
 
 .table-holder
@@ -12,27 +12,17 @@
         %th Registration month
         %th Inactive users
         %th Cohort total
-        %th Month 0
-        %th Month 1
-        %th Month 2
-        %th Month 3
-        %th Month 4
-        %th Month 5
-        %th Month 6
-        %th Month 7
-        %th Month 8
-        %th Month 9
-        %th Month 10
-        %th Month 11
+        - @cohorts[:months_included].times do |i|
+          %th Month #{i}
     %tbody
-      - @cohorts.each do |registration_month, cohort|
+      - @cohorts[:cohorts].each do |cohort|
         %tr
-          %td= registration_month.strftime('%b %Y')
-          %td= number_with_delimiter(cohort[:inactive])
-          %td= number_with_delimiter(cohort[:total])
-          - cohort[:months].each do |running_total|
+          %td= cohort[:registration_month]
+          %td= cohort[:inactive]
+          %td= cohort[:total]
+          - cohort[:activity_months].each do |activity_month|
             %td
-              - next if cohort[:total].zero?
-              = number_to_percentage(100 * running_total / cohort[:total], precision: 0)
+              - next if cohort[:total] == '0'
+              = activity_month[:percentage]
               %br
-              (#{number_with_delimiter(running_total)})
+              = activity_month[:total]
diff --git a/spec/services/cohorts_service_spec.rb b/spec/services/cohorts_service_spec.rb
index 878e9fcea05..5dd89e3e341 100644
--- a/spec/services/cohorts_service_spec.rb
+++ b/spec/services/cohorts_service_spec.rb
@@ -17,22 +17,83 @@ describe CohortsService do
 
       create(:user) # this user is inactive and belongs to the current month
 
-      expected = {
-        month_start(11) => { months: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], total: 0, inactive: 0 },
-        month_start(10) => { months: [2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1], total: 2, inactive: 0 },
-        month_start(9) => { months: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0], total: 0, inactive: 0 },
-        month_start(8) => { months: [2, 1, 1, 1, 1, 1, 1, 1, 1], total: 2, inactive: 0 },
-        month_start(7) => { months: [0, 0, 0, 0, 0, 0, 0, 0], total: 0, inactive: 0 },
-        month_start(6) => { months: [2, 1, 1, 1, 1, 1, 1], total: 2, inactive: 0 },
-        month_start(5) => { months: [0, 0, 0, 0, 0, 0], total: 0, inactive: 0 },
-        month_start(4) => { months: [2, 1, 1, 1, 1], total: 2, inactive: 0 },
-        month_start(3) => { months: [0, 0, 0, 0], total: 0, inactive: 0 },
-        month_start(2) => { months: [2, 1, 1], total: 2, inactive: 0 },
-        month_start(1) => { months: [0, 0], total: 0, inactive: 0 },
-        month_start(0) => { months: [2], total: 2, inactive: 1 }
-      }
+      expected_cohorts = [
+        {
+          registration_month: month_start(11),
+          activity_months: Array.new(12) { { total: 0, percentage: 0 } },
+          total: 0,
+          inactive: 0
+        },
+        {
+          registration_month: month_start(10),
+          activity_months: [{ total: 2, percentage: 100 }] + Array.new(10) { { total: 1, percentage: 50 } },
+          total: 2,
+          inactive: 0
+        },
+        {
+          registration_month: month_start(9),
+          activity_months: Array.new(10) { { total: 0, percentage: 0 } },
+          total: 0,
+          inactive: 0
+        },
+        {
+          registration_month: month_start(8),
+          activity_months: [{ total: 2, percentage: 100 }] + Array.new(8) { { total: 1, percentage: 50 } },
+          total: 2,
+          inactive: 0
+        },
+        {
+          registration_month: month_start(7),
+          activity_months: Array.new(8) { { total: 0, percentage: 0 } },
+          total: 0,
+          inactive: 0
+        },
+        {
+          registration_month: month_start(6),
+          activity_months: [{ total: 2, percentage: 100 }] + Array.new(6) { { total: 1, percentage: 50 } },
+          total: 2,
+          inactive: 0
+        },
+        {
+          registration_month: month_start(5),
+          activity_months: Array.new(6) { { total: 0, percentage: 0 } },
+          total: 0,
+          inactive: 0
+        },
+        {
+          registration_month: month_start(4),
+          activity_months: [{ total: 2, percentage: 100 }] + Array.new(4) { { total: 1, percentage: 50 } },
+          total: 2,
+          inactive: 0
+        },
+        {
+          registration_month: month_start(3),
+          activity_months: Array.new(4) { { total: 0, percentage: 0 } },
+          total: 0,
+          inactive: 0
+        },
+        {
+          registration_month: month_start(2),
+          activity_months: [{ total: 2, percentage: 100 }] + Array.new(2) { { total: 1, percentage: 50 } },
+          total: 2,
+          inactive: 0
+        },
+        {
+          registration_month: month_start(1),
+          activity_months: Array.new(2) { { total: 0, percentage: 0 } },
+          total: 0,
+          inactive: 0
+        },
+        {
+          registration_month: month_start(0),
+          activity_months: [{ total: 2, percentage: 100 }],
+          total: 2,
+          inactive: 1
+        },
+      ]
 
-      expect(described_class.new.execute).to eq(expected)
+      expect(described_class.new.execute).to eq(months_included: 12,
+                                                cohorts: expected_cohorts)
     end
   end
 end