ankitkalia1195 / searchbot

0 stars 0 forks source link

Api::V1::SearchResultsController should use strong params #7

Open olivierobert opened 6 years ago

olivierobert commented 6 years ago

While Api::V1::SearchTasksController uses strong params: params.require(:search_task).permit(:name, :keywords_csv), the other APi controller does not.

In addition the params naming is a kind of cryptic:

params[:q]

Instead, it could be named fully as query.


In the params, this following is kind of hard to understand:

params[:q][:search_report_search_task_user_id_eq] = current_resource_owner.id

I am guessing this is required by ransack but it's a bit awkward and undocumented. A better way would be to move this to a Service Class to remove this complexity from the controller. If this does not makes sense, minor improvements like this could help:

class SearchResultsController < BaseController
      def index
        search_result = SearchResult.ransack(search_params).result.includes(search_report: :search_task)
        [...]
        render json: search_tasks, load_associations: true, search_report_ids: search_report_ids, 
               earch_result_ids: search_result_ids, include: ['search_reports', 'search_results']
      end

      private

      def search_params
        params.require(:query)
        append_ransack_params
      end

      def append_ransack_params
        params.merge(search_report_search_task_user_id_eq: current_resource_owner.id)
      end
end
ankitkalia1195 commented 6 years ago

SearchTasks controller has create action which is using strong parameters(so it is absolute necessity)...For index action and search it might or might not be required depending on the situtation, we can also use ransackable attributes feature of ransack to allow ransack for only particular attributes. Altough i agree minor code refractoring can be done to make this action look more readable like using before_action :modify_params.