Notifications are now always recorded

Email notifications records are now marked as sent (along with a sent
time) instead of being destroyed.

This allows us to see how many notifications we are sending per time
period, and detect bugs in the notification code more accurately.

Tests included to ensure behaving as expected, all tests I have in this
spec pass.

@Jellyfishboy I did this in master in the hope to deploy it
immediately, however there are too many tests failing for me to risk
it, so I will simply merge it into staging. Commenting you as you may
be interested to know I've done this!

Gemfile

@@ -44,6 +44,9 @@ gem 'itunes-search-api'
 gem 'rails_admin'
 gem 'exception_notification', :require => 'exception_notifier'
 
+group :development, :test do
+  gem 'pry'
+end
 
 group :assets do
   # in production environments by default.

Gemfile.lock

@@ -53,6 +53,7 @@ GEM
       activesupport (>= 3.0)
     cocaine (0.5.1)
       climate_control (>= 0.0.3, < 1.0)
+    coderay (1.0.9)
     coffee-rails (3.2.2)
       coffee-script (>= 2.2.0)
       railties (~> 3.2.0)
@@ -141,6 +142,7 @@ GEM
       i18n (>= 0.4.0)
       mime-types (~> 1.16)
       treetop (~> 1.4.8)
+    method_source (0.8.1)
     mime-types (1.21)
     multi_json (1.7.2)
     multi_xml (0.5.3)
@@ -179,6 +181,10 @@ GEM
       mime-types
     polyglot (0.3.3)
     pr_geohash (1.0.0)
+    pry (0.9.12.2)
+      coderay (~> 1.0.5)
+      method_source (~> 0.8)
+      slop (~> 3.4)
     rack (1.4.5)
     rack-cache (1.2)
       rack (>= 0.4)
@@ -269,6 +275,7 @@ GEM
     slim (1.3.6)
       temple (~> 0.5.5)
       tilt (~> 1.3.3)
+    slop (3.4.5)
     sprockets (2.2.2)
       hike (~> 1.2)
       multi_json (~> 1.0)
@@ -331,6 +338,7 @@ DEPENDENCIES
   newrelic_rpm
   omniauth-facebook
   paperclip
+  pry
   rails (~> 3)
   rails_admin
   rspec-rails

app/mailers/user_mailer.rb

@@ -17,7 +17,11 @@ def new_releases(user,opts={:deliver => false})
     @user = user
     frequency = @user.email_frequency
 
-    @releases = user.release_notifications.order("date DESC").where("date < ?",Date.today+1).limit(20)
+    conditions = ["releases.date < ? and notifications.sent = ?",Date.today+1, false]
+
+    @releases = user.release_notifications.order("date DESC").where(conditions).limit(20)
+
+    notifications = user.notifications.all(:include => [:release], :conditions => conditions, :limit => 20, :order => "date DESC")
     if @releases.empty?
       return false
     end
@@ -50,7 +54,9 @@ def new_releases(user,opts={:deliver => false})
     if opts[:deliver]
       m.deliver
     end
-    user.release_notifications.delete(@releases)
+    notifications.each do |n|
+      n.mark_as_sent!
+    end
 
     return m
 

app/models/notification.rb

@@ -3,5 +3,10 @@ class Notification < ActiveRecord::Base
   belongs_to :release
   belongs_to :user
 
+  def mark_as_sent!
+  	self.sent = true
+  	self.sent_at = Time.now
+  	self.save
+  end
 
 end

app/models/release.rb

@@ -19,8 +19,8 @@ class Release < ActiveRecord::Base
     },
     :fog_public => true
 
-  has_many :notification, :dependent => :destroy
-  has_many :user_notifications, :through => :notification, :source => :user
+  has_many :notifications, :dependent => :destroy
+  has_many :user_notifications, :through => :notifications, :source => :user
   belongs_to :artist
 
   if !Rails.env.test?

app/models/user.rb

@@ -21,8 +21,8 @@ class User < ActiveRecord::Base
 
   has_many :following, :through => :follow, :source => :artist
   has_many :suggested_all, :through => :suggest, :source => :artist
-  has_many :notification, :dependent => :delete_all
-  has_many :release_notifications, :through => :notification, :source => :release
+  has_many :notifications, :dependent => :delete_all
+  has_many :release_notifications, :through => :notifications, :source => :release
 
   before_save :default_values
   def default_values

db/migrate/20130526111603_add_sent_to_notification.rb

@@ -0,0 +1,8 @@
+class AddSentToNotification < ActiveRecord::Migration
+  def change
+  	change_table :notifications do |t|
+	  	t.boolean :sent, :default => false
+	  	t.datetime :sent_at
+	  end
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended to check this file into your version control system.
 
-ActiveRecord::Schema.define(:version => 20130301200601) do
+ActiveRecord::Schema.define(:version => 20130526111603) do
 
   create_table "artists", :force => true do |t|
     t.string   "name",                                  :null => false
