Preserve existing users and leaders in NewRepo requests

modules/coactd.py

@@ -566,18 +566,63 @@ def do_new_repo(
                 self.logger.warning(f"Failed to create grouper POSIX group for {facility}:{repo}: {e}")
                 raise
 
+        # Query current users and leaders from database if repo already exists
+        repo_users = [principal]  # Default to just principal for new repos
+        repo_leaders = [principal]  # Default to principal as leader for new repos
+        repo_req = {'facility': facility, 'name': repo}
+        REPO_USERS_GQL = gql("""
+            query repo($facility: String!, $name: String!) {
+                repo(filter: {facility: $facility, name: $name}) {
+                    users
+                    leaders
+                }
+            }
+            """)
+        try:
+            repo_data = self.back_channel.execute(REPO_USERS_GQL, repo_req)
+
+            # repo: null means "not found" - use defaults
+            if repo_data.get('repo') is None:
+                self.logger.info(f"Repo {facility}:{repo} not found in database, creating new with principal only")
+            elif repo_data.get('repo'):
+                # Repo exists - preserve existing users and leaders
+                if repo_data['repo'].get('users'):
+                    repo_users = repo_data['repo']['users']
+                    self.logger.info(f"Found existing repo {facility}:{repo} with {len(repo_users)} users: {repo_users}")
+                    # Ensure principal is in the list
+                    if principal not in repo_users:
+                        repo_users.append(principal)
+
+                # Preserve existing leaders
+                if repo_data['repo'].get('leaders'):
+                    repo_leaders = repo_data['repo']['leaders']
+                    self.logger.info(f"Found existing repo {facility}:{repo} with {len(repo_leaders)} leaders: {repo_leaders}")
+                    # Ensure principal is in the leaders list
+                    if principal not in repo_leaders:
+                        repo_leaders.append(principal)
+        except Exception as e:
+            # Query failed due to other error - do not proceed
+            error_msg = f"Failed to query existing repo data for {facility}:{repo}: {e}"
+            self.logger.error(error_msg)
+            raise RuntimeError(
+                f"Cannot safely proceed with NewRepo: database query failed. "
+                f"Proceeding with defaults could overwrite existing users/leaders. "
+                f"Original error: {e}"
+            ) from e
+
         # run the facility tasks for this repo
         self.run_playbook(
             "coact/add_repo.yaml",
-            facility=facility, 
+            facility=facility,
             repo=repo,
             repo_principal=principal,
+            repo_users=repo_users,
             gidNumber=repo_gid,
             groupName=grouper_name
         )
 
-        leaders = [principal]
-        users = [principal]
+        leaders = repo_leaders
+        users = repo_users
 
         # write back to coact the repo information
         repo_create_req = {

tests/test_repo_registration_gid.py

@@ -59,6 +59,7 @@ def test_gid_extraction_success_cryoem(self, repo_registration: RepoRegistration
         # Mock extract_grouper_values to return the GID
         repo_registration.extract_grouper_values = Mock(return_value=('12345', 'sdf-cryoem-ct-test'))
         repo_registration.back_channel.execute.side_effect = [
+            {'repo': None},  # Query returns null (repo not found)
             {'repoUpsert': {'Id': 'repo-456'}},
             {'repoUpsertFeature': {'Id': 'feature-slurm'}},
             {'repoUpsertFeature': {'Id': 'feature-posix'}}
@@ -115,6 +116,7 @@ def test_non_grouper_facility_skips_gid(self, repo_registration, mock_ansible_ru
         # Setup
         repo_registration.run_playbook.return_value = mock_ansible_runner
         repo_registration.back_channel.execute.side_effect = [
+            {'repo': None},  # Query returns null (repo not found)
             {'repoUpsert': {'Id': 'repo-123'}},
             {'repoUpsertFeature': {'Id': 'feature-slurm'}}
         ]
@@ -130,16 +132,22 @@ def test_non_grouper_facility_skips_gid(self, repo_registration, mock_ansible_ru
         assert result is True
 
         # Only add_repo.yaml should run — grouper.yml should NOT be called
-        # The new implementation passes repo_principal, gidNumber=None, and groupName=''
+        # The new implementation passes repo_principal, repo_users, gidNumber=None, and groupName=''
         repo_registration.run_playbook.assert_called_once_with(
-            'coact/add_repo.yaml', facility='OTHER', repo='test-repo', repo_principal='test-user', gidNumber=None, groupName=''
+            'coact/add_repo.yaml',
+            facility='OTHER',
+            repo='test-repo',
+            repo_principal='test-user',
+            repo_users=['test-user'],  # New parameter added by idempotency fix
+            gidNumber=None,
+            groupName=''
         )
         # extract_grouper_values should not be called for non-grouper facilities
         if hasattr(repo_registration, 'extract_grouper_values') and isinstance(repo_registration.extract_grouper_values, Mock):
             repo_registration.extract_grouper_values.assert_not_called()
 
-        # Should only create slurm feature (2 back_channel calls: repoUpsert + feature)
-        assert repo_registration.back_channel.execute.call_count == 2
+        # Should only create slurm feature (3 back_channel calls: user query + repoUpsert + feature)
+        assert repo_registration.back_channel.execute.call_count == 3
 
     def test_all_cryoem_repos_use_grouper(self, repo_registration, mock_ansible_runner):
         """Test that ALL cryoem repos (not just ct/ce) unconditionally use grouper."""
@@ -148,6 +156,7 @@ def test_all_cryoem_repos_use_grouper(self, repo_registration, mock_ansible_runn
         # Mock extract_grouper_values to return a valid GID for any CryoEM repo
         repo_registration.extract_grouper_values = Mock(return_value=('54321', 'sdf-cryoem-other-repo'))
         repo_registration.back_channel.execute.side_effect = [
+            {'repo': None},  # Query returns null (repo not found)
             {'repoUpsert': {'Id': 'repo-123'}},
             {'repoUpsertFeature': {'Id': 'feature-slurm'}},
             {'repoUpsertFeature': {'Id': 'feature-posix'}}
@@ -176,5 +185,119 @@ def test_all_cryoem_repos_use_grouper(self, repo_registration, mock_ansible_runn
         repo_registration.logger.info.assert_any_call(
             "Retrieved repo GID for cryoem:other-repo: 54321"
         )
-        # Should create both slurm and posixgroup features (3 back_channel calls total)
-        assert repo_registration.back_channel.execute.call_count == 3
+        # Should create both slurm and posixgroup features (4 back_channel calls: user query + repoUpsert + 2 features)
+        assert repo_registration.back_channel.execute.call_count == 4
+
+
+class TestRepoIdempotency:
+    """Test idempotency fixes for NewRepo request workflow."""
+
+    @pytest.fixture
+    def repo_registration(self) -> RepoRegistration:
+        """Create a RepoRegistration instance with mocked dependencies."""
+        with patch('modules.coactd.GraphQlSubscriber.__init__'), \
+             patch('modules.coactd.AnsibleRunner.__init__'):
+            reg = RepoRegistration(
+                username='test-user',
+                password_file='/tmp/test-password',
+                client_name='test-client',
+                grouper_password_file='/tmp/test-grouper-password'
+            )
+            reg.logger = Mock()
+            reg.back_channel = Mock()
+            reg.run_playbook = Mock()
+            reg.extract_grouper_values = Mock(return_value=('12345', 'sdf-cryoem-test-repo'))
+            return reg
+
+    @pytest.fixture
+    def mock_ansible_runner(self) -> Mock:
+        """Mock ansible runner with realistic structure."""
+        runner = Mock()
+        runner.events = []
+        return runner
+
+    def test_fresh_newrepo_uses_principal_only(self, repo_registration: RepoRegistration, mock_ansible_runner: Mock):
+        """
+        When creating a brand new repo that doesn't exist in the database,
+        the playbook should receive only the principal user.
+        """
+        # Setup - repo doesn't exist in database
+        repo_registration.run_playbook.return_value = mock_ansible_runner
+        repo_registration.back_channel.execute.side_effect = [
+            {'repo': None},  # Normal "not found"
+            {'repoUpsert': {'Id': 'repo-123'}},  # Repo creation succeeds
+            {'repoUpsertFeature': {'Id': 'feature-slurm'}},
+            {'repoUpsertFeature': {'Id': 'feature-posix'}}
+        ]
+
+        # Execute
+        result = repo_registration.do_new_repo(
+            repo='test-new-1',
+            facility='cryoem',
+            principal='user1'
+        )
+
+        # Verify
+        assert result is True
+
+        # Check that the playbook was called with only the principal
+        playbook_call = [c for c in repo_registration.run_playbook.call_args_list
+                         if c[0][0] == 'coact/add_repo.yaml'][0]
+        assert playbook_call[1]['repo_users'] == ['user1']
+        assert playbook_call[1]['repo_principal'] == 'user1'
+
+        # Verify logging indicates repo not found
+        log_messages = [str(c) for c in repo_registration.logger.info.call_args_list]
+        assert any('not found in database' in msg for msg in log_messages)
+
+    def test_rerun_newrepo_preserves_existing_users_and_leaders(self, repo_registration: RepoRegistration, mock_ansible_runner: Mock):
+        """
+        Test complete idempotency: re-running NewRepo on an existing repo must preserve
+        both users and leaders lists, not reset them to just the principal.
+        """
+        # Setup - repo exists with 3 users and 2 leaders
+        repo_registration.run_playbook.return_value = mock_ansible_runner
+        repo_registration.back_channel.execute.side_effect = [
+            {
+                'repo': {
+                    'users': ['user1', 'user2', 'user3'],
+                    'leaders': ['user1', 'leader2']  # Multiple leaders
+                }
+            },
+            {'repoUpsert': {'Id': 'repo-123'}},
+            {'repoUpsertFeature': {'Id': 'feature-slurm'}},
+            {'repoUpsertFeature': {'Id': 'feature-posix'}}
+        ]
+
+        # Execute - re-run NewRepo for existing repo
+        result = repo_registration.do_new_repo(
+            repo='test-existing',
+            facility='cryoem',
+            principal='user1'
+        )
+
+        # Verify result
+        assert result is True
+
+        # Verify Ansible playbook received ALL existing users (for LDAP sync)
+        playbook_call = [c for c in repo_registration.run_playbook.call_args_list
+                         if c[0][0] == 'coact/add_repo.yaml'][0]
+        assert set(playbook_call[1]['repo_users']) == {'user1', 'user2', 'user3'}, \
+            "Ansible playbook must receive all users for LDAP sync"
+
+        # The second back_channel.execute call is the repoUpsert mutation
+        upsert_call = repo_registration.back_channel.execute.call_args_list[1]
+        upsert_data = upsert_call[0][1]  # Second argument is the variables
+
+        assert set(upsert_data['repo']['users']) == {'user1', 'user2', 'user3'}, \
+            f"Database write must preserve all users, not reset to principal! Got: {upsert_data['repo']['users']}"
+
+        assert set(upsert_data['repo']['leaders']) == {'user1', 'leader2'}, \
+            f"Database write must preserve all leaders, not reset to principal! Got: {upsert_data['repo']['leaders']}"
+
+        # Verify logging indicates existing data was found
+        log_messages = [str(c) for c in repo_registration.logger.info.call_args_list]
+        assert any('Found existing repo' in msg and '3 users' in msg for msg in log_messages), \
+            "Should log that existing users were found"
+        assert any('2 leaders' in msg for msg in log_messages), \
+            "Should log that existing leaders were found"