#595 - Added watch list form to AD forms; rolled back changes from #5…

…93 and #594

README

@@ -11,7 +11,6 @@ RELEASE NOTES
 * Feature #582 - Enable security controls on the Arch Decisions features - Basic permissions are now required in order to perform actions other than view the entries
 * Feature #592 - Users should have view permissions in order to see ADs and related data - Permissions are now required to view AD data
 * Feature #581 - Factors should have a "scope" attribute - Factors can now be restricted to a single project, or single AD. Existing Factors will be updated according to current assignments.
-* Feature #594 - Users automatically added to watch list on creation, assignment or comments
 * Bug #557 - Fixed problem with unwanted link activation when dragging a Factor for reprioritization
 
 CURRENT FEATURES

app/controllers/arch_decision_discussions_controller.rb

@@ -23,13 +23,6 @@ def new
       @discussion.created_by = User.current
       if @discussion.save
         attach_files(@discussion, params[:attachments])
-        if automatically_add_watchers
-          wad = ad_for_watch_list @discussion
-          unless wad.nil?
-            wad.set_watcher(User.current, true)
-            wad.save
-          end
-        end
         Mailer.deliver_arch_decision_discussion_add(@discussion)
         flash[:notice] = l(:notice_successful_create)
       else
@@ -125,17 +118,4 @@ def get_id_from_params(type)
     params[type + "_id"] ? params[type + "_id"] : (params[:discussion] ? params[:discussion][type + "_id"] : nil)
   end
 
-  def ad_for_watch_list(discussion)
-    case discussion.parent_type
-      when "Arch Decision" then discussion.arch_decision
-      when "Strategy" then discussion.parent.arch_decision
-      when "Factor" then
-        if discussion.factor.scope == Factor::SCOPE_ARCH_DECISION
-          discussion.factor.arch_decisions[0]
-        else
-          nil
-        end
-      else nil
-    end
-  end
 end

app/controllers/arch_decisions_controller.rb

@@ -50,15 +50,10 @@ def new
     @arch_decision = ArchDecision.new(params[:arch_decision])
     @arch_decision.project = @project
     @arch_decision_statuses = ArchDecisionStatus.find(:all)
-    @arch_decision.watcher_user_ids = [User.current.id]
     if request.post?
       @arch_decision.created_by = User.current
       @arch_decision.updated_by = User.current
       update_watch_list
-#      @arch_decision.set_watcher @arch_decision.created_by, true #TODO: make this optional by putting it in the form
-#      if (@arch_decision.assigned_to)
-#        @arch_decision.set_watcher(@arch_decision.assigned_to, true)
-#      end
       if @arch_decision.save
         Mailer.deliver_arch_decision_add(@arch_decision)
         flash[:notice] = l(:notice_successful_create)
@@ -71,11 +66,6 @@ def edit
     if request.post?
       update_watch_list
       if @arch_decision.update_attributes(params[:arch_decision])
-#        if (@arch_decision.watchers.select{|w| w.user == @arch_decision.assigned_to}.empty?)
-#          #Add new assignee to watch list
-#          @arch_decision.set_watcher(@arch_decision.assigned_to, true)
-#          @arch_decision.save!
-#        end
         Mailer.deliver_arch_decision_edit(@arch_decision)
         flash[:notice] = l(:notice_successful_update)
         redirect_to :action => 'show', :project_id => @project, :id => @arch_decision
@@ -184,10 +174,6 @@ def refresh_factors_table
   def update_watch_list
     wids = params[:arch_decision]['watcher_user_ids']
     wids = wids.nil? ? [] : wids
-    if automatically_add_watchers
-      wids = wids << params[:arch_decision]['assigned_to_id'].to_i unless params[:arch_decision]['assigned_to_id'].to_i == 0
-      wids.uniq!
-    end
     @arch_decision.watcher_user_ids = wids
   end
 end

app/helpers/arch_decisions_helper.rb

@@ -28,8 +28,4 @@ def text_for_discussion_count(item)
     return "<span class='discussion-count'>(" + l(label, item.arch_decision_discussions.size.to_s) + ")</span>"
   end
 
-  def automatically_add_watchers
-    Setting["plugin_redmine_arch_decisions"]["automatically_add_watchers"]
-  end
-
 end

init.rb

@@ -32,17 +32,4 @@
   menu :project_menu, :arch_decisions, { :controller => 'arch_decisions', :action => 'index' }, :caption => :label_arch_decision_plural, :param => :project_id
   menu :project_menu, :factors, { :controller => 'factors', :action => 'index' }, :caption => :label_factor_plural, :param => :project_id
 
