Merge pull request #92 from warpc/issues

[refs #54, #94] Fix comments and issues bugs. Add new notification settings.

app/controllers/comments_controller.rb

@@ -16,7 +16,7 @@ class CommentsController < ApplicationController
     @comment.user = current_user
     if @comment.save
       flash[:notice] = I18n.t("flash.comment.saved")
-      redirect_to :back
+      redirect_to [@commentable.project, @commentable]
     else
       flash[:error] = I18n.t("flash.comment.save_error")
       render :action => 'new'
@@ -55,7 +55,7 @@ class CommentsController < ApplicationController
     #  end
     #end
     #nil
-    return Issue.find(params[:issue_id])
+    return Issue.find_by_serial_id_and_project_id(params[:issue_id], params[:project_id])
   end
 
   def set_commentable

app/controllers/issues_controller.rb

@@ -30,7 +30,10 @@ class IssuesController < ApplicationController
     @issue = Issue.new(params[:issue])
     @issue.user_id = @user_id
     @issue.project_id = @project.id
+
     if @issue.save
+      @issue.subscribe_creator(current_user.id)
+
       flash[:notice] = I18n.t("flash.issue.saved")
       redirect_to project_issues_path(@project)
     else
@@ -41,12 +44,12 @@ class IssuesController < ApplicationController
 
   def edit
     @user_id = @issue.user_id
-    @user_uname = @issue.user.uname
+    @user_uname = @issue.assign_uname
   end
 
   def update
     @user_id = params[:user_id].blank? ? @issue.user_id : params[:user_id]
-    @user_uname = params[:user_uname].blank? ? @issue.user.uname : params[:user_uname]
+    @user_uname = params[:user_uname].blank? ? @issue.assign_uname : params[:user_uname]
 
     if @issue.update_attributes( params[:issue].merge({:user_id => @user_id}) )
       flash[:notice] = I18n.t("flash.issue.saved")

app/controllers/settings/notifiers_controller.rb

@@ -0,0 +1,22 @@
+class Settings::NotifiersController < ApplicationController
+  layout "sessions"
+
+  before_filter :authenticate_user!
+
+  load_and_authorize_resource :user
+  load_and_authorize_resource :class => Settings::Notifier, :through => :user, :singleton => true, :shallow => true
+
+  def show
+  end
+
+  def update
+    if @notifier.update_attributes(params[:settings_notifier])
+      flash[:notice] = I18n.t("flash.settings.saved")
+      redirect_to [@user, @notifier]
+    else
+      flash[:notice] = I18n.t("flash.settings.save_error")
+      redirect_to [@user, @notifier]
+    end
+  end
+
+end

app/helpers/settings/notifiers_helper.rb

@@ -0,0 +1,2 @@
+module Settings::NotifiersHelper
+end

app/mailers/user_mailer.rb

@@ -1,7 +1,7 @@
 # coding: UTF-8
 
 class UserMailer < ActionMailer::Base
-  default :from => APP_CONFIG['no-reply-email']
+  default :from => APP_CONFIG['do-not-reply-email']
 
   def new_user_notification(user)
     @user = user
@@ -18,6 +18,14 @@ class UserMailer < ActionMailer::Base
     end
   end
 
+  def new_comment_reply_notification(comment, user)
+    @user = user
+    @comment = comment
+    mail(:to => user.email, :subject => I18n.t("notifications.subjects.new_comment_reply_notification")) do |format|
+      format.html
+    end
+  end
+
   def new_issue_notification(issue, user)
     @user = user
     @issue = issue

app/models/ability.rb

@@ -28,6 +28,8 @@ class Ability
       else # Registered user rights
         can [:show, :autocomplete_user_uname], User
 
+        can [:show, :update], Settings::Notifier, :user_id => user.id
+
         can [:read, :create], Group
         can [:update, :manage_members], Group do |group|
           group.objects.exists?(:object_type => 'User', :object_id => user.id, :role => 'admin') # or group.owner_id = user.id

app/models/comment.rb

@@ -4,17 +4,25 @@ class Comment < ActiveRecord::Base
 
   validates :body, :user_id, :commentable_id, :commentable_type, :presence => true
 
+  after_create :subscribe_on_reply
   after_create :deliver_new_comment_notification
 
   protected
 
   def deliver_new_comment_notification
-    recipients = self.commentable.project.relations.by_role('admin').where(:object_type => 'User').map { |rel| rel.read_attribute(:object_id) }
-    recipients = recipients | [self.commentable.user_id]
-    recipients = recipients | [self.commentable.project.owner_id] if self.commentable.project.owner_type == 'User'
-    recipients.each do |recipient_id|
-      recipient = User.find(recipient_id)
-      UserMailer.delay.new_comment_notification(self, recipient)
+    subscribes = self.commentable.subscribes
+    subscribes.each do |subscribe|
+      if self.user_id != subscribe.user_id && User.find(subscribe.user).notifier.new_comment_reply && User.find(subscribe.user).notifier.can_notify
+        if self.commentable.comments.exists?(:user_id => subscribe.user.id)
+          UserMailer.delay.new_comment_reply_notification(self, subscribe.user)
+        else
+          UserMailer.delay.new_comment_notification(self, subscribe.user)
+        end
+      end
     end
   end
+
+  def subscribe_on_reply
+    self.commentable.subscribes.create(:user_id => self.user_id) if !self.commentable.subscribes.exists?(:user_id => self.user_id)
+  end
 end

app/models/issue.rb

@@ -16,6 +16,7 @@ class Issue < ActiveRecord::Base
   after_create :deliver_new_issue_notification
   after_create :deliver_issue_assign_notification
   after_update :deliver_issue_assign_notification
+  after_update :subscribe_issue_assigned_user
 
   def assign_uname
     user.uname if user
@@ -25,6 +26,12 @@ class Issue < ActiveRecord::Base
     serial_id.to_s
   end
 
+  def subscribe_creator(creator_id)
+    if !self.subscribes.exists?(:user_id => creator_id)
+      self.subscribes.create(:user_id => creator_id)
+    end
+  end
+
   protected
 
   def set_serial_id
@@ -36,12 +43,12 @@ class Issue < ActiveRecord::Base
     recipients = collect_recipient_ids
     recipients.each do |recipient_id|
       recipient = User.find(recipient_id)
-      UserMailer.delay.new_issue_notification(self, recipient)#.deliver
+      UserMailer.delay.new_issue_notification(self, recipient) if User.find(recipient).notifier.can_notify && User.find(recipient).notifier.new_issue
     end
   end
 
   def deliver_issue_assign_notification
-    UserMailer.delay.issue_assign_notification(self, self.user) if self.user_id_was != self.user_id
+    UserMailer.delay.issue_assign_notification(self, self.user) if self.user_id_was != self.user_id && self.user.notifier.issue_assign && self.user.notifier.can_notify
   end
 
   def subscribe_users
@@ -56,7 +63,21 @@ class Issue < ActiveRecord::Base
     recipients = self.project.relations.by_role('admin').where(:object_type => 'User').map { |rel| rel.read_attribute(:object_id) }
     recipients = recipients | [self.user_id] if self.user_id
     recipients = recipients | [self.project.owner_id] if self.project.owner_type == 'User'
+
+    # filter by notification settings
+    recipients = recipients.select do |recipient|
+      User.find(recipient).notifier.new_issue && User.find(recipient).notifier.can_notify
+    end
+
     recipients
   end
 
+  def subscribe_issue_assigned_user
+    if self.user_id_was != self.user_id
+      self.subscribes.where(:user_id => self.user_id_was).first.destroy unless self.user_id_was.blank?
+      if self.user.notifier.issue_assign && !self.subscribes.exists?(:user_id => self.user_id)
+        self.subscribes.create(:user_id => self.user_id)
+      end
+    end
+  end
 end

app/models/settings.rb

@@ -0,0 +1,5 @@
+module Settings
+  def self.table_name_prefix
+    'settings_'
+  end
+end

app/models/settings/notifier.rb

@@ -0,0 +1,5 @@
+class Settings::Notifier < ActiveRecord::Base
+  belongs_to :user
+
+  validates :user_id, :presence => true
+end

app/models/user.rb

@@ -4,6 +4,8 @@ class User < ActiveRecord::Base
   devise :database_authenticatable, :registerable, :omniauthable, # :token_authenticatable, :encryptable, :timeoutable
          :recoverable, :rememberable, :validatable #, :trackable, :confirmable, :lockable
 
+  has_one :notifier, :class_name => 'Settings::Notifier' #:notifier
+
   has_many :authentications, :dependent => :destroy
   has_many :build_lists, :dependent => :destroy
 
@@ -31,6 +33,8 @@ class User < ActiveRecord::Base
   attr_readonly :uname
   attr_accessor :login
 
+  after_create :create_settings_notifier
+
   def admin?
     role == 'admin'
   end
@@ -75,5 +79,11 @@ class User < ActiveRecord::Base
     clean_up_passwords
     result
   end
+  
+  private
+
+  def create_settings_notifier
+    self.create_notifier
+  end
 
 end

app/views/devise/registrations/edit.html.haml

@@ -68,3 +68,5 @@
         %span.text_button_padding
         = link_to t('layout.back'), :back, :class => "text_button_padding link_button"
 
+      .group.navform.wat-cf
+        = link_to t('layout.settings.notifier'), user_settings_notifier_path(current_user)#, :class => "text_button_padding link_button"

app/views/issues/index.html.haml

@@ -19,4 +19,4 @@
       = render :partial => 'issues/list'
       .actions-bar.wat-cf
         .actions
-          = will_paginate @issues, :param_name => :issue_page
+          = will_paginate @issues#, :param_name => :issue_page

app/views/issues/show.html.haml

@@ -36,7 +36,7 @@
       = t("layout.issues.comments_header")
     .inner
       %ul.list
-        - @issue.comments.each do |comment|
+        - @issue.comments.order(:created_at).each do |comment|
           %li
             .left
               = link_to comment.user.uname, user_path(comment.user.uname)
@@ -44,8 +44,8 @@
               = comment.body
               %br
               %br
-              = link_to t("layout.edit"), edit_project_issue_comment_path(@project, @issue.id, comment) if can? :update, comment
-              = link_to image_tag("web-app-theme/icons/cross.png", :alt => t("layout.delete")) + " " + t("layout.delete"), project_issue_comment_path(@project, @issue.id, comment), :method => "delete", :class => "button", :confirm => t("layout.comments.confirm_delete") if can? :delete, comment
+              = link_to t("layout.edit"), edit_project_issue_comment_path(@project, @issue, comment) if can? :update, comment
+              = link_to image_tag("web-app-theme/icons/cross.png", :alt => t("layout.delete")) + " " + t("layout.delete"), project_issue_comment_path(@project, @issue, comment), :method => "delete", :class => "button", :confirm => t("layout.comments.confirm_delete") if can? :delete, comment
 
 .block
   .content

app/views/settings/notifiers/_form.html.haml

@@ -0,0 +1,29 @@
+.group
+  = f.label :can_notify, t('activerecord.attributes.settings.notifier.can_notify'), :class => :label
+  = f.check_box :can_notify#, :class => 'text_field'
+
+.group
+  = f.label :new_comment, t('activerecord.attributes.settings.notifier.new_comment'), :class => :label
+  = f.check_box :new_comment, :class => 'notify_cbx'
+
+.group
+  = f.label :new_comment_reply, t('activerecord.attributes.settings.notifier.new_comment_reply'), :class => :label
+  = f.check_box :new_comment_reply, :class => 'notify_cbx'
+
+.group
+  = f.label :new_issue, t('activerecord.attributes.settings.notifier.new_issue'), :class => :label
+  = f.check_box :new_issue, :class => 'notify_cbx'
+
+.group
+  = f.label :issue_assign, t('activerecord.attributes.settings.notifier.issue_assign'), :class => :label
+  = f.check_box :issue_assign, :class => 'notify_cbx'
+  
+.group.navform.wat-cf
+  %button.button{:type => "submit"}
+    = image_tag("web-app-theme/icons/tick.png", :alt => t("layout.save"))
+    = t("layout.save")
+  %span.text_button_padding= t("layout.or")
+  = link_to t("layout.cancel"), user_settings_notifier_path(@user), :class => "text_button_padding link_button"
+
+:javascript
+  disableNotifierCbx($('#settings_notifier_can_notify'));

app/views/settings/notifiers/show.html.haml

@@ -0,0 +1,5 @@
+#block-signup.block
+  %h2= title t("layout.settings.notifiers.edit_header")
+  .content
+    = form_for @notifier, :url => user_settings_notifier_path(@user), :html => { :class => :form } do |f|
+      = render :partial => "form", :locals => {:f => f}

app/views/user_mailer/new_comment_notification.haml

@@ -3,5 +3,7 @@
 
 %p К задаче #{ link_to @comment.commentable.title, [@comment.commentable.project, @comment.commentable] } был добавлен новый комментарий.
 
+%p "#{ @comment.body }"
+
 
 %p== Команда поддержки «ROSA Build System»

app/views/user_mailer/new_comment_reply_notification.haml

@@ -0,0 +1,9 @@
+%p== Здравствуйте, #{@user.name}.
+
+
+%p На Ваш комментарий в задаче #{ link_to @comment.commentable.title, [@comment.commentable.project, @comment.commentable] } был дан ответ.
+
+%p "#{ @comment.body }"
+
+
+%p== Команда поддержки «ROSA Build System»

config/locales/ru.yml

@@ -40,6 +40,10 @@ ru:
     not_access: Нет доступа!
     owner: Владелец
     confirm: Уверенны?
+    settings:
+      notifier: Настройки оповещений
+      notifiers:
+        edit_header: Настройки оповещений
     processing: Обрабатывается...
 
     downloads:
@@ -364,6 +368,11 @@ ru:
         project_version_not_found: версия не найден
 
   flash:
+    settings:
+      saved: Настройки успешно сохранены
+      save_error: При обновлении настроек произошла ошибка
+      
+
     subscribe:
       saved: Вы подписаны на оповещения для этой задачи
       destroyed: Подписка на оповещения для этой задачи убрана
@@ -489,8 +498,18 @@ ru:
       build_list_item: Элемент сборочного листа
       download: Статистика
       auto_build_list: Автоматическая пересборка пакетов
+      settings:
+        notifier: Настройки оповещений
 
     attributes:
+      settings:
+        notifier:
+          can_notify: Включить оповещения по электронной почте
+          new_comment: Оповещать о новом комментарии в задаче
+          new_comment_reply: Оповещать о новом ответе на мой комментарий
+          new_issue: Оповещать о новых задачах в моих проектах
+          issue_assign: Оповещать, когда на меня выставляют задачу
+
       auto_build_list:
         project_id: Проект
         project: Проект

config/routes.rb

@@ -9,6 +9,9 @@ Rosa::Application.routes.draw do
   resources :users do
     resources :groups, :only => [:new, :create, :index]
     get :autocomplete_user_uname, :on => :collection
+    namespace :settings do
+      resource :notifier, :only => [:show, :update]
+    end
   end
 
   resources :event_logs, :only => :index

db/migrate/20120111072106_create_settings_notifiers.rb

@@ -0,0 +1,19 @@
+class CreateSettingsNotifiers < ActiveRecord::Migration
+  def self.up
+    create_table :settings_notifiers do |t|
+      t.integer :user_id, :null => false
+
+      t.boolean :can_notify, :default => true
+      t.boolean :new_comment, :default => true
+      t.boolean :new_comment_reply, :default => true
+      t.boolean :new_issue, :default => true
+      t.boolean :issue_assign, :default => true
+
+      t.timestamps
+    end
+  end
+
+  def self.down
+    drop_table :settings_notifiers
+  end
+end

db/migrate/20120111135443_add_settings_notifier_to_all_users.rb

@@ -0,0 +1,13 @@
+class AddSettingsNotifierToAllUsers < ActiveRecord::Migration
+  def self.up
+    User.all.each do |user|
+      user.create_notifier
+    end
+  end
+
+  def self.down
+    User.all.each do |user|
+      user.notifier.destroy
+    end
+  end
+end

db/migrate/20120113121748_add_issue_status_default_value.rb

@@ -0,0 +1,9 @@
+class AddIssueStatusDefaultValue < ActiveRecord::Migration
+  def self.up
+    change_column :issues, :status, :string, :default => 'open'
+  end
+
+  def self.down
+    change_column :issues, :status, :string, :null => true
+  end
+end

db/schema.rb

@@ -159,7 +159,7 @@ ActiveRecord::Schema.define(:version => 20120113151305) do
     t.integer  "user_id"
     t.string   "title"
     t.text     "body"
-    t.string   "status"
+    t.string   "status",     :default => "open"
     t.datetime "created_at"
     t.datetime "updated_at"
   end
@@ -273,6 +273,17 @@ ActiveRecord::Schema.define(:version => 20120113151305) do
   add_index "rpms", ["project_id", "arch_id"], :name => "index_rpms_on_project_id_and_arch_id"
   add_index "rpms", ["project_id"], :name => "index_rpms_on_project_id"
 
+  create_table "settings_notifiers", :force => true do |t|
+    t.integer  "user_id",                             :null => false
+    t.boolean  "can_notify",        :default => true
+    t.boolean  "new_comment",       :default => true
+    t.boolean  "new_comment_reply", :default => true
+    t.boolean  "new_issue",         :default => true
+    t.boolean  "issue_assign",      :default => true
+    t.datetime "created_at"
+    t.datetime "updated_at"
+  end
+
   create_table "subscribes", :force => true do |t|
     t.integer  "subscribeable_id"
     t.string   "subscribeable_type"

public/javascripts/application.js

@@ -1,3 +1,13 @@
+function disableNotifierCbx(global_cbx) {
+  if ($(global_cbx).attr('checked')) {
+    $('.notify_cbx').removeAttr('disabled');
+    $('.notify_cbx').each(function(i,el) { $(el).prev().removeAttr('disabled'); })
+  } else {
+    $('.notify_cbx').attr('disabled', 'disabled');
+    $('.notify_cbx').each(function(i,el) { $(el).prev().attr('disabled', 'disabled'); })
+  }
+}
+
 $(document).ready(function() {
 	$('select#build_list_pl_id').change(function() {
 	  var platform_id = $(this).val();
@@ -29,4 +39,8 @@ $(document).ready(function() {
 			}
 		});
 	});
+
+  $('#settings_notifier_can_notify').click(function() {
+    disableNotifierCbx($(this));
+  });
 });

