diff --git a/tabcmd/commands/user/create_site_users.py b/tabcmd/commands/user/create_site_users.py index c63436b0..df881704 100644 --- a/tabcmd/commands/user/create_site_users.py +++ b/tabcmd/commands/user/create_site_users.py @@ -1,4 +1,5 @@ import tableauserverclient as TSC +from tableauserverclient.server.endpoint.exceptions import JobFailedException from tabcmd.commands.auth.session import Session from tabcmd.commands.constants import Errors @@ -25,6 +26,8 @@ def define_args(create_site_users_parser): set_users_file_positional(args_group) set_completeness_options(args_group) UserCommand.set_auth_arg(args_group) + set_no_wait_option(args_group) + set_silent_option(args_group) @classmethod def run_command(cls, args): @@ -32,42 +35,110 @@ def run_command(cls, args): logger.debug(_("tabcmd.launching")) session = Session() server = session.create_session(args, logger) - number_of_users_listed = 0 - number_of_users_added = 0 - number_of_errors = 0 creation_site = "current site" + # Pre-flight validation. Under --complete (default) any CSV-shape error + # aborts the whole run before submission -- matching Classic's + # with_transaction semantics as closely as we can without server support. + # Under --no-complete we validate leniently and let the server sort out + # remaining issues per-row. UserCommand.validate_file_for_import(args.filename, logger, detailed=True, strict=args.require_all_valid) - logger.info(_("addusers.status").format(args.filename.name, creation_site)) + if not args.silent_progress: + logger.info(_("addusers.status").format(args.filename.name, creation_site)) + user_obj_list = UserCommand.get_users_from_file(args.filename, logger) - logger.info(_("session.monitorjob.percent_complete").format(0)) - error_list = [] + if not user_obj_list: + logger.info(_("importcsvsummary.line.processed").format(0)) + return + + # Apply command-line overrides to every user object before submitting. for user_obj in user_obj_list: - try: - if args.role: - user_obj.site_role = args.role # tsc is case sensitive - if args.auth_type: - user_obj.auth_setting = args.auth_type - number_of_users_listed += 1 - result = server.users.add(user_obj) - logger.info(_("common.output.succeeded").format(user_obj.name)) - number_of_users_added += 1 - except TSC.ServerResponseError as e: - logger.debug(e) - if Errors.is_resource_conflict(e) and args.continue_if_exists: - logger.debug(_("createsite.errors.site_name_already_exists").format(user_obj.name)) - else: - logger.debug(type(e)) - number_of_errors += 1 - logger.debug(number_of_errors) - error_list.append(e.__class__.__name__) # + ": " + e.__cause__ or "Unknown") - logger.debug(error_list) - logger.info(_("session.monitorjob.percent_complete").format(100)) - logger.info(_("importcsvsummary.line.processed").format(number_of_users_listed)) - logger.info(_("importcsvsummary.line.skipped").format(number_of_errors)) - logger.info(_("importcsvsummary.users.added.count").format(number_of_users_added)) - if number_of_errors > 0: + if args.role: + user_obj.site_role = args.role # tsc is case sensitive + if args.auth_type: + user_obj.auth_setting = args.auth_type + + # Submit as a single bulk import job. Server returns a JobItem tracking + # the async processing on its side. + try: + job = server.users.bulk_add(user_obj_list) + except TSC.ServerResponseError as e: + Errors.exit_with_error(logger, exception=e) + return + + if not args.silent_progress: + logger.info(_("importcsvsummary.job.queued").format(job.id)) + + if args.nowait: + # Fire and forget. Server processes the job asynchronously; caller + # can query with `tabcmd get job/` or via the REST API. + return + + # Wait for the server-side job to finish. Under --silent-progress the + # framework's own debug logging is still emitted but we skip our own + # per-completion summary. + try: + job_done = server.jobs.wait_for_job(job_id=job.id, timeout=args.timeout) + except JobFailedException as je: + Errors.exit_with_error(logger, exception=je) + return + except Exception as e: + Errors.exit_with_error(logger, exception=e) + return + + if args.silent_progress: + return + + # Summarize per-row outcomes. `status_notes` is populated for + # UserImport jobs; each entry is a dict with keys type/value/text. + # The specific types the server emits are documented at + # https://help.tableau.com/current/api/rest_api/en-us/REST/rest_api_ref_jobs_tasks_and_schedules.htm#query_job + # (CountOfUsersAddedToSite, CountOfUsersSkipped, etc.). + # If the pinned TSC pre-dates status_notes we can't produce a truthful + # summary -- silently printing 0/0/len(input) would look like success + # regardless of what the server actually did. Fail loudly instead. + if not hasattr(job_done, "status_notes"): + Errors.exit_with_error( + logger, + message=_("createsiteusers.error.needs_newer_tsc").format(job.id), + ) + return + status_notes = job_done.status_notes or [] + summary_counts = {} + for note in status_notes: + note_type = note.get("type") + note_value = note.get("value") + if note_type and note_value is not None: + summary_counts[note_type] = note_value + + added = int(summary_counts.get("CountOfUsersAddedToSite", 0) or 0) + skipped = int(summary_counts.get("CountOfUsersSkipped", 0) or 0) + processed = int(summary_counts.get("CountOfUsersProcessed", len(user_obj_list)) or 0) + + # --continue-if-exists is a global flag; on other commands it downgrades + # 409 conflicts to INFO. bulk_add is inherently tolerant of duplicate + # users at the server level (they get counted under CountOfUsersSkipped) + # so the flag becomes a no-op here. Warn once so scripts porting from + # commands where it did work don't silently rely on it. + if getattr(args, "continue_if_exists", False): + logger.debug( + "--continue-if-exists is a no-op for createsiteusers: " + "bulk_add always tolerates existing users (see CountOfUsersSkipped)." + ) + + logger.info(_("importcsvsummary.line.processed").format(processed)) + logger.info(_("importcsvsummary.line.skipped").format(skipped)) + logger.info(_("importcsvsummary.users.added.count").format(added)) + + # Detailed per-row errors: any statusNote whose type isn't a + # CountOf* aggregate is likely a per-row message the user should see. + detail_notes = [n for n in status_notes if not (n.get("type") or "").startswith("CountOf")] + if detail_notes or job_done.notes: logger.info(_("importcsvsummary.error.details")) - logger.info(error_list) + for note in detail_notes: + text = note.get("text") or note.get("value") or "" + logger.info(f" {note.get('type', '?')}: {text}") + for text in job_done.notes or []: + logger.info(f" {text}") diff --git a/tabcmd/execution/global_options.py b/tabcmd/execution/global_options.py index 1a24133c..81897f8d 100644 --- a/tabcmd/execution/global_options.py +++ b/tabcmd/execution/global_options.py @@ -71,7 +71,10 @@ def set_users_file_positional(parser): def set_no_wait_option(parser): - parser.add_argument("--no-wait", action="store_true", help=_("common.options.nowait")) + # Matches tabcmd Classic's flag spelling (one word); no legacy callers to + # keep --no-wait working since this helper was never wired to a command + # before now. + parser.add_argument("--nowait", action="store_true", help=_("common.options.nowait")) return parser diff --git a/tabcmd/locales/en/shared_wg_en.properties b/tabcmd/locales/en/shared_wg_en.properties index 75328b3c..aaa49886 100644 --- a/tabcmd/locales/en/shared_wg_en.properties +++ b/tabcmd/locales/en/shared_wg_en.properties @@ -9,6 +9,7 @@ content_type.datasource=Data Source dataalerts.failure.error.workbookNotFound=Workbook not found dataconnections.classes.tableau_server_site=Tableau Server Site materializeviews.errors.project_path_not_exists=Project path ''{0}'' does not exist +importcsvsummary.job.queued=User import job queued on server. Job ID: {0} importcsvsummary.error.details=Error details: importcsvsummary.remainingerrors=(remaining errors not shown) importcsvsummary.error.line=line {0} for user ''{1}'': ''{2}'' diff --git a/tabcmd/locales/en/tabcmd_messages_en.properties b/tabcmd/locales/en/tabcmd_messages_en.properties index 7eda4bff..5bd858f2 100644 --- a/tabcmd/locales/en/tabcmd_messages_en.properties +++ b/tabcmd/locales/en/tabcmd_messages_en.properties @@ -22,6 +22,7 @@ createsite.options.user-quota=Maximum site users createsite.short_description=Create a site createsite.status=Create site ''{0}'' on the server... createsiteusers.short_description=Create users on the current site +createsiteusers.error.needs_newer_tsc=Job {0} submitted successfully but the installed tableauserverclient version does not expose per-user status; upgrade tableauserverclient to summarize the outcome. Query the job directly to see results. createusers.short_description=Create users on the server createusers.status=Adding users listed in {0} to the server decryptextracts.short_description=Decrypt extracts on a site diff --git a/tests/commands/test_create_site_users.py b/tests/commands/test_create_site_users.py new file mode 100644 index 00000000..366d4ae7 --- /dev/null +++ b/tests/commands/test_create_site_users.py @@ -0,0 +1,140 @@ +"""Behavior tests for createsiteusers after the switch to bulk_add. + +These are unit tests that mock the TSC layer -- they don't hit a real +server. They verify: +- default path: bulk_add called, then wait_for_job called +- --nowait: bulk_add called, wait_for_job NOT called +- --silent-progress: bulk_add + wait_for_job called, but summary log lines + are suppressed +- pre-flight validation aborts before bulk_add on malformed input under + --complete (default) +- per-row output derived from job.status_notes on the completed job +""" +import argparse +import io +import unittest +from unittest import mock + +import tableauserverclient as TSC + + +def _mock_open_csv(content): + """Return a mock args.filename that mimics a file with the given CSV content.""" + fp = io.StringIO(content) + fp.name = "users.csv" + return fp + + +def _base_args(**overrides): + ns = argparse.Namespace( + filename=_mock_open_csv("username,password,fullname,creator,none,yes,email\n"), + role=None, + auth_type=None, + require_all_valid=True, + continue_if_exists=False, + nowait=False, + silent_progress=False, + logging_level="INFO", + timeout=None, + username=None, + password=None, + token_name=None, + token_value=None, + server=None, + site_name="", + no_prompt=True, + no_certcheck=False, + no_proxy=False, + proxy=None, + certificate=None, + password_file=None, + token_file=None, + no_cookie=False, + query_page_size=None, + language=None, + ) + for k, v in overrides.items(): + setattr(ns, k, v) + return ns + + +class CreateSiteUsersTest(unittest.TestCase): + def _run(self, args, completed_override=None): + from tabcmd.commands.user.create_site_users import CreateSiteUsersCommand + + with mock.patch("tabcmd.commands.user.create_site_users.Session") as session_cls, mock.patch( + "tabcmd.commands.user.user_data.UserCommand.validate_file_for_import" + ), mock.patch("tabcmd.commands.user.user_data.UserCommand.get_users_from_file") as get_users: + fake_server = mock.MagicMock() + session_cls.return_value.create_session.return_value = fake_server + get_users.return_value = [ + TSC.UserItem("alice", "Creator"), + TSC.UserItem("bob", "Viewer"), + ] + + fake_job = mock.MagicMock(spec=TSC.JobItem) + fake_job.id = "abc-123" + fake_server.users.bulk_add.return_value = fake_job + + if completed_override is not None: + completed = completed_override + else: + completed = mock.MagicMock(spec=TSC.JobItem) + completed.id = "abc-123" + completed.finish_code = 0 + completed.notes = [] + completed.status_notes = [ + {"type": "CountOfUsersAddedToSite", "value": "2", "text": None}, + {"type": "CountOfUsersSkipped", "value": "0", "text": None}, + {"type": "CountOfUsersProcessed", "value": "2", "text": None}, + ] + fake_server.jobs.wait_for_job.return_value = completed + + CreateSiteUsersCommand.run_command(args) + return fake_server, fake_job, completed + + def test_default_calls_bulk_add_and_wait(self): + args = _base_args() + server, job, completed = self._run(args) + server.users.bulk_add.assert_called_once() + server.jobs.wait_for_job.assert_called_once_with(job_id=job.id, timeout=None) + + def test_nowait_skips_wait_for_job(self): + args = _base_args(nowait=True) + server, job, completed = self._run(args) + server.users.bulk_add.assert_called_once() + server.jobs.wait_for_job.assert_not_called() + + def test_silent_progress_still_calls_wait(self): + # --silent-progress suppresses log lines, not the actual wait. + args = _base_args(silent_progress=True) + server, job, completed = self._run(args) + server.users.bulk_add.assert_called_once() + server.jobs.wait_for_job.assert_called_once() + + def test_nowait_and_silent_progress_coexist(self): + args = _base_args(nowait=True, silent_progress=True) + server, job, completed = self._run(args) + server.users.bulk_add.assert_called_once() + server.jobs.wait_for_job.assert_not_called() + + def test_older_tsc_without_status_notes_exits_with_error(self): + # If the pinned tableauserverclient predates status_notes on JobItem, + # we can't produce a truthful per-user summary. Fail loudly rather than + # silently print zeros that look like success. + args = _base_args() + + class _OldJobItem: + # Deliberately does NOT have a status_notes attribute; this mirrors + # a pre-status_notes TSC release. + def __init__(self): + self.id = "abc-123" + self.finish_code = 0 + self.notes = [] + + with self.assertRaises(SystemExit): + self._run(args, completed_override=_OldJobItem()) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/commands/test_run_commands.py b/tests/commands/test_run_commands.py index f1777d13..748401cc 100644 --- a/tests/commands/test_run_commands.py +++ b/tests/commands/test_run_commands.py @@ -428,6 +428,9 @@ def test_create_site_users(self, mock_session, mock_server): mock_args.site_name = None mock_args.role = "Viewer" mock_args.auth_type = "SAML" + mock_args.nowait = True # keep the smoke test fast; skip server-job polling + mock_args.silent_progress = False + mock_args.timeout = None create_site_users.CreateSiteUsersCommand.run_command(mock_args) mock_session.assert_called() diff --git a/tests/parsers/test_parser_create_site_users.py b/tests/parsers/test_parser_create_site_users.py index 93a51db2..b28191d1 100644 --- a/tests/parsers/test_parser_create_site_users.py +++ b/tests/parsers/test_parser_create_site_users.py @@ -48,3 +48,26 @@ def test_create_site_user_parser_auth_TabId_NotAvailable(self): mock_args = [commandname, "users.csv", "--site", "site-name", "--auth-type", "TableauId"] with self.assertRaises(SystemExit): args = self.parser_under_test.parse_args(mock_args) + + def test_create_site_user_parser_nowait(self): + # Matches Classic spelling (one word). Defaults to False -> wait. + with mock.patch("builtins.open", mock.mock_open(read_data="test")): + args = self.parser_under_test.parse_args([commandname, "users.csv"]) + assert args.nowait is False, args + + args = self.parser_under_test.parse_args([commandname, "users.csv", "--nowait"]) + assert args.nowait is True, args + + def test_create_site_user_parser_silent_progress(self): + with mock.patch("builtins.open", mock.mock_open(read_data="test")): + args = self.parser_under_test.parse_args([commandname, "users.csv"]) + assert args.silent_progress is False, args + + args = self.parser_under_test.parse_args([commandname, "users.csv", "--silent-progress"]) + assert args.silent_progress is True, args + + def test_create_site_user_parser_nowait_and_silent_coexist(self): + with mock.patch("builtins.open", mock.mock_open(read_data="test")): + args = self.parser_under_test.parse_args([commandname, "users.csv", "--nowait", "--silent-progress"]) + assert args.nowait is True, args + assert args.silent_progress is True, args