Positions CLI #11

r345liu · 2021-08-27T23:11:50-04:00

r345liu commented

2021-08-27 23:11:50 -04:00

Closes #9

One change from #8 is that the get subcommand is not optional, makes it easier to implement using click.

Closes #9 One change from #8 is that the `get` subcommand is not optional, makes it easier to implement using click.

merenber was assigned by r345liu

2021-08-27 23:11:50 -04:00

r345liu added 2 commits 2021-08-27 23:11:51 -04:00

7b289cda56 positions cli

continuous-integration/drone/pr Build is passing Details

01d02fe66c fix flake

merenber requested changes 2021-08-29 14:02:26 -04:00

merenber left a comment

Looking good - just a few minor adjustments.
Also, we still need CLI unit tests.

Looking good - just a few minor adjustments. Also, we still need CLI unit tests.

ceo/cli/positions.py Outdated

													
				@ -0,0 +24,4 @@

				@positions.command(short_help='Update positions')

				def update(**kwargs):

merenber commented

2021-08-29 13:56:18 -04:00

I think that this should be called 'set' instead of 'update'.

r345liu marked this conversation as resolved

ceo/cli/positions.py Outdated

													
				@ -0,0 +47,4 @@

				    required = cfg.get('positions_required')

				    for pos in avail:

				        update = click.option(f'--{pos}', metavar='USER', required=(pos in required))(update)

merenber commented

2021-08-29 13:57:40 -04:00

metavar should be 'USERNAME'.
Also, users should be prompted for required positions (set prompt=True).

metavar should be 'USERNAME'. Also, users should be prompted for required positions (set `prompt=True`).

r345liu marked this conversation as resolved

ceod/transactions/members/UpdateMemberPositionsTransaction.py Outdated

													
				@ -29,3 +29,3 @@

				        self.positions = defaultdict(list)

				        for position, username in positions_reversed.items():

				            self.positions[username].append(position)

				            if username is not None:

merenber commented

2021-08-29 13:59:32 -04:00

If you're going to allow clients to POST null values, you need to check them in api/positions.py as well.
In particular, you have a snippet of code which does this:

    for position in required:
        if position not in body:
            return {
                'error': f'missing required position: {position}'
            }, 400

If the body has a required position set to null, this check will fail to catch that.
It might be easier to simply remove null entries from the body at the very beginning.

If you're going to allow clients to POST null values, you need to check them in api/positions.py as well. In particular, you have a snippet of code which does this: ```python for position in required: if position not in body: return { 'error': f'missing required position: {position}' }, 400 ``` If the body has a required position set to null, this check will fail to catch that. It might be easier to simply remove null entries from the body at the very beginning.

r345liu commented

2021-08-30 16:55:14 -04:00

the change doesn't allow positions to be null, only usernames. So it should handle it fine.

merenber commented

2021-09-04 20:29:52 -04:00

This sort of thing is now possible:

curl --negotiate -u : --service-name ceod --delegation always -X POST \
    -d '{"president":"ctdalek","sysadmin":null,"vice-president":null}' \
    http://phosphoric-acid:9987/api/positions

The API should return a 400 error for a request like this.

Here's an easy fix: change

if position not in body:

to

if not body.get(position):

This sort of thing is now possible: ``` curl --negotiate -u : --service-name ceod --delegation always -X POST \ -d '{"president":"ctdalek","sysadmin":null,"vice-president":null}' \ http://phosphoric-acid:9987/api/positions ``` The API should return a 400 error for a request like this. Here's an easy fix: change ```python if position not in body: ``` to ```python if not body.get(position): ```

r345liu commented

2021-09-06 21:06:40 -04:00

ah I see what you mean. Will fix

r345liu marked this conversation as resolved

r345liu added 1 commit 2021-08-30 16:55:53 -04:00

continuous-integration/drone/pr Build is failing Details

6b289a1ec4 fix some pr feedbacks

r345liu force-pushed positions-cli from 6b289a1ec4 to 5bae89a9fd

2021-08-30 17:38:01 -04:00

Compare

r345liu added 1 commit 2021-08-30 23:43:08 -04:00

continuous-integration/drone/pr Build is failing Details

36def99b28 add unit test for positions and replace provideUtility() with getGlobalSiteManager().registerUtility() in unit tests

r345liu commented

2021-08-30 23:46:42 -04:00

Added unit tests. I changed more provideUtility with registerUtility() so the dynamic position parameter works correctly. (It seems like it's where the library want people to use too judging by this comment 5bd246ed53/src/zope/component/globalregistry.py (L66))

Added unit tests. I changed more provideUtility with registerUtility() so the dynamic position parameter works correctly. (It seems like it's where the library want people to use too judging by this comment https://github.com/zopefoundation/zope.component/blob/5bd246ed532d8ace5034bb09777b9f5b8eb87150/src/zope/component/globalregistry.py#L66)

r345liu added 1 commit 2021-08-30 23:52:42 -04:00

continuous-integration/drone/pr Build is passing Details

6577fb3ea6 fix flake8

r345liu requested review from merenber 2021-08-30 23:53:26 -04:00

merenber reviewed 2021-09-04 20:32:00 -04:00

ceo/cli/positions.py

													
				@ -0,0 +39,4 @@

				def _handler(event):

				    global set

				    if not (IUtilityRegistration.providedBy(event.object) and IConfig.providedBy(event.object.component)):

merenber commented

2021-09-04 20:32:00 -04:00

What is this doing?

r345liu commented

2021-09-06 21:12:34 -04:00

zope's registerUtility emits an IUtilityRegistration containing the component that got registered. Here I'm just checking for that, and whether the registered component is an IConfig.

zope's `registerUtility` emits an IUtilityRegistration containing the component that got registered. Here I'm just checking for that, and whether the registered component is an IConfig.

👍 1

r345liu commented

2021-09-06 21:13:20 -04:00

The reason why this whole thing is here is to make the parameters (--president, --syscom, etc) follow what is in the config.