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.

I think the Concern is the best of the 4 options. Dedicated finder objects (#4) are nice, but introducing this whole new concept only makes sense if we plan to refactor (much) more of the code base into this direction.

However TimeLogController#find_time_entries checks for editable_by?, not visible? - so any extracted method would have to be parametrized somehow. I therefore lean towards keeping it simple by living with the slight duplication of params handling and 404/403 raising in the controllers and would maybe just try to push the duplicate finder logic down into the model (TimeEntry.find_with_preloads(ids) maybe?) where it could also easily be used by jobs / rake tasks.

The extraction of projects / project from the list of time_entries might be extracted on it's own - this would even fit into ApplicationController since it could potentially be used for arrays of other models that listen to project (I did not check if there are such cases at all).