public
/
pyceo
25
1
Fork 2
Code Issues 19 Pull Requests 4 Projects Releases 1 Wiki Activity

Positions CLI #11

Merged
merenber merged 10 commits from positions-cli into v1 2021-09-08 09:32:35 -04:00
Conversation 2 Commits 10 Files Changed 9
r345liu commented 2021-08-27 23:11:50 -04:00
repo.issues.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 2021-08-27 23:11:50 -04:00
r345liu added 2 commits 2021-08-27 23:11:51 -04:00
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
repo.issues.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.
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
repo.issues.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
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
repo.issues.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
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
repo.issues.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.
r345liu commented 2021-08-30 16:55:14 -04:00
repo.issues.poster
repo.issues.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.
merenber commented 2021-09-04 20:29:52 -04:00
repo.issues.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): ```
r345liu commented 2021-09-06 21:06:40 -04:00
repo.issues.poster
repo.issues.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 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
r345liu commented 2021-08-30 23:46:42 -04:00
repo.issues.poster
repo.issues.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 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
repo.issues.owner

What is this doing?

What is this doing?
r345liu commented 2021-09-06 21:12:34 -04:00
repo.issues.poster
repo.issues.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.
👍 1
r345liu commented 2021-09-06 21:13:20 -04:00
repo.issues.poster
repo.issues.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 2021-09-06 21:08:03 -04:00
continuous-integration/drone/pr Build is failing Details
9c0891eb7d handle None for required position
merenber added 3 commits 2021-09-08 09:20:58 -04:00
continuous-integration/drone/pr Build is passing Details
da3f1f49da remove falsy values from body
merenber commented 2021-09-08 09:22:01 -04:00
repo.issues.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 2021-09-08 09:32:35 -04:00
merenber referenced this issue from a commit 2021-09-08 09:32:35 -04:00
Positions CLI (#11)
merenber deleted branch positions-cli 2021-09-08 09:33:03 -04:00
Sign in to join this conversation.
Reviewers
No reviewers
merenber
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
merenber
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: public/pyceo#11
No description provided.
Delete Branch "positions-cli"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?