Skip to content
Snippets Groups Projects

WIP: Testing raising an error on IssuableFinder invalid params

Open James EJ requested to merge jej-csv-export-filter-raise-error-test into master

Just a CI test, to see where we pass in invalid params from

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
146 146 end
147 147
148 148 def export_csv
149 csv_params = filter_params.permit(IssuableFinder::VALID_PARAMS)
149 csv_params = IssuableFinder.valid_params(filter_params)
  • actually, should we do the permit here, in the controller, instead of some random class? It feels like it's one of the very few things a controller should do 😆

  • That will mean undoing this change...

  • Author Maintainer

    Could have here filter_params.permit(IssuableFinder::VALID_PARAMS, IssuableFinder::VALID_ARRAY_PARAMS), but this would need to be repeated in the view where we filter out params such as controller. Might be another way to avoid that though.

  • you can have a method here called csv_params and call that in the view with @csv_params or a helper....

    Edited by James Lopez
  • Author Maintainer

    What about using query_parameters unfiltered in the view, since they will be filtered here anyway? Could then move valid_params to IssuableCollections where there is already a filter_params method.

  • I think that should be okay too 👍

  • Author Maintainer

    Resolved (but there is no resolve button)

  • Please register or sign in to reply
  • James Lopez
    James Lopez @jameslopez started a thread on commit 64ec68f6
  • 21 21 class IssuableFinder
    22 22 NONE = '0'.freeze
    23 23 VALID_PARAMS = %i(scope state group_id project_id milestone_title assignee_id search label_name sort assignee_username author_id author_username authorized_only due_date iids non_archived weight).freeze
  • James EJ mentioned in merge request !1444 (merged)

    mentioned in merge request !1444 (merged)

  • Please register or sign in to reply
    Loading