-
Notifications
You must be signed in to change notification settings - Fork 105
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
Add page for finding and merging duplicate lots #5361
Add page for finding and merging duplicate lots #5361
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmcameron since you asked for a review of the functionality, I won't dive into the code yet.
Overall, looks really good for a first cut. Some suggestions:
- Instead of "Find Lots" in the action menu, let's just call it "Merge Lots". To me, this is intuitive: instead of finding the lots, we've found them, now we are opening the modal to merge them.
- Can you put a loading indicator on the loading button when merging lots? It seems like we should be waiting for that to be done on the server, then closing the modal, then reloading the page. It dismisses almost immediately, which surprised me. Most other modals that do something in the system (trial balance, credit note, etc) hang out until the action is performed with a loading indicator.
Here is what that looks like on a "good 2G network" that I emulated.
Again, haven't taken a look at the code, but it surprised me how quick that was!
768de98
to
3bfae3e
Compare
@jniles This is now ready for a real review. QUESTION: Should we remove the "Find Duplicate Lots" in the Lots registry action menu? |
No, I do not think so. Let's wait until a couple of releases, then we'll remove it. I think that folks are much more inclined to absent-mindedly look at that registry (which they are using all the time) than they will be for this new module. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionally, this looks good to me. I've made a few clean up comments, but none of them are serious. Once addressed, this PR can be merged.
client/src/modules/stock/lots-duplicates/templates/action.cell.html
Outdated
Show resolved
Hide resolved
bors delegate+ |
✌️ jmcameron can now approve this pull request. To approve and merge a pull request, simply reply with |
372a222
to
5d99380
Compare
5d99380
to
f07f4c7
Compare
bors r+ |
Build succeeded: |
Create a page where operators can find and merge potential duplicate lots.
Note that this PR adds a new page under the Stock navigation menu: "Find Duplicate Lots". In order to use or test this, be sure to edit the user role permissions and enable the new page (under the Stock section).
Closes #5166
Testing