Fix and refactor build lists, comments and other specs. Refactor comments controller, models and views. Code cleanup. Refs #263

app/controllers/build_lists_controller.rb

@@ -45,7 +45,7 @@ class BuildListsController < ApplicationController
         @build_list = @project.build_lists.build(params[:build_list])
         @build_list.commit_hash = @project.git_repository.commits(@build_list.project_version.match(/^latest_(.+)/).to_a.last || @build_list.project_version).first.id if @build_list.project_version
         @build_list.bpl = bpl; @build_list.arch = arch; @build_list.user = current_user
-        @build_list.include_repos = @build_list.include_repos.select { |ir| @build_list.bpl.repository_ids.include? ir.to_i }
+        @build_list.include_repos = @build_list.include_repos.select {|ir| @build_list.bpl.repository_ids.include? ir.to_i}
         @build_list.priority = 100 # User builds more priority than mass rebuild with zero priority
         flash_options = {:project_version => @build_list.project_version, :arch => arch.name, :bpl => bpl.name, :pl => @build_list.pl}
         if @build_list.save

app/controllers/comments_controller.rb

@@ -1,26 +1,17 @@
 # -*- encoding : utf-8 -*-
 class CommentsController < ApplicationController
   before_filter :authenticate_user!
+  load_and_authorize_resource :project
+  before_filter :find_commentable
+  before_filter :find_or_build_comment
+  load_and_authorize_resource
 
-  load_resource :project
-  before_filter :set_commentable
-  before_filter :find_comment, :only => [:edit, :update, :destroy]
-  authorize_resource
-
-  def index
-    @comments = @commentable.comments
-  end
+  include CommentsHelper
 
   def create
-    @comment = @commentable.comments.build(params[:comment]) if @commentable.class == Issue
-    if @commentable.class == Grit::Commit
-      @comment = Comment.new(params[:comment].merge(:commentable_id => @commentable.id.hex, :commentable_type => @commentable.class.name))
-    end
-    @comment.project = @project
-    @comment.user_id = current_user.id
     if @comment.save
       flash[:notice] = I18n.t("flash.comment.saved")
-      redirect_to commentable_path
+      redirect_to project_commentable_path(@project, @commentable)
     else
       flash[:error] = I18n.t("flash.comment.save_error")
       render :action => 'new'
@@ -28,19 +19,12 @@ class CommentsController < ApplicationController
   end
 
   def edit
-    @update_url = case @commentable.class.name
-                  when "Issue"
-                    project_issue_comment_path(@project, @commentable, @comment)
-                  when "Grit::Commit"
-                    project_commit_comment_path(@project, @commentable, @comment)
-                  end
-    @commentable_path = commentable_path
   end
 
   def update
     if @comment.update_attributes(params[:comment])
       flash[:notice] = I18n.t("flash.comment.saved")
-      redirect_to commentable_path
+      redirect_to project_commentable_path(@project, @commentable)
     else
       flash[:error] = I18n.t("flash.comment.save_error")
       render :action => 'new'
@@ -49,30 +33,19 @@ class CommentsController < ApplicationController
 
   def destroy
     @comment.destroy
-
     flash[:notice] = t("flash.comment.destroyed")
-    redirect_to commentable_path
+    redirect_to project_commentable_path(@project, @commentable)
   end
 
-  private
+  protected
 
-  def set_commentable
-    @commentable = if params[:issue_id].present?
-                                  @project.issues.find_by_serial_id params[:issue_id]
-                                elsif params[:commit_id].present?
-                                  @project.git_repository.commit params[:commit_id]
-                                end
+  def find_commentable
+    @commentable = params[:issue_id].present? && @project.issues.find_by_serial_id(params[:issue_id]) ||
+                   params[:commit_id].present? && @project.git_repository.commit(params[:commit_id])
   end
 
