From ed121db523c82161f4d7e8e47b4e7bb0770f31a3 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Sat, 30 Mar 2019 18:10:18 -0700
Subject: [PATCH] Notify owner that group is invalid when LDAP "Sync now" fails

If a group is in an invalid state, it will never transition properly to
the right LDAP sync state, leading to confusion as to what the problem
is. In the previous implementation, clicking on the "Sync now" button
would erronenously tell the owner, "The group sync is already
scheduled", when in fact, it was stuck in the "ready" state.

This would help diagnose
https://gitlab.com/gitlab-org/gitlab-ee/issues/1529 and
https://gitlab.com/gitlab-org/gitlab-ee/issues/9613.
---
 ee/app/controllers/groups/ldaps_controller.rb | 10 +++-
 .../sh-check-group-ldap-sync-validity.yml     |  5 ++
 .../groups/ldaps_controller_spec.rb           | 53 +++++++++++++++++++
 3 files changed, 67 insertions(+), 1 deletion(-)
 create mode 100644 ee/changelogs/unreleased/sh-check-group-ldap-sync-validity.yml
 create mode 100644 ee/spec/controllers/groups/ldaps_controller_spec.rb

diff --git a/ee/app/controllers/groups/ldaps_controller.rb b/ee/app/controllers/groups/ldaps_controller.rb
index e11c33d33cd13f..326379fbc1f82e 100644
--- a/ee/app/controllers/groups/ldaps_controller.rb
+++ b/ee/app/controllers/groups/ldaps_controller.rb
@@ -6,11 +6,19 @@ class Groups::LdapsController < Groups::ApplicationController
   before_action :check_enabled_extras!
 
   def sync
+    # A group can transition to pending if it is in the ready or failed
+    # state. If it is in the started or pending state, then that means
+    # it is already running. If the group doesn't validate, then it's
+    # likely the group will never transition out of its current state,
+    # so we should tell the group owner.
     if @group.pending_ldap_sync
       LdapGroupSyncWorker.perform_async(@group.id)
       message = 'The group sync has been scheduled'
-    else
+    elsif @group.valid?
       message = 'The group sync is already scheduled'
+    else
+      message = "This group is in an invalid state: #{@group.errors.full_messages.join(', ')}"
+      return redirect_to group_group_members_path(@group), alert: message
     end
 
     redirect_to group_group_members_path(@group), notice: message
diff --git a/ee/changelogs/unreleased/sh-check-group-ldap-sync-validity.yml b/ee/changelogs/unreleased/sh-check-group-ldap-sync-validity.yml
new file mode 100644
index 00000000000000..add6e62b3693e4
--- /dev/null
+++ b/ee/changelogs/unreleased/sh-check-group-ldap-sync-validity.yml
@@ -0,0 +1,5 @@
+---
+title: Notify owner that group is invalid when LDAP "Sync now" fails
+merge_request: 10509
+author:
+type: fixed
diff --git a/ee/spec/controllers/groups/ldaps_controller_spec.rb b/ee/spec/controllers/groups/ldaps_controller_spec.rb
new file mode 100644
index 00000000000000..5f7a6b16cce5b8
--- /dev/null
+++ b/ee/spec/controllers/groups/ldaps_controller_spec.rb
@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Groups::LdapsController do
+  include LdapHelpers
+
+  let(:group) { create(:group) }
+  let(:user)  { create(:user) }
+
+  before do
+    stub_ldap_setting(enabled: true)
+    group.add_owner(user)
+
+    sign_in(user)
+  end
+
+  describe 'POST #sync' do
+    subject do
+      Sidekiq::Testing.fake! do
+        post :sync, params: { group_id: group.to_param }
+      end
+    end
+
+    it 'transitions to the pending state' do
+      subject
+
+      expect(group.reload.ldap_sync_pending?).to be_truthy
+
+      expect(controller).to redirect_to(group_group_members_path(group))
+    end
+
+    it 'notifies user that the group is already pending' do
+      group.update_columns(ldap_sync_status: 'pending')
+
+      subject
+
+      expect(flash[:notice]).to eq('The group sync is already scheduled')
+      expect(controller).to redirect_to(group_group_members_path(group))
+    end
+
+    it 'returns an error if the group does not validate' do
+      group.update_columns(repository_size_limit: -1)
+
+      expect(group).not_to be_valid
+
+      subject
+
+      expect(flash[:alert]).to include('This group is in an invalid state')
+      expect(controller).to redirect_to(group_group_members_path(group))
+    end
+  end
+end
-- 
GitLab