OBPIH-6363 Validation and items requiring attention after clearing pick#4611
OBPIH-6363 Validation and items requiring attention after clearing pick#4611awalkowiak merged 5 commits intofeature/upgrade-to-grails-3.3.10from
Conversation
jmiranda
changed the title
| showAlert: false, | ||
| alertMessage: '', | ||
| itemFilter: '', | ||
| showOnlyErroredItems: false, |
There was a problem hiding this comment.
wouldn't errored -> invalid work better?
There was a problem hiding this comment.
It was also named like that previously, so I would stick to this name at this moment.
| this.setState({ | ||
| alertMessage, | ||
| showAlert: emptyPicks.length, | ||
| }); |
There was a problem hiding this comment.
I assume this is currently done like this (using showAlert flag), but it feels weird to me that we are eventually assinging an alertMessage to the state and in the end we are not displaying it.
Imho from the "logical point of view" we should do:
if (emptyPicks.length) {
this.setState({
alertMessage,
showAlert: true,
});
}but since you are not the one introducing the showAlert flag, treat it just as whining
| .catch(() => { this.props.hideSpinner(); }); | ||
| } | ||
|
|
||
| // Calculating which rows have quantity picked 0, and don't have subitems |
There was a problem hiding this comment.
Please either add in the comment that the returned value is a list of indexes, not rows or change the name to indicate it returns row indexes.
| className={`float-right mb-1 btn btn-outline-secondary align-self-end btn-xs ml-3 ${showOnlyErroredItems ? 'active' : ''}`} | ||
| > | ||
| <span> | ||
| {this.getRowsWithEmptyPicks(values?.pickPageItems).length} |
There was a problem hiding this comment.
Shouldn't this number be dynamically adjusted when the user is fixing lines?
| <button | ||
| type="button" | ||
| onClick={() => { | ||
| this.validateEmptyPicks(values); |
There was a problem hiding this comment.
Yup, because this function sets hasError - an indicator of whether we have to hide/show a row
| // When the quantity picked is equal to 0 we have to check its subitems, | ||
| // because the item can be edited to 0, not cleared. | ||
| if (!item.quantityPicked && !item.picklistItems.length) { | ||
| return [...acc, index + 1]; |
There was a problem hiding this comment.
what is the reason for index + 1 ?
As I am reading a function and the comment we get a list of indexes where quantity is zero or empty and we don't have pickListItems, but we are appending an index of the next item 🤔
There was a problem hiding this comment.
I think I am going to answer this myself...
this is for user readability
"The picked lot is missing at rows: 1,2,3 ..."
instead of
"The picked lot is missing at rows: 0, 1,2 ..."
| getRowsWithEmptyPicks(pickPageItems) { | ||
| return pickPageItems.reduce((acc, item, index) => { | ||
| // When the quantity picked is equal to 0 we have to check its subitems, | ||
| // because the item can be edited to 0, not cleared. | ||
| if (!item.quantityPicked && !item.picklistItems.length) { | ||
| return [...acc, index + 1]; |
There was a problem hiding this comment.
| getRowsWithEmptyPicks(pickPageItems) { | |
| return pickPageItems.reduce((acc, item, index) => { | |
| // When the quantity picked is equal to 0 we have to check its subitems, | |
| // because the item can be edited to 0, not cleared. | |
| if (!item.quantityPicked && !item.picklistItems.length) { | |
| return [...acc, index + 1]; | |
| getIndexesOfRowsWithEmptyPicks(pickPageItems) { | |
| return pickPageItems.reduce((acc, item, index) => { | |
| // When the quantity picked is equal to 0 we have to check its subitems, | |
| // because the item can be edited to 0, not cleared. | |
| if (!item.quantityPicked && !item.picklistItems.length) { | |
| return [...acc, index]; |
I saw Arturs comment suggesting adding a comment which you did, but I feel strongly that this should have a function name with clear intent, this should say that it returns indexes (normal indexes, 0,1,2,3...) and when we need to display these indexes to a user just do indexes.map(it => it+1)
I saw you use this same function later where you had to do this indexes.includes(index + 1) which doesn't sit right with me :/
Also the name might be confusing since getRowsWithEmptyPicks suggests that we return rows (object of values)
4 participants
This one has to be merged after merging OBPIH-6153