-  def find_comment
-    @comment = Comment.find(params[:id])
-    if @comment.commit_comment?
-      @comment.project = @project
-    end
+  def find_or_build_comment
+    @comment = params[:id].present? && Comment.find(params[:id]) ||
+               Comment.new(params[:comment]){|c| c.commentable = @commentable; c.project = @project; c.user_id = current_user.id}
   end
-
-  def commentable_path
-    @commentable.class == Issue ? [@project, @commentable] : commit_path(@project, @commentable.id)
-  end
-
 end

app/helpers/comments_helper.rb

@@ -1,3 +1,20 @@
 # -*- encoding : utf-8 -*-
 module CommentsHelper
+  def project_commentable_comment_path(project, commentable, comment)
+    case
+    when Comment.issue_comment?(commentable.class)
+      project_issue_comment_path(project, commentable, comment)
+    when Comment.commit_comment?(commentable.class)
+      project_commit_comment_path(project, commentable, comment)
+    end
+  end
+
+  def project_commentable_path(project, commentable)
+    case
+    when Comment.issue_comment?(commentable.class)
+      polymorphic_path [project, commentable]
+    when Comment.commit_comment?(commentable.class)
+      commit_path project, commentable.id
+    end
+  end
 end

app/models/activity_feed_observer.rb

@@ -35,7 +35,7 @@ class ActivityFeedObserver < ActiveRecord::Observer
       end
 
     when 'Comment'
-      if record.commentable.class == Issue
+      if record.issue_comment?
         subscribes = record.commentable.subscribes
         subscribes.each do |subscribe|
           if record.user_id != subscribe.user_id

app/models/comment.rb

@@ -6,23 +6,45 @@ class Comment < ActiveRecord::Base
 
   validates :body, :user_id, :commentable_id, :commentable_type, :project_id, :presence => true
 
+  scope :for_commit, lambda {|c| where(:commentable_id => c.id.hex, :commentable_type => c.class)}
   default_scope order('created_at')
 
   after_create :subscribe_on_reply, :unless => lambda {|c| c.commit_comment?}
   after_create :subscribe_users
-  after_initialize do |comment|
-    class_eval { def commentable; project.git_repository.commit(commentable_id.to_s(16)); end } if commit_comment?
+
+  attr_accessible :body
+
+  def commentable
+    commit_comment? ? project.git_repository.commit(commentable_id.to_s(16)) : super
   end
 
-  attr_accessible :body, :commentable_id, :commentable_type
-  attr_readonly :commentable_id, :commentable_type
+  def commentable=(c)
+    if self.class.commit_comment?(c.class)
+      self.commentable_id = c.id.hex
+      self.commentable_type = c.class.name
+    else
+      super
+    end
+  end
 
-  def own_comment?(user)
-    user_id == user.id
+  def self.commit_comment?(class_name)
+    class_name.to_s == 'Grit::Commit'
   end
 
   def commit_comment?
-    commentable_type == 'Grit::Commit'
+    self.class.commit_comment?(commentable_type)
+  end
+
+  def self.issue_comment?(class_name)
+    class_name.to_s == 'Issue'
+  end
+
+  def issue_comment?
+    self.class.issue_comment?(commentable_type)
+  end
+
+  def own_comment?(user)
+    user_id == user.id
   end
 
   def can_notify_on_new_comment?(subscribe)
@@ -32,19 +54,19 @@ class Comment < ActiveRecord::Base
   protected
 
   def subscribe_on_reply
-    self.commentable.subscribes.create(:user_id => self.user_id) if !self.commentable.subscribes.exists?(:user_id => self.user_id)
+    commentable.subscribes.create(:user_id => user_id) if !commentable.subscribes.exists?(:user_id => user_id)
   end
 
   def subscribe_users
