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

@@ -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

ee/changelogs/unreleased/sh-check-group-ldap-sync-validity.yml

+---
+title: Notify owner that group is invalid when LDAP "Sync now" fails
+merge_request: 10509
+author:
+type: fixed

ee/spec/controllers/groups/ldaps_controller_spec.rb

+# 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