Rewrote NPS class; it's a better-encapsulated object and should be re…

…usable elsewhere.

app/models/metrics/nps.rb

@@ -1,75 +1,65 @@
 class Metrics::Nps
+  attr_reader :actions, :subscribes, :unsubscribes, :id
 
-  def initialize
-    @ignore_member = 79459 # test user (aaron)
-    @signature_referer_filter = "(referer_id != #{@ignore_member} or referer_id is null)"
+  def initialize(params={})
+    @actions      = params[:actions].try(:nonzero?) || 1
+    @subscribes   = params[:subscribes] || 0
+    @unsubscribes = params[:unsubscribes] || 0
+    @id           = params[:id]
   end
 
-  def single petition
-    petition_id = as_id petition
-    sent = ScheduledEmail.where(petition_id: petition_id).count
-    subscribes = Signature.where(petition_id: petition_id).where(created_member: true).where(@signature_referer_filter).count
-    unsubscribes = Unsubscribe.joins(:sent_email).where("sent_emails.petition_id = #{petition_id}").count      
-    assemble petition_id, sent, subscribes, unsubscribes
+  def sps
+    subscribes.to_f / actions.to_f
   end
 
-  def multiple petitions
-    sent = ScheduledEmail.group(:petition_id).count
-    subscribes = Signature.where(created_member: true).where(@signature_referer_filter).group(:petition_id).count
-    unsubscribes = SentEmail.group(:petition_id).where(id: Unsubscribe.select("sent_email_id").where("sent_email_id IS NOT NULL")).count
-    assemble_multiple petitions, sent, subscribes, unsubscribes
+  def ups
+    unsubscribes.to_f / actions.to_f
   end
 
-  def timespan time_ago, sent_threshold=1000
-    sent = ScheduledEmail.where("created_at > ?", time_ago).where(:petition_id => Petition.where(to_send: true)).group(:petition_id).count
-    subscribes = Signature.where("created_at > ?", time_ago).where(created_member: true).where(@signature_referer_filter).group(:petition_id).count
-    unsubscribes = SentEmail.group(:petition_id).where(id: Unsubscribe.select("sent_email_id").where("sent_email_id IS NOT NULL").where("created_at > ?", time_ago)).count
+  def nps
+    sps - ups
+  end
 
-    petitions = sent.reject{ |k,v| v < sent_threshold }.keys
+  class << self
 
-    assemble_multiple petitions, sent, subscribes, unsubscribes
-  end
+    def email_by_petition(*args)
+      opts = args.last.is_a?(Hash) ? args.pop : {}
+      petition_ids = args.flatten.map(&:to_i)
 
-  def aggregate since
-    sent = ScheduledEmail.where("created_at > ?", since).count
-    subscribes = Signature.where("created_at > ?", since).where(created_member: true).where(@signature_referer_filter).count
-    unsubscribes  = Unsubscribe.where("cause='unsubscribed' and created_at > ?", since).count
-    rates = calculate_rates sent, subscribes, unsubscribes
-    {sent: sent, subscribes: subscribes, unsubscribes: unsubscribes}.merge rates
-  end
+      actions      = ScheduledEmail.where(petition_id: petition_ids).group(:petition_id).count
+      subscribes   = Signature.created.where(petition_id: petition_ids).group(:petition_id).count
+      unsubscribes = Unsubscribe.not_bounced.joins(:sent_email).where(sent_emails: { petition_id: petition_ids }).group("sent_emails.petition_id").count
 
-  private
+      results = petition_ids.map do |id|
+        Metrics::Nps.new id: id, actions: actions[id], subscribes: subscribes[id], unsubscribes: unsubscribes[id.to_s]
+      end
 
-  def assemble petition_id, sent, subscribes, unsubscribes
-    rates = calculate_rates(sent, subscribes, unsubscribes)
-    {
-      petition_id: petition_id,
-      sent: sent,
-      subscribes: subscribes,
-      unsubscribes: unsubscribes
-    }.merge rates
-  end
+      # Return a single NPS object if a single petition ID given.
+      petition_ids.length == 1 ? results.first : results
+    end
 
-  def assemble_multiple petitions, sent, subscribes, unsubscribes
-    sent.default, subscribes.default, unsubscribes.default = 0, 0, 0
+    def email_by_timeframe(time_ago, opts={})
+      actions      = ScheduledEmail.where("created_at > ?", time_ago).where(:petition_id => Petition.where(to_send: true)).group(:petition_id).count
+      subscribes   = Signature.created.where("created_at > ?", time_ago).group(:petition_id).count
+      unsubscribes = SentEmail.group(:petition_id).where(id: Unsubscribe.select("sent_email_id").where("sent_email_id IS NOT NULL").where("created_at > ?", time_ago)).count
 
