I can't imagine this to be important but throughout the years it's been
expected by users that "abort" can be quoted and passed to a plugin like
one that prints barcodes.
It's still not possible to pass a literal string `abort` to a follow-up
prompt, leaving this feature only available to advanced users who (hope
to) know what they're doing.
The ancient decision to let undo perform the checkout by itself still
makes sense from a UX perspective, but keeps requiring specific handling
of edge cases.
In this case, the easiest way to deal with trailing input is to just
abort entirely.
Also: updated lib/RevBank/Plugins.pm to import 'isa' and get up to 5.32
level.
There's a slight mismatch between what users experience as a command,
and how commands are defined in RevBank. Specifically, the common input
"<productid> <username>" is two separate commands: the first adds the
product to the cart, the second finalizes the transaction. This also
means that "<productid> <username> <productid> <username>" was four
separate commands, resulting in TWO transactions.
That's all fine and useful, but when using this advanced input method,
where input is split on whitespace, it lead to unexpected results if
there are insufficient arguments for the follow-up questions of a
command. For example, "take jantje 10 take pietje 10" will interpret the
second "take" as the description, then "pietje" als the first command of
a new transaction, and finally, "10" which is typically not a valid
command. It is much more likely that the user intended two separate
"take" commands and just forgot to provide the description argument, but
RevBank had no way of inferring that intent.
From this commit on, whenever the user intends to enter further input
words beyond the one that finalizes a transaction ($cart->checkout), a
';' is required. If trailing input is present, the checkout is refused
and the user gets a retry prompt.
Similarly, if the user indicates the intention of having finished a
command by inserting a ';' while there are insufficient words in the
command line to satisfy all follow-up prompts (command arguments), the
rest of the command line is rejected with a retry prompt.
There is, however, still no specific requirement for a ';' separator
after a command that does not finalize a transaction (e.g. "<productid>
<username>" or even "<productid> x2 <productid> <username>" remains
valid), or for a command that precedes a ';' to finalize a transaction
(e.g. "<productid>; <username>;" is also valid).
This change catches many, but not all, mistakes.
Was already implicitly required (since 59387ddb) because RevBank::Amount
uses the "isa" feature, which was introduced in Perl 5.32 (but no longer
experimental since 5.36, not 5.32 as the old comment said).
Perl 5.32 was released in June 2020, and ships with Debian bullseye
("oldstable") which was released in August 2021.
A space had a custom plugin that died during hook_checkout, which caused
the CHECKOUT lines to be logged without the corresponding BALANCE, and
indeed no account balances were updated. While the plugin had a bug, it
should not cause a half transaction in RevBank.
After some hesitation, I went with ON ERROR RESUME NEXT because if a
hook throws an exception, that should not interfere with other plugins
(the hook can return ABORT if this it was intentional), including the
calling plugin. An error message is printed (but not logged... TODO: add
hook_plugin_fail to plugins/log) but the show must go on.
During hook_checkout_prepare, however, nothing is set in stone yet, so
this could be used for something that might die, and this instance of
call_hooks() is now the one place where a failing hook should result in
the transaction getting aborted. For this, call_hooks() now returns a
success status boolean. Maybe it would make sense in more places, but I
didn't identify any such calls yet.
RevBank::Cart->checkout used to return a success status boolean, but it
could just as well just die (indirectly, to abort the transaction) since
it can't be called a second time within the same transaction anyway
(because ->set_user must be called exactly once), so continuing with the
same transaction can't result in anything useful anyway.
In some places, error messages were slightly improved to contain a bit
more information.
When asked to fix the bug, it came up with a different regex, which
would completely change what's valid and what's not, so that's totally
wrong:
/^\s*(-)?([0-9]+)(?:[,.]([0-9]{1,2}))?\s*$/
When asked to fix it in another way, without changing the regex, it
suggested stripping the sign completely, which is even more wrong.
So I fixed it myself :)
- No more red messages
- Accept "yes" case insensitively
- Change entry description and amount so the voiding is logged, which is
more code but less complex than passing an attribute to be used during
checkout.
(Bumps version to 3.8 because admins should update the plugin list.)
Deduplication didn't work on quantified additions, i.e. if you added
"20x clubmate" when there was already clubmate in the cart, it would add
just ONE item, and have a lingering message that the next thing would be
multiplied by 20.
This old bug was especially annoying if there is a barcode "20x
clubmate" to scan 20 bottles (which is the size of a crate), and this is
repeated.
The fix also uncovered another bug: newly added entries were selected
too early. There are two hooks, hook_add_entry and hook_added_entry, and
of course the selection should happen in between, not before the former.
No entry in UPGRADING.md, because I think it is extremely unlikely that
any plugin author will have used the selection feature yet, which is
very new.
Experimental code (never committed) had ANSI escape sequences there, and
required manual padding. Those were gone, but I forgot to change the
manual padding into normal sprintf padding.
This also makes it explicit that the left alignment is actually intended
here. (Actually looks better here.)
- Promote to public function since it's used in other plugins anyway
- Move resolving of addons to read_products (print errors immediately)
- Cache product list based on mtime; mostly to reduce the amount of spam
from errors as performance was never an issue.
- Cache product object in cart entry, so statiegeld_tokens plugin
doesn't have to do the lookup all over again.
In hindsight, it was a bad idea to allow manipulating the cart (entries)
in hook_checkout, because that hook is used by the `log` plugin. You now
get unused entries in the log.
Although that plugin should maybe have used hook_checkout_done, existing
log file readers (including scripts) and custom plugins may depend on
the CHECKOUT items in the log being before the BALANCE items.
Fixes deadlock if hook_checkout returns ABORT.
One of these days I want to implement the abort mechanism through
exceptions, even though that means handling it explicitly in more
places. Or maybe *because* that means handling it explicitly in more
places.
Apparently nobody uses "return ABORT;" in a hook, because it emitted an
ugly warning. main::abort() takes a list, so destructuring the message
to a scalar was wrong.
add_info was a thing that grew organically to account for hidden
contras, but just wasn't right. The assumption was that if the
contra account is hidden, the contra itself should be hidden from
view - the sign of the amount would be wrong anyway.
The correct approach, however, would of course to flip the sign so it
matches the user's perspective, and to add a separate description string
to display to the user.
One more character so values >= 100.00 don't mess up the columns, at
least up to 999.99. I hope nobody's actually parsing the logs with fixed
character offsets.
It stored the old content, so effectively not changing the file.
I don't really understand why *this* was the version I committed,
because I was sure I tested it and it worked :)