@@ -33,7 +33,7 @@
     t.string   "fbid"
     t.boolean  "ignore",             :default => false, :null => false
     t.text     "twitter"
-    t.string   "image_file_name"
+    t.string   "image"
     t.string   "image_content_type"
     t.integer  "image_file_size"
     t.datetime "image_updated_at"
@@ -42,14 +42,22 @@
 
   add_index "artists", ["ignore"], :name => "index_artists_on_ignore"
 
-  create_table "feedbacks", :force => true do |t|
-    t.integer  "user_id"
-    t.text     "feedback"
-    t.string   "url"
-    t.datetime "created_at", :null => false
-    t.datetime "updated_at", :null => false
+  create_table "delayed_jobs", :force => true do |t|
+    t.integer  "priority",   :default => 0
+    t.integer  "attempts",   :default => 0
+    t.text     "handler"
+    t.text     "last_error"
+    t.datetime "run_at"
+    t.datetime "locked_at"
+    t.datetime "failed_at"
+    t.string   "locked_by"
+    t.string   "queue"
+    t.datetime "created_at",                :null => false
+    t.datetime "updated_at",                :null => false
   end
 
+  add_index "delayed_jobs", ["priority", "run_at"], :name => "delayed_jobs_priority"
+
   create_table "follows", :force => true do |t|
     t.integer  "user_id"
     t.integer  "artist_id"
@@ -62,8 +70,10 @@
   create_table "notifications", :force => true do |t|
     t.integer  "release_id"
     t.integer  "user_id"
-    t.datetime "created_at", :null => false
-    t.datetime "updated_at", :null => false
+    t.datetime "created_at",                    :null => false
+    t.datetime "updated_at",                    :null => false
+    t.boolean  "sent",       :default => false
+    t.datetime "sent_at"
   end
 
   create_table "releases", :force => true do |t|
@@ -82,7 +92,7 @@
     t.text     "label_name"
     t.boolean  "scraped"
     t.text     "sd_id"
-    t.string   "image_file_name"
+    t.string   "image"
     t.string   "image_content_type"
     t.integer  "image_file_size"
     t.datetime "image_updated_at"
@@ -98,6 +108,16 @@
   add_index "releases", ["date", "artist_id"], :name => "index_releases_on_date_and_artist_id"
   add_index "releases", ["ignore", "upc"], :name => "index_releases_on_ignore_and_upc"
 
+  create_table "reports", :force => true do |t|
+    t.integer  "user_id"
+    t.text     "comments"
+    t.string   "url"
+    t.string   "elements"
+    t.datetime "created_at", :null => false
+    t.datetime "updated_at", :null => false
+    t.string   "release"
+  end
+
   create_table "roles", :force => true do |t|
     t.string   "name"
     t.datetime "created_at", :null => false

spec/spec_helper.rb

@@ -2,7 +2,6 @@
 ENV["RAILS_ENV"] ||= 'test'
 require File.expand_path("../../config/environment", __FILE__)
 require 'rspec/rails'
-require 'rspec/autorun'
 require 'database_cleaner'
 
 # Requires supporting ruby files with custom matchers and macros, etc,

spec/user_mailer_spec.rb

@@ -24,6 +24,32 @@
 			email = UserMailer.new_releases(@user2)
 			email.is_a?(ActionMailer::Base::NullMail).should eq true
 		end
+
+		it "marks sent notifications as sent" do
+			Notification.count.should eq 0
+			@user.following << @artist
+			@artist.releases.build(attributes_for(:release)).save
+			Notification.first.sent.should eq false
+			UserMailer.new_releases(@user)
+
+			Notification.first.sent.should eq true
+			sent_ago = Time.now - Notification.first.sent_at
+			sent_ago.should < 10.seconds
+		end
+
+		it "leaves unsent notifications as unsent" do
+			Notification.count.should eq 0
+			@user.following << @artist
+			@user2 = create(:user)
+			@user2.following << @artist
+			@artist.releases.build(attributes_for(:release)).save
+			Notification.where(:sent => true).length.should eq 0
+			UserMailer.new_releases(@user)
+
+			Notification.where(:sent => false).first.user.should eq @user2
+			Notification.where(:sent => true).first.user.should eq @user
+		end
+
 	end
 
 	context "when there are no new releases" do
@@ -32,6 +58,16 @@
 			email = UserMailer.new_releases(@user)
 			email.is_a?(ActionMailer::Base::NullMail).should eq true
 		end
+
+		it "does not resend sent notifications" do
+			@user.following << @artist
+			release = @artist.releases.build(attributes_for(:release))
+			release.save
+			UserMailer.new_releases(@user)
+			# Resend again, but now with notification marked as sent
+			email = UserMailer.new_releases(@user)
+			email.is_a?(ActionMailer::Base::NullMail).should eq true
+		end
 	end
 
 end