-    petitions.map do |p|
-      as_id(p)
-    end.map do |id|
-      assemble(id, sent[id], subscribes[id], unsubscribes[id])
+      # FFS use "having" instead. but this is a refactor. keep it together, guthrie.
+      sent_threshold = opts[:sent_threshold] || 1000
+      petition_ids = actions.reject{ |k,v| v < sent_threshold }.keys
+
+      petition_ids.map do |id|
+        Metrics::Nps.new id: id, actions: actions[id], subscribes: subscribes[id], unsubscribes: unsubscribes[id.to_s]
+      end
     end
-  end
 
-  def calculate_rates sent, subscribes, unsubscribes
-    sent = sent.nonzero? || 1
-    sps = subscribes.to_f / sent.to_f
-    ups = unsubscribes.to_f / sent.to_f
-    nps = sps - ups
-    {sps: sps, ups: ups, nps: nps}
-  end
+    def email_aggregate(since)
+      actions       = ScheduledEmail.where("created_at > ?", since).count
+      subscribes    = Signature.where("created_at > ?", since).where(created_member: true).where(@signature_referer_filter).count
+      unsubscribes  = Unsubscribe.where("cause='unsubscribed' and created_at > ?", since).count
+
+      Metrics::Nps.new id: since, actions: actions, subscribes: subscribes, unsubscribes: unsubscribes
+    end
 
-  def as_id p
-    p.respond_to?("id")? p.id : p
   end
 
 end

app/models/signature.rb

@@ -17,6 +17,12 @@ class Signature < ActiveRecord::Base
   after_save :clear_cache
   before_destroy { |record| record.sent_email.destroy if record.sent_email }
 
+  TEST_MEMBER = 79459 # (aaron)
+
+  def self.created
+    where(created_member: true).where("referer_id != ? or referer_id is null", TEST_MEMBER)
+  end
+
   module ReferenceType
     FACEBOOK_LIKE = 'facebook_like'
     FACEBOOK_SHARE = 'facebook_share'

app/models/statistics.rb

@@ -41,19 +41,19 @@ def nps_summary
   end
 
   def fetch_nps_summary
-    nps7d = Metrics::Nps.new.aggregate(1.week.ago)
-    nps24h = Metrics::Nps.new.aggregate(1.day.ago)
-    nps60m = Metrics::Nps.new.aggregate(1.hour.ago)
+    nps7d = Metrics::Nps.email_aggregate(1.week.ago)
+    nps24h = Metrics::Nps.email_aggregate(1.day.ago)
+    nps60m = Metrics::Nps.email_aggregate(1.hour.ago)
     {
-      nps7d: nps7d[:nps],
-      sps7d: nps7d[:sps],
-      ups7d: nps7d[:ups],
-      nps24h: nps24h[:nps],
-      sps24h: nps24h[:sps],
-      ups24h: nps24h[:ups],
-      nps60m: nps60m[:nps],
-      sps60m: nps60m[:sps],
-      ups60m: nps60m[:ups]
+      nps7d: nps7d.nps,
+      sps7d: nps7d.sps,
+      ups7d: nps7d.ups,
+      nps24h: nps24h.nps,
+      sps24h: nps24h.sps,
+      ups24h: nps24h.ups,
+      nps60m: nps60m.nps,
+      sps60m: nps60m.sps,
+      ups60m: nps60m.ups
     }
   end
 
@@ -166,7 +166,7 @@ def petition_extremes
   def fetch_petition_extremes count, threshold
     timespan = 1.send(timeframe).ago
     threshold = extremes_threshold.to_i
