From 2bc580500df7e021b3094acc18065ec90cf2f78f Mon Sep 17 00:00:00 2001
From: Vokhmin Alexey V <avokhmin@gmail.com>
Date: Wed, 3 Apr 2013 01:23:00 +0400
Subject: [PATCH 1/6] #51: Change rights for operations with tags and assignee

---
 app/controllers/projects/issues_controller.rb |  8 ++++++++
 app/views/projects/issues/_form.html.haml     | 19 ++++++++++---------
 .../projects/issues/_manage_sidebar.html.haml |  6 +++---
 .../projects/issues/_user_container.html.haml |  2 +-
 .../projects/pull_requests/new.html.haml      | 10 +++++-----
 5 files changed, 27 insertions(+), 18 deletions(-)

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index b4b03d4fd..52c951d01 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -41,6 +41,10 @@ class Projects::IssuesController < Projects::BaseController
     @assignee_uname = params[:assignee_uname]
     @issue.user_id = current_user.id
 
+    unless can?(:write, @project)
+      @issue.assignee_id = nil
+      @issue.labelings = []
+    end
     if @issue.save
       @issue.subscribe_creator(current_user.id)
       flash[:notice] = I18n.t("flash.issue.saved")
@@ -56,6 +60,10 @@ class Projects::IssuesController < Projects::BaseController
   end
 
   def update
+    unless can?(:write, @project)
+      [:labelings, :labelings_attributes, :assignee_id].each{ |k| params[:issue].delete k }
+      params.delete :update_labels
+    end
     @issue.labelings.destroy_all if params[:update_labels]
     if params[:issue] && status = params[:issue][:status]
       @issue.set_close(current_user) if status == 'closed'
diff --git a/app/views/projects/issues/_form.html.haml b/app/views/projects/issues/_form.html.haml
index a0948aebb..d960bafa9 100644
--- a/app/views/projects/issues/_form.html.haml
+++ b/app/views/projects/issues/_form.html.haml
@@ -1,13 +1,14 @@
 =render 'title_body', :f => f, :id => 'new'
-.leftlist= t('activerecord.attributes.issue.assignee') + ':'
-#assigned-container.rightlist
-  =render 'user_container', :user => @issue.assignee
-.both
-.leftlist= t('layout.issues.labels') + ':'
-.rightlist
-  %span#flag-span.small-text= t('layout.issues.choose_labels_on_left')
-  #issue_labels
-.both
+- if can?(:write, @project)
+  .leftlist= t('activerecord.attributes.issue.assignee') + ':'
+  #assigned-container.rightlist
+    =render 'user_container', :user => @issue.assignee
+  .both
+  .leftlist= t('layout.issues.labels') + ':'
+  .rightlist
+    %span#flag-span.small-text= t('layout.issues.choose_labels_on_left')
+    #issue_labels
+  .both
 .leftlist
 .rightlist
   %input{:type => "submit", :value => t(@issue.new_record? ? 'layout.create' : 'layout.update')}
diff --git a/app/views/projects/issues/_manage_sidebar.html.haml b/app/views/projects/issues/_manage_sidebar.html.haml
index 98144b2d2..1ba6fa829 100644
--- a/app/views/projects/issues/_manage_sidebar.html.haml
+++ b/app/views/projects/issues/_manage_sidebar.html.haml
@@ -1,8 +1,8 @@
 -content_for :sidebar do
-  - can_manage = can?(:update, @issue) && @issue.persisted? || @issue.new_record?
   - if @issue.persisted?
     .bordered.nopadding
       %h3=t('activerecord.attributes.issue.status')
+      - can_manage = can?(:update, @issue)
       #switcher.issue_status{:class => "#{@issue.closed? ? 'switcher-off' : 'switcher'} #{can_manage ? "switch_issue_status" : ''}"}
         .swleft=t('layout.issues.status.open')
         .swright=t('layout.issues.status.closed')
@@ -11,7 +11,7 @@
             =hidden_field_tag "issue_status", @issue.closed? ? 'closed' : 'open', :name => "issue[status]"
   .block
     %h3=t('layout.issues.labels')
-    - if can_manage
+    - if can?(:write, @project)
       .current_labels
         - (@project.labels || []).each do |label|
           - is_issue_label = @issue.labels.include? label
@@ -29,6 +29,6 @@
         .label.nopointer
           .labeltext.selected{:style => "background: ##{label.color};"}=label.name
           .both
