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

(fix) O3-3557: Ensure selectable batches have sufficient quantities f… #109

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

mogoodrich
Copy link
Member

…or dispensing

Requirements

  • This PR has a title that briefly describes the work done, including the ticket number if there is a ticket.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

Screenshots

Related Issue

Other

? quantityRemaining
: medicationDispense.quantity.value
}
value={medicationDispense.quantity.value}
Copy link
Member Author

@mogoodrich mogoodrich Jul 16, 2024

Choose a reason for hiding this comment

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

So I think we should revert this to revert this part of the previous commit for O3-3557, as it is restricting the quantity dispensed even if the quantity dispensed flag is not set. I also think it's confusing, because it's not ever letting the user type in a number greater than the quantity remaining... I think it's better to allow the user to do this but give an error. Also, as it currently works the "Dispense" button doesn't appear to be enabled until you re-type the number.

I think I do understand the reasoning for this now... prior to this change, when starting a new dispense, the default value on this form is set to whatever the order quantity was... which is likely not what you want if the order quantity is less than the quantity remaining. However, if we want to enforce this, I think we need to do it when the the medication dispense object is initially set up (which I think is in the "initiateMedicationDispenseBody" method in the medication dispense resource).

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that admittedly I haven't tested this with the stock management functionality, as I don't have a demo environment for that, not sure if further changes are needed if we revert this to meet your use case.

Choose a reason for hiding this comment

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

So I think we should revert this to revert this part of the previous commit for O3-3557, as it is restricting the quantity dispensed even if the quantity dispensed flag is not set. I also think it's confusing, because it's not ever letting the user type in a number greater than the quantity remaining... I think it's better to allow the user to do this but give an error. Also, as it currently works the "Dispense" button doesn't appear to be enabled until you re-type the number.

I think I do understand the reasoning for this now... prior to this change, when starting a new dispense, the default value on this form is set to whatever the order quantity was... which is likely not what you want if the order quantity is less than the quantity remaining. However, if we want to enforce this, I think we need to do it when the the medication dispense object is initially set up (which I think is in the "initiateMedicationDispenseBody" method in the medication dispense resource).
Am tagging @FelixKiprotich350 to help test the change

Copy link
Contributor

Choose a reason for hiding this comment

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

So Antony, from your views I think we can keep the filtering of the batches and on selecting that do not change the quantity to dispense ... Let that component remain the way it is.... So generally it will be a filter of batches but let the user input the dispense value

Choose a reason for hiding this comment

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

Thanks, @FelixKiprotich350. Please go ahead and provide your review and approval.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes @FelixKiprotich350 please let me know if this looks okay to you.

Sorry if I don't understand the how the stock integration functionality it supposed to work. If the idea is that selecting a batch should change the quantity to dispense, that seems doable, but I would think it should be done via function that is passed into the StockDispense component, like the setAvailableQuantity currently is. I don't think we can just set the available quantity when the user selected a batch and then determine the dispense amount from that. Does that make sense? I'm happy to look at some screen shots to understand the stock dispense functionality better. fyi @ojwanganto

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah sure this looks fine to let the user provide his quantity

Copy link
Contributor

Choose a reason for hiding this comment

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

hello @mogoodrich i am attaching a short video of how the component works within the stock-dispensing module
screen-capture.webm

Copy link
Contributor

@FelixKiprotich350 FelixKiprotich350 left a comment

Choose a reason for hiding this comment

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

hello @mogoodrich and @ojwanganto , i have checked the code and i have a suggestion, to keep the previous flow of the medication-dispense-review component i can just undo the little changes on that component specifically the new code that updates the quantity based on the selected batch that i added within this PR . In this case we will have achieved the intentions to have only possible selectable and consumable batches and let the user decide on the quantity. incase there is an error let it be raised by that component instead of the batch selection component.
Just like it worked before the user will be able to edit his/her preferred quantity against the prescribed quantity

? quantityRemaining
: medicationDispense.quantity.value
}
value={medicationDispense.quantity.value}
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah sure this looks fine to let the user provide his quantity

Copy link

@ojwanganto ojwanganto left a comment

Choose a reason for hiding this comment

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

@mogoodrich
Copy link
Member Author

hello @mogoodrich and @ojwanganto , i have checked the code and i have a suggestion, to keep the previous flow of the medication-dispense-review component i can just undo the little changes on that component specifically the new code that updates the quantity based on the selected batch that i added within this PR . In this case we will have achieved the intentions to have only possible selectable and consumable batches and let the user decide on the quantity. incase there is an error let it be raised by that component instead of the batch selection component. Just like it worked before the user will be able to edit his/her preferred quantity against the prescribed quantity

Sure that work's for me, @FelixKiprotich350 feel free to issue a new PR for that (or just revert).

@FelixKiprotich350
Copy link
Contributor

following your suggestions i have undo the changes affecting the dispensequantity and reverted back to the previous state

@ojwanganto
Copy link

following your suggestions i have undo the changes affecting the dispensequantity and reverted back to the previous state

Is there a PR for this change? Also, kindly share a video showing how things look after the change

@FelixKiprotich350
Copy link
Contributor

FelixKiprotich350 commented Jul 18, 2024 via email

@mogoodrich
Copy link
Member Author

Thanks @FelixKiprotich350 and @ojwanganto , merging this in... I will take a look at the other PR now...

@mogoodrich mogoodrich merged commit c72448f into main Jul 18, 2024
4 checks passed
@mogoodrich mogoodrich deleted the O3-3557-2 branch July 18, 2024 16:38
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.

3 participants