-
Notifications
You must be signed in to change notification settings - Fork 5
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 worker unavailability #84
Add worker unavailability #84
Conversation
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.
Looks great mostly! Just have a couple implementation choices to ask about (open to everyone else to reply too I guess?)
|
||
if (!arePrefixesPresent(argMultimap, PREFIX_NAME, PREFIX_ADDRESS, PREFIX_PHONE, PREFIX_PAY) | ||
if (!arePrefixesPresent(argMultimap, PREFIX_NAME, PREFIX_ADDRESS, PREFIX_PHONE, PREFIX_PAY, PREFIX_ROLE) |
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.
I think role is an optional argument so perhaps no need to check if it is present.
Actually since we are on this topic, I think pay isn't either? But if these are necessary then perhaps updating the User Guide will do.
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.
Mm I just thought that it made sense for all workers to have a role so I included it as a compulsory field, but we can take it out also I guess? So if a worker doesn't have any roles specified, then we can assign them to any role in a shift
If we're going to be implementing the Pay calculation feature then we should make Pay compulsory
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.
I think no role should be unable to assign to anything, kind of like when someone new join and havent do the training I guess. Makes sense for pay to be compulsory, so agreed there.
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.
Ah ok agreed for role! Will make the change
FRI, | ||
SAT, | ||
SUN | ||
} |
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.
Thanks for abstracting this out properly xD
public static final String MESSAGE_CONSTRAINTS = | ||
"Shift time should only contain one of the following values: AM, PM"; | ||
public final TimeValue time; | ||
"Shift time should only contain one of the following values: AM, PM, FULL"; |
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.
I'm not sure if the FULL
value should be implemented into the code itself, or should it just be an argument that we accept that from user input that is essentially a wrapper for creating both an AM
and a PM
unavailability. Not sure if this might result in duplicates when creating shifts of AM
and of FULL
of the same day?
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.
Made the change to convert FULL to AM and PM unavailabilities
Codecov Report
@@ Coverage Diff @@
## master #84 +/- ##
============================================
- Coverage 65.03% 64.51% -0.52%
- Complexity 568 587 +19
============================================
Files 100 105 +5
Lines 2059 2181 +122
Branches 231 248 +17
============================================
+ Hits 1339 1407 +68
- Misses 657 702 +45
- Partials 63 72 +9
Continue to review full report at Codecov.
|
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.
Looks great!
Resolves #62
Resolves #64