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

chore: improve find transfer/purchase modals #5644

Merged

Conversation

jniles
Copy link
Collaborator

@jniles jniles commented May 11, 2021

This PR tightens up the code of both transfer modals for stock entry. Their behavior should be nearly identical.

Here are the improvements:

  1. Both use the headercrumb properly so their formatting looks like other modules.
  2. Both use the bh-filter-toggle directive for toggling the grid's filtering.
  3. Both have a submit button that shows a loading indicator while submitting.
  4. Neither listen on every grid change and update the selectedRow. Instead, that information is only queried on submit.
  5. Both use receipt components for their receipts instead of importing the ReceiptModal service.

jniles added 2 commits May 11, 2021 16:16
This commit refactors and improves the find transfer modal in the stock
entry section. Notably, it adds a working loading button and removes the
listener on every grid change, as well as tightening up the HTML.
This commit brings some minor improvements to the find purchase modal.
@jniles jniles requested a review from jmcameron May 11, 2021 15:44
Copy link
Collaborator

@jmcameron jmcameron left a comment

Choose a reason for hiding this comment

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

I tested this and doing Stock entry from Purchase Orders works nicely.
I was unable to test it using Transfers because I not sure how to do that.

I did notice one small oddity. To reproduce

  • In (fresh) Bhima_test, there are two purchase orders.
  • Edit the status and confirm the second (unconfirmed) one.
  • Do stock entry and select the PO you just confirmed
  • Enter the lots for second item (pred...) and increase the global number to 400 and do the entry for that number
  • Back in the main entry form delete the first item (pretend that a partial delivery was sent).
  • Submit the form to complete the entry
  • Go back to the Purchase registry and notice the status.
  • It is correct but it hides the fact that the order is incomplete.
    image

@jniles
Copy link
Collaborator Author

jniles commented May 11, 2021

I'll take a look at your scenarios a bit more tomorrow. We should have migrated to a new location by then. Thanks for your review!

@jniles
Copy link
Collaborator Author

jniles commented May 23, 2021

@jmcameron finally took a look at this.

You are right, this is confusing. There are two simultaneous errors - one item wasn't received at all and the other one was received over-stock. Yet, we only show the second case.

Perhaps, instead of trying to say "received excessively", we should put something more generic like "Received Incoherently" and just link the "Analysis of Purchase Order" report (note though, this this has errors in master - #5429).

I'll create an issue for further discussion.

@jniles
Copy link
Collaborator Author

jniles commented May 23, 2021

Since this got approval and the discussion revolves around purchase orders, I'm merging this.

bors r+

@bors
Copy link
Contributor

bors bot commented May 23, 2021

Build succeeded:

@bors bors bot merged commit 05ca110 into Third-Culture-Software:master May 23, 2021
@jniles jniles deleted the chore-improve-find-transfer-modal branch May 23, 2021 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants