Daily Refactor #6: Move recipients method to acts_as_event

This week I’m planning to do some refactoring on Redmine’s code base. Today I fixed a smell that Caliper found with flay.

The Refactoring

Redmine has several model objects that generate Events, which are listed on it’s Activity Page and cause email notifications to be sent. A few of these models had identical recipients methods defined, which caused a Flay score of 368 (bad). Using move method I as able to move 3 of the 4 into Redmine’s acts_as_event plugin. The fourth will need some more refactoring before it could be moved.

Before

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32

# vendor/plugins/acts_as_event/lib/acts_as_event.rb
# ... nothing related to recipients
 
# app/models/news.rb
class News < ActiveRecord::Base
  # Returns the mail adresses of users that should be notified
  def recipients
    notified = project.notified_users
    notified.reject! {|user| !visible?(user)}
    notified.collect(&:mail)
  end
end
 
# app/models/document.rb
class Document < ActiveRecord::Base
  # Returns the mail adresses of users that should be notified
  def recipients
    notified = project.notified_users
    notified.reject! {|user| !visible?(user)}
    notified.collect(&:mail)
  end
end
 
# app/models/message.rb
class Message < ActiveRecord::Base
  # Returns the mail adresses of users that should be notified
  def recipients
    notified = project.notified_users
    notified.reject! {|user| !visible?(user)}
    notified.collect(&:mail)
  end
end

After

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22

# vendor/plugins/acts_as_event/lib/acts_as_event.rb
  # Returns the mail adresses of users that should be notified
  def recipients
    notified = project.notified_users
    notified.reject! {|user| !visible?(user)}
    notified.collect(&:mail)
  end
 
# app/models/news.rb
class News < ActiveRecord::Base
 # ...
end
 
# app/models/document.rb
class Document < ActiveRecord::Base
 # ...
end
 
# app/models/message.rb
class Message < ActiveRecord::Base
 # ...
end

Review

While this refactor is pretty simple, duplication in a large system like Redmine can really affect development. That’s why Test Driven Development has that final refactor step at the end of every cycle. Hopefully this refactor will make it easier to work on Redmine’s event notification system.

Reference commit