-  # This plugin contains settings
-  settings :default => {
-    'automatically_add_watchers' => true,
-    'notify_on_new_ad' => true,
-    'notify_on_not_started' => false,
-    'notify_on_under_discussion' => false,
-    'notify_on_decision_made' => true,
-    'notify_on_work_scheduled' => false,
-    'notify_on_implemented' => false,
-    'notify_on_canceled' => false,
-    'notify_on_deprecated' => false
-  }, :partial => 'settings/arch_decisions'
-
 end

test/functional/arch_decisions_controller_test.rb

@@ -260,6 +260,7 @@ def test_new
       assert_select 'select#arch_decision_status_id'
       assert_select 'select#arch_decision_assigned_to_id'
     end
+    # TODO: test watchers form
   end
 
   def test_new_post
@@ -271,7 +272,7 @@ def test_new_post
                   :resolution => "test_new_post resolution",
                   :status_id => arch_decision_statuses(:valid).id,
                   :assigned_to_id => users(:users_001).id,
-                  :watcher_user_ids => [User.current.id]
+                  :watcher_user_ids => [User.current.id, users(:users_001).id]
                 }
     assert_equal ad_count+1, ArchDecision.count(:all)
     ad = assigns(:arch_decision)
@@ -280,7 +281,7 @@ def test_new_post
     assert_equal "test_new_post resolution", ad.resolution
     assert_equal arch_decision_statuses(:valid), ad.status
     assert_equal users(:users_001), ad.assigned_to
-    # Make sure watchers are automatically assigned
+    # Make sure watchers are assigned
     assert_equal 2, ad.watchers.size
     assert ad.watchers.collect{ |w| w.user }.include?(ad.created_by)
     assert ad.watchers.collect{ |w| w.user }.include?(ad.assigned_to)
@@ -294,19 +295,20 @@ def test_new_post_one_watcher
                   :problem_description => "test_new_post problem_description",
                   :resolution => "test_new_post resolution",
                   :status_id => arch_decision_statuses(:valid).id,
-                  :assigned_to_id => User.current.id
+                  :assigned_to_id => User.current.id,
+                  :watcher_user_ids => [users(:users_001).id]
                 }
     assert_equal ad_count+1, ArchDecision.count(:all)
     ad = assigns(:arch_decision)
     assert_equal ad.created_by, ad.assigned_to
     # There should only be one watcher
     assert_equal 1, ad.watchers.size
-    assert ad.watchers.collect{ |w| w.user }.include?(ad.created_by)
-    assert ad.watchers.collect{ |w| w.user }.include?(ad.assigned_to)
+    assert !ad.watchers.collect{ |w| w.user }.include?(ad.created_by)
+    assert !ad.watchers.collect{ |w| w.user }.include?(ad.assigned_to)
+    assert ad.watchers.collect{ |w| w.user }.include?(users(:users_001))
   end
 
   def test_new_post_dont_add_watchers
