Skip to content
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

#638 reset out of date stocktake items #655

Merged
merged 11 commits into from
Apr 26, 2018

Conversation

Chris-Petty
Copy link
Contributor

@Chris-Petty Chris-Petty commented Apr 13, 2018

Merge #653 first, this is branched off that. Or rebase this to that branch?

@Chris-Petty
Copy link
Contributor Author

@andreievg this should be go for review

Copy link
Contributor

@andreievg andreievg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from a couple of code comments, i've added a note in #638. Please check it out.

get isOutdated() {
if (this.batches.some(batch => batch.isSnapshotOutdated)) return true;
// Check all item batches (with stock) are included by finding matching id in
// the stocktakeBatches for this this stocktakeItem
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this this

@@ -47,8 +47,9 @@ export const modalStrings = new LocalizedStrings({
start_typing_to_select_supplier: 'Start typing to select supplier',
stock_quantity_greater_then_zero: 'Stock quantity must be greater then zero before finalising',
stocktake_no_counted_items: "Can't finalise a stocktake with no counted items",
stocktake_invalid_stock: 'In this stocktake:\n{0} item(s) counted will cause negative stock levels for for at least one batch\n{1} uncounted item(s) are out of date, to count them now will cause incorrect adjustments\n\nWould you like to reset counts and snapshots for these items?',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for for

*/
reset(database) {
database.delete('StocktakeBatch', this.batches);
this.item.batches.forEach(itemBatch => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.item.batchesWithStock

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks a bit funky sometimes like this - if the SOH is 0 there will be no batches at all when the line is reset. Which is totally correct, just seems a little weird.

onClose={this.closeModal}
title={this.getModalTitle()}
>
{this.renderModalContent()}
</PageContentModal>
<ConfirmModal
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing a reason, but why is ConfirmModal not the same as others ? i.e. as content of PageContentModal ?
I would understand a requirement for a new Modal if ConfirmModal is not dismissible (i.e. either way the snapshot quantity is refreshed and a message showing that it's being done, maybe even with item names)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The page content modal only covers the page, not the top nav bar - this means finalise was accessible.

Copy link
Contributor Author

@Chris-Petty Chris-Petty Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I omitted item names because there was already a lot of text in the modal. If the user is on to it they can sort by the Actual Quantity column to find the reset ones.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A modal that stops finalisation due to negative quantity adjustment seems to fit the item names quite well. Plus the warning message here can be a lot shorter, something like I've mentioned in issue 638 -> Quantities for these items have changed while the stocktake is in progress (due to stock issued or received), both Counted Quantity and Snapshot quantity will be reset: {list items}

I don't think we should allow cancel button

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to get @alainsussol to re-translate 😝

Sounds good, I'll get that done... before lunch!?

@Chris-Petty
Copy link
Contributor Author

Chris-Petty commented Apr 24, 2018

Pr requests addressed @andreievg (though I dig if you're waiting on discussions at #638 )

@andreievg andreievg merged commit 8616741 into development Apr 26, 2018
@andreievg andreievg deleted the #638-reset-out-of-date-stocktakeItems branch April 26, 2018 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants