-
Notifications
You must be signed in to change notification settings - Fork 17
7285 prevent ungiving vaccinations from another store #7323
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
7285 prevent ungiving vaccinations from another store #7323
Conversation
Bundle size differenceComparing this PR to
|
@@ -2099,7 +2099,6 @@ | |||
"messages.no-shipments-yet": "لم يتم إنشاء أي شحنات حتى الآن.", | |||
"messages.no-status-logs": "لم يتم إنشاء سجلات حالة لهذا الجهاز. انقر على زر ”تحديث الحالة“ لإنشاء سجل حالة جديد.", | |||
"messages.no-stock-available": "لا يوجد مخزون متاح", | |||
"messages.no-transaction-other-facility": "لا يتم تسجيل معاملات المخزون للتطعيمات المعطاة في منشآت أخرى", |
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.
message no longer used, removed....
stockLine, | ||
setStockLine, | ||
}: SelectBatchProps) => { | ||
const { data, isLoading } = useStockLines(itemId); | ||
|
||
// Auto-select if there is only one stock line (and not already selected) | ||
useEffect(() => { | ||
if (data?.nodes?.length === 1 && !stockLine) { | ||
if (data?.nodes?.length === 1 && !stockLine && isNewlyGiven) { |
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.
We had some functionality to help speed up process, auto select batch if there is only one. But this was doing some strange things on edit, showing stock line as selected even if I hadn't actually selected the stock line on my original dispensing of the vaccine. So only autoselect for new, or not given -> given
import { VaccinationDraft } from '../api'; | ||
import { VaccinationCourseDoseFragment } from '../api/operations.generated'; | ||
|
||
export const SelectItem = ({ |
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.
This is identical, just pulled out into own component. Item select was tied in with stock line/invoice creation, but we want to record/manage separately
if (!vaccineItemOptions.length) { | ||
return <InfoText>{t('messages.no-vaccine-items-configured')}</InfoText>; | ||
} | ||
const transactionSwitchReason = getSwitchReason(draft, vaccination); |
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.
I found it confusing, there was this SelectItemAndBatch
component, but the create transaction switch display was being managed in the VaccinationModal
component, even though it impacted display of components here. Was hard to follow. This component now handles display of item select, batch picker, and the toggle to create transactions.
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.
const somethingThatDescribesThisCase = (!isHistorical || draft.createTransactions || vaccination?.stockLine)
/> | ||
) : null; | ||
const givenAtOtherStore = | ||
!!vaccination?.given && vaccination.givenStoreId !== storeId; |
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.
Used to disable most inputs if vaccination is in given status from another store
item { | ||
id | ||
name | ||
} |
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.
item info should be available on vaccination, even if stock line is not selected/is for different store
allow_tables_to_appear_in_same_query!(vaccine_course_dose, name_link); | ||
allow_tables_to_appear_in_same_query!(vaccine_course_dose, name); | ||
allow_tables_to_appear_in_same_query!(vaccine_course_dose, item_link); | ||
allow_tables_to_appear_in_same_query!(vaccine_course_dose, item); |
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.
These were in the vaccination row file, figured they should probably live with the relevant table...
stock_line_id, | ||
item_link_id: item_id, |
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.
SO I think if FINALLY have link ids in my head, when it is ok to assign item_link_id = item_id, vs when it is not.
I'll do a wiki write up asap.
But this is fine - when items & item links created, item_link_id is item id, so:
Link ID | Item ID |
---|---|
item_a | item_a |
item_b | item_b |
Then Item A and B get merged, to just be Item A:
Link ID | Item ID |
---|---|
item_a | item_a |
item_b | item_a |
So, conflating item_id to be item_link_id in this direction is fine - both ids continue to exist as link ids and will point back to correct item. Where we need to do lookup is where two records could have different link ids, can't just say where link ID = X, as multiple link ids could actually be pointing to same item
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.
Thanks : #7429
@@ -72,15 +73,32 @@ pub fn validate( | |||
return Err(InsertVaccinationError::FacilityDoesNotExist); | |||
} | |||
} | |||
// If not given, reason is required | |||
if !input.given { |
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.
For some reason, this check was in the "get_stock_line" function... lol?
given: true, | ||
given_store_id: Some(store_id.to_string()), |
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.
The bit that actually solves the PR, assigning given store ID where we mark as given - and in validate method, checking this matches the calling store id
I've run out of time! Heaven forbid it's still here but I will be back next Tuesday if it still needs review 🥲 |
@@ -67,6 +71,19 @@ impl VaccinationNode { | |||
} | |||
} | |||
|
|||
pub async fn item(&self, ctx: &Context<'_>) -> Result<Option<ItemNode>> { | |||
let loader = ctx.get_loader::<DataLoader<ItemLoader>>(); |
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.
Hmm, at this point we don't need a join in the repository.
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.
Yeah... or like BasicItemNode, which is only the fields from ItemRow? Bc need to use loader, to get all the fields required ItemNode...
// If not given, reason is required | ||
if input.given == Some(false) { | ||
if input.not_given_reason.is_none() { | ||
return Err(UpdateVaccinationError::ReasonNotProvided); | ||
}; | ||
}; | ||
|
||
// If selected item is changing - validate it |
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.
I was wondering if there is a description somewhere about what happens in the different use case for immunizations/vaccinations ? (like if i ungive vaccination, if vaccination is given, with stock line issued, and then item is changed etc..)
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.
ugh not really... public docs do sort of discuss the different things you can do, but not clear set of use cases like you describe here... where would you write that up??
) | ||
.unwrap(); | ||
|
||
assert_eq!(result.vaccination_row.given, false); |
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.
worth checking that store id was set correct, and unset in the one below
// Allow un-selecting of stock line, if don't want to record | ||
// transaction | ||
setStockLine={newStockLine => | ||
setStockLine( |
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.
"toggleStockLine"
disabled={givenAtOtherStore} | ||
dose={dose} | ||
draft={draft} | ||
updateDraft={updateDraft} | ||
/> |
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.
Almost feel like there should be explicit button to change stock line (or delegate error to server, or something like this), allowing SelectBatch to be simpler, and just displaying stock line in a different component (when it's been already set)
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.
Thanks Lache, there is nothing in this PR that I can see that would stop approval. I haven't thougth too deeply about use cases here, I think we need to ecourage QA to make try different use case.
thougth too deeply about use cases here
I deed think of a few use case, and all of them were covered in this PR, and I was impressed
Fixes #7285
👩🏻💻 What does this PR do?
Disables editing of vaccination data (expect comment) from other stores, once vaccination has been given.
Also adds item_link_id to vaccination, so we can see which vaccine item was given to a patient even when it was given in another store. Before, we only determined item via related stock line/invoice, which only exists for the "giving" store.
💌 Any notes for the reviewer?
Apologies, little bit of frontend refactoring, found it hard to reason about where to add conditions, will add comments
There is breaking API change in that we now expect passed in stock line and item id to match, but i don't think vaccination graphql is used externally...
🧪 Testing
given
vaccinations are not editable, except for comment field📃 Documentation
1.
2.
📃 Reviewer Checklist
The PR Reviewer(s) should fill out this section before approving the PR
Breaking Changes
Issue Review
Tests Pass