-    nps = Metrics::Nps.new.timespan(timespan, threshold).sort_by { |n| n[:nps] }.reverse
+    nps = Metrics::Nps.email_by_timeframe(timespan, sent_threshold: threshold).sort_by(&:nps).reverse
     best = nps.first(count)
     worst = nps.last(count) - best
     {
@@ -176,9 +176,9 @@ def fetch_petition_extremes count, threshold
   end
 
   def associate_petitions stats
-    ids = stats.map{ |n| n[:petition_id] }
+    ids = stats.map &:id
     petitions = Petition.select("id, title").where("id in (?)", ids)
-    stats.map {|s| [petitions.find {|p| p.id == s[:petition_id]}, s]}
+    stats.map {|s| [petitions.find {|p| p.id == s.id}, s]}
   end
 
   def map_to_threshold value, thresholds

app/models/unsubscribe.rb

@@ -6,6 +6,7 @@ class Unsubscribe < ActiveRecord::Base
   after_create :destroy_membership
 
   scope :between, ->(from, to) { where(:created_at => from..to) }
+  scope :not_bounced, -> { where(cause: "unsubscribed") }
   delegate :full_name, to: :member
 
   def self.unsubscribe_member(member)

spec/models/nps_spec.rb

@@ -1,129 +1,95 @@
 describe Metrics::Nps do
+  def create_petition(*args)
+    params  = args.last.is_a?(Hash) ? args.pop : {}
+    actions = args.flatten
+    created_at = params[:created_at] || Time.now
 
-  before(:each) do  
-    @month = 1.month.ago
-    @week = 1.week.ago
-    @day = 1.day.ago
-
-    @petition_a = create(:petition, to_send: true, created_at: @month)
-    @petition_b = create(:petition, to_send: true, created_at: @week)
-    @petition_c = create(:petition, to_send: true, created_at: @day)
-    @petition_unfeatured = create(:petition, to_send: false, created_at: @month)
-
-    @member_old = create(:member)
-    @member_x = create(:member)
-    @member_y = create(:member)
-    @member_z = create(:member)
-
-    # petition_a: send, sign, unsubscribe
-    @sent_aold = create(:scheduled_email, member: @member_old, petition: @petition_a, created_at: @month)
-    @sent_ax = create(:scheduled_email, petition: @petition_a, member: @member_x, created_at: @month)
-    @sent_ay = create(:scheduled_email, petition: @petition_a, member: @member_y, created_at: @month)
-    @sent_az = create(:scheduled_email, petition: @petition_a, member: @member_z, created_at: @week)
-
-    @signature_aold = create(:signature, petition: @petition_a, member: @member_old, created_member: false, created_at: @month)
-    @signature_ax = create(:signature, petition: @petition_a, member: @member_x, created_member: true, created_at: @month)
-    @signature_ay = create(:signature, petition: @petition_a, member: @member_y, created_member: true, created_at: @week)
-
-    @unsubscribe_az = create(:unsubscribe, member: @member_z, sent_email: @sent_az, created_at: @day, cause: "unsubscribed")
-
-    # petition_b: send, sign, unsubscribe
-    @sent_bx = create(:scheduled_email, petition: @petition_b, member: @member_x, created_at: @week)
-    @signature_bx = create(:signature, petition: @petition_b, member: @member_x, created_member: true, created_at: @day)
-
-    # petition_unfeatured: send, sign, unsubscribe
-    # petition could have been featured, then un-featured. 
-    # could have led to a new member signature, but should still be excluded from timeframe queries
-    @sent_unfeaturedold = create(:scheduled_email, petition: @petition_unfeatured, member: @member_old, created_at: @month)
-    @signature_unfeaturedold = create(:signature, petition: @petition_unfeatured, member: @member_old, created_member: true, created_at: @month)
-
+    create(:petition, params.merge(to_send: true)).tap do |petition|
+      actions.each { |action| self.send "create_#{action}", petition, created_at }
+    end
   end
 
-  it "should calculate nps for a single petition" do
-    nps = Metrics::Nps.new.single @petition_a
-    nps[:sent].should eq 4
-    nps[:subscribes].should eq 2
-    nps[:unsubscribes].should eq 1
-    nps[:nps].should eq 0.25
+  def create_subscribe(petition, created_at=Time.now)
+    create :signature, created_member: true, petition: petition, created_at: created_at
   end
 
-  it "should calculate nps for each petition" do
-    nps = Metrics::Nps.new.multiple [@petition_a, @petition_b]
-    nps.count.should eq 2
-
-    nps_a = find_for @petition_a, nps
-    nps_a[:sent].should eq 4
-    nps_a[:subscribes].should eq 2
-    nps_a[:unsubscribes].should eq 1
-    nps_a[:nps].should eq 0.25
-
-    nps_b = find_for @petition_b, nps
-    nps_b[:sent].should eq 1
-    nps_b[:subscribes].should eq 1
-    nps_b[:unsubscribes].should eq 0
-    nps_b[:nps].should eq 1.0
+  def create_unsubscribe(petition, created_at=Time.now)
+    email = ScheduledEmail.where(petition_id: petition.id).last or raise "No email found"
+    create :unsubscribe, sent_email: email, cause: "unsubscribed", created_at: created_at
   end
 
-  context "aggregate" do
+  def create_email(petition, created_at=Time.now)
+    create :scheduled_email, petition: petition, created_at: created_at
+  end
 
-    it "should calculate nps in aggregate over a month" do
-      nps = Metrics::Nps.new.aggregate @month-1.second
-      nps[:sent].should eq 6
-      nps[:subscribes].should eq 4
-      nps[:unsubscribes].should eq 1
-      nps[:nps].should eq 0.5
-    end
+  context 'email_by_petition' do
+
+    context 'with a single petition' do
+      subject { Metrics::Nps.email_by_petition(petition.id) }
+
+      context 'and a signature' do
+        let(:petition) { create_petition :email, :subscribe }
+        specify { expect(subject.nps).to eq(1.0) }
+      end
+
+      context 'and a signature, and an unsubscribe' do
+        let(:petition) { create_petition :email, :subscribe, :unsubscribe }
+        specify { expect(subject.nps).to eq(0.0) }
+      end
 
-    it "should calculate nps in aggregrate over a week" do
-      nps = Metrics::Nps.new.aggregate @week-1.second
-      nps[:sent].should eq 2
-      nps[:subscribes].should eq 2
-      nps[:unsubscribes].should eq 1
-      nps[:nps].should eq 0.5
+      context 'and multiple signatures and unsubscribes' do
+        let(:petition) {
+          create_petition :email, :email, :email, :email, :subscribe, :subscribe, :unsubscribe
+        }
+        specify { expect(subject.nps).to eq(0.25) }
+      end
     end
 
-  end
 
-  context "timespan" do
-
-    it "should calculate nps per item over a month" do
-      nps = Metrics::Nps.new.timespan @month - 1.second, 0
-      nps.count.should eq 2
-
-      a = find_for @petition_a, nps
-      a[:sent].should eq 4
-      a[:subscribes].should eq 2
-      a[:unsubscribes].should eq 1
-      a[:nps].should eq 0.25
-
-      b = find_for @petition_b, nps
-      b[:sent].should eq 1
-      b[:subscribes].should eq 1
-      b[:unsubscribes].should eq 0
-      b[:nps].should eq 1.0
+    context 'with multiple petitions' do
+      let(:first)  { create_petition :email, :subscribe }
+      let(:second) { create_petition :email, :subscribe, :unsubscribe }
+      let(:third)  { create_petition :email, :email, :email, :email, :subscribe, :subscribe, :unsubscribe }
+
+      subject { Metrics::Nps.email_by_petition([ first, second, third ].map(&:id)) }
+
+      specify {
+        expect(subject[0].nps).to eq(1.0)
+        expect(subject[1].nps).to eq(0.0)
+        expect(subject[2].nps).to eq(0.25)
+      }
     end
+  end
 
-    it "should calculate nps per item over a week" do
-      nps = Metrics::Nps.new.timespan @week - 1.second, 0
-      nps.count.should eq 2
-
-      a = find_for @petition_a, nps
-      a[:sent].should eq 1
-      a[:subscribes].should eq 1
-      a[:unsubscribes].should eq 1
-      a[:nps].should eq 0.0
-
-      b = find_for @petition_b, nps
-      b[:sent].should eq 1
-      b[:subscribes].should eq 1
-      b[:unsubscribes].should eq 0
-      b[:nps].should eq 1.0
+  context 'email_by_timeframe' do
+    let(:relevant)  { create_petition :email, :email, :subscribe, created_at: 1.week.ago }
+    let(:ignored)   { create_petition :email, :subscribe,         created_at: 1.month.ago }
+    before { relevant; ignored }
+    subject { Metrics::Nps.email_by_timeframe(timeframe, sent_threshold: 0) }
+
+    context 'with a short timeframe' do
+      let(:timeframe) { 8.days.ago }
+      specify {
+        expect(subject.length).to eq(1)
+        expect(subject[0].nps).to eq(0.5)
+      }
     end
 
+    context 'with a long timeframe' do
+      let(:timeframe) { 2.months.ago }
+      specify {
+        expect(subject.length).to eq(2)
+        expect(subject.map(&:nps)).to match_array([1.0, 0.5])
+      }
+    end
   end
 
-  def find_for p, nps
-    nps.find{|n| n[:petition_id] == p.id}
+  context 'email_aggregate' do
+    let(:oldish) { create_petition :email, :email, :subscribe, :unsubscribe, created_at: 1.month.ago }
+    let(:newish) { create_petition :email, :email, :subscribe, :subscribe,   created_at: 1.week.ago }
+    let(:newest) { create_petition :email, :email, :subscribe,               created_at: 1.day.ago }
+    before { oldish; newish; newest }
+    subject { Metrics::Nps.email_aggregate(1.month.ago - 1.second) }
+    specify { expect(subject.nps).to eq(0.5) }
   end
-
 end