From d4286ca9e3b1186b226928fc59f65dd6677e53a7 Mon Sep 17 00:00:00 2001
From: Simon Pichugin <spichugi@redhat.com>
Date: Thu, 26 Nov 2015 16:55:52 +0100
Subject: [PATCH] Ticket 48360 - Refactor the delete agreement function

Description: The delete agreement function of agreement.py module
doesn't cover all usage varieties and should be rewritten.

Now it requires suffix and replica(DirSrv object of the server
that the agreement points to). By some reasons, this replica object
may be missing, then we couldn't delete the agreement.

We don't have test coverage for that function.
It should be added to tests/agreement_test.py.

Also test_changes fails, fix is required.

Fix description: Change arguments of the delete agreement function to:
- suffix
- consumer_host - of the server that the agreement points to
- consumer_port - of the server that the agreement points to
or
- agmtdn - DN of the replica agreement

With that, we can clearly define the required agreement.

Add the delete test to tests/agreement_test.py

Also fix the changes test by adding time.sleep(2) before
change number checking, because there was not enough time
for updating change number on master.

https://fedorahosted.org/389/ticket/48360

Reviewed by: ?
---
 lib389/agreement.py     | 44 +++++++++++++++++++++++---------------------
 tests/agreement_test.py | 42 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 23 deletions(-)

diff --git a/lib389/agreement.py b/lib389/agreement.py
index 700f591..33dd130 100644
--- a/lib389/agreement.py
+++ b/lib389/agreement.py
@@ -546,40 +546,42 @@ class Agreement(object):
 
         return dn_agreement
 
-    def delete(self, suffix, replica):
-        '''
-            Delete a replication agreement
+        def delete(self, suffix=None, consumer_host=None, consumer_port=None,
+                   agmtdn=None):
+        """Delete a replication agreement
 
             @param suffix - the suffix that the agreement is configured for
-            @replica - DirSrv object of the server that the agreement points to
+            @param consumer_host - of the server that the agreement points to
+            @param consumer_port - of the server that the agreement points to
+            @param agmtdn - DN of the replica agreement
+
             @raise ldap.LDAPError - for ldap operation failures
-        '''
+            @raise TypeError - if too many agreements were found
+            @raise NoSuchEntryError - if no agreements were found
+        """
 
-        if replica.sslport:
-            # We assume that if you are using SSL, you are using it with
-            # replication
-            port = replica.sslport
-        else:
-            port = replica.port
+        if not (suffix and consumer_host and consumer_port) and not agmtdn:
+            raise InvalidArgumentError("Suffix with consumer_host and consumer_port" +
+                                       " or agmtdn are required")
+
+        agmts = self.list(suffix, consumer_host, consumer_port, agmtdn)
 
-        agmts = self.list(suffix=suffix, consumer_host=replica.host,
-                          consumer_port=port)
         if agmts:
             if len(agmts) > 1:
-                # Found too many agreements
-                self.log.error('Too many agreements found')
-                raise
+                raise TypeError('Too many agreements were found')
             else:
-                # delete the agreement
+                # Delete the agreement
                 try:
-                    self.conn.delete_s(agmts[0].dn)
+                    agmt_dn = agmts[0].dn
+                    self.conn.delete_s(agmt_dn)
+                    self.log.info('Agreement (%s) was successfully removed' %
+                                       agmt_dn)
                 except ldap.LDAPError as e:
                     self.log.error('Failed to delete agreement (%s), ' +
-                                   'error: %s' % (agmts[0].dn, str(e)))
+                                   'error: %s' % (agmt_dn, str(e)))
                     raise
         else:
-            # No agreements, error?
-            self.log.error('No agreements found')
+            raise NoSuchEntryError('No agreements were found')
 
     def init(self, suffix=None, consumer_host=None, consumer_port=None):
         """Trigger a total update of the consumer replica
diff --git a/tests/agreement_test.py b/tests/agreement_test.py
index 5b0af12..2390ea9 100644
--- a/tests/agreement_test.py
+++ b/tests/agreement_test.py
@@ -29,6 +29,7 @@ SERVERID_CONSUMER = 'consumer'
 
 SUFFIX = DEFAULT_SUFFIX
 ENTRY_DN = "cn=test_entry, %s" % SUFFIX
+SECOND_AGMT_TEST_PORT = 12345
 
 
 class TopologyReplication(object):
@@ -152,14 +153,15 @@ def test_list(topology):
         str(topology.consumer.port)
 
     # Create a second RA to check .list returns 2 RA
-    properties = {RA_NAME: r'meTo_%s:%d' % (topology.consumer.host, 12345),
+    properties = {RA_NAME: r'meTo_%s:%d' % (topology.consumer.host,
+                                            SECOND_AGMT_TEST_PORT),
                   RA_BINDDN: defaultProperties[REPLICATION_BIND_DN],
                   RA_BINDPW: defaultProperties[REPLICATION_BIND_PW],
                   RA_METHOD: defaultProperties[REPLICATION_BIND_METHOD],
                   RA_TRANSPORT_PROT: defaultProperties[REPLICATION_TRANSPORT]}
     topology.master.agreement.create(suffix=SUFFIX,
                                      host=topology.consumer.host,
-                                     port=12345,
+                                     port=SECOND_AGMT_TEST_PORT,
                                      properties=properties)
     ents = topology.master.agreement.list(suffix=SUFFIX)
     assert len(ents) == 2
@@ -175,6 +177,39 @@ def test_list(topology):
         str(topology.consumer.port)
 
 
+def test_delete(topology):
+    """Delete previously created the replica agreement
+
+    PREREQUISITE: it exists a replica for SUFFIX and a replica agreement
+    with a SECOND_AGMT_TEST_PORT specified
+    """
+
+    # TODO: add a few test cases for different sets of arguments
+    #       using fixtures and parameters.
+    topology.master.log.info("\n\n##############\n### DELETE\n##############\n")
+    ents = topology.master.agreement.list(suffix=SUFFIX,
+                                          consumer_host=topology.consumer.host,
+                                          consumer_port=SECOND_AGMT_TEST_PORT)
+    assert len(ents) == 1
+
+    # Find DN of the second agreement
+    replica_entries = topology.master.replica.list(SUFFIX)
+    replica = replica_entries[0]
+    agreement_cn = r'meTo_%s:%d' % (topology.consumer.host,
+                                    SECOND_AGMT_TEST_PORT)
+    agreement_dn = ','.join(["cn=%s" % agreement_cn, replica.dn])
+
+    topology.master.agreement.delete(suffix=SUFFIX,
+                                     consumer_host=topology.consumer.host,
+                                     consumer_port=SECOND_AGMT_TEST_PORT,
+                                     agmtdn=agreement_dn)
+
+    ents = topology.master.agreement.list(suffix=SUFFIX,
+                                          consumer_host=topology.consumer.host,
+                                          consumer_port=SECOND_AGMT_TEST_PORT)
+    assert not ents
+
+
 def test_status(topology):
     """Test that status is returned from agreement"""
 
@@ -311,6 +346,9 @@ def test_changes(topology):
         loop += 1
     assert loop <= 10
 
+    # Give a little time to update a change number on master
+    time.sleep(2)
+
     # Check change number
     newvalue = topology.master.agreement.changes(agmnt_dn=ents[0].dn)
     topology.master.log.info("\ntest_changes: %d changes\n" % newvalue)
-- 
2.5.0

