Closed rmeritz closed 4 years ago
@rmeritz While experimenting with the code, I found it was creating a bunch of extra assignments in the table. They weren't visible from the Block because they were all being created for dates that preceded the interval I had selected, but I could see them in the Grid view.
The problem came from the fact that we created a "moment" instance outside of an iteration and called its subtract
method inside an iteration. Unfortunately (and I mean that: state is awful) moment#subtract
mutates the underlying object. For every assignment we considered, we extended the "start date" 7 days farther into the past.
The tricky part is that this only matters when you are filtering more than one assignment. So even though you wrote a test for the "ignore early dates" condition, it included just one assignment--not enough to trigger the bug.
I moved the "start date" calculation outside of the forEach
predicate so that it's consistent across every iteration, and I extended the test to "prove" the correction.
Does that look right to you?
I moved the "start date" calculation outside of the forEach predicate so that it's consistent across every iteration, and I extended the test to "prove" the correction.
Does that look right to you?
Thanks for the through QA is looks good to me.
The feature was originally described in this issue: https://github.com/bocoup/blocks-capacity-planner/issues/5
Description:
Additional Thoughts about UI/Edge cases:
Tooltip
to describe what the feature did but couldn't easily display a bunch of info in aTooltip
. I think the feature is pretty self-explanatory so this isn't really needed. But I could go back and add aConfirmationDialouge
to provide more context about what is going to happen and allow a user to confirm.producer
orconsumer
is removed between the weeks when you copy, but with the bulk API no errors are thrown and these assignments just don't get created. Seeing as it is an edge case the restaurant/hospital is removed but the delivery is left on the table is an edge case I think this is fine, but maybe it would be better to alert the user.onAssign
function but that doesn't work with the need to do the operation in bulk.Other Cleanup Done:
eslint
config. Addedmocha
to the environment and added a plugin for mocha style guides.mocha
to pass the--debug
argument tonode
directly so I could just add a command topackage.config
.