Merge branch 'rp/improve-redirect-behavior' into 'master'

Retain all query parameters when adding environment parameter to metrics dashboard URL

See merge request gitlab-org/gitlab!40878

app/controllers/projects/metrics_dashboard_controller.rb

@@ -20,10 +20,11 @@ module Projects
       elsif default_environment
         redirect_to project_metrics_dashboard_path(
           project,
-          **permitted_params
-            .to_h
-            .symbolize_keys
-            .merge(environment: default_environment)
+          # Reverse merge the query parameters so that a query parameter named dashboard_path doesn't
+          # override the dashboard_path path parameter.
+          **permitted_params.to_h.symbolize_keys
+            .merge(environment: default_environment.id)
+            .reverse_merge(request.query_parameters.symbolize_keys)
         )
       else
         render 'projects/environments/empty_metrics'

config/routes/project.rb

@@ -25,7 +25,10 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do
       # Use this scope for all new project routes.
       scope '-' do
         get 'archive/*id', constraints: { format: Gitlab::PathRegex.archive_formats_regex, id: /.+?/ }, to: 'repositories#archive', as: 'archive'
-        get 'metrics(/:dashboard_path)(/:page)', constraints: { dashboard_path: /.+\.yml/, page: 'panel/new' },
+        # Since the page parameter can contain slashes (panel/new), use Rails'
+        # "Route Globbing" syntax (/*page) so that the route helpers do not encode
+        # the slash character.
+        get 'metrics(/:dashboard_path)(/*page)', constraints: { dashboard_path: /.+\.yml/, page: 'panel/new' },
           to: 'metrics_dashboard#show', as: :metrics_dashboard, format: false
 
         namespace :metrics, module: :metrics do

spec/requests/projects/metrics_dashboard_spec.rb

@@ -25,15 +25,16 @@ RSpec.describe 'Projects::MetricsDashboardController' do
     end
 
     it 'retains existing parameters when redirecting' do
-      get "#{dashboard_route(dashboard_path: '.gitlab/dashboards/dashboard_path.yml')}/panel/new"
-
-      expect(response).to redirect_to(
-        dashboard_route(
-          dashboard_path: '.gitlab/dashboards/dashboard_path.yml',
-          page: 'panel/new',
-          environment: environment
-        )
-      )
+      params = {
+        dashboard_path: '.gitlab/dashboards/dashboard_path.yml',
+        page: 'panel/new',
+        group: 'System metrics (Kubernetes)',
+        title: 'Memory Usage (Pod average)',
+        y_label: 'Memory Used per Pod (MB)'
+      }
+      send_request(params)
+
+      expect(response).to redirect_to(dashboard_route(params.merge(environment: environment.id)))
     end
 
     context 'with anonymous user and public dashboard visibility' do
@@ -110,15 +111,13 @@ RSpec.describe 'Projects::MetricsDashboardController' do
 
   describe 'GET :/namespace/:project/-/metrics/:page' do
     it 'returns 200 with path param page' do
-      # send_request(page: 'panel/new') cannot be used because it encodes '/'
-      get "#{dashboard_route}/panel/new?environment=#{environment.id}"
+      send_request(page: 'panel/new', environment: environment.id)
 
       expect(response).to have_gitlab_http_status(:ok)
     end
 
     it 'returns 200 with dashboard and path param page' do
-      # send_request(page: 'panel/new') cannot be used because it encodes '/'
-      get "#{dashboard_route(dashboard_path: 'dashboard.yml')}/panel/new?environment=#{environment.id}"
+      send_request(dashboard_path: 'dashboard.yml', page: 'panel/new', environment: environment.id)
 
       expect(response).to have_gitlab_http_status(:ok)
     end