Redmine

Beside moving to dedicated and namespaced sub-controllers, there is still one more issue to resolve, the duplicated code for find_* methods that are shared by multiple controllers. This can be observed in the patch, the method find_time_entries is very similar to TimelogController#find_time_entries.

Options to fix this duplicated code:

1. Each dedicated sub-controller inherits the controller. In our case, ContextMenus::TimeEntriesController inherits TimelogController. The logic from the ContextMenus::BaseController can be moved to a concern.

2. Move the find_* methods that are shared by multiple controllers to ApplicationController, but this bloats this controller which is already quite huge.

3. Move these methods to concerns:

app/controllers/concerns/time_entry_finder.rb:

app/controllers/concerns/time_entry_finder.rb
module TimeEntryFinder
  extend ActiveSupport::Concern
  private

  def find_time_entries
    @time_entries = TimeEntry.where(:id => params[:id] || params[:ids]).
      preload(:project => :time_entry_activities).
      preload(:user).to_a
    raise ActiveRecord::RecordNotFound if @time_entries.empty?
    raise Unauthorized unless @time_entries.all?(&:visible?)

    @projects = @time_entries.filter_map(&:project).uniq
    @project = @projects.first if @projects.size == 1
  end
end

Then, TimelogController and TimeEntriesController include this concern. The downside is that the finder method remains tightly coupled to the controller context and it cannot be easily reused in background jobs or rake tasks.

4. Move the find_* logic to dedicated objects, for example:

app/finders/time_entry_finder.rb

# app/finders/time_entry_finder.rb
class TimeEntryFinder
  class UnauthorizedError < StandardError; end

  def initialize(ids, user = User.current)
    @ids = Array(ids).reject(&:blank?)
    @user = user
  end

  # Executes the lookup with preloading and visibility authorization
  def find_visible!
    entries = TimeEntry.where(:id => @ids).
      preload(:project => :time_entry_activities).
      preload(:user).to_a

    raise ActiveRecord::RecordNotFound if entries.empty?
    raise UnauthorizedError unless entries.all? { |e| e.visible?(@user) }
    entries
  end
end

Any feedback is welcome.