Merge branch 'xanf-improve-fork-page-design' into 'master'

Improve fork page design

See merge request gitlab-org/gitlab!35592

app/assets/javascripts/pages/projects/forks/new/components/fork_groups_list_item.vue

@@ -35,7 +35,7 @@ export default {
     },
   },
   data() {
-    return { namespaces: null };
+    return { namespaces: null, isForking: false };
   },
 
   computed: {
@@ -67,6 +67,13 @@ export default {
     },
   },
 
+  methods: {
+    fork() {
+      this.isForking = true;
+      this.$refs.form.submit();
+    },
+  },
+
   i18n: {
     hasReachedProjectLimitMessage: __('You have reached your project limit'),
     insufficientPermissionsMessage: __(
@@ -124,14 +131,17 @@ export default {
           >
           <template v-else>
             <div ref="selectButtonWrapper">
-              <form method="POST" :action="group.fork_path">
+              <form ref="form" method="POST" :action="group.fork_path">
                 <input type="hidden" name="authenticity_token" :value="$options.csrf.token" />
                 <gl-button
                   type="submit"
-                  class="gl-h-7 gl-text-decoration-none!"
+                  class="gl-h-7"
                   :data-qa-name="group.full_name"
+                  category="secondary"
                   variant="success"
                   :disabled="isSelectButtonDisabled"
+                  :loading="isForking"
+                  @click="fork"
                   >{{ __('Select') }}</gl-button
                 >
               </form>

app/assets/javascripts/pages/projects/forks/new/index.js

-import ProjectFork from '~/project_fork';
+import Vue from 'vue';
+import { parseBoolean } from '~/lib/utils/common_utils';
+import ForkGroupsList from './components/fork_groups_list.vue';
 
-document.addEventListener('DOMContentLoaded', () => new ProjectFork());
+document.addEventListener('DOMContentLoaded', () => {
+  const mountElement = document.getElementById('fork-groups-mount-element');
+
+  const { endpoint, canCreateProject } = mountElement.dataset;
+
+  const hasReachedProjectLimit = !parseBoolean(canCreateProject);
+
+  return new Vue({
+    el: mountElement,
+    render(h) {
+      return h(ForkGroupsList, {
+        props: {
+          endpoint,
+          hasReachedProjectLimit,
+        },
+      });
+    },
+  });
+});

app/assets/javascripts/project_fork.js

-import $ from 'jquery';
-
-export default () => {
-  $('.js-fork-thumbnail').on('click', function forkThumbnailClicked() {
-    if ($(this).hasClass('disabled')) return false;
-
-    return $('.js-fork-content').toggleClass('hidden');
-  });
-};

app/assets/stylesheets/pages/projects.scss

@@ -472,18 +472,10 @@
     margin-right: auto;
   }
 
-  a {
-    display: block;
-    width: 100%;
-    height: 100%;
-    padding-top: $gl-padding;
-    text-decoration: none;
-
-    &.disabled {
+  a.disabled {
     opacity: 0.3;
     cursor: not-allowed;
   }
-  }
 }
 
 .fork-thumbnail-container {
@@ -1538,3 +1530,10 @@ pre.light-well {
     }
   }
 }
+
+@include media-breakpoint-down(xs) {
+  .fork-filtered-search {
+    width: 100%;
+    margin: $gl-spacing-scale-2 0;
+  }
+}

app/controllers/projects/forks_controller.rb

@@ -36,7 +36,19 @@ class Projects::ForksController < Projects::ApplicationController
   end
 
   def new
-    @namespaces = fork_service.valid_fork_targets - [project.namespace]
+    respond_to do |format|
+      format.html do
+        @own_namespace = current_user.namespace if fork_service.valid_fork_targets.include?(current_user.namespace)
+        @project = project
+      end
+
+      format.json do
+        namespaces = fork_service.valid_fork_targets - [current_user.namespace, project.namespace]
+        render json: {
+          namespaces: ForkNamespaceSerializer.new.represent(namespaces, project: project, current_user: current_user)
+        }
+      end
+    end
   end
 
   # rubocop: disable CodeReuse/ActiveRecord

app/services/projects/fork_service.rb

@@ -14,10 +14,10 @@ module Projects
       @valid_fork_targets ||= ForkTargetsFinder.new(@project, current_user).execute
     end
 
-    def valid_fork_target?
+    def valid_fork_target?(namespace = target_namespace)
       return true if current_user.admin?
 
-      valid_fork_targets.include?(target_namespace)
+      valid_fork_targets.include?(namespace)
     end
 
     private

app/views/projects/forks/_fork_button.html.haml

 - avatar = namespace_icon(namespace, 100)
 - can_create_project = current_user.can?(:create_projects, namespace)
 
-- if forked_project = namespace.find_fork_of(@project)
-  .bordered-box.fork-thumbnail.text-center.gl-ml-3.gl-mr-3.gl-mt-3.gl-mb-3.forked
-    = link_to project_path(forked_project) do
+.bordered-box.fork-thumbnail.text-center.gl-m-3{ class: ("disabled" unless can_create_project) }
   - if /no_((\w*)_)*avatar/.match(avatar)
     = group_icon(namespace, class: "avatar rect-avatar s100 identicon mx-auto")
   - else
-        .avatar-container.s100.mx-auto
+    .avatar-container.s100.mx-auto.gl-mt-5
       = image_tag(avatar, class: "avatar s100")
   %h5.gl-mt-3
     = namespace.human_name
-- else
-  .bordered-box.fork-thumbnail.text-center.gl-ml-3.gl-mr-3.gl-mt-3.gl-mb-3{ class: ("disabled" unless can_create_project) }
-    = link_to project_forks_path(@project, namespace_key: namespace.id),
-        method: "POST",
-        class: ("disabled has-tooltip" unless can_create_project),
-        title: (_('You have reached your project limit') unless can_create_project) do
-      - if /no_((\w*)_)*avatar/.match(avatar)
-        = group_icon(namespace, class: "avatar rect-avatar s100 identicon mx-auto")
+  - if forked_project = namespace.find_fork_of(@project)
+    = link_to _("Go to project"), project_path(forked_project), class: "btn"
   - else
-        .avatar-container.s100.mx-auto
-          = image_tag(avatar, class: "avatar s100")
-      %h5.gl-mt-3{ data: { qa_selector: 'fork_namespace_content', qa_name: namespace.human_name } }
-        = namespace.human_name
+    %div{ class: ('has-tooltip' unless can_create_project),
+      title: (_('You have reached your project limit') unless can_create_project) }
+      = link_to _("Select"), project_forks_path(@project, namespace_key: namespace.id),
+          data: { qa_selector: 'fork_namespace_button', qa_name: namespace.human_name },
+          method: "POST",
+          class: ["btn btn-success", ("disabled" unless can_create_project)]

app/views/projects/forks/new.html.haml

@@ -7,15 +7,10 @@
     %p
       = _("A fork is a copy of a project.<br />Forking a repository allows you to make changes without affecting the original project.").html_safe
   .col-lg-9
-    - if @namespaces.present?
+    - if @own_namespace.present?
       .fork-thumbnail-container.js-fork-content
         %h5.gl-mt-0.gl-mb-0.gl-ml-3.gl-mr-3
           = _("Select a namespace to fork the project")
-        - @namespaces.each do |namespace|
-          = render 'fork_button', namespace: namespace
-    - else
-      %strong
-        = _("No available namespaces to fork the project.")
-      %p.gl-mt-3
-        = _("You must have permission to create a project in a namespace before forking.")
+        = render 'fork_button', namespace: @own_namespace
+    #fork-groups-mount-element{ data: { endpoint: new_project_fork_path(@project, format: :json), can_create_project: current_user.can_create_project?.to_s } }
 

changelogs/unreleased/xanf-improve-fork-page-design.yml

+---
+title: Improved fork page design
+merge_request: 35592
+author:
+type: changed

doc/user/project/repository/forking_workflow.md

@@ -26,7 +26,7 @@ Forking a project is, in most cases, a two-step process.
    NOTE: **Note:**
    The project path must be unique within the namespace.
 
-   ![Choose namespace](img/forking_workflow_choose_namespace.png)
+   ![Choose namespace](img/forking_workflow_choose_namespace_v13_2.png)
 
 The fork is created. The permissions you have in the namespace are the permissions you will have in the fork.
 

locale/gitlab.pot

@@ -15713,9 +15713,6 @@ msgstr ""
 msgid "No available groups to fork the project."
 msgstr ""
 
-msgid "No available namespaces to fork the project."
-msgstr ""
-
 msgid "No branches found"
 msgstr ""
 

qa/qa/page/project/fork/new.rb

@@ -6,11 +6,11 @@ module QA
       module Fork
         class New < Page::Base
           view 'app/views/projects/forks/_fork_button.html.haml' do
-            element :fork_namespace_content
+            element :fork_namespace_button
           end
 
           def choose_namespace(namespace = Runtime::Namespace.path)
-            click_element(:fork_namespace_content, name: namespace)
+            click_element(:fork_namespace_button, name: namespace)
           end
         end
       end

spec/controllers/projects/forks_controller_spec.rb

@@ -162,9 +162,25 @@ RSpec.describe Projects::ForksController do
     end
 
     context 'when user is signed in' do
-      it 'responds with status 200' do
+      before do
         sign_in(user)
+      end
 
+      context 'when JSON requested' do
+        it 'responds with available groups' do
+          get :new,
+              format: :json,
+              params: {
+                namespace_id: project.namespace,
+                project_id: project
+              }
+
+          expect(json_response['namespaces'].length).to eq(1)
+          expect(json_response['namespaces'].first['id']).to eq(group.id)
+        end
+      end
+
+      it 'responds with status 200' do
         subject
 
         expect(response).to have_gitlab_http_status(:ok)

spec/features/projects/fork_spec.rb

@@ -15,7 +15,7 @@ RSpec.describe 'Project fork' do
   it 'allows user to fork project' do
     visit project_path(project)
 
-    expect(page).not_to have_css('a.disabled', text: 'Fork')
+    expect(page).not_to have_css('a.disabled', text: 'Select')
   end
 
   it 'disables fork button when user has exceeded project limit' do
@@ -40,7 +40,7 @@ RSpec.describe 'Project fork' do
         visit project_path(project)
 
         expect(page).to have_css('a', text: 'Fork')
-        expect(page).not_to have_css('a.disabled', text: 'Fork')
+        expect(page).not_to have_css('a.disabled', text: 'Select')
       end
 
       it 'renders new project fork page' do
@@ -116,7 +116,7 @@ RSpec.describe 'Project fork' do
     click_link 'Fork'
 
     page.within '.fork-thumbnail-container' do
-      click_link user.name
+      click_link 'Select'
     end
 
     expect(page).to have_content 'Forked from'
@@ -156,7 +156,7 @@ RSpec.describe 'Project fork' do
     click_link 'Fork'
 
     page.within '.fork-thumbnail-container' do
-      click_link user.name
+      click_link 'Select'
     end
 
     visit project_forks_path(project)
@@ -193,7 +193,7 @@ RSpec.describe 'Project fork' do
       click_link 'Fork'
 
       page.within '.fork-thumbnail-container' do
-        click_link user.name
+        click_link 'Select'
       end
 
       visit project_forks_path(project)
@@ -218,7 +218,7 @@ RSpec.describe 'Project fork' do
       click_link 'Fork'
 
       page.within '.fork-thumbnail-container' do
-        click_link user.name
+        click_link 'Select'
       end
 
       expect(page).to have_content "Name has already been taken"
@@ -232,39 +232,43 @@ RSpec.describe 'Project fork' do
       group.add_maintainer(user)
     end
 
-    it 'allows user to fork project to group or to user namespace' do
+    it 'allows user to fork project to group or to user namespace', :js do
       visit project_path(project)
+      wait_for_requests
 
       expect(page).not_to have_css('a.disabled', text: 'Fork')
 
       click_link 'Fork'
 
-      expect(page).to have_css('.fork-thumbnail', count: 2)
+      expect(page).to have_css('.fork-thumbnail')
+      expect(page).to have_css('.group-row')
       expect(page).not_to have_css('.fork-thumbnail.disabled')
     end
 
-    it 'allows user to fork project to group and not user when exceeded project limit' do
+    it 'allows user to fork project to group and not user when exceeded project limit', :js do
       user.projects_limit = 0
       user.save!
 
       visit project_path(project)
+      wait_for_requests
 
       expect(page).not_to have_css('a.disabled', text: 'Fork')
 
       click_link 'Fork'
 
-      expect(page).to have_css('.fork-thumbnail', count: 2)
       expect(page).to have_css('.fork-thumbnail.disabled')
+      expect(page).to have_css('.group-row')
     end
 
-    it 'links to the fork if the project was already forked within that namespace', :sidekiq_might_not_need_inline do
+    it 'links to the fork if the project was already forked within that namespace', :sidekiq_might_not_need_inline, :js do
       forked_project = fork_project(project, user, namespace: group, repository: true)
 
       visit new_project_fork_path(project)
+      wait_for_requests
 
-      expect(page).to have_css('div.forked', text: group.full_name)
+      expect(page).to have_css('.group-row a.btn', text: 'Go to fork')
 
-      click_link group.full_name
+      click_link 'Go to fork'
 
       expect(current_path).to eq(project_path(forked_project))
     end

spec/services/projects/fork_service_spec.rb

@@ -439,11 +439,12 @@ RSpec.describe Projects::ForkService do
   end
 
   describe '#valid_fork_target?' do
-    subject { described_class.new(project, user, params).valid_fork_target? }
-
     let(:project) { Project.new }
     let(:params) { {} }
 
+    context 'when target is not passed' do
+      subject { described_class.new(project, user, params).valid_fork_target? }
+
       context 'when current user is an admin' do
         let(:user) { build(:user, :admin) }
 
@@ -473,4 +474,37 @@ RSpec.describe Projects::ForkService do
         end
       end
     end
+
+    context 'when target is passed' do
+      let(:target) { create(:group) }
+
+      subject { described_class.new(project, user, params).valid_fork_target?(target) }
+
+      context 'when current user is an admin' do
+        let(:user) { build(:user, :admin) }
+
+        it { is_expected.to be_truthy }
+      end
+
+      context 'when current user is not an admin' do
+        let(:user) { create(:user) }
+
+        before do
+          allow(ForkTargetsFinder).to receive(:new).with(project, user).and_return(finder_mock)
+        end
+
+        context 'when target namespace is in valid fork targets' do
+          let(:finder_mock) { instance_double('ForkTargetsFinder', execute: [target]) }
+
+          it { is_expected.to be_truthy }
+        end
+
+        context 'when target namespace is not in valid fork targets' do
+          let(:finder_mock) { instance_double('ForkTargetsFinder', execute: [create(:group)]) }
+
+          it { is_expected.to be_falsey }
+        end
+      end
+    end
+  end
 end