-    if self.commentable.class == Issue
-      self.commentable.subscribes.create(:user => self.user) if !self.commentable.subscribes.exists?(:user_id => self.user.id)
-    elsif self.commit_comment?
-      recipients = self.project.relations.by_role('admin').where(:object_type => 'User').map &:object # admins
-      recipients << self.user << User.where(:email => self.commentable.committer.email).first # commentor and committer
-      recipients << self.project.owner if self.project.owner_type == 'User' # project owner
+    if issue_comment?
+      commentable.subscribes.create(:user => user) if !commentable.subscribes.exists?(:user_id => user.id)
+    elsif commit_comment?
+      recipients = project.relations.by_role('admin').where(:object_type => 'User').map &:object # admins
+      recipients << user << User.where(:email => commentable.committer.email).first # commentor and committer
+      recipients << project.owner if project.owner_type == 'User' # project owner
       recipients.compact.uniq.each do |user|
-        options = {:project_id => self.project.id, :subscribeable_id => self.commentable_id, :subscribeable_type => self.commentable.class.name, :user_id => user.id}
-        Subscribe.subscribe_to_commit(options) if Subscribe.subscribed_to_commit?(self.project, user, self.commentable)
+        options = {:project_id => project.id, :subscribeable_id => commentable_id, :subscribeable_type => commentable.class.name, :user_id => user.id}
+        Subscribe.subscribe_to_commit(options) if Subscribe.subscribed_to_commit?(project, user, commentable)
       end
     end
   end

app/models/project.rb

@@ -180,10 +180,6 @@ class Project < ActiveRecord::Base
     system("#{Rails.root.join('bin', 'import_srpm.sh')} #{srpm_path} #{path} #{branch_name} >> /dev/null 2>&1")
   end
 
-  def self.commit_comments(commit, project)
-    comments = Comment.where(:commentable_id => commit.id.hex, :commentable_type => 'Grit::Commit')
-  end
-
   def owner?(user)
     owner == user
   end

app/presenters/comment_presenter.rb

@@ -30,10 +30,10 @@ class CommentPresenter < ApplicationPresenter
   def buttons
     project = options[:project]
     commentable = options[:commentable]
-    (ep, dp) = if commentable.class == Issue
+    (ep, dp) = if Comment.issue_comment?(commentable.class)
       [edit_project_issue_comment_path(project, commentable, comment),
        project_issue_comment_path(project, commentable, comment)]
-    elsif commentable.class == Grit::Commit
+    elsif Comment.commit_comment?(commentable.class)
       [edit_project_commit_comment_path(project, commentable, comment),
        project_commit_comment_path(project, commentable, comment)]
     end

app/views/comments/_add.html.haml

@@ -1,10 +1,10 @@
 #open-comment.comment.view
   %h3.tmargin0= t("layout.comments.new_header")
-  - if commentable.class == Issue
+  - if Comment.issue_comment?(commentable.class)
     - new_path = project_issue_comments_path(project, commentable)
     - is_subscribed = commentable.subscribes.exists?(:user_id => current_user.id)
     - subscribe_path = is_subscribed ? project_issue_subscribe_path(project, commentable, current_user.id) : project_issue_subscribes_path(project, commentable)
-  - else commentable.class == Grit::Commit
+  - else Comment.commit_comment?(commentable.class)
     - new_path = project_commit_comments_path(project, commentable)
     - is_subscribed = Subscribe.subscribed_to_commit?(project, current_user, commentable)
     - subscribe_path = is_subscribed ? unsubscribe_commit_path(project, commentable) : subscribe_commit_path(project, commentable)

app/views/comments/_form.html.haml

@@ -1,14 +1,2 @@
-.wrapper
-  = f.text_area :body, :cols => 80
-.comment-right
-  = submit_tag t("layout.save")
--#.group
-  = f.label :body, t("activerecord.attributes.comment.body"), :class => :label
-  = f.text_area :body, :class => 'text_field', :cols => 80
-
--#.group.navform.wat-cf
-  %button.button{:type => "submit"}
-    = image_tag("choose.png", :alt => t("layout.save"))
-    = t("layout.save")
-  %span.text_button_padding= t("layout.or")
-  = link_to t("layout.cancel"), @commentable_path , :class => "text_button_padding link_button"
+.wrapper= f.text_area :body, :cols => 80
+.comment-right= submit_tag t("layout.save")

