ulsdevteam / lcsu

0 stars 0 forks source link

Clarify output, especially in event of error #9

Closed ctgraham closed 5 years ago

ctgraham commented 5 years ago

https://github.com/ulsdevteam/lcsu/blob/fd1e3dc5a1307831f0275fafc29c4aa746a60f90/src/Controller/ModulesController.php#L138-L141

This code doesn't trap any errors in PhpNetworkLprPrinter::printShelfLabel() and reports success for each shelf.

PhpNetworkLprPrinter::printShelfLabel() should either return success or failure (or raise an exception), and the printLabels() method should respond to that. The user probably doesn't need a flash message for every success (they could be consolidated into a single flash message), but should receive feedback on failure. For example, no feedback is currently given if $lpr is not initialized successfully.

Other instances are better structured for the results of the $lpr calls, but do not alert the user if the $lpr was unable to be initialized: https://github.com/ulsdevteam/lcsu/blob/fd1e3dc5a1307831f0275fafc29c4aa746a60f90/src/Controller/ShelvesController.php#L152-L159

ctgraham commented 5 years ago

Partially resolved via: https://github.com/ulsdevteam/lcsu/commit/0be6eeaa0237496a2d7169c29d8f42c501f9215f#diff-927d0e425df88d427cb8736ae6dc96f9R134 and https://github.com/ulsdevteam/lcsu/commit/0be6eeaa0237496a2d7169c29d8f42c501f9215f#diff-256e16d0d4c91ba67f5da8aacec3bd74R148

If $lpr cannot be instantiated, an error will be shown.

The return value of $lpr->printShelfLabel remains unchecked, however.

ctgraham commented 5 years ago

@edensung25, does PHP implicitly return values from the last function statement? If so, the result of calls to printShelfLabel and printTrayLabel will come from printText:

https://github.com/ulsdevteam/lcsu/blob/92deadf280ae7c2ac98bb9522dd82b509dcbf421/src/Utility/PhpNetworkLprPrinter.php#L278-L285

via

https://github.com/ulsdevteam/lcsu/blob/92deadf280ae7c2ac98bb9522dd82b509dcbf421/src/Utility/PhpNetworkLprPrinter.php#L253

This will mean that the test of $errMsg = $lpr->printShelfLabel(...), etc. will be inverted.

https://github.com/ulsdevteam/lcsu/commit/92deadf280ae7c2ac98bb9522dd82b509dcbf421#diff-256e16d0d4c91ba67f5da8aacec3bd74R148

Note: PhpNetworkLprPrinter may need to be modified to explicitly return, if PHP doesn't do implicit returns.

ctgraham commented 5 years ago

The return of PhpNetworkLprPrinter's printShelfLabel and printTrayLabel remains out of sync.

https://github.com/ulsdevteam/lcsu/blob/058735aa732fbc9134b454ac40f60cbbb981c603/src/Utility/PhpNetworkLprPrinter.php#L239-L276

compare: https://github.com/ulsdevteam/lcsu/blob/058735aa732fbc9134b454ac40f60cbbb981c603/src/Utility/PhpNetworkLprPrinter.php#L247 vs. https://github.com/ulsdevteam/lcsu/blob/058735aa732fbc9134b454ac40f60cbbb981c603/src/Utility/PhpNetworkLprPrinter.php#L253 with https://github.com/ulsdevteam/lcsu/commit/058735aa732fbc9134b454ac40f60cbbb981c603#diff-256e16d0d4c91ba67f5da8aacec3bd74R146

and

https://github.com/ulsdevteam/lcsu/blob/058735aa732fbc9134b454ac40f60cbbb981c603/src/Utility/PhpNetworkLprPrinter.php#L264 vs. https://github.com/ulsdevteam/lcsu/blob/058735aa732fbc9134b454ac40f60cbbb981c603/src/Utility/PhpNetworkLprPrinter.php#L275 with https://github.com/ulsdevteam/lcsu/commit/058735aa732fbc9134b454ac40f60cbbb981c603#diff-53bcb7439999e8c5d3fb34d7b4351003R349

We need to document the @return via PHP doc and make the usage consistent. Either: @return boolean|string true indicates success, false or string indicates error or, better: @return boolean true indicates success, on false check getErrStr() for possible message

ctgraham commented 5 years ago

Should the phrase "Error" be translated in this commit? https://github.com/ulsdevteam/lcsu/commit/9993b62018d93522633816d6a509b2bc65211341#diff-927d0e425df88d427cb8736ae6dc96f9R135

Since the error message is unlikely to be translated, and is likely to be English, perhaps the string "Error" should be left untranslated, as you have done.

I'm tempted to say, however, that either the prefix of "Error:" should be translated, or the whole concatenation should be translated.

What do you think?

edensung25 commented 5 years ago

Translate prefix only!