From 7a4c95888225bf465187f9a186fb5373e8405a5f Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 8 Aug 2012 21:26:56 -0400 Subject: [PATCH 1/3] Add empty IssueCommonality module; include in Issue and MergeRequest --- app/models/issue.rb | 1 + app/models/merge_request.rb | 1 + app/roles/issue_commonality.rb | 3 +++ 3 files changed, 5 insertions(+) create mode 100644 app/roles/issue_commonality.rb diff --git a/app/models/issue.rb b/app/models/issue.rb index 7f935900..662431f9 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -1,4 +1,5 @@ class Issue < ActiveRecord::Base + include IssueCommonality include Upvote acts_as_taggable_on :labels diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 2581f3be..642db17d 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1,6 +1,7 @@ require File.join(Rails.root, "app/models/commit") class MergeRequest < ActiveRecord::Base + include IssueCommonality include Upvote BROKEN_DIFF = "--broken-diff" diff --git a/app/roles/issue_commonality.rb b/app/roles/issue_commonality.rb new file mode 100644 index 00000000..e3c2c047 --- /dev/null +++ b/app/roles/issue_commonality.rb @@ -0,0 +1,3 @@ +# Contains common functionality shared between Issues and MergeRequests +module IssueCommonality +end From f36f0dac9d2a009f29d2253dcd7c66d5a46ffd56 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 8 Aug 2012 21:40:57 -0400 Subject: [PATCH 2/3] Consolidate functionality shared between Issue and MergeRequest Any associations, validations, delegates, scopes and methods that were exactly the same in both Issue and MergeRequest models have been moved to a new IssueCommonality module (role) that gets included by each class. There was actually quite a bit of duplication, because MergeRequests are basically just specialized Issues. --- app/models/issue.rb | 41 ---------------------------- app/models/merge_request.rb | 50 ++++------------------------------ app/roles/issue_commonality.rb | 47 ++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 86 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index 662431f9..d8ca59c2 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -4,58 +4,17 @@ class Issue < ActiveRecord::Base acts_as_taggable_on :labels - belongs_to :project belongs_to :milestone - belongs_to :author, :class_name => "User" - belongs_to :assignee, :class_name => "User" - has_many :notes, :as => :noteable, :dependent => :destroy - - attr_protected :author, :author_id, :project, :project_id - attr_accessor :author_id_of_changes - - validates_presence_of :project_id - validates_presence_of :author_id - - delegate :name, - :email, - :to => :author, - :prefix => true - - delegate :name, - :email, - :to => :assignee, - :allow_nil => true, - :prefix => true - - validates :title, - :presence => true, - :length => { :within => 0..255 } validates :description, :length => { :within => 0..2000 } - scope :opened, where(:closed => false) - scope :closed, where(:closed => true) - scope :assigned, lambda { |u| where(:assignee_id => u.id)} - acts_as_list def self.open_for(user) opened.assigned(user) end - def self.search query - where("title like :query", :query => "%#{query}%") - end - - def today? - Date.today == created_at.to_date - end - - def new? - today? && created_at == updated_at - end - def is_assigned? !!assignee_id end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 642db17d..a77081c9 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -10,47 +10,15 @@ class MergeRequest < ActiveRecord::Base CAN_BE_MERGED = 2 CANNOT_BE_MERGED = 3 - belongs_to :project - belongs_to :author, :class_name => "User" - belongs_to :assignee, :class_name => "User" - has_many :notes, :as => :noteable, :dependent => :destroy - serialize :st_commits serialize :st_diffs - attr_protected :author, :author_id, :project, :project_id - attr_accessor :author_id_of_changes, - :should_remove_source_branch + attr_accessor :should_remove_source_branch - validates_presence_of :project_id - validates_presence_of :author_id validates_presence_of :source_branch validates_presence_of :target_branch validate :validate_branches - delegate :name, - :email, - :to => :author, - :prefix => true - - delegate :name, - :email, - :to => :assignee, - :allow_nil => true, - :prefix => true - - validates :title, - :presence => true, - :length => { :within => 0..255 } - - scope :opened, where(:closed => false) - scope :closed, where(:closed => true) - scope :assigned, lambda { |u| where(:assignee_id => u.id)} - - def self.search query - where("title like :query", :query => "%#{query}%") - end - def self.find_all_by_branch(branch_name) where("source_branch like :branch or target_branch like :branch", :branch => branch_name) end @@ -96,14 +64,6 @@ class MergeRequest < ActiveRecord::Base self.save end - def today? - Date.today == created_at.to_date - end - - def new? - today? && created_at == updated_at - end - def diffs st_diffs || [] end @@ -136,7 +96,7 @@ class MergeRequest < ActiveRecord::Base commits.first end - def merged? + def merged? merged && merge_event end @@ -153,7 +113,7 @@ class MergeRequest < ActiveRecord::Base end def probably_merged? - unmerged_commits.empty? && + unmerged_commits.empty? && commits.any? && open? end @@ -171,8 +131,8 @@ class MergeRequest < ActiveRecord::Base self.update_attributes :state => CANNOT_BE_MERGED end - def reloaded_commits - if open? && unmerged_commits.any? + def reloaded_commits + if open? && unmerged_commits.any? self.st_commits = unmerged_commits save end diff --git a/app/roles/issue_commonality.rb b/app/roles/issue_commonality.rb index e3c2c047..424b9cfb 100644 --- a/app/roles/issue_commonality.rb +++ b/app/roles/issue_commonality.rb @@ -1,3 +1,50 @@ # Contains common functionality shared between Issues and MergeRequests module IssueCommonality + extend ActiveSupport::Concern + + included do + attr_protected :author, :author_id, :project, :project_id + + belongs_to :project + belongs_to :author, :class_name => "User" + belongs_to :assignee, :class_name => "User" + has_many :notes, :as => :noteable, :dependent => :destroy + + validates_presence_of :project_id + validates_presence_of :author_id + + validates :title, + :presence => true, + :length => { :within => 0..255 } + + + scope :opened, where(:closed => false) + scope :closed, where(:closed => true) + scope :assigned, lambda { |u| where(:assignee_id => u.id)} + + delegate :name, + :email, + :to => :author, + :prefix => true + + delegate :name, + :email, + :to => :assignee, + :allow_nil => true, + :prefix => true + + attr_accessor :author_id_of_changes + end + + def self.search query + where("title like :query", :query => "%#{query}%") + end + + def today? + Date.today == created_at.to_date + end + + def new? + today? && created_at == updated_at + end end From b7f9b8223e40aa9e4aa8579d9a062df7e2e1d959 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 9 Aug 2012 13:45:12 -0400 Subject: [PATCH 3/3] Fix `search` class method for IssueCommonality Also adds specs to the two affected classes that would have caught my dumb mistake. --- app/roles/issue_commonality.rb | 6 ++++-- spec/models/issue_spec.rb | 8 ++++++++ spec/models/merge_request_spec.rb | 9 +++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/app/roles/issue_commonality.rb b/app/roles/issue_commonality.rb index 424b9cfb..353d6a5c 100644 --- a/app/roles/issue_commonality.rb +++ b/app/roles/issue_commonality.rb @@ -36,8 +36,10 @@ module IssueCommonality attr_accessor :author_id_of_changes end - def self.search query - where("title like :query", :query => "%#{query}%") + module ClassMethods + def search(query) + where("title like :query", :query => "%#{query}%") + end end def today? diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index fbd4031f..806b8ee7 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -106,6 +106,14 @@ describe Issue do end end + describe ".search" do + let!(:issue) { Factory.create(:issue, :title => "Searchable issue", + :project => Factory.create(:project)) } + + it "matches by title" do + Issue.search('able').all.should == [issue] + end + end end # == Schema Information # diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index ac986cce..4ef6becf 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -56,6 +56,15 @@ describe MergeRequest do subject.upvotes.should == 2 end end + + describe ".search" do + let!(:issue) { Factory.create(:issue, :title => "Searchable issue", + :project => Factory.create(:project)) } + + it "matches by title" do + Issue.search('able').all.should == [issue] + end + end end # == Schema Information #