app/views/comments/_list.html.haml

@@ -3,37 +3,4 @@
 %h3= t("layout.comments.comments_header")
 - list.each do |comment|
   - CommentPresenter.present(comment, :project => project, :commentable => commentable) do |presenter|
-    = render :partial => 'shared/feed_message', :locals => {:presenter => presenter}
--#.block#block-list
-  .content
-    %h2.title
-      = t("layout.issues.comments_header")
-    .inner
-      %ul.list
-        - list.each do |comment|
-          %li
-            .left
-              = link_to comment.user.uname, user_path(comment.user.uname)
-            .item
-              = comment.body
-              %br
-              %br
-              - if commentable.class == Issue
-                - edit_path = edit_project_issue_comment_path(project, commentable, comment)
-                - delete_path = project_issue_comment_path(project, commentable, comment)
-              - elsif commentable.class == Grit::Commit
-                - edit_path = edit_project_commit_comment_path(project, commentable, comment)
-                - delete_path = project_commit_comment_path(project, commentable, comment)
-              = link_to t("layout.edit"), edit_path if can? :update, comment
-              =# link_to image_tag("x.png", :alt => t("layout.delete")) + " " + t("layout.delete"), delete_path, :method => "delete", :class => "button", :confirm => t("layout.comments.confirm_delete") if can? :delete, comment
-              = link_to t("layout.delete"), delete_path, :method => "delete", :confirm => t("layout.comments.confirm_delete") if can? :delete, comment
-
--#.block
-  .content
-    %h2.title
-      = t("layout.comments.new_header")
-    .inner
-      - new_path = project_issue_comments_path(project, commentable) if commentable.class == Issue
-      - new_path = project_commit_comments_path(project, commentable) if commentable.class == Grit::Commit
-      = form_for :comment, :url => new_path, :method => :post, :html => { :class => :form } do |f|
-        = render :partial => "comments/form", :locals => {:f => f}
+    = render :partial => 'shared/feed_message', :locals => {:presenter => presenter}

app/views/comments/edit.html.haml

@@ -2,10 +2,10 @@
 .block
   .secondary-navigation
     %ul.wat-cf
-      %li.first= link_to t("layout.comments.back"), @commentable_path
+      %li.first= link_to t("layout.comments.back"), project_commentable_path(@project, @commentable)
   .content
     %h2.title
       = t("layout.comments.edit_header")
     .inner
-      = form_for @comment, :url => @update_url, :html => { :class => :form } do |f|
+      = form_for @comment, :url => project_commentable_comment_path(@project, @commentable, @comment), :html => {:class => :form} do |f|
         = render :partial => "form", :locals => {:f => f}

app/views/git/commits/show.html.haml

@@ -16,5 +16,5 @@
 #repo-wrapper
   = render :partial => 'show'
 
-  = render :partial => "comments/list", :locals => {:list => Project.commit_comments(@commit, @project), :project => @project, :commentable => @commit}
+  = render :partial => "comments/list", :locals => {:list => Comment.for_commit(@commit), :project => @project, :commentable => @commit}
   = render :partial => "comments/add", :locals => {:project => @project, :commentable => @commit} if current_user

app/views/user_mailer/new_comment_notification.en.haml

@@ -1,6 +1,6 @@
 %p== Hello, #{@user.user_appeal}.
 
-- if @comment.commentable.class == Issue
+- if @comment.issue_comment?
   - link = link_to @comment.commentable.title, project_issue_url(@comment.commentable.project, @comment.commentable)
   - object = 'issue'
 - elsif @comment.commit_comment?

