Tag Archives: plugins

Daily Refactor #3: Behavior Change in the Bulk Time Entry patch

Today’s “refactoring” isn’t a refactoring in the classic sense, since it includes a behavior change. But if you squint your eyes enough to only see the Controller as the “public interface”, then we can call it a refactor.

The Refactoring

Yesterday I moved the create_bulk_time_entry into the TimeEntry class where it fit better. Today I wanted to refine it more, since the Controller is still doing the actual save on the Model.

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
# bulk_time_entries_controller.rb
  def save
    if request.post? 
      @time_entries = params[:time_entries]
 
      render :update do |page|
        @time_entries.each_pair do |html_id, entry|
          @time_entry = TimeEntry.create_bulk_time_entry(entry)
          unless @time_entry && @time_entry.save
            page.replace "entry_#{html_id}", :partial => 'time_entry', :object => @time_entry
          else
            time_entry_target = if @time_entry.issue
                                  "#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
                                else
                                  "#{h(@time_entry.project.name)}"
                                end
            page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count =&gt; @time_entry.hours, :target =&gt; time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
          end
        end
      end
    end    
  end
 
# time_entry_patch.rb
def create_bulk_time_entry(entry)
  return false unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
  time_entry = TimeEntry.new(entry)
  time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours &lt;= 0
  time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
  time_entry.user = User.current
  time_entry
end

After

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
33
34
# bulk_time_entries_controller.rb
  def save
    if request.post? 
      @time_entries = params[:time_entries]
 
      render :update do |page|
        @time_entries.each_pair do |html_id, entry|
          @time_entry = TimeEntry.create_bulk_time_entry(entry)
          if @time_entry.new_record?
            page.replace "entry_#{html_id}", :partial =&gt; 'time_entry', :object =&gt; @time_entry
          else
            time_entry_target = if @time_entry.issue
                                  "#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
                                else
                                  "#{h(@time_entry.project.name)}"
                                end
            page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count =&gt; @time_entry.hours, :target =&gt; time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
          end
        end
      end
    end    
  end
 
# time_entry_patch.rb
def create_bulk_time_entry(entry)
  time_entry = TimeEntry.new(entry)
  time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours &lt;= 0
  if BulkTimeEntriesController.allowed_project?(entry[:project_id])
    time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
  end
  time_entry.user = User.current
  time_entry.save
  time_entry
end

Review

The big behavior change is that instead of returning false or an unsaved TimeEntry, the method will now return a TimeEntry (saved if it’s valid). This is a better design, since the caller (Controller) will not have to worry about the different types of objects that are returned. It will still need to check that the TimeEntry was saved, but the code is a lot easier to understand (if @time_entry.new_record?).

This refactoring really cleaned up the major flaws in the model method. I think it’s now time to fix the controller/view interaction now, especially the inline RJS rendering.

Reference commit.

Daily Refactor #2: Move Method in the BulkTimeEntriesController – saving a new TimeEntry

The Refactoring

Following up to yesterday’s refactoring, today I kept refactoring the Bulk Time Entry plugin’s controller. The save_time_entry_from_params method I extracted yesterday only uses the TimeEntry class, so it would be perfect to move the method to the TimeEntry class. Since the TimeEntry class is defined by Redmine, I had to monkey-patch that class. But the main content of this refactor is below:

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
33
  def save
    if request.post? 
      @time_entries = params[:time_entries]
 
      render :update do |page|
        @time_entries.each_pair do |html_id, entry|
          @time_entry = save_time_entry_from_params(entry)
          unless @time_entry.save
            page.replace "entry_#{html_id}", :partial =&gt; 'time_entry', :object =&gt; @time_entry
          else
            time_entry_target = if @time_entry.issue
                                  "#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
                                else
                                  "#{h(@time_entry.project.name)}"
                                end
            page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count =&gt; @time_entry.hours, :target =&gt; time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
          end
        end
      end
    end    
  end
 
# ...
 
  def save_time_entry_from_params(entry)
    next unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
    time_entry = TimeEntry.new(entry)
    time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours &lt;= 0
    time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
    time_entry.user = User.current
    time_entry
  end
  helper_method :save_time_entry_from_params

After

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
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
# bulk_time_entries_controller.rb
  def save
    if request.post? 
      @time_entries = params[:time_entries]
 
      render :update do |page|
        @time_entries.each_pair do |html_id, entry|
          @time_entry = TimeEntry.create_bulk_time_entry(entry)
          unless @time_entry &amp;&amp; @time_entry.save
            page.replace "entry_#{html_id}", :partial =&gt; 'time_entry', :object =&gt; @time_entry
          else
            time_entry_target = if @time_entry.issue
                                  "#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
                                else
                                  "#{h(@time_entry.project.name)}"
                                end
            page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count =&gt; @time_entry.hours, :target =&gt; time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
          end
        end
      end
    end    
  end
 
# time_entry_patch.rb
module BulkTimeEntryPlugin
  module Patches
    module TimeEntryPatch
      def self.included(base)
        base.extend ClassMethods
      end
 
      module ClassMethods
 
        def create_bulk_time_entry(entry)
          return false unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
          time_entry = TimeEntry.new(entry)
          time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours &lt;= 0
          time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
          time_entry.user = User.current
          time_entry
        end
 
      end
 
    end
  end
end

Few things to note in this refactoring:

  1. I renamed the method to create_bulk_time_entry to better describe it’s behavior.
  2. I replaced the call to save_time_entry_from_params to TimeEntry.create_bulk_time_entry.
  3. I changed the method body to return false if the user isn’t allowed to log time to the project.
  4. I changed the controller to make sure the create_bulk_time_entry didn’t return false (user not allowed to access the project).

Review

This refactoring really cleaned up the model/controller interaction. There are still a few refactoring that can be done for the controller/view interaction now. But the TimeEntry patch could still use some work first, it’s very procedural and has it’s own code smells already.

In the reference commit I also included some unit tests for the refactoring and even happened to fix two small bugs.

Daily Refactor #1: Extract Method in the BulkTimeEntriesController – saving a new TimeEntry

I’m having two problems right now:

  1. Not enough time to write to my blogs
  2. Not enough time to keep my Redmine plugins maintained

To try and fix both of these problems, I’m going to try and start something new here: a daily refactoring. I’m going do a small refactoring to my Redmine plugins or Redmine each day and post my thoughts about it here.

The Refactoring

I was working on my Bulk Time Entry plugin earlier today and found some very bad code in a controller.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
  def save
    if request.post? 
      @time_entries = params[:time_entries]
 
      render :update do |page|
        @time_entries.each_pair do |html_id, entry|
          next unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
          @time_entry = TimeEntry.new(entry)
          @time_entry.hours = nil if @time_entry.hours.blank? or @time_entry.hours  'time_entry', :object =&gt; @time_entry
          else
            time_entry_target = if @time_entry.issue
                                  "#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
                                else
                                  "#{h(@time_entry.project.name)}"
                                end
            page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count =&gt; @time_entry.hours, :target =&gt; time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
          end
        end
      end
    end    
  end

