Fix Paginating, Sorting, and Searching Issues Within "Research Outputs" Tab

[aaronskiba]

Fixes #935

Changes proposed in this PR:

• Add scope :search to ResearchOutput model: e289fba
  • Search query is performed by research_outputs.title (case-insensitive)
• Create app/controllers/paginable/research_outputs_controller.rb 3c5a478
  • This new controller fixes the following features on the /plans/:plan_id/research_outputs page:
    • The clickable pagination options (e.g. View all,1,2,Next,Last)
    • The search box (for research_outputs.title matches)
    • The clickable sorting arrows

[lagoan]

I just have some questions here to start a conversation. Sorry if I missed something.

[lagoan]

I see this authorize handling on app/controllers/research_outputs_controller.rb. Why are we creating a new ResearchOutput if we cannot find one ?

[aaronskiba]

In the case of app/controllers/research_outputs_controller.rb:
If I only use authorize @research_outputs.first, I encounter the following if the plan has no research_outputs:

Pundit::NotDefinedError - unable to find policy `NilClassPolicy` for `nil`:
  app/controllers/research_outputs_controller.rb:17:in `index'

If I use authorize @research_outputs.first if @research_outputs.first.present?, I encounter the following if the plan has no research_outputs:

Pundit::AuthorizationNotPerformedError at /plans/14224/research_outputs
ResearchOutputsController

However, it may be the case that there is always at least one research output when app/controllers/paginable/research_outputs_controller.rb is called. But it might still be safest to keep authorize @research_outputs.first || ResearchOutput.new(plan_id: @plan.id)? (I could at least add a comment explaining this a bit).

[lagoan]

A comment would be good. Also add the method call parenthesis to clarify the expected order.

[aaronskiba]

However, it may be the case that there is always at least one research output when app/controllers/paginable/research_outputs_controller.rb is called. But it might still be safest to keep authorize @research_outputs.first || ResearchOutput.new(plan_id: @plan.id)? (I could at least add a comment explaining this a bit).

I guess || ResearchOutput.new(plan_id: @plan.id) prevents Pundit::NotDefinedError when a direct GET /paginable/plans/:id/research_outputs request is made on a plan with 0 research_outputs.

[aaronskiba]

A comment would be good. Also add the method call parenthesis to clarify the expected order.

Cool, I made one more commit addressing this.

[lagoan]

Do we need @plan and @research_outputs to be instance variables?

[aaronskiba]

There is a corresponding app/views/paginable/research_outputs/_index.html.erb. That file has usage of @plan, but I think @research_outputs can be changed to research_outputs.

[lagoan]

Do we need to authorize by plan ? Something like

roadmap/app/controllers/paginable/contributors_controller.rb

Lines 14 to 15 in 60f89e6

	@plan = Plan.find_by(id: params[:plan_id])
	authorize @plan, :show?

We would need to add an application policy on app/policies/paginable/

[aaronskiba]

app/policies/research_output_policy.rb might be taking care of this. Here's a bit of that file's contents:

  def index?
    @research_output.plan.readable_by?(@user.id)
  end

  def new?
    @research_output.plan.administerable_by?(@user.id)
  end

  def edit?
    @research_output.plan.administerable_by?(@user.id)
  end

  def create?
    @research_output.plan.administerable_by?(@user.id)
  end

[lagoan]