app/views/user_mailer/new_comment_notification.ru.haml

@@ -1,6 +1,6 @@
 %p== Здравствуйте, #{@user.user_appeal}.
 
-- if @comment.commentable.class == Issue
+- if @comment.issue_comment?
   - link = link_to @comment.commentable.title, project_issue_url(@comment.commentable.project, @comment.commentable)
   - object = 'задаче'
 - elsif @comment.commit_comment?

db/migrate/20120330201229_remove_containers_and_rpms.rb

@@ -1,3 +1,4 @@
+# -*- encoding : utf-8 -*-
 class RemoveContainersAndRpms < ActiveRecord::Migration
   def up
     drop_table :containers

db/migrate/20120331180541_customize_platform.rb

@@ -1,3 +1,4 @@
+# -*- encoding : utf-8 -*-
 class CustomizePlatform < ActiveRecord::Migration
   def self.up
     change_column_null :platforms, :name, false
@@ -16,4 +17,4 @@ class CustomizePlatform < ActiveRecord::Migration
     change_column_null :platforms, :visibility, true
     remove_index "platforms", ["name"]
   end  
-end
+end

db/migrate/20120403110931_clear_product.rb

@@ -1,3 +1,4 @@
+# -*- encoding : utf-8 -*-
 class ClearProduct < ActiveRecord::Migration
   def self.up
     remove_column :products, :build_status
@@ -10,4 +11,4 @@ class ClearProduct < ActiveRecord::Migration
     add_column :products, :build_path, :string
     add_column :products, :system_wide, :boolean, :default => false
   end
-end
+end

db/schema.rb

@@ -1,4 +1,4 @@
-# encoding: UTF-8
+# -*- encoding : utf-8 -*-
 # This file is auto-generated from the current state of the database. Instead
 # of editing this file, please use the migrations feature of Active Record to
 # incrementally modify your database, and then regenerate this schema definition.
@@ -17,8 +17,8 @@ ActiveRecord::Schema.define(:version => 20120403110931) do
     t.integer  "user_id",    :null => false
     t.string   "kind"
     t.text     "data"
-    t.datetime "created_at"
-    t.datetime "updated_at"
+    t.datetime "created_at", :null => false
+    t.datetime "updated_at", :null => false
   end
 
   create_table "arches", :force => true do |t|
@@ -331,16 +331,16 @@ ActiveRecord::Schema.define(:version => 20120403110931) do
     t.string   "name"
     t.string   "email",                                  :default => "",   :null => false
     t.string   "encrypted_password",      :limit => 128, :default => "",   :null => false
-    t.string   "password_salt",                          :default => "",   :null => false
     t.string   "reset_password_token"
     t.datetime "remember_created_at"
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.text     "ssh_key"
     t.string   "uname"
     t.string   "role"
     t.string   "language",                               :default => "en"
-    t.integer  "own_projects_count",                     :default => 0,    :null => false
     t.datetime "reset_password_sent_at"
+    t.integer  "own_projects_count",                     :default => 0,    :null => false
     t.text     "professional_experience"
     t.string   "site"
     t.string   "company"

spec/controllers/build_lists_controller_spec.rb

@@ -83,9 +83,16 @@ describe BuildListsController do
     end
 
     context 'for guest' do
-      it 'should not be able to perform index action' do
-        get :index
-        response.should redirect_to(new_user_session_path)
+      if APP_CONFIG['anonymous_access']
+        it 'should be able to perform index action' do
+          get :index
+          response.should be_success
+        end
+      else
+        it 'should not be able to perform index action' do
+          get :index
+          response.should redirect_to(new_user_session_path)
+        end
       end
     end
 

spec/controllers/comments_controller_for_commit_spec.rb

@@ -2,9 +2,7 @@
 require 'spec_helper'
 
 def create_comment user
