Fix mass assign issues #189
Conversation
…into mass-assign-bugfix # Conflicts: # src/main/java/mcscheduler/logic/commands/AssignCommand.java
| */ | ||
| // Count for RoleRequirements | ||
| if (!(role instanceof Leave)) { | ||
| if (!requiredRoles.containsKey(role)) { | ||
| requiredRoles.put(role, getQuantityRequiredForRole(shiftToAssign, role)); | ||
| } | ||
| requiredRoles.put(role, requiredRoles.get(role) - 1); | ||
| if (requiredRoles.get(role) < 0) { | ||
| // This assignCommand will exceed the role's requirement | ||
| throw new CommandException( | ||
| String.format(Messages.MESSAGE_INVALID_ASSIGNMENT_NOT_REQUIRED, role, shiftToAssign)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure if this is necessary. Would it work if instead of this, we just put the original check
if (!shiftToAssign.isRoleRequired(role)) {
throw new CommandException(Messages.MESSAGE_INVALID_ASSIGNMENT_NOT_REQUIRED);
}
within the for loop before each model.addAssignment(assignment) call (ie. between lines 128 and 129)?
There was a problem hiding this comment.
Doing it that way will still add the previous assignments before the error assignment to the model. Then the feedback will be a command error rather than a command success, yet it still adds some new assignments. Though if there's a solution to this, do let me know!
There was a problem hiding this comment.
Since assignStringBuilder keeps track of the assignments already added, perhaps we can concat the list of successful assignments to the CommandException thrown.
There was a problem hiding this comment.
I think if it returns an error, you shouldnt accept any part of the command to keep it consistent with other commands. Its also to makes sure the user knows that they're doing something wrong, and not accidentally add unwanted assignments.
| // Prevent duplicates in model & Prevent duplicates in a single call | ||
| if (model.hasAssignment(assignmentToAdd) || assignmentsToAdd.contains(assignmentToAdd)) { |
There was a problem hiding this comment.
Same comment as above, I think we can just place this check in the for loop before adding each assignment as well.
resolves #125 and #126 .
Edited message for MESSAGE_INVALID_ASSIGNMENT_NOT_REQUIRED for clarity ("This role is not required for this position" -> "The %1$s role is not required or is filled for %2$s"