-    - if can_manage && @issue.persisted?
+    - if can?(:write, @project) && @issue.persisted?
       =link_to(t('layout.issues.label_manage'), '#', :class => "button tmargin10 manage_labels")
       =link_to(t('layout.issues.done'), '#', :class => "button tmargin10 update_labels", :style => 'display:none')
diff --git a/app/views/projects/issues/_user_container.html.haml b/app/views/projects/issues/_user_container.html.haml
index 47b78037d..21c1ca3a6 100644
--- a/app/views/projects/issues/_user_container.html.haml
+++ b/app/views/projects/issues/_user_container.html.haml
@@ -6,5 +6,5 @@
     %span= t('layout.issues.is_assigned')
   - else
     %span= t('layout.issues.no_one_is_assigned')
-  -if can?(:update, @issue) || @issue.new_record?
+  -if can?(:write, @project)
     %span.icon-share
\ No newline at end of file
diff --git a/app/views/projects/pull_requests/new.html.haml b/app/views/projects/pull_requests/new.html.haml
index a4d30a2d8..9b148e318 100644
--- a/app/views/projects/pull_requests/new.html.haml
+++ b/app/views/projects/pull_requests/new.html.haml
@@ -32,11 +32,11 @@
               %div{:class => @pull.ready? ? 'notice' : 'alert'}
                 =pull_status @pull
           .both
-
-          .leftlist.big-list= t('activerecord.attributes.issue.assignee') + ':'
-          #assigned-container.rightlist
-            =render 'projects/issues/user_container', :user => @pull.assignee
-          .both
+          - if can?(:write, @pull.to_project)
+            .leftlist.big-list= t('activerecord.attributes.issue.assignee') + ':'
+            #assigned-container.rightlist
+              =render 'projects/issues/user_container', :user => @pull.assignee
+            .both
           .leftlist.big-list
           .rightlist
             =f.submit t('.submit'), :class => 'btn btn-primary disabled', 'data-loading-text' => t('layout.processing'), :id => 'create_pull' unless @pull.already?

From 8b6a0eab0f30d89493d611d404ddb217386f6350 Mon Sep 17 00:00:00 2001
From: Vokhmin Alexey V <avokhmin@gmail.com>
Date: Wed, 3 Apr 2013 01:59:33 +0400
Subject: [PATCH 2/6] #51: some refactoring

---
 app/controllers/projects/issues_controller.rb       | 13 ++++---------
 .../projects/pull_requests_controller.rb            |  2 ++
 app/models/issue.rb                                 | 13 +++++++++++++
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 52c951d01..c5a9495ec 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -41,10 +41,7 @@ class Projects::IssuesController < Projects::BaseController
     @assignee_uname = params[:assignee_uname]
     @issue.user_id = current_user.id
 
-    unless can?(:write, @project)
-      @issue.assignee_id = nil
-      @issue.labelings = []
-    end
+    @issue.can_write_project = can?(:write, @project)
     if @issue.save
       @issue.subscribe_creator(current_user.id)
       flash[:notice] = I18n.t("flash.issue.saved")
@@ -60,11 +57,9 @@ class Projects::IssuesController < Projects::BaseController
   end
 
   def update
-    unless can?(:write, @project)
-      [:labelings, :labelings_attributes, :assignee_id].each{ |k| params[:issue].delete k }
-      params.delete :update_labels
-    end
-    @issue.labelings.destroy_all if params[:update_labels]
+    can_write_project = can?(:write, @project)
+    @issue.can_write_project = can_write_project
+    @issue.labelings.destroy_all if can_write_project && params[:update_labels]
     if params[:issue] && status = params[:issue][:status]
       @issue.set_close(current_user) if status == 'closed'
       @issue.set_open if status == 'open'
diff --git a/app/controllers/projects/pull_requests_controller.rb b/app/controllers/projects/pull_requests_controller.rb
index 5a4e7cbd9..055928aac 100644
--- a/app/controllers/projects/pull_requests_controller.rb
+++ b/app/controllers/projects/pull_requests_controller.rb
@@ -43,6 +43,7 @@ class Projects::PullRequestsController < Projects::BaseController
     @pull.from_project_owner_uname = @pull.from_project.owner.uname
     @pull.from_project_name = @pull.from_project.name
 
+    @pull.issue.can_write_project = can?(:write, @project)
     if @pull.valid? # FIXME more clean/clever logics
       @pull.save # set pull id
       @pull.check(false) # don't make event transaction
@@ -67,6 +68,7 @@ class Projects::PullRequestsController < Projects::BaseController
   end
 
   def update
