Some refactoring, to simplify user model, optimize, and allow messages to be sorted by date (although are not now.)

Also, rather than use whenever gem, will have cron job created to call task.

10 files changed, 66 insertions, 52 deletions

diff --git a/users/app/controllers/v1/messages_controller.rb b/users/app/controllers/v1/messages_controller.rb
index b58dfe9..371b83e 100644
--- a/users/app/controllers/v1/messages_controller.rb
+++ b/users/app/controllers/v1/messages_controller.rb
@@ -1,30 +1,27 @@
 module V1
   class MessagesController < ApplicationController
 
-    before_filter :authorize_admin # not sure this is best way
+    skip_before_filter :verify_authenticity_token
+    before_filter :authorize
+
     respond_to :json
 
-    # for now, will not pass unseen, so unseen will always be true
-    def user_messages(unseen = true)
-      user = User.find(params[:user_id])
-      render json: (user ? user.messages : [] )
+    def index
+      render json: (current_user ? current_user.messages : [] )
     end
 
-    # routes ensure this is only for PUT
-    def mark_read
-
-      # make sure user and message exist
-      if (user = User.find(params[:user_id])) && Message.find(params[:message_id])
-
-        user.message_ids_seen << params[:message_id] if !user.message_ids_seen.include?(params[:message_id]) #TODO: is it quicker to instead call uniq! after adding?
-        user.message_ids_to_see.delete(params[:message_id])
-        user.save
+    def update
+      message = Message.find(params[:id])
+      if (message and current_user)
+        message.user_ids_to_show.delete(current_user.id)
+        # is it necessary to keep track of what users have already seen it?:
+        message.user_ids_have_shown << current_user.id if !message.user_ids_have_shown.include?(current_user.id) #TODO: is it quicker to instead call uniq! after adding?
+        message.save
         render json: true
-        return
       else
         render json: false
       end
-
     end
+
   end
 end
diff --git a/users/app/designs/message/by_user_ids_to_show.js b/users/app/designs/message/by_user_ids_to_show.js
new file mode 100644
index 0000000..95ccd0d
--- /dev/null
+++ b/users/app/designs/message/by_user_ids_to_show.js
@@ -0,0 +1,7 @@
+function (doc) {
+  if (doc.type === 'Message' && doc.user_ids_to_show && Array.isArray(doc.user_ids_to_show)) {
+    doc.user_ids_to_show.forEach(function (userIdsToShow) {
+      emit(userIdsToShow, 1);
+    });
+  }
+}
\ No newline at end of file
diff --git a/users/app/designs/message/by_user_ids_to_show_and_created_at.js b/users/app/designs/message/by_user_ids_to_show_and_created_at.js
new file mode 100644
index 0000000..7bd7c2c
--- /dev/null
+++ b/users/app/designs/message/by_user_ids_to_show_and_created_at.js
@@ -0,0 +1,8 @@
+// not using at moment
+function (doc) {
+  if (doc.type === 'Message' && doc.user_ids_to_show && Array.isArray(doc.user_ids_to_show)) {
+    doc.user_ids_to_show.forEach(function (userIdsToShow) {
+	emit([userIdsToShow, doc.created_at], 1);
+    });
+  }
+}
\ No newline at end of file
diff --git a/users/app/models/message.rb b/users/app/models/message.rb
index 38fa71e..16a19f4 100644
--- a/users/app/models/message.rb
+++ b/users/app/models/message.rb
@@ -3,8 +3,12 @@ class Message < CouchRest::Model::Base
   use_database :messages
 
   property :text, String
+  property :user_ids_to_show, [String]
+  property :user_ids_have_shown, [String] # is this necessary to store?
 
   design do
+    own_path = Pathname.new(File.dirname(__FILE__))
+    load_views(own_path.join('..', 'designs', 'message'))
   end
 
 end
diff --git a/users/app/models/user.rb b/users/app/models/user.rb
index 6e445be..71e57f3 100644
--- a/users/app/models/user.rb
+++ b/users/app/models/user.rb
@@ -13,8 +13,6 @@ class User < CouchRest::Model::Base
   property :desired_service_level_code, Integer, :accessible => true
   property :effective_service_level_code, Integer, :accessible => true
 
-  property :message_ids_to_see, [String]
-  property :message_ids_seen, [String]
   property :one_month_warning_sent, TrueClass
 
   before_save :update_effective_service_level
@@ -77,12 +75,11 @@ class User < CouchRest::Model::Base
   end
 
   def messages(unseen = true)
-
-    message_ids = unseen ? self.message_ids_to_see : self.message_ids_to_see + self.message_ids_seen # TODO check unique?
-
+    #TODO for now this only shows unseen messages. Will we ever want seen ones? Is it necessary to store?
+    #Message.by_user_ids_to_show.key(self.id).all # we don't want to emit all the userids associated with a message, so looping through to only emit text and id.
     messages = []
-    message_ids.each do |message_id|
-      messages << Message.find(message_id)
+    Message.by_user_ids_to_show.key(self.id).each do |message|
+      messages << [message.id, message.text]
     end
     messages
 
@@ -140,7 +137,8 @@ class User < CouchRest::Model::Base
         @message.save
       end
 