-    Setting["plugin_redmine_arch_decisions"]["automatically_add_watchers"] = false
     ad_count = ArchDecision.count(:all)
     post :new, :project_id => @ad.project.id, 
                 :arch_decision => {
@@ -321,7 +323,6 @@ def test_new_post_dont_add_watchers
     assert_equal ad.created_by, ad.assigned_to
     # There should be no watchers despite assigned to value
     assert_equal 0, ad.watchers.size
-    Setting["plugin_redmine_arch_decisions"]["automatically_add_watchers"] = true
   end
 
   def test_new_no_perms
@@ -341,6 +342,7 @@ def test_edit
       assert_select 'select#arch_decision_status_id'
       assert_select 'select#arch_decision_assigned_to_id'
     end
+    # TODO: assert watchers form
   end
 
   def test_edit_post
@@ -363,11 +365,10 @@ def test_edit_post
     assert_equal "test_edit_post resolution", ad.resolution
     assert_equal arch_decision_statuses(:valid_name_max_length), ad.status
     assert_equal assignee, ad.assigned_to
-    # Make sure new assignee is added to watch list 
-    assert_equal 3, ad.watchers.size
+    assert_equal 2, ad.watchers.size
     watching_users = ad.watchers.collect{ |w| w.user }
     assert watching_users.include?(ad.created_by)
-    assert watching_users.include?(ad.assigned_to)
+    assert !watching_users.include?(ad.assigned_to)
     assert watching_users.include?(old_assignee)
   end
 
@@ -393,7 +394,6 @@ def test_edit_post_assigned_to_unchanged
   end
 
   def test_edit_post_dont_add_watchers
-    Setting["plugin_redmine_arch_decisions"]["automatically_add_watchers"] = false
     assignee = users(:users_002)
     post :edit, :project_id => @ad.project.id,
                 :id => @ad.id,
@@ -412,7 +412,6 @@ def test_edit_post_dont_add_watchers
     watching_users = ad.watchers.collect{ |w| w.user }
     assert watching_users.include?(ad.created_by)
     assert !watching_users.include?(ad.assigned_to)
-    Setting["plugin_redmine_arch_decisions"]["automatically_add_watchers"] = true
   end
 
 

test/functional/arch_decisions_discussions_controller_test.rb

@@ -27,7 +27,6 @@ def setup
 
   def test_new_ad_discussion
     expected_counts = get_counts
-    assert !@ad.watchers.collect{|w| w.user}.include?(User.current), "User shouldn't be on AD watch list yet"
     post :new, 
           :project_id => 1, 
           :arch_decision_id => @ad.id, 
@@ -37,32 +36,11 @@ def test_new_ad_discussion
     assert_not_nil assigns(:arch_decision)
     add_counts expected_counts, :arch_decisions, 1
     assert_counts expected_counts
-    @ad.reload
-    assert @ad.watchers.collect{|w| w.user}.include?(User.current), "After comment, user should be on watch list"
-  end
-
-  def test_new_ad_discussion_dont_add_watchers
-    Setting["plugin_redmine_arch_decisions"]["automatically_add_watchers"] = false
-    expected_counts = get_counts
-    assert !@ad.watchers.collect{|w| w.user}.include?(User.current), "User shouldn't be on AD watch list yet"
-    post :new, 
-          :project_id => 1, 
-          :arch_decision_id => @ad.id, 
-          :discussion => {:subject => "New discussion", :content => "New content"}
-    assert_redirected_to :controller => "arch_decisions", :action => "show"
-    #assert_equal 'Post was successfully created.', flash[:notice]    assert_not_nil assigns(:project)
-    assert_not_nil assigns(:arch_decision)
-    add_counts expected_counts, :arch_decisions, 1
-    assert_counts expected_counts
-    @ad.reload
-    assert !@ad.watchers.collect{|w| w.user}.include?(User.current), "After comment, user still shouldn't be on watch list"
-    Setting["plugin_redmine_arch_decisions"]["automatically_add_watchers"] = true
   end
 
   def test_new_factor_discussion
     expected_counts = get_counts
     factor = factors(:valid)
-    assert !@ad.watchers.collect{|w| w.user}.include?(User.current), "User shouldn't be on AD watch list yet"
     post :new, 
           :project_id => 1, 
           :factor_id => factor.id, 
@@ -72,14 +50,11 @@ def test_new_factor_discussion
     assert_not_nil assigns(:factor)
     add_counts expected_counts, :factors, 1
     assert_counts expected_counts
-    @ad.reload
-    assert !@ad.watchers.collect{|w| w.user}.include?(User.current), "After comment, user still shouldn't be on watch list"
   end
 
   def test_new_ad_factor_discussion
     expected_counts = get_counts
     factor = factors(:valid_2)
-    assert !@ad.watchers.collect{|w| w.user}.include?(User.current), "User shouldn't be on AD watch list yet"
     post :new, 
           :project_id => 1, 
           :factor_id => factor.id, 
@@ -89,14 +64,11 @@ def test_new_ad_factor_discussion
     assert_not_nil assigns(:factor)
     add_counts expected_counts, :factors, 1
     assert_counts expected_counts
-    @ad.reload
-    assert @ad.watchers.collect{|w| w.user}.include?(User.current), "After comment, user should be on watch list"
   end
 
   def test_new_ad_factor_discussion_no_ad
     expected_counts = get_counts
     factor = factors(:valid_ad_no_ad)
-    assert !@ad.watchers.collect{|w| w.user}.include?(User.current), "User shouldn't be on AD watch list yet"
     post :new, 
           :project_id => 1, 
           :factor_id => factor.id, 
@@ -106,14 +78,11 @@ def test_new_ad_factor_discussion_no_ad
     assert_not_nil assigns(:factor)
     add_counts expected_counts, :factors, 1
     assert_counts expected_counts
-    @ad.reload
-    assert !@ad.watchers.collect{|w| w.user}.include?(User.current), "After comment, user still shouldn't be on watch list"
   end
 
   def test_new_strategy_discussion
     expected_counts = get_counts
     strategy = strategies(:valid)
-    assert !@ad.watchers.collect{|w| w.user}.include?(User.current), "User shouldn't be on AD watch list yet"
     post :new, 
           :project_id => 1, 
           :arch_decision_id => @ad.id, 
@@ -124,8 +93,6 @@ def test_new_strategy_discussion
     assert_not_nil assigns(:strategy)
     add_counts expected_counts, :strategies, 1
     assert_counts expected_counts
-    @ad.reload
-    assert @ad.watchers.collect{|w| w.user}.include?(User.current), "After comment, user should be on watch list"
   end
 
   def test_new_ad_discussion_no_perms