Skip to content
Snippets Groups Projects
Commit d0e0924b authored by Mark Chao's avatar Mark Chao :island:
Browse files

Merge branch 'fix_topic_path' into 'master'

Handle non-ASCII characters for topic_explore_projects_path

See merge request !122970



Merged-by: Mark Chao's avatarMark Chao <mchao@gitlab.com>
Approved-by: Mark Chao's avatarMark Chao <mchao@gitlab.com>
Approved-by: default avatarAbdul Wadood <awadood@gitlab.com>
Reviewed-by: Mark Chao's avatarMark Chao <mchao@gitlab.com>
Co-authored-by: Thong Kuah's avatarThong Kuah <tkuah@gitlab.com>
parents 8ce48881 335c6b28
No related branches found
No related tags found
2 merge requests!122970Handle non-ASCII characters for topic_explore_projects_path,!119439Draft: Prevent file variable content expansion in downstream pipeline
Pipeline #894640554 failed
Pipeline: E2E Omnibus GitLab EE

#894679968

    Pipeline: GitLab

    #894653144

      Pipeline: E2E GDK

      #894648681

        ......@@ -113,7 +113,9 @@ def load_topics
        end
        def load_topic
        @topic = Projects::Topic.find_by_name_case_insensitive(params[:topic_name])
        topic_name = Feature.enabled?(:explore_topics_cleaned_path) ? URI.decode_www_form_component(params[:topic_name]) : params[:topic_name]
        @topic = Projects::Topic.find_by_name_case_insensitive(topic_name)
        end
        # rubocop: disable CodeReuse/ActiveRecord
        ......
        # frozen_string_literal: true
        module Projects
        module TopicsHelper
        # To ensure a route will always generate, we need to encode `topic_name`.
        # Otherwise, various pages will encounter `No route matches` error.
        #
        # This does mean some double encoding as Rails ActionDispatch also encodes
        # segments but that is OK
        #
        # Also, controllers that use `params[:topic_name]` will now need to perform
        # decode_www_form_component.
        def topic_explore_projects_cleaned_path(topic_name:)
        topic_name = URI.encode_www_form_component(topic_name) if Feature.enabled?(:explore_topics_cleaned_path)
        topic_explore_projects_path(topic_name: topic_name)
        end
        end
        end
        ......@@ -9,6 +9,7 @@ class Topic < ApplicationRecord
        validates :name, presence: true, length: { maximum: 255 }
        validates :name, uniqueness: { case_sensitive: false }, if: :name_changed?
        validate :validate_name_format, if: :name_changed?
        validates :title, presence: true, length: { maximum: 255 }, on: :create
        validates :description, length: { maximum: 1024 }
        ......@@ -62,6 +63,18 @@ def update_non_private_projects_counter(ids_before, ids_after, project_visibilit
        where(id: topics_to_decrement).where('non_private_projects_count > 0').update_counters(non_private_projects_count: -1) unless topics_to_decrement.empty?
        end
        end
        private
        def validate_name_format
        return if name.blank?
        # /\R/ - A linebreak: \n, \v, \f, \r \u0085 (NEXT LINE),
        # \u2028 (LINE SEPARATOR), \u2029 (PARAGRAPH SEPARATOR) or \r\n.
        return unless name =~ /\R/
        errors.add(:name, 'has characters that are not allowed')
        end
        end
        end
        ......
        ......@@ -6,7 +6,7 @@
        .gl-min-w-0.gl-flex-grow-1.gl-ml-3
        .title
        = link_to title, topic_explore_projects_path(topic_name: topic.name)
        = link_to title, topic_explore_projects_cleaned_path(topic_name: topic.name)
        %div
        = topic.name
        ......
        ......@@ -5,7 +5,7 @@
        %span.gl-p-2.gl-text-gray-500
        = _('Topics') + ':'
        - project.topics_to_show.each do |topic|
        - explore_project_topic_path = topic_explore_projects_path(topic_name: topic[:name])
        - explore_project_topic_path = topic_explore_projects_cleaned_path(topic_name: topic[:name])
        - if topic[:title].length > max_project_topic_length
        %a.gl-p-2.has-tooltip{ data: { container: "body" }, title: topic[:title], href: explore_project_topic_path, itemprop: 'keywords' }
        = gl_badge_tag truncate(topic[:title], length: max_project_topic_length)
        ......@@ -18,7 +18,7 @@
        - content = capture do
        %span.gl-display-inline-flex.gl-flex-wrap
        - project.topics_not_shown.each do |topic|
        - explore_project_topic_path = topic_explore_projects_path(topic_name: topic[:name])
        - explore_project_topic_path = topic_explore_projects_cleaned_path(topic_name: topic[:name])
        - if topic[:title].length > max_project_topic_length
        %a.gl-mr-3.gl-mb-3.has-tooltip{ data: { container: "body" }, title: topic[:title], href: explore_project_topic_path, itemprop: 'keywords' }
        = gl_badge_tag truncate(topic[:title], length: max_project_topic_length)
        ......
        - max_topic_title_length = 30
        - detail_page_link = topic_explore_projects_path(topic_name: topic.name)
        - detail_page_link = topic_explore_projects_cleaned_path(topic_name: topic.name)
        .col-lg-3.col-md-4.col-sm-12
        = render Pajamas::CardComponent.new(card_options: { class: 'gl-mb-5' },
        ......
        ---
        name: explore_topics_cleaned_path
        introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/122970
        rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/414793
        milestone: '16.1'
        type: development
        group: group::tenant scale
        default_enabled: false
        # frozen_string_literal: true
        require 'spec_helper'
        RSpec.describe Projects::TopicsHelper, feature_category: :groups_and_projects do
        describe '#topic_explore_projects_cleaned_path' do
        using RSpec::Parameterized::TableSyntax
        where(:topic_name, :expected_path) do
        [
        %w[cat /explore/projects/topics/cat],
        %w[cat🐈emoji /explore/projects/topics/cat%25F0%259F%2590%2588emoji],
        %w[cat/mouse /explore/projects/topics/cat%252Fmouse],
        ['cat space', '/explore/projects/topics/cat+space']
        ]
        end
        with_them do
        subject(:path) { topic_explore_projects_cleaned_path(topic_name: topic_name) }
        it { is_expected.to eq(expected_path) }
        end
        context 'when explore_topics_cleaned_path feature flag is disabled' do
        before do
        stub_feature_flags(explore_topics_cleaned_path: false)
        end
        it 'does no cleaning' do
        expect(topic_explore_projects_cleaned_path(topic_name: 'cat/mouse'))
        .to eq('/explore/projects/topics/cat%2Fmouse')
        end
        end
        end
        end
        ......@@ -21,12 +21,17 @@
        end
        describe 'validations' do
        let(:name_format_message) { 'has characters that are not allowed' }
        it { is_expected.to validate_presence_of(:name) }
        it { is_expected.to validate_uniqueness_of(:name).case_insensitive }
        it { is_expected.to validate_length_of(:name).is_at_most(255) }
        it { is_expected.to validate_length_of(:description).is_at_most(1024) }
        it { expect(Projects::Topic.new).to validate_presence_of(:title) }
        it { expect(Projects::Topic.new).to validate_length_of(:title).is_at_most(255) }
        it { is_expected.not_to allow_value("new\nline").for(:name).with_message(name_format_message) }
        it { is_expected.not_to allow_value("new\rline").for(:name).with_message(name_format_message) }
        it { is_expected.not_to allow_value("new\vline").for(:name).with_message(name_format_message) }
        end
        describe 'scopes' do
        ......
        0% Loading or .
        You are about to add 0 people to the discussion. Proceed with caution.
        Finish editing this message first!
        Please register or to comment