Conversation
printing/paper
Outdated
| def staff_creds(): | ||
| """Return dictionary of staff creds, if we can read them.""" | ||
| return json.load(open('/etc/ocfprinting.json')) | ||
| return json.load(open('/etc/ocfprinting-dev.json')) |
MrMinos
left a comment
MrMinos
left a comment
There was a problem hiding this comment.
I think this is a quite useful feature, might want to publicize this to the users at some point?
I don't think this at all needs to be hush-hush thing.
|
|
||
| parser_forward = subparsers.add_parser('forward', help="allow a user to print above today's quota") | ||
| parser_forward.add_argument('--pages', '-p', required=True, type=int) | ||
| parser_forward.add_argument('--reason', '-r', required=True, type=str) |
There was a problem hiding this comment.
Forward probably in itself is the reason, I don't know if this needs to be a required field. What do you think?
There was a problem hiding this comment.
I think this is still fine, and it's a database constraint anyways
There was a problem hiding this comment.
Despite the reason being clear, I think this would be important to include due to the human aspect of it. A person who is issuing the forward request will need to think twice before actually increasing the daily quota for a particular user.
printing/paper
Outdated
| return 1 | ||
|
|
||
| return commands[args.command](args) | ||
| if args.command == 'refund' or args.command == 'forward': |
There was a problem hiding this comment.
Instead of an if statement here, which makes the subcommand logic harder to understand, you should put the same if statement in the adjust command (which has access to args and therefore args.command).
|
|
||
| parser_forward = subparsers.add_parser('forward', help="allow a user to print above today's quota") | ||
| parser_forward.add_argument('--pages', '-p', required=True, type=int) | ||
| parser_forward.add_argument('--reason', '-r', required=True, type=str) |
There was a problem hiding this comment.
I think this is still fine, and it's a database constraint anyways
printing/paper
Outdated
| user=args.user, | ||
| time=datetime.now(), | ||
| pages=args.pages, | ||
| action=type, |
There was a problem hiding this comment.
You can just leave refund as a method with 1 argument, but use args.command for the action parameter instead of adding parameter type (This is probably what @dkess says and in this case my comment can serve as a clarification).
printing/paper
Outdated
| prompt = bold('Refund {} pages to {}? [yN] '.format(refund.pages, refund.user)) | ||
|
|
||
| stringtype = 'Refund' if type == 'refund' else 'Forward' | ||
| prompt = bold(stringtype + ' {} pages to {}? [yN] '.format(payload.pages, payload.user)) |
There was a problem hiding this comment.
If we're already using format here, just use it consistently.
| prompt = bold(stringtype + ' {} pages to {}? [yN] '.format(payload.pages, payload.user)) | |
| prompt = bold('{} {} pages to {}? [yN] '.format(stringtype, payload.pages, payload.user)) |
jyxzhang
force-pushed
the
add-adjustments
branch
from
00dab71 to
3a8d1e6
Compare
8 participants
Depends on ocf/ocflib#182