+    @pull.issue.can_write_project = can?(:write, @project)
     if (action = params[:pull_request_action]) && %w(close reopen).include?(params[:pull_request_action])
       if @pull.send("can_#{action}?")
         @pull.set_user_and_time current_user
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 1d61be583..a247f475a 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -13,6 +13,7 @@ class Issue < ActiveRecord::Base
   has_many :labels, :through => :labelings, :uniq => true
   has_one :pull_request, :dependent => :destroy
 
+  before_validation :sanitize_params_for_current_user
   validates :title, :body, :project_id, :presence => true
 
   after_create :set_serial_id
@@ -21,6 +22,7 @@ class Issue < ActiveRecord::Base
 
   attr_accessible :labelings_attributes, :title, :body, :assignee_id
   accepts_nested_attributes_for :labelings, :allow_destroy => true
+  attr_accessor :can_write_project
 
   scope :opened, where(:status => 'open')
   scope :closed, where(:status => 'closed')
@@ -69,6 +71,17 @@ class Issue < ActiveRecord::Base
 
   protected
 
+  def sanitize_params_for_current_user
+    return true if can_write_project
+    if persisted?
+      self.assignee_id  = self.assignee_id
+      self.labelings    = self.labelings
+    else
+      self.assignee_id  = nil
+      self.labelings    = []
+    end
+  end
+
   def set_serial_id
     self.serial_id = self.project.issues.count
     self.save!

From 54040405b6dc516539b71c5ad7f3c749f432ffe9 Mon Sep 17 00:00:00 2001
From: Vokhmin Alexey V <avokhmin@gmail.com>
Date: Wed, 3 Apr 2013 02:12:21 +0400
Subject: [PATCH 3/6] #51: fixed sanitizing of labels

---
 app/models/issue.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/issue.rb b/app/models/issue.rb
index a247f475a..5b2d2ec61 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -75,7 +75,7 @@ class Issue < ActiveRecord::Base
     return true if can_write_project
     if persisted?
       self.assignee_id  = self.assignee_id
-      self.labelings    = self.labelings
+      self.labelings    = self.labelings.select{ |i| i.id }
     else
       self.assignee_id  = nil
       self.labelings    = []

From 8358713db2616d1965e79b078352b402fb1cb58c Mon Sep 17 00:00:00 2001
From: Vokhmin Alexey V <avokhmin@gmail.com>
Date: Wed, 3 Apr 2013 02:13:59 +0400
Subject: [PATCH 4/6] #51: small refactoring

---
 app/models/issue.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/issue.rb b/app/models/issue.rb
index 5b2d2ec61..5a1d245aa 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -13,7 +13,7 @@ class Issue < ActiveRecord::Base
   has_many :labels, :through => :labelings, :uniq => true
   has_one :pull_request, :dependent => :destroy
 
-  before_validation :sanitize_params_for_current_user
+  before_validation :sanitize_params
   validates :title, :body, :project_id, :presence => true
 
   after_create :set_serial_id
@@ -71,7 +71,7 @@ class Issue < ActiveRecord::Base
 
   protected
 
-  def sanitize_params_for_current_user
+  def sanitize_params
     return true if can_write_project
     if persisted?
       self.assignee_id  = self.assignee_id

From 6585c3a92bd4e8e8c0fc289a45c732e01ec1b9a6 Mon Sep 17 00:00:00 2001
From: Vokhmin Alexey V <avokhmin@gmail.com>
Date: Wed, 3 Apr 2013 13:59:34 +0400
Subject: [PATCH 5/6] #51: move logic to controller

---
 app/controllers/projects/issues_controller.rb     | 15 +++++++++++----
 .../projects/pull_requests_controller.rb          |  4 +---
 app/models/issue.rb                               | 13 -------------
 3 files changed, 12 insertions(+), 20 deletions(-)

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index c5a9495ec..10a539d08 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -41,7 +41,10 @@ class Projects::IssuesController < Projects::BaseController
     @assignee_uname = params[:assignee_uname]
     @issue.user_id = current_user.id
 
-    @issue.can_write_project = can?(:write, @project)
+    unless can?(:write, @project)
+      @issue.assignee_id  = nil
+      @issue.labelings    = []
+    end
     if @issue.save
       @issue.subscribe_creator(current_user.id)
       flash[:notice] = I18n.t("flash.issue.saved")
@@ -57,9 +60,13 @@ class Projects::IssuesController < Projects::BaseController
   end
 
   def update
