Skip to content
Snippets Groups Projects

Allow event_filter to be set via GET/POST in addition to cookie

Closed Allow event_filter to be set via GET/POST in addition to cookie
muteor/gitlab-ce:1801-allow-event-filters-via-get-post into master
2 unresolved threads
Closed username-removed-642334 requested to merge muteor/gitlab-ce:1801-allow-event-filters-via-get-post into master
2 unresolved threads

What does this MR do?

Allows event_filter to be set via GET (and POST) in addition to cookie

Are there points in the code the reviewer needs to double check?

Why was this MR needed?

Make it easier to send filters.

What are the relevant issue numbers?

#1801 (moved)

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

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
CHANGELOG.md
1270 1270 - Fix skip_repo parameter being ignored when destroying a namespace
1271 1271 - Add all builds into stage/job dropdowns on builds page
1272 1272 - Change requests_profiles resource constraint to catch virtually any file
1273 - Allow event_filter to be set via GET/POST in addition to cookie (muteor)
  • username-removed-378947
    username-removed-378947 @adamniedzielski started a thread on the diff
    • app/controllers/application_controller.rb
    192 192 def event_filter
    193 193 # Split using comma to maintain backward compatibility Ex/ "filter1,filter2"
    194 194 filters = cookies['event_filter'].split(',')[0] if cookies['event_filter'].present?
    195 filters = params[:event_filter].split(',') if params[:event_filter].present?
    • username-removed-378947
      username-removed-378947 @adamniedzielski ·
      Contributor

      Why do we want to use split here? In the previous line we use split(',')[0] to get the first filter out of the comma separated list, but as the comment above explains, it's only for legacy reasons. It looks like EventFilter only accepts a single filter now. However, EventFilter code is really confusing right now. It accepts a string but it refers to it as params :disappointed:.

    • username-removed-642334
      username-removed-642334 @muteor ·
      Author Contributor

      You are right, I was assuming params would be an list... What do you think is best, update eventFilter to support multiple filters or only accept a single filter like cookies do?

    • username-removed-378947
      username-removed-378947 @adamniedzielski ·
      Contributor

      @muteor I'm sorry, I missed your comment. I think that we should only accept a single filter.

    • Please register or sign in to reply
  • username-removed-378947
    username-removed-378947 @adamniedzielski ·
    Contributor

    @muteor Thank you so much for getting back to this!

    I think that we should also pass these parameters when generating URLs to the RSS feeds. If we don't do that, basically nobody will discover this option.

  • username-removed-128633 added ~480950 label

    added ~480950 label

  • username-removed-128633
    username-removed-128633 @rymai ·
    Maintainer

    I'm closing this MR since it looks abandoned. It still can be used as a starting point if anyone else wants to take a stab at it!

  • username-removed-128633 closed

    closed

  • username-removed-128633 mentioned in issue #1801 (moved)

    mentioned in issue #1801 (moved)

  • username-removed-128633 mentioned in merge request !13342

    mentioned in merge request !13342

    • Please register or sign in to reply