-
Notifications
You must be signed in to change notification settings - Fork 15
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
5364 gis scanner handling #5382
Conversation
Bundle size differenceComparing this PR to
|
onConfirm: () => { | ||
if (newAssetData.current) { | ||
saveNewAsset(newAssetData.current) | ||
.catch(e => error(t('error.unable-to-save-asset', { error: e }))()) |
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 catch doesn't work for some reason!
I tired adding throw e
into the useAssets.document.insert();
useMutation:onError
but that didn't work.
Help please!
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.
It appears to work fine for me? Though you'd need to add {{ error }}
to the 'error.unable-to-save-asset' translation for the error message to show... not entirely sure it should though!
The only time I can seem to see where it doesn't show a message is when newAssetData.current
isn't defined.... I think somewhere we have an example of passing data into confirm (so we can pass directly from handleScanResult, and skip the ref thing completely) - but I can't find right now, and I know we're on a time crunch to get something for RC!
Can be a clean up for another PR, maybe as part of that other issue?
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 would move the catch below the .then() though! If saveNewAsset
errors, we don't want to catch, and then try do the insertLog/redirect, should throw to below that
.catch(e => error(t('error.unable-to-save-asset', { error: e }))()) | ||
.then(async () => { | ||
if (newAssetData.current) { | ||
await insertLog({ |
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 duplicated functionality as the add new asset modal does this too.
Would be nice to consolidate in #5392
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.
Sorry James, seem to be getting CCE not found with id
errors rather than the create modal? Looks like this is because it is not the expected GS1 format... I thought these data matrices should work though? Otherwise everything behaving as expected - so sorry I'm so late on review π
let repository = AssetRepository::new(&ctx.connection); | ||
|
||
let mut result = | ||
repository.query_by_filter(AssetFilter::new().id(EqualFilter::equal_to(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.
NIT: but any reason to use query_by_filter rather than just find_one_by_id
off the row 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.
This won't find deleted items, but I guess either way is ok? If its been deleted what should be the behaviour?
} | ||
|
||
pub fn gtin(&self) -> Option<String> { | ||
self.gs1.get("01").cloned() |
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.
Might be nice to add a comment with link to spec for where you got these from? Though if this is all about to change then maybe fine π
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.
It's all changing...
onConfirm: () => { | ||
if (newAssetData.current) { | ||
saveNewAsset(newAssetData.current) | ||
.catch(e => error(t('error.unable-to-save-asset', { error: e }))()) |
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.
It appears to work fine for me? Though you'd need to add {{ error }}
to the 'error.unable-to-save-asset' translation for the error message to show... not entirely sure it should though!
The only time I can seem to see where it doesn't show a message is when newAssetData.current
isn't defined.... I think somewhere we have an example of passing data into confirm (so we can pass directly from handleScanResult, and skip the ref thing completely) - but I can't find right now, and I know we're on a time crunch to get something for RC!
Can be a clean up for another PR, maybe as part of that other issue?
onConfirm: () => { | ||
if (newAssetData.current) { | ||
saveNewAsset(newAssetData.current) | ||
.catch(e => error(t('error.unable-to-save-asset', { error: e }))()) |
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 would move the catch below the .then() though! If saveNewAsset
errors, we don't want to catch, and then try do the insertLog/redirect, should throw to below that
Merge it! We can clean up in RC2 π |
Fixes #5364
Fixes #4757
π©π»βπ» What does this PR do?
Adds a new endpoint to parse the human readable GS1 AI format data for assets. (NOTE: This is actually an incorrect understanding of how the barcode is structured)
Adds an ability to scan a GS1 formatted barcode and create an asset with pre-populated data.
This is good enough to demo and test the capability, but not the final solution see follow up issue below.
π Any notes for the reviewer?
Follow up issues
π§ͺ Testing
Scan
button (this doesn't show on web)2d Matrix examples:
Here's an example raw string you can use to create new QR Code using https://barcode.tec-it.com/en/DataMatrix
(01)00012345600012(11)241007(21)S12345678(241)E003/002(3121)82(3131)67(3111)63(8013)HBD 116(90)001(91)241007-310101(92){"pqs":"https://apps.who.int/immunization_standards/vaccine_quality/pqs_catalogue/LinkPDF.aspx?UniqueID=3bf9439f-3316-49b4-845e-d50360f8280f&TipoDoc=DataSheet&ID=0β}
PQS Asset Type: E003/002
Serial Number: S1
PQS Asset Type: E003/002
Serial Number: S2
PQS Asset Type: E004/055
Serial Number S1
π Documentation
1.
2.