From 5007c4e9178d2cf9149dd1bee8fa826799375b98 Mon Sep 17 00:00:00 2001
From: cmoussa1 <moussa1@llnl.gov>
Date: Tue, 22 Oct 2024 09:35:19 -0700
Subject: [PATCH] list-banks: use new common formatter

Problem: The list-banks command defines its own formatting for the
output of the command, but the Python bindings now has its own common
formatter and SQL utilities.

Edit the list-banks command to make use of the new formatter and
external Python utilities. Change the default for the --fields optional
argument to be None if it is not specified.
---
 .../fluxacct/accounting/bank_subcommands.py   | 50 +++++++++----------
 src/cmd/flux-account-service.py               |  3 +-
 src/cmd/flux-account.py                       |  8 ++-
 3 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/src/bindings/python/fluxacct/accounting/bank_subcommands.py b/src/bindings/python/fluxacct/accounting/bank_subcommands.py
index c5349c7b..6781867b 100644
--- a/src/bindings/python/fluxacct/accounting/bank_subcommands.py
+++ b/src/bindings/python/fluxacct/accounting/bank_subcommands.py
@@ -10,9 +10,11 @@
 # SPDX-License-Identifier: LGPL-3.0
 ###############################################################
 import sqlite3
-import json
 
+import fluxacct.accounting
 from fluxacct.accounting import user_subcommands as u
+from fluxacct.accounting import formatter as fmt
+from fluxacct.accounting import sql_util as sql
 
 ###############################################################
 #                                                             #
@@ -422,45 +424,39 @@ def edit_bank(
 def list_banks(
     conn,
     inactive=False,
-    fields=None,
+    cols=None,
+    table=False,
 ):
     """
-    List all banks in the bank_table in JSON format.
+    List all banks in bank_table.
 
     Args:
         inactive: whether to include inactive banks. By default, only banks that are
-        active will be included in the output.
-
-        fields: a list of fields to include in the output. By default, all fields are
-        included.
+            active will be included in the output.
+        cols: a list of columns from the table to include in the output. By default, all
+            columns are included.
+        table: output data in bank_table in table format. By default, the format of any
+            returned data is in JSON.
     """
-    default_fields = {"bank_id", "bank", "active", "parent_bank", "shares", "job_usage"}
-    # if fields is None, just use the default fields
-    fields = fields or default_fields
+    # use all column names if none are passed in
+    cols = cols or fluxacct.accounting.BANK_TABLE
 
     try:
         cur = conn.cursor()
 
-        # validate the fields passed in
-        invalid_fields = [field for field in fields if field not in default_fields]
-        if invalid_fields:
-            raise ValueError(f"invalid fields: {', '.join(invalid_fields)}")
-
+        sql.validate_columns(cols, fluxacct.accounting.BANK_TABLE)
         # construct SELECT statement
-        select_fields = ", ".join(fields)
-        select_stmt = f"SELECT {select_fields} FROM bank_table"
+        select_stmt = f"SELECT {', '.join(cols)} FROM bank_table"
         if not inactive:
             select_stmt += " WHERE active=1"
-
         cur.execute(select_stmt)
-        result = cur.fetchall()
-
-        # create individual object for each row in the query result
-        banks = [
-            {field: row[idx] for idx, field in enumerate(fields)} for row in result
-        ]
 
-        json_string = json.dumps(banks, indent=2)
-        return json_string
+        # initialize AccountingFormatter object
+        formatter = fmt.AccountingFormatter(cur)
+        if table:
+            return formatter.as_table()
+        return formatter.as_json()
     except sqlite3.Error as err:
-        raise sqlite3.Error(f"an sqlite3.Error occurred: {err}")
+        raise sqlite3.Error(f"list-banks: an sqlite3.Error occurred: {err}")
+    except ValueError as exc:
+        raise ValueError(f"list-banks: {exc}")
diff --git a/src/cmd/flux-account-service.py b/src/cmd/flux-account-service.py
index 01eff28e..50867b4d 100755
--- a/src/cmd/flux-account-service.py
+++ b/src/cmd/flux-account-service.py
@@ -331,7 +331,8 @@ def list_banks(self, handle, watcher, msg, arg):
             val = b.list_banks(
                 self.conn,
                 msg.payload["inactive"],
-                msg.payload["fields"].split(","),
+                msg.payload["fields"].split(",") if msg.payload.get("fields") else None,
+                msg.payload["table"],
             )
 
             payload = {"list_banks": val}
diff --git a/src/cmd/flux-account.py b/src/cmd/flux-account.py
index 7bcd3342..6301df6e 100755
--- a/src/cmd/flux-account.py
+++ b/src/cmd/flux-account.py
@@ -365,9 +365,15 @@ def add_list_banks_arg(subparsers):
         "--fields",
         type=str,
         help="list of fields to include in JSON output",
-        default="bank_id,bank,parent_bank,shares,job_usage",
+        default=None,
         metavar="BANK_ID,BANK,ACTIVE,PARENT_BANK,SHARES,JOB_USAGE",
     )
+    subparser_list_banks.add_argument(
+        "--table",
+        action="store_const",
+        const=True,
+        help="list all banks in table format",
+    )
 
 
 def add_update_usage_arg(subparsers):