-      user.message_ids_to_see << @message.id
+      @message.user_ids_to_show << user.id
+      @message.save
       user.one_month_warning_sent = true
       user.save
     end
diff --git a/users/config/routes.rb b/users/config/routes.rb
index 9a7c531..2819fa9 100644
--- a/users/config/routes.rb
+++ b/users/config/routes.rb
@@ -6,8 +6,7 @@ Rails.application.routes.draw do
     resources :sessions, :only => [:new, :create, :update]
     delete "logout" => "sessions#destroy", :as => "logout"
     resources :users, :only => [:create, :update, :destroy, :index]
-    get "user_messages/:user_id" => "messages#user_messages"
-    put "mark_read/:user_id/:message_id" => "messages#mark_read"
+    resources :messages, :only => [:index, :update]
   end
 
   scope "(:locale)", :locale => MATCH_LOCALE do
diff --git a/users/leap_web_users.gemspec b/users/leap_web_users.gemspec
index 06965a7..7d1f220 100644
--- a/users/leap_web_users.gemspec
+++ b/users/leap_web_users.gemspec
@@ -19,5 +19,4 @@ Gem::Specification.new do |s|
 
   s.add_dependency "ruby-srp", "~> 0.2.1"
   s.add_dependency "rails_warden"
-  s.add_dependency "whenever"
 end
diff --git a/users/lib/leap_web_users/engine.rb b/users/lib/leap_web_users/engine.rb
index 61131ef..f8ed71c 100644
--- a/users/lib/leap_web_users/engine.rb
+++ b/users/lib/leap_web_users/engine.rb
@@ -8,7 +8,6 @@ require "warden/session_serializer"
 require "warden/strategies/secure_remote_password"
 
 require "webfinger"
-require "whenever"
 
 module LeapWebUsers
   class Engine < ::Rails::Engine
diff --git a/users/lib/tasks/leap_web_users_tasks.rake b/users/lib/tasks/leap_web_users_tasks.rake
index 3d0649c..62bcbe9 100644
--- a/users/lib/tasks/leap_web_users_tasks.rake
+++ b/users/lib/tasks/leap_web_users_tasks.rake
@@ -2,3 +2,9 @@
 # task :leap_web_users do
 #   # Task goes here
 # end
+
+# recommended that for our setup, we should have this triggered from a cron job in puppet rather than using whenever gem
+desc "Send one month warning messages"
+task :leap_web_users do
+  User.send_one_month_warnings
+end
diff --git a/users/test/functional/v1/messages_controller_test.rb b/users/test/functional/v1/messages_controller_test.rb
index 0bc09be..24a5b1f 100644
--- a/users/test/functional/v1/messages_controller_test.rb
+++ b/users/test/functional/v1/messages_controller_test.rb
@@ -3,57 +3,54 @@ require 'test_helper'
 class V1::MessagesControllerTest < ActionController::TestCase
 
   setup do
-    @message = Message.new(:text => 'a test message')
-    @message.save
     @user = FactoryGirl.build(:user)
-    @user.message_ids_to_see << @message.id
     @user.save
-    login :is_admin? => true
+    @message = Message.new(:text => 'a test message')
+    @message.user_ids_to_show << @user.id
+    @message.save
   end
 
   teardown do
-    @user.destroy
     @message.destroy
+    @user.destroy
   end
 
   test "get messages for user" do
-    get :user_messages, :user_id => @user.id
+    login @user
+    get :index
     assert response.body.include? @message.text
     assert response.body.include? @message.id
   end
 
   test "mark message read for user" do
-    assert @user.message_ids_to_see.include?(@message.id)
-    assert !@user.message_ids_seen.include?(@message.id)
-
-    put :mark_read, :user_id => @user.id, :message_id => @message.id
-    @user.reload
-    assert !@user.message_ids_to_see.include?(@message.id)
-    assert @user.message_ids_seen.include?(@message.id)
+    login @user
+    assert @message.user_ids_to_show.include?(@user.id)
+    assert !@message.user_ids_have_shown.include?(@user.id)
+    put :update, :id => @message.id
+    @message.reload
+    assert !@message.user_ids_to_show.include?(@user.id)
+    assert @message.user_ids_have_shown.include?(@user.id)
     assert_json_response true
   end
 
   test "do not get seen messages" do
-    put :mark_read, :user_id => @user.id, :message_id => @message.id
-    @user.reload
-    get :user_messages, :user_id => @user.id
+    login @user
+    put :update, :id => @message.id
+    @message.reload
+    get :index
     assert !(response.body.include? @message.text)
     assert !(response.body.include? @message.id)
   end
 
-  test "empty messages for non-existing user" do
-    get :user_messages, :user_id => 'some random string'
-    assert_json_response []
-  end
 
   test "mark read responds even with bad inputs" do
-    put :mark_read, :user_id => 'nonsense', :message_id => 'more nonsense'
+    login @user
+    put :update, :id => 'more nonsense'
     assert_json_response false
  end
 
-  test "fails if not admin" do
-    login :is_admin? => false
-    get :user_messages, :user_id => @user.id
+  test "fails if not authenticated" do
+    get :index, :format => :json
     assert_access_denied
   end