-
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
Remove invalid assignments after worker/shift edit #113
Remove invalid assignments after worker/shift edit #113
Conversation
…into remove-invalid-assignments
…into remove-invalid-assignments
Codecov Report
@@ Coverage Diff @@
## master #113 +/- ##
============================================
- Coverage 66.53% 66.18% -0.35%
Complexity 715 715
============================================
Files 127 127
Lines 2552 2570 +18
Branches 329 333 +4
============================================
+ Hits 1698 1701 +3
- Misses 743 756 +13
- Partials 111 113 +2
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.
WorkerEditCommand
looks good but I think there is a somewhat major problem with ShiftEditCommand
.
Role assignedRole = assignment.getRole(); | ||
Set<Role> editedWorkerRoles = editedWorker.getRoles(); | ||
if (!editedWorkerRoles.contains(assignedRole)) { |
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.
Role assignedRole = assignment.getRole(); | |
Set<Role> editedWorkerRoles = editedWorker.getRoles(); | |
if (!editedWorkerRoles.contains(assignedRole)) { | |
if (!editedWorker.isFitForRole(assignment.getRole())) { |
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 better, will change!
Set<RoleRequirement> newRoleRequirements = editedShift.getRoleRequirements(); | ||
newRoleRequirements.forEach(roleRequirement -> newRoles.add(roleRequirement.getRole())); |
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.
Perhaps these two lines can be moved out of and before the for
loop.
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've made this into a method called getRoles() inside the Shift class!
if (!newRoles.contains(assignmentRole)) { | ||
assignmentsToDelete.add(assignment); |
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.
Seems like this only works for the case where a role requirement that was present in a shift is completely removed from it. I don't think the case where the quantityRequired
of a role is changed but the role
remains unchanged has been accounted for.
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.
Yes, thanks for reminding me! Will add the checks for this
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.
LGTM other than the merge conflict to be resolved
Resolves #109