Positions CLI #11

Merged
merenber merged 10 commits from positions-cli into v1 1 year ago
r345liu commented 1 year ago
Owner

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 1 year ago
r345liu added 2 commits 1 year ago
01d02fe66c fix flake
merenber requested changes 1 year ago
merenber left a comment
Owner

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.
@positions.command(short_help='Update positions')
def update(**kwargs):
Owner

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

I think that this should be called 'set' instead of 'update'.
r345liu marked this conversation as resolved
required = cfg.get('positions_required')
for pos in avail:
update = click.option(f'--{pos}', metavar='USER', required=(pos in required))(update)
Owner

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
self.positions = defaultdict(list)
for position, username in positions_reversed.items():
self.positions[username].append(position)
if username is not None:
Owner

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.
Poster
Owner

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

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

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): ```
Poster
Owner

ah I see what you mean. Will fix

ah I see what you mean. Will fix
r345liu marked this conversation as resolved
r345liu added 1 commit 1 year ago
6b289a1ec4 fix some pr feedbacks
r345liu force-pushed positions-cli from 6b289a1ec4 to 5bae89a9fd 1 year ago
r345liu added 1 commit 1 year ago
Poster
Owner

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 1 year ago
6577fb3ea6 fix flake8
r345liu requested review from merenber 1 year ago
merenber reviewed 1 year ago
def _handler(event):
global set
if not (IUtilityRegistration.providedBy(event.object) and IConfig.providedBy(event.object.component)):
Owner

What is this doing?

What is this doing?
Poster
Owner

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.
Poster
Owner

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

The reason why this whole thing is here is to make the parameters (--president, --syscom, etc) follow what is in the config.
r345liu added 1 commit 1 year ago
9c0891eb7d handle None for required position
merenber added 3 commits 1 year ago
Owner

After I merged v1 in I couldn't get the zope callback to work anymore, so I just created an explicit funtion which is called in positions().

After I merged v1 in I couldn't get the zope callback to work anymore, so I just created an explicit funtion which is called in positions().
merenber merged commit 651988bb08 into v1 1 year ago
merenber referenced this issue from a commit 1 year ago
merenber deleted branch positions-cli 1 year ago

Reviewers

merenber was requested for review 1 year ago
continuous-integration/drone/pr Build is passing
The pull request has been merged as 651988bb08.
Sign in to join this conversation.
No reviewers
No Label
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: public/pyceo#11
Loading…
There is no content yet.