-    can_write_project = can?(:write, @project)
-    @issue.can_write_project = can_write_project
-    @issue.labelings.destroy_all if can_write_project && params[:update_labels]
+    unless can?(:write, @project)
+      params.delete :update_labels
+      [:assignee_id, :labelings, :labelings_attributes].each do |k|
+        params[:issue].delete k
+      end if params[:issue]
+    end
+    @issue.labelings.destroy_all if params[:update_labels]
     if params[:issue] && status = params[:issue][:status]
       @issue.set_close(current_user) if status == 'closed'
       @issue.set_open if status == 'open'
diff --git a/app/controllers/projects/pull_requests_controller.rb b/app/controllers/projects/pull_requests_controller.rb
index 055928aac..c53bea847 100644
--- a/app/controllers/projects/pull_requests_controller.rb
+++ b/app/controllers/projects/pull_requests_controller.rb
@@ -38,12 +38,11 @@ class Projects::PullRequestsController < Projects::BaseController
     authorize! :read, to_project
 
     @pull = to_project.pull_requests.new pull_params
-    @pull.issue.assignee_id = (params[:issue] || {})[:assignee_id]
+    @pull.issue.assignee_id = (params[:issue] || {})[:assignee_id] if can?(:write, to_project)
     @pull.issue.user, @pull.issue.project, @pull.from_project = current_user, to_project, @project
     @pull.from_project_owner_uname = @pull.from_project.owner.uname
     @pull.from_project_name = @pull.from_project.name
 
-    @pull.issue.can_write_project = can?(:write, @project)
     if @pull.valid? # FIXME more clean/clever logics
       @pull.save # set pull id
       @pull.check(false) # don't make event transaction
@@ -68,7 +67,6 @@ class Projects::PullRequestsController < Projects::BaseController
   end
 
   def update
-    @pull.issue.can_write_project = can?(:write, @project)
     if (action = params[:pull_request_action]) && %w(close reopen).include?(params[:pull_request_action])
       if @pull.send("can_#{action}?")
         @pull.set_user_and_time current_user
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 5a1d245aa..1d61be583 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -13,7 +13,6 @@ class Issue < ActiveRecord::Base
   has_many :labels, :through => :labelings, :uniq => true
   has_one :pull_request, :dependent => :destroy
 
-  before_validation :sanitize_params
   validates :title, :body, :project_id, :presence => true
 
   after_create :set_serial_id
@@ -22,7 +21,6 @@ class Issue < ActiveRecord::Base
 
   attr_accessible :labelings_attributes, :title, :body, :assignee_id
   accepts_nested_attributes_for :labelings, :allow_destroy => true
-  attr_accessor :can_write_project
 
   scope :opened, where(:status => 'open')
   scope :closed, where(:status => 'closed')
@@ -71,17 +69,6 @@ class Issue < ActiveRecord::Base
 
   protected
 
-  def sanitize_params
-    return true if can_write_project
-    if persisted?
-      self.assignee_id  = self.assignee_id
-      self.labelings    = self.labelings.select{ |i| i.id }
-    else
-      self.assignee_id  = nil
-      self.labelings    = []
-    end
-  end
-
   def set_serial_id
     self.serial_id = self.project.issues.count
     self.save!

From a33e20c5e426d78d37f927acdfdbf694ac7f8977 Mon Sep 17 00:00:00 2001
From: Vokhmin Alexey V <avokhmin@gmail.com>
Date: Wed, 3 Apr 2013 15:11:50 +0400
Subject: [PATCH 6/6] #51: add new specs for Issue

---
 app/controllers/projects/issues_controller.rb |  1 -
 .../projects/issues_controller_spec.rb        | 50 ++++++++++++++++---
 spec/factories/label.rb                       |  8 +++
 spec/factories/labeling.rb                    |  7 +++
 4 files changed, 57 insertions(+), 9 deletions(-)
 create mode 100644 spec/factories/label.rb
 create mode 100644 spec/factories/labeling.rb

diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb
index 10a539d08..0e11f2c38 100644
--- a/app/controllers/projects/issues_controller.rb
+++ b/app/controllers/projects/issues_controller.rb
@@ -38,7 +38,6 @@ class Projects::IssuesController < Projects::BaseController
   end
 
   def create
-    @assignee_uname = params[:assignee_uname]
     @issue.user_id = current_user.id
 
     unless can?(:write, @project)
diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb
index 65873a0a3..d6a7dd000 100644
--- a/spec/controllers/projects/issues_controller_spec.rb
+++ b/spec/controllers/projects/issues_controller_spec.rb
@@ -2,13 +2,14 @@
 require 'spec_helper'
 
 shared_context "issues controller" do
-  before(:each) do
+  before do
     stub_symlink_methods
 
     @project = FactoryGirl.create(:project)
     @issue_user = FactoryGirl.create(:user)
 
     @issue = FactoryGirl.create(:issue, :project_id => @project.id, :assignee_id => @issue_user.id)
+    @label = FactoryGirl.create(:label, :project_id => @project.id)
 
     @project_with_turned_off_issues = FactoryGirl.create(:project, :has_issues => false)
     @turned_of_issue = FactoryGirl.create(:issue, :project_id => @project_with_turned_off_issues.id, :assignee_id => @issue_user.id)
@@ -20,10 +21,10 @@ shared_context "issues controller" do
       :owner_name => @project.owner.uname, :project_name => @project.name,
       :issue => {
         :title => "issue1",
-        :body => "issue body"
-      },
-      :assignee_id => @issue_user.id,
-      :assignee_uname => @issue_user.uname
+        :body => "issue body",
+        :labelings_attributes => { @label.id => {:label_id => @label.id}},
+        :assignee_id => @issue_user.id
+      }
     }
 
     @update_params = {
@@ -56,9 +57,7 @@ shared_examples_for 'issue user with project reader rights' do
     get :index, :owner_name => @project.owner.uname, :project_name => @project.name
     response.should render_template(:index)
   end
-end
 
-shared_examples_for 'issue user with project writer rights' do
   it 'should be able to perform create action' do
     post :create, @create_params
     response.should redirect_to(project_issues_path(@project))
@@ -69,6 +68,30 @@ shared_examples_for 'issue user with project writer rights' do
   end
 end
 
+shared_examples_for 'issue user with project writer rights' do
+  it 'should be able to perform index action on hidden project' do
+    @project.update_attributes(:visibility => 'hidden')
+    get :index, :owner_name => @project.owner.uname, :project_name => @project.name
+    response.should render_template(:index)
+  end
+
+  it 'should create issue object into db' do
+    lambda{ post :create, @create_params }.should change{ Issue.count }.by(1)
+  end
+
+  context 'perform create action' do
+    before { post :create, @create_params }
+
+    it 'user should be assigned to issue' do
+      @project.issues.last.assignee_id.should_not be_nil
+    end
+
+    it 'label should be attached to issue' do
+      @project.issues.last.labels.should have(1).item
+    end
+  end
+end
+
 shared_examples_for 'user with issue update rights' do
   it 'should be able to perform update action' do
     put :update, {:id => @issue.serial_id}.merge(@update_params)
@@ -167,11 +190,22 @@ describe Projects::IssuesController do
 
     it_should_behave_like 'issue user with project guest rights'
     it_should_behave_like 'issue user with project reader rights'
-    it_should_behave_like 'issue user with project writer rights'
     it_should_behave_like 'user without issue update rights'
     it_should_behave_like 'project with issues turned off'
     it_should_behave_like 'user without issue destroy rights'
 
+    context 'perform create action' do
+      before { post :create, @create_params }
+
+      it 'user should not be assigned to issue' do
+        @project.issues.last.assignee_id.should be_nil
+      end
+
+      it 'label should not be attached to issue' do
+        @project.issues.last.labels.should have(:no).items
+      end
+    end
+
     # it 'should not be able to perform create action on project' do
     #   post :create, @create_params
     #   response.should redirect_to(forbidden_path)
diff --git a/spec/factories/label.rb b/spec/factories/label.rb
new file mode 100644
index 000000000..b985a2d98
--- /dev/null
+++ b/spec/factories/label.rb
@@ -0,0 +1,8 @@
+# -*- encoding : utf-8 -*-
+FactoryGirl.define do
+  factory :label do
+    name { FactoryGirl.generate(:string) }
+    color 'FFF'
+    association :project, :factory => :project
+  end
+end
\ No newline at end of file
diff --git a/spec/factories/labeling.rb b/spec/factories/labeling.rb
new file mode 100644
index 000000000..51d1f6d70
--- /dev/null
+++ b/spec/factories/labeling.rb
@@ -0,0 +1,7 @@
+# -*- encoding : utf-8 -*-
+FactoryGirl.define do
+  factory :labeling do
+    association :project, :factory => :project
+    association :label,   :factory => :label
+  end
+end
\ No newline at end of file