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

Implement bhBarcodeScanner Component #2630

Merged

Conversation

jniles
Copy link
Collaborator

@jniles jniles commented Mar 28, 2018

This PR implements the bhBarcodeScanner component and uses it to replace the barcode scanning functionality on the Cash Payments page.

Component Signature

<bh-barcode-scanner on-scan-callback="callback(record)"></bh-barcode-scanner>

Description
The bhBarcodeScanner uses the underlying BarcodeService to query the barcode API and determine which record to return to the client. If the API returns a success HTTP code, it will call the onScanCallback function.

The key difference between this component and the previous implementation is the ability to re-focus the hidden barcode input if focus is lost. This is accomplished by clicking a button that is only visible once focus is lost from the hidden input.

In Action
2018-03-28_14-44-36

Copy link
Collaborator

@mbayopanda mbayopanda left a comment

Choose a reason for hiding this comment

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

The code is excellent and the test also, i just made some comments about messages according the step.

"READ_SUCCESS" : "Found Record",
"READ_ERROR" : "Unreadable Barcode! Please enter the barcode manually.",
"LOST_FOCUS" : "The barcode input has lost focus. Please click \"Read Barcode\" to ready the barcode component.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -121,7 +121,7 @@ function CashFormService(AppCache, Session, Patients, Exchange) {
CashForm.prototype.configure = function configure(config) {

if (config.patient) {
this.setPatient(config.invoices);
this.setPatient(config.patient);
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 good catch

$scope.$digest();
}));

// TODO - how do you find a focused element?
Copy link
Collaborator

Choose a reason for hiding this comment

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

The TODO message is about to find a hidden input box or a focused element ? Because the test below is about a hidden input.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I can see how that isn't clear.

I originally tried to write a test to find a "hidden, focused" input, but never got the focus to work. The CSS selector $(':focus') is supposed to work, but I never got that working...

So it's currently a TODO. I should probably just make a skipped test for it.

angular.element(input).triggerHandler('blur');
$scope.$digest();

