From c7e66852324714a166dd35dc3d5873a0053dcb9b Mon Sep 17 00:00:00 2001 From: jessib Date: Tue, 7 Jan 2014 12:57:01 -0800 Subject: 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. --- users/app/controllers/v1/messages_controller.rb | 29 +++++++------- users/app/designs/message/by_user_ids_to_show.js | 7 ++++ .../message/by_user_ids_to_show_and_created_at.js | 8 ++++ users/app/models/message.rb | 4 ++ users/app/models/user.rb | 14 +++---- users/config/routes.rb | 3 +- users/leap_web_users.gemspec | 1 - users/lib/leap_web_users/engine.rb | 1 - users/lib/tasks/leap_web_users_tasks.rake | 6 +++ .../test/functional/v1/messages_controller_test.rb | 45 ++++++++++------------ 10 files changed, 66 insertions(+), 52 deletions(-) create mode 100644 users/app/designs/message/by_user_ids_to_show.js create mode 100644 users/app/designs/message/by_user_ids_to_show_and_created_at.js (limited to 'users') 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 -- cgit v1.2.3