-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add staff hours #23
Add staff hours #23
Conversation
Just remembered, I'll need to write a custom test to make sure none of the hour ranges overlap. I'll need to finish the ocflib rewrite though |
…one automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cancellation scheme seems reasonable. We might also want to say that PRs that change only this file don't need to be approved as long as they pass the tests.
Also, I think it's okay to have overlapping hour ranges.
schemas/staff_hours.schema.json
Outdated
], | ||
"items": { | ||
"$ref": "#/definitions/staff_hours_entry" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make a type for staff_hours_day or staff_hours_entry_list or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean having the days as an array instead of hardcoded properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be made more concise, see https://github.com/ocf/etc/blob/master/schemas/hours.schema.json#L79-L109
I was thinking it might be hard to display overlapping hour ranges clearly, but maybe if we sort by start hour it'll be ok |
Saturday: | ||
Sunday: | ||
|
||
staff-positions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine for now, but we should plan to replace this with data from officers (see #3)
schemas/staff_hours.schema.json
Outdated
], | ||
"items": { | ||
"$ref": "#/definitions/staff_hours_entry" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be made more concise, see https://github.com/ocf/etc/blob/master/schemas/hours.schema.json#L79-L109
I think it's OK to keep cancellations in this file, but I suppose a separate file could work too. Just make sure it doesn't fail silently if the file isn't found or can't be read. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tyty!
Fixes #6
I decided while I was at it to rewrite the format to the one vaibhav was working on, since it looks better IMO. I'll dig up his PR when rewriting the ocflib code, or if he wants to work on it again, that would be very much appreciated.
I wanted to keep canceling staff hours fast and simple, so I was thinking that
ocflib
would read a file of newline-separated usernames from~staff
and cancel staff hours from those usernames--if the file was malformed or didn't exist, the ocflib code would assume that no staff hours were canceled.If you think that making a PR to
etc
is not too high a barrier for canceling, or have a better method in mind, pls suggest.