expect(isHidden(btn)).to.equal(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

"AWAITING_INPUT" : "En Attente d'Entrée",
"AWAITING_HTTP" : "Code à barres lu! Recherche l'enregistrement....",
"READ_SUCCESS" : "Trouvée",
Copy link
Collaborator

@mbayopanda mbayopanda Mar 29, 2018

Choose a reason for hiding this comment

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

Trouvé et non Trouvée (parce que le mot enregistrement est au masculin).

A good message can be : Enregistrement trouvé instead of simply Trouvé

"AWAITING_HTTP" : "Code à barres lu! Recherche l'enregistrement....",
"READ_SUCCESS" : "Trouvée",
"READ_ERROR" : "Code à barres illisible! Entrez le code à barres manuellement.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

for the "READ_ERROR" a good key and message can be :

"READ_FAILED" : "Enregistrement non trouvé"

and for any errors during the reading :

"READ_ERROR" : "Une erreur est survenue lors de la lecture du code barre"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That is an excellent idea. I'll expand the API for it.

$ctrl.onScanCallback({ record });
})
.catch(() => {
$ctrl.currentStep = steps.READ_ERROR;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the record is not found this code falls in the .catch section, and set the step to an error.

I think if the record is not found the step must be READ_FAILED.

On the server side if we use db.exec, on the client we can have :

if (records.length === 0) {
  $ctrl.currentStep = steps.READ_FAILED;
}

and the READ_ERROR must be set only if we have a real error.


<span class="text-danger" ng-if="$ctrl.currentStep === $ctrl.steps.READ_ERROR">
<i class="fa fa-danger-sign"></i> <span translate>BARCODE.READ_ERROR</span>
</span>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add also a case for a READ_FAILED

@jniles
Copy link
Collaborator Author

jniles commented Mar 29, 2018

@mbayopanda, this is ready for a second review!

Copy link
Collaborator

@mbayopanda mbayopanda left a comment

Choose a reason for hiding this comment

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

LGTM

"READ_SUCCESS" : "Found Record",
"READ_ERROR" : "An error occurred during lookup. Please use another method to find the record.",
"NOT_FOUND" : "Cannot find a record with that code. Please search for the document manually.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 good

READ_SUCCESS : 8,
READ_ERROR : 16,
LOST_FOCUS : 32,
NOT_FOUND : 64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

💃

})
.catch(err => {
const isNotFound = (err.status === 404);
$ctrl.currentStep = isNotFound ? steps.NOT_FOUND : steps.READ_ERROR;
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -36,7 +36,7 @@ function generate(receiptIdentifier, uuid) {
// YYYYYYYY - First characters of the entity UUID
// - returns the full UUID of the entity
function reverseLookup(barcodeKey) {
const code = barcodeKey.substr(0, 2);
const code = barcodeKey.substr(0, 2).toUpperCase();
Copy link
Collaborator

Choose a reason for hiding this comment

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

great 👍

@mbayopanda
Copy link
Collaborator

bors r+

bors bot added a commit that referenced this pull request Mar 29, 2018
2630: Implement bhBarcodeScanner Component r=mbayopanda a=jniles

This PR implements the `bhBarcodeScanner` component and uses it to replace the barcode scanning functionality on the Cash Payments page.

**Component Signature**
```html
<bh-barcode-scanner on-scan-callback="callback(record)"></bh-barcode-scanner>
```

**Description**
The `bhBarcodeScanner` uses the underlying `BarcodeService` to query the barcode API and determine which record to return to the client.  If the API returns a success HTTP code, it will call the `onScanCallback` function.

The key difference between this component and the previous implementation is the ability to re-focus the hidden barcode input if focus is lost.  This is accomplished by clicking a button that is only visible once focus is lost from the hidden input.

**In Action**
![2018-03-28_14-44-36](https://user-images.githubusercontent.com/896472/38032906-a0942c82-3296-11e8-9623-dd777c3dd908.gif)
@bors
Copy link
Contributor

bors bot commented Mar 29, 2018

Build failed

This commit implements a barcode component to make working with barcodes
a bit easier.  It replaces the cash barcode modal's custom logic with
generic logic to be tested.

Key features:
 1. If the input loses focus, the user is notified and a button appears
 to re-establish focus on the hidden input.
 2. Simple API - only a single callback is provided for the success
 case.
 3. Record agnostic.

Closes Third-Culture-Software#1529.
This directive has been superseded now that the barcode component is
implemented.
This commit adds unit tests to the bhBarcodeComponent.
This commit differentiates between a "NOT FOUND" barcode (doesn't exist
in the database) and all other errors.  In theory, we shouldn't
encounter a "NOT FOUND" error if users are only scanning barcodes that
are generated by the system, unless someone has deleted it.

This also refactors the tests to remove repetition.
@jniles jniles force-pushed the feat-barcode-scanning-component branch from e8c0708 to 65e1f27 Compare March 29, 2018 14:15
@jniles
Copy link
Collaborator Author

jniles commented Apr 2, 2018

bors r+

bors bot added a commit that referenced this pull request Apr 2, 2018
2630: Implement bhBarcodeScanner Component r=jniles a=jniles

This PR implements the `bhBarcodeScanner` component and uses it to replace the barcode scanning functionality on the Cash Payments page.

**Component Signature**
```html
<bh-barcode-scanner on-scan-callback="callback(record)"></bh-barcode-scanner>
```

**Description**
The `bhBarcodeScanner` uses the underlying `BarcodeService` to query the barcode API and determine which record to return to the client.  If the API returns a success HTTP code, it will call the `onScanCallback` function.

The key difference between this component and the previous implementation is the ability to re-focus the hidden barcode input if focus is lost.  This is accomplished by clicking a button that is only visible once focus is lost from the hidden input.

**In Action**
![2018-03-28_14-44-36](https://user-images.githubusercontent.com/896472/38032906-a0942c82-3296-11e8-9623-dd777c3dd908.gif)
@bors
Copy link
Contributor

bors bot commented Apr 2, 2018

Build succeeded

@bors bors bot merged commit 65e1f27 into Third-Culture-Software:master Apr 2, 2018
@jniles jniles deleted the feat-barcode-scanning-component branch April 2, 2018 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants