Merge pull request #573 from cmoussa1/issue#571-banks

`delete-bank`: add `--force` option to actually remove a row from the `bank_table`

src/bindings/python/fluxacct/accounting/bank_subcommands.py

@@ -171,11 +171,27 @@ def view_bank(conn, bank, tree=False, users=False, parsable=False, cols=None):
         raise ValueError(f"view-bank: {exc}")
 
 
-def delete_bank(conn, bank):
+def delete_bank(conn, bank, force=False):
+    """
+    Deactivate a bank row in the bank_table by setting its 'active' status to 0.
+    If force=True, actually remove the bank row from the bank_table. If the bank contains
+    multiple sub-banks and associations, either disable or actually remove those rows as
+    well.
+
+    Args:
+        conn: The SQLite Connection object
+        bank: the name of the bank
+        force: an option to actually remove the row from the bank_table instead of
+            just setting the 'active' column to 0.
+    """
     cursor = conn.cursor()
+    if force:
+        sql_stmt = "DELETE FROM bank_table WHERE bank=?"
+    else:
+        sql_stmt = "UPDATE bank_table SET active=0 WHERE bank=?"
 
     try:
-        cursor.execute("UPDATE bank_table SET active=0 WHERE bank=?", (bank,))
+        cursor.execute(sql_stmt, (bank,))
 
         # helper function to traverse the bank table and disable all of its sub banks
         def get_sub_banks(bank):
@@ -190,13 +206,13 @@ def get_sub_banks(bank):
                     FROM association_table WHERE bank=?
                     """
                 for assoc_row in cursor.execute(select_assoc_stmt, (bank,)):
-                    u.delete_user(conn, username=assoc_row[0], bank=assoc_row[1])
+                    u.delete_user(
+                        conn, username=assoc_row[0], bank=assoc_row[1], force=force
+                    )
             # else, disable all of its sub banks and continue traversing
             else:
                 for row in result:
-                    cursor.execute(
-                        "UPDATE bank_table SET active=0 WHERE bank=?", (row[0],)
-                    )
+                    cursor.execute(sql_stmt, (row[0],))
                     get_sub_banks(row[0])
 
         get_sub_banks(bank)

src/cmd/flux-account-service.py

@@ -301,7 +301,7 @@ def add_bank(self, handle, watcher, msg, arg):
 
     def delete_bank(self, handle, watcher, msg, arg):
         try:
-            val = b.delete_bank(self.conn, msg.payload["bank"])
+            val = b.delete_bank(self.conn, msg.payload["bank"], msg.payload["force"])
 
             payload = {"delete_bank": val}
 

src/cmd/flux-account.py

@@ -387,6 +387,17 @@ def add_delete_bank_arg(subparsers):
         help="bank name",
         metavar="BANK",
     )
+    subparser_delete_bank.add_argument(
+        "--force",
+        action="store_const",
+        const=True,
+        default=False,
+        help=(
+            "actually remove bank from bank_table (WARNING: removing a row from "
+            "the bank_table can affect a bank and its users' fair-share value; "
+            "proceed with caution)"
+        ),
+    )
 
 
 def add_edit_bank_arg(subparsers):

t/python/t1003_bank_cmds.py

@@ -153,6 +153,23 @@ def test_12_reactivate_bank(self):
         rows = cur.fetchall()
         self.assertEqual(rows[0][0], 1)
 
+    # actually remove a bank row from the bank_table
+    def test_13_delete_bank_force(self):
+        b.delete_bank(acct_conn, bank="G", force=True)
+        cur.execute("SELECT * FROM bank_table WHERE bank='G'")
+        rows = cur.fetchall()
+        self.assertEqual(len(rows), 0)
+
+    # actually delete multiple banks from bank_table by deleting a parent bank
+    def test_14_delete_parent_bank_force(self):
+        b.delete_bank(acct_conn, bank="C", force=True)
+        cur.execute("SELECT * FROM bank_table WHERE bank='C'")
+        rows = cur.fetchall()
+        self.assertEqual(len(rows), 0)
+        cur.execute("SELECT * FROM bank_table WHERE parent_bank='C'")
+        rows = cur.fetchall()
+        self.assertEqual(len(rows), 0)
+
     # remove database and log file
     @classmethod
     def tearDownClass(self):

t/t1023-flux-account-banks.t

@@ -151,6 +151,24 @@ test_expect_success 'combining --tree with --fields does not work' '
     grep "tree option does not support custom formatting" error.out
 '
 
+test_expect_success 'delete a bank with --force; ensure users also get deleted' '
+	flux account delete-bank C --force &&
+	test_must_fail flux account view-bank C > nonexistent_bank.out 2>&1 &&
+	grep "view-bank: bank C not found in bank_table" nonexistent_bank.out &&
+	test_must_fail flux account view-user user5014 > nonexistent_user.out 2>&1 &&
+	grep "view-user: user user5014 not found in association_table" nonexistent_user.out
+'
+
+test_expect_success 'delete a bank with multiple sub-banks and users with --force' '
+	flux account delete-bank D --force &&
+	test_must_fail flux account view-bank E > bankE_noexist.out 2>&1 &&
+	grep "view-bank: bank E not found in bank_table" bankE_noexist.out &&
+	test_must_fail flux account view-bank F > bankF_noexist.out 2>&1 &&
+	grep "view-bank: bank F not found in bank_table" bankF_noexist.out &&
+	test_must_fail flux account view-user user5030 > nonexistent_user.out 2>&1 &&
+	grep "view-user: user user5030 not found in association_table" nonexistent_user.out
+'
+
 test_expect_success 'remove flux-accounting DB' '
 	rm $(pwd)/FluxAccountingTest.db
 '