Without getting into all of the smells (e.g. rendering and building model objects in the controller), the one that really stood out to me was that it was creating and saving a new TimeEntries in a loop. Ideally, this would be done in a model method but first I needed to extract out all of the parts used. So I used extract method with gave me this:

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
33
  def save
    if request.post? 
      @time_entries = params[:time_entries]
 
      render :update do |page|
        @time_entries.each_pair do |html_id, entry|
          @time_entry = save_time_entry_from_params(entry)
          unless @time_entry.save
            page.replace "entry_#{html_id}", :partial =&gt; 'time_entry', :object =&gt; @time_entry
          else
            time_entry_target = if @time_entry.issue
                                  "#{h(@time_entry.project.name)} - #{h(@time_entry.issue.subject)}"
                                else
                                  "#{h(@time_entry.project.name)}"
                                end
            page.replace_html "entry_#{html_id}", "<div class='flash notice'>#{l(:text_time_added_to_project, :count =&gt; @time_entry.hours, :target =&gt; time_entry_target)}#{" (#{@time_entry.comments})" unless @time_entry.comments.blank?}.</div>"
          end
        end
      end
    end    
  end
 
# ...
 
  def save_time_entry_from_params(entry)
    next unless BulkTimeEntriesController.allowed_project?(entry[:project_id])
    time_entry = TimeEntry.new(entry)
    time_entry.hours = nil if time_entry.hours.blank? or time_entry.hours &lt;= 0
    time_entry.project_id = entry[:project_id] # project_id is protected from mass assignment
    time_entry.user = User.current
    time_entry
  end
  helper_method :save_time_entry_from_params

Review

This code still is a bit smelly, but it’s getting better. Now I can decide if I want to push save_time_entry_from_params to the model or clean up save‘s RJS rendering.

Reference commit

Redmine Plugin Dependencies

I’ve been doing a lot of integration work with Redmine lately and needed one Redmine plugin to be depend on another plugin. So yesterday I added a new method to the Redmine plugin API called requires_redmine_plugin. It’s a simple method that will make sure another Redmine plugin is installed.

Using the API is simple, just call required_redmine_plugin in your init.rb where you register your plugin. For example, if I wanted to make the budget_plugin require the redmine_rate plugin I would register the budget_plugin with:

1
2
3
4
5
6
7
8
9
Redmine::Plugin.register :budget_plugin do
  name 'Budget'
  author 'Eric Davis'
  version '0.2.0'
 
  requires_redmine :version_or_higher =&gt; '0.8.0'
 
  requires_redmine_plugin :redmine_rate, :version_or_higher =&gt; '0.1.0'
end

This follows the same format as the requires_redmine API so you can define the specific version in several ways:

  • Must have a specific version:
    1
    
    requires_redmine_plugin :redmine_rate, :version =&gt; '0.1.0'

    or

    1
    
    requires_redmine_plugin :redmine_rate, '0.1.0'
  • Must be at least a version:
    1
    
    requires_redmine_plugin :redmine_rate, :version_or_higher =&gt; '0.1.0'
  • May be one of a few versions:
    1
    
    requires_redmine_plugin :redmine_rate, :version =&gt; ['0.1.0', '0.1.1', '0.1.2']

I like to define the version requirements as :version_or_higher since it will let the plugin run on newer versions without any changes.

Plugin load order

There is one gotcha with this API right now. The required_redmine_plugin method runs as Redmine is loading plugins alphabetically. This means if your plugin is loaded before the dependent plugin, there is a chance your plugin will not find the dependent plugin. To work around this, you will need to change the load order for the plugins so that the dependent plugin is loaded first.

The example from above is a perfect example of this, since the budget_plugin will be loaded before the redmine_rate plugin. By adding the following line to the config/additional_environment.rb the redmine_rate plugin will be loaded first, before any other plugins:

1
config.plugins = [ :redmine_rate, :all ]

This new API will be included in Redmine 0.9, so you will be able to take advantage of it with the next major release of Redmine.

Question: Bug fix releases or release new plugins?

I recently asked a question on Twitter and I wanted to get some more feedback on here.


Redmine poll: Which would you prefer? Bug fix releases to my existing plugins or the release of new plugins that are complete?

To add a some context: I have about 20 Redmine plugins Open Sourced that are ready for another release. With Redmine 0.9 coming up, I’d like to do a final bugfix release of them for the final 0.8 branch and then start to require Redmine 0.9 from then on. But I also have another dozen or so unreleased plugins that need some final documentation and testing.

Let me know what you think in the comments below or on Twitter. Thanks.

Eric