feat: shift as an option for allocation time period#1563
Open
zacharyjhankin wants to merge 6 commits into
Open
feat: shift as an option for allocation time period#1563zacharyjhankin wants to merge 6 commits into
zacharyjhankin wants to merge 6 commits into
Conversation
zacharyjhankin
changed the title
zacharyjhankin
changed the title
jekabs-karklins
left a comment
jekabs-karklins
left a comment
Contributor
There was a problem hiding this comment.
Just some minor comments
| unit: AllocationTimeUnits | ||
| ) => number; | ||
|
|
||
| export const getSecondsFromAllocationTimeUnits: AllocationTimeUnitConverter = ( |
Contributor
There was a problem hiding this comment.
suggestion: maybe it is a good idea to call the filename the same as the exported function
| ) => { | ||
| // NOTE: Default AllocationTimeUnit is 'Day'. | ||
| // 'Shift' is configurable per organisation; it defaults to 8 hours here. | ||
| switch (unit) { |
Contributor
There was a problem hiding this comment.
nitpick: adding a quick unit test would be nice
| return timeAllocation * 7 * 24 * 60 * 60; | ||
| case AllocationTimeUnits.Shift: | ||
| return timeAllocation * 60 * 60 * 8; | ||
| default: |
Contributor
There was a problem hiding this comment.
Suggested change
| default: | |
| case AllocationTimeUnits.Day: | |
| default: |
Can we have this? so it's more intutitve by looking at the code what is the default?
| unit: AllocationTimeUnits | ||
| ) => { | ||
| // NOTE: Default AllocationTimeUnit is 'Day'. | ||
| // 'Shift' is configurable per organisation; it defaults to 8 hours here. |
Contributor
There was a problem hiding this comment.
Suggested change
| // 'Shift' is configurable per organisation; it defaults to 8 hours here. | |
| // NOTE: Shift duration defaults to 8 hours. | |
| // To customize this per organization, inject a configuration provider | |
| // that supplies the specific shift duration for the facility. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds a shift as a new unit for calls. This ability is vital for diamond as we allocate beam time from the synchrotron based on 8 hour units, 3 per day. All existing proposals base the scientists requests for beam time in shift units, so this allows us to be consistent with the existing infrastructure. It also prevents requests for 3 hours.
The way I have coded this allows other organisations to customise the shift unit to be any length of time they want. So it could be useful to them.
Motivation and Context
At diamond we use shift as a time unit for our calls which is 8 hours. This PR allows us to have shifts and lets other companies edit it.
How Has This Been Tested
Changes
A new allocation period unit.
Tests included/Docs Updated?