spec/controllers/settings/notifiers_controller_spec.rb

@@ -0,0 +1,5 @@
+require 'spec_helper'
+
+describe Settings::NotifiersController do
+
+end

spec/factories/settings_notifiers.rb

@@ -0,0 +1,6 @@
+# Read about factories at http://github.com/thoughtbot/factory_girl
+
+FactoryGirl.define do
+  factory :notifier do
+    end
+end

spec/helpers/settings/notifiers_helper_spec.rb

@@ -0,0 +1,15 @@
+require 'spec_helper'
+
+# Specs in this file have access to a helper object that includes
+# the Settings::NotifiersHelper. For example:
+#
+# describe Settings::NotifiersHelper do
+#   describe "string concat" do
+#     it "concats two strings with spaces" do
+#       helper.concat_strings("this","that").should == "this that"
+#     end
+#   end
+# end
+describe Settings::NotifiersHelper do
+  pending "add some examples to (or delete) #{__FILE__}"
+end

spec/mailers/user_mailer_spec.rb

@@ -2,4 +2,118 @@ require "spec_helper"
 
 describe UserMailer do
   pending "add some examples to (or delete) #{__FILE__}"
+
+  context 'On Issue create' do
+    before(:each) do
+      stub_rsync_methods
+
+      @project = Factory(:project)
+      @issue_user = Factory(:user)
+
+      any_instance_of(Project, :versions => ['v1.0', 'v2.0'])
+
+      @issue = Factory(:issue, :project_id => @project.id, :user_id => @issue_user.id)
+      @email = UserMailer.new_issue_notification(@issue, @issue_user).deliver
+    end
+
+    it 'should have correct subject' do
+      @email.subject.should == I18n.t("notifications.subjects.new_issue_notification")
+    end
+
+    it 'should render receiver email' do
+      @email.to.should == [@issue_user.email]
+    end
+
+    it 'should render the sender email' do
+      @email.from.should == [APP_CONFIG['do-not-reply-email']]
+    end
+
+    it 'should assign user name' do
+      @email.body.encoded.should match(@issue_user.name)
+    end
+
+    it 'should assign issue project name' do
+      @email.body.encoded.should match(@issue.project.name)
+    end
+
+    it 'should assign issue title' do
+      @email.body.encoded.should match(@issue.title)
+    end
+  end
+
+  context 'On Issue assign' do
+    before(:each) do
+      stub_rsync_methods
+
+      @project = Factory(:project)
+      @issue_user = Factory(:user)
+      @user = Factory(:user)
+
+      any_instance_of(Project, :versions => ['v1.0', 'v2.0'])
+
+      @issue = Factory(:issue, :project_id => @project.id, :user_id => @issue_user.id)
+      @email = UserMailer.issue_assign_notification(@issue, @user).deliver
+    end
+
+    it 'should have correct subject' do
+      @email.subject.should == I18n.t("notifications.subjects.issue_assign_notification")
+    end
+
+    it 'should render receiver email' do
+      @email.to.should == [@user.email]
+    end
+
+    it 'should render the sender email' do
+      @email.from.should == [APP_CONFIG['do-not-reply-email']]
+    end
+
+    it 'should assign user name' do
+      @email.body.encoded.should match(@user.name)
+    end
+
+    it 'should assign issue title' do
+      @email.body.encoded.should match(@issue.title)
+    end
+  end
+
+
+  context 'On Comment create' do
+    before(:each) do
+      stub_rsync_methods
+
+      @project = Factory(:project)
+      @issue_user = Factory(:user)
+      @user = Factory(:user)
+
+      any_instance_of(Project, :versions => ['v1.0', 'v2.0'])
+
+      @issue = Factory(:issue, :project_id => @project.id, :user_id => @issue_user.id)
+      @comment = Factory(:comment, :commentable => @issue, :user_id => @user.id)
+      @email = UserMailer.new_comment_notification(@comment, @issue_user).deliver
+    end
+
+    it 'should have correct subject' do
+      @email.subject.should == I18n.t("notifications.subjects.new_comment_notification")
+    end
+
+    it 'should render receiver email' do
+      @email.to.should == [@issue_user.email]
+    end
+
+    it 'should render the sender email' do
+      @email.from.should == [APP_CONFIG['do-not-reply-email']]
+    end
+
+    it 'should assign user name' do
+      @email.body.encoded.should match(@issue_user.name)
+    end
+
+    it 'should assign comment body' do
+      @email.body.encoded.should match(@comment.body)
+    end
+
+    it 'should assign issue title' do
+      @email.body.encoded.should match(@issue.title)
+    end
+  end
 end