-  comment = user.comments.create(:commentable_id => @commit.id.hex, :commentable_type => @commit.class.name,
-    :body => 'test', :project_id => @project.id)
-  comment
+  FactoryGirl.create(:comment, :user => user, :commentable => @commit, :project => @project)
 end
 
 shared_examples_for 'user with create comment rights for commits' do

spec/controllers/product_build_lists_controller_spec.rb

@@ -55,10 +55,17 @@ describe ProductBuildListsController do
         response.should redirect_to(new_user_session_path)
       end
 
-      it 'should not be able to view ProductBuildLists' do
-        get :index
-        response.should redirect_to(new_user_session_path)
-      end      
+      if APP_CONFIG['anonymous_access']
+        it 'should be able to view ProductBuildLists' do
+          get :index
+          response.should be_success
+        end
+      else
+        it 'should not be able to view ProductBuildLists' do
+          get :index
+          response.should redirect_to(new_user_session_path)
+        end
+      end
     end
 
     context 'for user' do

spec/controllers/users_controller_spec.rb

@@ -38,10 +38,6 @@ describe UsersController do
     end
 
     context 'with mass assignment' do
-      it 'should not be able to update uname' do
-        @simple_user.should_not allow_mass_assignment_of :uname
-      end
-
       it 'should not be able to update role' do
         @simple_user.should_not allow_mass_assignment_of :role
       end

spec/factories/platforms.rb

@@ -1,19 +1,18 @@
 # -*- encoding : utf-8 -*-
 FactoryGirl.define do
   factory :platform do
-    description { FactoryGirl.generate(:string) }
     name { FactoryGirl.generate(:unixname) }
+    description { FactoryGirl.generate(:string) }
     platform_type 'main'
     distrib_type APP_CONFIG['distr_types'].first
     association :owner, :factory => :user
-  end
 
-  factory :platform_with_repos, :parent => :platform do
-    repositories {|r| [r.association(:repository)]}
+    factory :platform_with_repos do
+      after_create {|p| FactoryGirl.create_list(:repository, 1, platform: p)}
+    end
+
+    factory :personal_platform do
+      platform_type 'personal'
+    end
   end
-  
-  factory :personal_platform, :parent => :platform do
-    platform_type 'personal'
-  end
-  
 end

spec/models/cancan_spec.rb

@@ -64,10 +64,6 @@ describe CanCan do
     	@ability.should_not be_able_to(:read, hidden_platform)
     end
 
-    it 'should be able to auto build projects' do
-    	@ability.should be_able_to(:auto_build, Project)
-    end
-
 		[:publish_build, :status_build, :pre_build, :post_build, :circle_build, :new_bbdt].each do |action|
 			it "should be able to #{ action } build list" do
 				@ability.should be_able_to(action, BuildList)
@@ -90,7 +86,7 @@ describe CanCan do
       @ability.should_not be_able_to(:destroy, register_request)
     end
 
-		it 'should be able to register new user' do
+		pending 'should be able to register new user' do # while self registration is closed
 			@ability.should be_able_to(:create, User)
 		end
 	end

spec/models/comment_for_commit_spec.rb

@@ -3,9 +3,7 @@ require 'spec_helper'
 require "cancan/matchers"
 
 def create_comment user
-  comment = user.comments.create(:commentable_id => @commit.id.hex, :commentable_type => @commit.class.name,
-    :body => 'test', :project_id => @project.id)
-  comment
+  FactoryGirl.create(:comment, :user => user, :commentable => @commit, :project => @project)
 end
 
 def set_comments_data_for_commit

spec/models/product_build_list_spec.rb

@@ -24,4 +24,4 @@ describe ProductBuildList do
   it { should allow_mass_assignment_of(:notified_at) }
   it { should allow_mass_assignment_of(:base_url) }
   
-end
+end

spec/models/product_spec.rb

@@ -31,4 +31,4 @@ describe Product do
     FileUtils.rm_rf(APP_CONFIG['root_path'])
   end
 
-end
+end