From a08a28c98f0a4c01fbcc4b3e4aa2f7c1424c35fd Mon Sep 17 00:00:00 2001 From: Rio6 Date: Fri, 27 Aug 2021 22:57:50 -0400 Subject: [PATCH 1/9] positions cli --- ceo/cli/entrypoint.py | 9 +++- ceo/cli/positions.py | 48 +++++++++++++++++++ ceo/operation_strings.py | 3 ++ .../UpdateMemberPositionsTransaction.py | 3 +- tests/ceo_dev.ini | 5 ++ 5 files changed, 65 insertions(+), 3 deletions(-) create mode 100644 ceo/cli/positions.py diff --git a/ceo/cli/entrypoint.py b/ceo/cli/entrypoint.py index 04f5306..2248938 100644 --- a/ceo/cli/entrypoint.py +++ b/ceo/cli/entrypoint.py @@ -8,6 +8,7 @@ from zope import component from ..krb_check import krb_check from .members import members from .groups import groups +from .positions import positions from .updateprograms import updateprograms from ceo_common.interfaces import IConfig, IHTTPClient from ceo_common.model import Config, HTTPClient @@ -29,10 +30,14 @@ def cli(ctx): cli.add_command(members) cli.add_command(groups) +cli.add_command(positions) cli.add_command(updateprograms) def register_services(): + # Using base component directly so events get triggered + baseComponent = component.getGlobalSiteManager() + # Config # This is a hack to determine if we're in the dev env or not if socket.getfqdn().endswith('.csclub.internal'): @@ -41,8 +46,8 @@ def register_services(): else: config_file = os.environ.get('CEO_CONFIG', '/etc/csc/ceo.ini') cfg = Config(config_file) - component.provideUtility(cfg, IConfig) + baseComponent.registerUtility(cfg, IConfig) # HTTPService http_client = HTTPClient() - component.provideUtility(http_client, IHTTPClient) + baseComponent.registerUtility(http_client, IHTTPClient) diff --git a/ceo/cli/positions.py b/ceo/cli/positions.py new file mode 100644 index 0000000..875d1f7 --- /dev/null +++ b/ceo/cli/positions.py @@ -0,0 +1,48 @@ +import click +from zope import component +from zope.interface.interfaces import IRegistered, IUtilityRegistration +import zope.component.event + +from ceo_common.interfaces import IConfig +from ceod.transactions.members import UpdateMemberPositionsTransaction + +from .utils import handle_sync_response, handle_stream_response, print_colon_kv +from ..utils import http_get, http_post + +@click.group(short_help='List or change exec positions') +def positions(): + pass + + +@positions.command(short_help='Get current positions') +def get(): + resp = http_get('/api/positions') + result = handle_sync_response(resp) + print_colon_kv(result.items()) + + +@positions.command(short_help='Update positions') +def update(**kwargs): + click.echo('The positions will be updated:') + print_colon_kv(kwargs.items()) + click.confirm('Do you want to continue?', abort=True) + + resp = http_post('/api/positions', json={k.replace('_', '-'): v for k, v in kwargs.items()}) + handle_stream_response(resp, UpdateMemberPositionsTransaction.operations) + + +# Provides dynamic parameter for update command using config file +@component.provideHandler +@component.adapter(IRegistered) +def _handler(event): + global update + + if not (IUtilityRegistration.providedBy(event.object) and IConfig.providedBy(event.object.component)): + return + + cfg = event.object.component + avail = cfg.get('positions_available') + required = cfg.get('positions_required') + + for pos in avail: + update = click.option(f'--{pos}', metavar='USER', required=(pos in required))(update) diff --git a/ceo/operation_strings.py b/ceo/operation_strings.py index 8ff44f7..2d874e4 100644 --- a/ceo/operation_strings.py +++ b/ceo/operation_strings.py @@ -24,4 +24,7 @@ descriptions = { 'remove_user_from_auxiliary_groups': 'Remove user from auxiliary groups', 'unsubscribe_user_from_auxiliary_mailing_lists': 'Unsubscribe user from auxiliary mailing lists', 'remove_sudo_role': 'Remove sudo role from LDAP', + 'update_positions_ldap': 'Update positions in LDAP', + 'update_exec_group_ldap': 'Update executive group in LDAP', + 'subscribe_to_mailing_lists': 'Subscribe to mailing lists', } diff --git a/ceod/transactions/members/UpdateMemberPositionsTransaction.py b/ceod/transactions/members/UpdateMemberPositionsTransaction.py index 32ac77e..d38400f 100644 --- a/ceod/transactions/members/UpdateMemberPositionsTransaction.py +++ b/ceod/transactions/members/UpdateMemberPositionsTransaction.py @@ -28,7 +28,8 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): # Reverse the dict so it's easier to use (username -> positions) self.positions = defaultdict(list) for position, username in positions_reversed.items(): - self.positions[username].append(position) + if username is not None: + self.positions[username].append(position) # a cached Dict of the Users who need to be modified (username -> User) self.users: Dict[str, IUser] = {} diff --git a/tests/ceo_dev.ini b/tests/ceo_dev.ini index e74895e..978f6f5 100644 --- a/tests/ceo_dev.ini +++ b/tests/ceo_dev.ini @@ -7,3 +7,8 @@ uw_domain = uwaterloo.internal admin_host = phosphoric-acid use_https = false port = 9987 + +[positions] +required = president,vice-president,sysadmin +available = president,vice-president,treasurer,secretary, + sysadmin,cro,librarian,imapd,webmaster,offsck -- 2.39.2 From 8c9ddd2d276083d57191b771c43b67088714508a Mon Sep 17 00:00:00 2001 From: Rio6 Date: Fri, 27 Aug 2021 23:01:36 -0400 Subject: [PATCH 2/9] fix flake --- ceo/cli/positions.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ceo/cli/positions.py b/ceo/cli/positions.py index 875d1f7..15f2688 100644 --- a/ceo/cli/positions.py +++ b/ceo/cli/positions.py @@ -1,7 +1,8 @@ import click + from zope import component from zope.interface.interfaces import IRegistered, IUtilityRegistration -import zope.component.event +import zope.component.event # noqa: F401 from ceo_common.interfaces import IConfig from ceod.transactions.members import UpdateMemberPositionsTransaction @@ -9,6 +10,7 @@ from ceod.transactions.members import UpdateMemberPositionsTransaction from .utils import handle_sync_response, handle_stream_response, print_colon_kv from ..utils import http_get, http_post + @click.group(short_help='List or change exec positions') def positions(): pass -- 2.39.2 From 8f5a2803a633a7fcbbc1265a56fafb04cbf64ca9 Mon Sep 17 00:00:00 2001 From: Rio Liu Date: Mon, 30 Aug 2021 16:55:46 -0400 Subject: [PATCH 3/9] fix some pr feedbacks --- ceo/cli/positions.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ceo/cli/positions.py b/ceo/cli/positions.py index 15f2688..024c826 100644 --- a/ceo/cli/positions.py +++ b/ceo/cli/positions.py @@ -24,7 +24,7 @@ def get(): @positions.command(short_help='Update positions') -def update(**kwargs): +def set(**kwargs): click.echo('The positions will be updated:') print_colon_kv(kwargs.items()) click.confirm('Do you want to continue?', abort=True) @@ -37,7 +37,7 @@ def update(**kwargs): @component.provideHandler @component.adapter(IRegistered) def _handler(event): - global update + global set if not (IUtilityRegistration.providedBy(event.object) and IConfig.providedBy(event.object.component)): return @@ -47,4 +47,5 @@ def _handler(event): required = cfg.get('positions_required') for pos in avail: - update = click.option(f'--{pos}', metavar='USER', required=(pos in required))(update) + r = pos in required + set = click.option(f'--{pos}', metavar='USERNAME', required=r, prompt=r)(set) -- 2.39.2 From 5bae89a9fd9951d0ae8d44b86d6b826adce3efbc Mon Sep 17 00:00:00 2001 From: Rio Liu Date: Mon, 30 Aug 2021 17:37:48 -0400 Subject: [PATCH 4/9] handle empty input in print_colon_kv --- ceo/cli/utils.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ceo/cli/utils.py b/ceo/cli/utils.py index 98aa23f..6a8c73a 100644 --- a/ceo/cli/utils.py +++ b/ceo/cli/utils.py @@ -28,6 +28,9 @@ def print_colon_kv(pairs: List[Tuple[str, str]]): key1: value1 key1000: value2 """ + if len(pairs) == 0: + return + maxlen = max(len(key) for key, val in pairs) for key, val in pairs: if key != '': -- 2.39.2 From 36def99b28db0dbc50740a1278ebd43cc0a89405 Mon Sep 17 00:00:00 2001 From: Rio Liu Date: Mon, 30 Aug 2021 23:42:51 -0400 Subject: [PATCH 5/9] add unit test for positions and replace provideUtility() with getGlobalSiteManager().registerUtility() in unit tests --- tests/ceo/cli/test_positions.py | 55 +++++++++++++++++++++++++++++++++ tests/conftest.py | 20 ++++++------ 2 files changed, 65 insertions(+), 10 deletions(-) create mode 100644 tests/ceo/cli/test_positions.py diff --git a/tests/ceo/cli/test_positions.py b/tests/ceo/cli/test_positions.py new file mode 100644 index 0000000..bfb548e --- /dev/null +++ b/tests/ceo/cli/test_positions.py @@ -0,0 +1,55 @@ +from click.testing import CliRunner +from ceo.cli import cli + +def test_positions(cli_setup): + runner = CliRunner() + + # Setup test data + for i in range(5): + runner.invoke(cli, ['members', 'add', f'test_{i}', '--cn', f'Test {i}', '--program', 'Math', '--terms', '1' ], input='y\n') + runner.invoke(cli, ['groups', 'add', 'exec', '--description', 'Test Group'], input='y\n') + + result = runner.invoke(cli, ['positions', 'set', + '--president', 'test_0', + '--vice-president', 'test_1', + '--sysadmin', 'test_2', + '--secretary', 'test_3', + '--webmaster', 'test_4', + ], input='y\n') + + assert result.exit_code == 0 + assert result.output == \ +''' +The positions will be updated: +president: test_0 +vice_president: test_1 +sysadmin: test_2 +secretary: test_3 +webmaster: test_4 +treasurer: +cro: +librarian: +imapd: +offsck: +Do you want to continue? [y/N]: y +Update positions in LDAP... Done +Update executive group in LDAP... Done +Subscribe to mailing lists... Done +Transaction successfully completed. +'''[1:] + + result = runner.invoke(cli, ['positions', 'get']) + assert result.exit_code == 0 + assert result.output == \ +''' +president: test_0 +secretary: test_3 +sysadmin: test_2 +vice-president: test_1 +webmaster: test_4 +'''[1:] + + # Cleanup test data + for i in range(5): + runner.invoke(cli, ['members', 'delete', f'test_{i}'], input='y\n') + runner.invoke(cli, ['groups', 'delete', 'exec'], input='y\n') diff --git a/tests/conftest.py b/tests/conftest.py index de146d4..68f1edb 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -51,7 +51,7 @@ def cfg(_drone_hostname_mock): with importlib.resources.path('tests', 'ceod_test_local.ini') as p: config_file = p.__fspath__() _cfg = Config(config_file) - component.provideUtility(_cfg, IConfig) + component.getGlobalSiteManager().registerUtility(_cfg, IConfig) return _cfg @@ -75,7 +75,7 @@ def krb_srv(cfg): else: principal = 'ceod/' + socket.getfqdn() krb = KerberosService(principal) - component.provideUtility(krb, IKerberosService) + component.getGlobalSiteManager().registerUtility(krb, IKerberosService) delete_test_princs(krb) yield krb @@ -160,7 +160,7 @@ def ldap_srv_session(cfg, krb_srv, ldap_conn): conn.add(base_dn, 'organizationalUnit') _ldap_srv = LDAPService() - component.provideUtility(_ldap_srv, ILDAPService) + component.getGlobalSiteManager().registerUtility(_ldap_srv, ILDAPService) yield _ldap_srv @@ -180,7 +180,7 @@ def ldap_srv(ldap_srv_session, g_admin_ctx): @pytest.fixture(scope='session') def file_srv(cfg): _file_srv = FileService() - component.provideUtility(_file_srv, IFileService) + component.getGlobalSiteManager().registerUtility(_file_srv, IFileService) members_home = cfg.get('members_home') clubs_home = cfg.get('clubs_home') @@ -194,7 +194,7 @@ def file_srv(cfg): @pytest.fixture(scope='session') def http_client(cfg): _client = HTTPClient() - component.provideUtility(_client, IHTTPClient) + component.getGlobalSiteManager().registerUtility(_client, IHTTPClient) return _client @@ -210,7 +210,7 @@ def mock_mailman_server(): def mailman_srv(mock_mailman_server, cfg, http_client): # TODO: test the RemoteMailmanService as well mailman = MailmanService() - component.provideUtility(mailman, IMailmanService) + component.getGlobalSiteManager().registerUtility(mailman, IMailmanService) return mailman @@ -223,7 +223,7 @@ def uwldap_srv(cfg, ldap_conn): conn.add(base_dn, 'organizationalUnit') _uwldap_srv = UWLDAPService() - component.provideUtility(_uwldap_srv, IUWLDAPService) + component.getGlobalSiteManager().registerUtility(_uwldap_srv, IUWLDAPService) yield _uwldap_srv delete_subtree(conn, base_dn) @@ -240,21 +240,21 @@ def mock_mail_server(): @pytest.fixture(scope='session') def mail_srv(cfg, mock_mail_server): _mail_srv = MailService() - component.provideUtility(_mail_srv, IMailService) + component.getGlobalSiteManager().registerUtility(_mail_srv, IMailService) return _mail_srv @pytest.fixture(scope='session') def mysql_srv(cfg): mysql_srv = MySQLService() - component.provideUtility(mysql_srv, IDatabaseService, 'mysql') + component.getGlobalSiteManager().registerUtility(mysql_srv, IDatabaseService, 'mysql') return mysql_srv @pytest.fixture(scope='session') def postgresql_srv(cfg): psql_srv = PostgreSQLService() - component.provideUtility(psql_srv, IDatabaseService, 'postgresql') + component.getGlobalSiteManager().registerUtility(psql_srv, IDatabaseService, 'postgresql') return psql_srv -- 2.39.2 From 6577fb3ea6e76d9e1af86432f29aaca21da9017d Mon Sep 17 00:00:00 2001 From: Rio Liu Date: Mon, 30 Aug 2021 23:52:37 -0400 Subject: [PATCH 6/9] fix flake8 --- tests/ceo/cli/test_positions.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/ceo/cli/test_positions.py b/tests/ceo/cli/test_positions.py index bfb548e..453685f 100644 --- a/tests/ceo/cli/test_positions.py +++ b/tests/ceo/cli/test_positions.py @@ -1,15 +1,17 @@ from click.testing import CliRunner from ceo.cli import cli + def test_positions(cli_setup): runner = CliRunner() # Setup test data for i in range(5): - runner.invoke(cli, ['members', 'add', f'test_{i}', '--cn', f'Test {i}', '--program', 'Math', '--terms', '1' ], input='y\n') + runner.invoke(cli, ['members', 'add', f'test_{i}', '--cn', f'Test {i}', '--program', 'Math', '--terms', '1'], input='y\n') runner.invoke(cli, ['groups', 'add', 'exec', '--description', 'Test Group'], input='y\n') - result = runner.invoke(cli, ['positions', 'set', + result = runner.invoke(cli, [ + 'positions', 'set', '--president', 'test_0', '--vice-president', 'test_1', '--sysadmin', 'test_2', @@ -18,8 +20,7 @@ def test_positions(cli_setup): ], input='y\n') assert result.exit_code == 0 - assert result.output == \ -''' + assert result.output == ''' The positions will be updated: president: test_0 vice_president: test_1 @@ -36,12 +37,11 @@ Update positions in LDAP... Done Update executive group in LDAP... Done Subscribe to mailing lists... Done Transaction successfully completed. -'''[1:] +'''[1:] # noqa: W291 result = runner.invoke(cli, ['positions', 'get']) assert result.exit_code == 0 - assert result.output == \ -''' + assert result.output == ''' president: test_0 secretary: test_3 sysadmin: test_2 -- 2.39.2 From 9c0891eb7daefc54aa304a41b7c402b691007c85 Mon Sep 17 00:00:00 2001 From: Rio Liu Date: Mon, 6 Sep 2021 21:05:52 -0400 Subject: [PATCH 7/9] handle None for required position --- ceod/api/positions.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ceod/api/positions.py b/ceod/api/positions.py index 9d9f4c4..93b757f 100644 --- a/ceod/api/positions.py +++ b/ceod/api/positions.py @@ -36,7 +36,7 @@ def update_positions(): }, 400 for position in required: - if position not in body: + if not body.get(position): return { 'error': f'missing required position: {position}' }, 400 -- 2.39.2 From 9f549ee58bd86cb49e3dbdf1f0188888a4f49917 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Wed, 8 Sep 2021 13:03:52 +0000 Subject: [PATCH 8/9] call update_commands from positions() group --- ceo/__main__.py | 2 -- ceo/cli/positions.py | 27 ++++++++++----------------- ceo/utils.py | 2 ++ tests/ceo/cli/test_positions.py | 2 +- 4 files changed, 13 insertions(+), 20 deletions(-) diff --git a/ceo/__main__.py b/ceo/__main__.py index d1df4fc..fbc4473 100644 --- a/ceo/__main__.py +++ b/ceo/__main__.py @@ -24,12 +24,10 @@ def register_services(): else: config_file = os.environ.get('CEO_CONFIG', '/etc/csc/ceo.ini') cfg = Config(config_file) - component.provideUtility(cfg, IConfig) baseComponent.registerUtility(cfg, IConfig) # HTTPService http_client = HTTPClient() - component.provideUtility(http_client, IHTTPClient) baseComponent.registerUtility(http_client, IHTTPClient) diff --git a/ceo/cli/positions.py b/ceo/cli/positions.py index 024c826..733e8c0 100644 --- a/ceo/cli/positions.py +++ b/ceo/cli/positions.py @@ -1,19 +1,15 @@ import click - from zope import component -from zope.interface.interfaces import IRegistered, IUtilityRegistration -import zope.component.event # noqa: F401 +from ..utils import http_get, http_post +from .utils import handle_sync_response, handle_stream_response, print_colon_kv from ceo_common.interfaces import IConfig from ceod.transactions.members import UpdateMemberPositionsTransaction -from .utils import handle_sync_response, handle_stream_response, print_colon_kv -from ..utils import http_get, http_post - @click.group(short_help='List or change exec positions') def positions(): - pass + update_commands() @positions.command(short_help='Get current positions') @@ -25,24 +21,21 @@ def get(): @positions.command(short_help='Update positions') def set(**kwargs): + body = {k.replace('_', '-'): v for k, v in kwargs.items()} + print_body = {k: v or '' for k, v in body.items()} click.echo('The positions will be updated:') - print_colon_kv(kwargs.items()) + print_colon_kv(print_body.items()) click.confirm('Do you want to continue?', abort=True) - resp = http_post('/api/positions', json={k.replace('_', '-'): v for k, v in kwargs.items()}) + resp = http_post('/api/positions', json=body) handle_stream_response(resp, UpdateMemberPositionsTransaction.operations) -# Provides dynamic parameter for update command using config file -@component.provideHandler -@component.adapter(IRegistered) -def _handler(event): +# Provides dynamic parameters for `set' command using config file +def update_commands(): global set - if not (IUtilityRegistration.providedBy(event.object) and IConfig.providedBy(event.object.component)): - return - - cfg = event.object.component + cfg = component.getUtility(IConfig) avail = cfg.get('positions_available') required = cfg.get('positions_required') diff --git a/ceo/utils.py b/ceo/utils.py index b23604f..28e4a13 100644 --- a/ceo/utils.py +++ b/ceo/utils.py @@ -69,6 +69,8 @@ def space_colon_kv(pairs: List[Tuple[str, str]]) -> List[str]: key1000: val3 val4 """ + if not pairs: + return [] lines = [] maxlen = max(len(key) for key, val in pairs) for key, val in pairs: diff --git a/tests/ceo/cli/test_positions.py b/tests/ceo/cli/test_positions.py index 453685f..c7ae111 100644 --- a/tests/ceo/cli/test_positions.py +++ b/tests/ceo/cli/test_positions.py @@ -23,7 +23,7 @@ def test_positions(cli_setup): assert result.output == ''' The positions will be updated: president: test_0 -vice_president: test_1 +vice-president: test_1 sysadmin: test_2 secretary: test_3 webmaster: test_4 -- 2.39.2 From da3f1f49da03adddc8c6a7ff945d6e996304d718 Mon Sep 17 00:00:00 2001 From: Max Erenberg Date: Wed, 8 Sep 2021 13:20:45 +0000 Subject: [PATCH 9/9] remove falsy values from body --- ceod/api/positions.py | 8 +++++++- .../members/UpdateMemberPositionsTransaction.py | 3 +-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/ceod/api/positions.py b/ceod/api/positions.py index 93b757f..a194565 100644 --- a/ceod/api/positions.py +++ b/ceod/api/positions.py @@ -29,6 +29,12 @@ def update_positions(): required = cfg.get('positions_required') available = cfg.get('positions_available') + # remove falsy values + body = { + positions: username for positions, username in body.items() + if username + } + for position in body.keys(): if position not in available: return { @@ -36,7 +42,7 @@ def update_positions(): }, 400 for position in required: - if not body.get(position): + if position not in body: return { 'error': f'missing required position: {position}' }, 400 diff --git a/ceod/transactions/members/UpdateMemberPositionsTransaction.py b/ceod/transactions/members/UpdateMemberPositionsTransaction.py index d38400f..32ac77e 100644 --- a/ceod/transactions/members/UpdateMemberPositionsTransaction.py +++ b/ceod/transactions/members/UpdateMemberPositionsTransaction.py @@ -28,8 +28,7 @@ class UpdateMemberPositionsTransaction(AbstractTransaction): # Reverse the dict so it's easier to use (username -> positions) self.positions = defaultdict(list) for position, username in positions_reversed.items(): - if username is not None: - self.positions[username].append(position) + self.positions[username].append(position) # a cached Dict of the Users who need to be modified (username -> User) self.users: Dict[str, IUser] = {} -- 2.39.2