spec/models/comment_spec.rb

@@ -1,7 +1,7 @@
 require 'spec_helper'
 require "cancan/matchers"
 
-def set_testable_data
+def set_comments_data
   @ability = Ability.new(@user)
 
   @project = Factory(:project)
@@ -19,13 +19,17 @@ describe Comment do
       @user = Factory(:admin)
       @stranger = Factory(:user)
 
-      set_testable_data
+      set_comments_data
     end
 
     it 'should create comment' do
       @ability.should be_able_to(:create, Comment.new(:commentable => @issue, :user => @user))
     end
 
+    pending "sends an e-mail" do
+      ActionMailer::Base.deliveries.last.to.include?(@stranger.email).should == true
+    end
+
     it 'should update comment' do
       @ability.should be_able_to(:update, @comment)
     end
@@ -35,7 +39,7 @@ describe Comment do
     end
 
     it 'should destroy own comment' do
-        @ability.should be_able_to(:destroy, @comment)
+      @ability.should be_able_to(:destroy, @comment)
     end
 
     it 'should destroy stranger comment' do
@@ -48,7 +52,7 @@ describe Comment do
       @user = Factory(:user)
       @stranger = Factory(:user)
 
-      set_testable_data
+      set_comments_data
 
       @project.relations.create!(:object_type => 'User', :object_id => @user.id, :role => 'admin')
     end
@@ -75,7 +79,7 @@ describe Comment do
       @user = Factory(:user)
       @stranger = Factory(:user)
 
-      set_testable_data
+      set_comments_data
 
       @project.update_attribute(:owner, @user)
       @project.relations.create!(:object_type => 'User', :object_id => @user.id, :role => 'admin')
@@ -103,7 +107,7 @@ describe Comment do
       @user = Factory(:user)
       @stranger = Factory(:user)
 
-      set_testable_data
+      set_comments_data
     end
 
     it 'should create comment' do

spec/models/settings/notifier_spec.rb

@@ -0,0 +1,5 @@
+require 'spec_helper'
+
+describe Settings::Notifier do
+  pending "add some examples to (or delete) #{__FILE__}"
+end