From fffb2d72e973bc37c7d18640b677ff18cf6af508 Mon Sep 17 00:00:00 2001 From: Juerd Waalboer Date: Sun, 12 Feb 2023 17:53:14 +0100 Subject: [PATCH] Fix deduplication bug, refactor deduplication to own plugin (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. --- UPGRADING.md | 8 ++++++++ lib/RevBank/Cart.pm | 6 +++--- plugins/deduplicate | 31 +++++++++++++++++++++++++++++++ plugins/market | 17 +++++------------ plugins/products | 18 ++++++------------ plugins/revspace_barcode | 6 +++++- plugins/statiegeld | 18 +++++------------- revbank | 2 +- revbank.plugins | 1 + 9 files changed, 65 insertions(+), 42 deletions(-) create mode 100644 plugins/deduplicate diff --git a/UPGRADING.md b/UPGRADING.md index 7c846ca..106fe37 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -1,3 +1,11 @@ +# (2023-02-12) RevBank 3.8 + +## Update your `revbank.plugins` + +Deduplication is moved from individual plugins to a plugin that does that. If +you want to keep deduplication of cart items, and you probably do want that, +add `deduplicate` to `revbank.plugins` just below `repeat`. + # (2022-12-25) RevBank 3.6 ## Update your `revbank.plugins` diff --git a/lib/RevBank/Cart.pm b/lib/RevBank/Cart.pm index ba6e77f..7c7df7c 100644 --- a/lib/RevBank/Cart.pm +++ b/lib/RevBank/Cart.pm @@ -21,6 +21,8 @@ sub add_entry($self, $entry) { push @{ $self->{entries} }, $entry; $self->{changed}++; + $self->select($entry); + RevBank::Plugins::call_hooks("added_entry", $self, $entry); return $entry; @@ -35,9 +37,7 @@ sub add($self, $amount, $description, $data = {}) { # ->add($user, ...) => use $cart->add(...)->add_contra($user, ...) # ->add($entry) => use $cart->add_entry($entry) - my $entry = $self->add_entry(RevBank::Cart::Entry->new($amount, $description, $data)); - $self->select($entry); - return $entry; + return $self->add_entry(RevBank::Cart::Entry->new($amount, $description, $data)); } sub select($self, $entry) { diff --git a/plugins/deduplicate b/plugins/deduplicate new file mode 100644 index 0000000..2fd042e --- /dev/null +++ b/plugins/deduplicate @@ -0,0 +1,31 @@ +#!perl + +# Deduplication merges duplicate entries in the cart, e.g. +# 3x cola + 4x cola = 7x cola. +# +# Plugins that support this, set the "deduplicate" attribute to a string key +# that is used to determine which entries are equal. It is the responsibility +# of the plugin that sets this, to ensure that the entries are indeed exactly +# the same, if their deduplicate keys are equal. +# +# The recommended value for the deduplicate attribute is join("/", $plugin_id, +# $unique_id), where $plugin_id can be obtained from $self->id in interactive +# methods or $class->id in hooks. Including the plugin id avoids deduplicating +# across plugins, that are probably not aware of eachothers $unique_id's. + +use List::Util qw(sum any); + +sub hook_added_entry($class, $cart, $added_entry, @) { + my $key = $added_entry->attribute('deduplicate') or return; + + my @dedupe = grep { + $_->attribute('deduplicate') eq $key + } $cart->entries('deduplicate'); + + @dedupe >= 2 or return; + + $dedupe[0]->quantity(sum map { $_->quantity } @dedupe); + $cart->select($dedupe[0]); + + $cart->delete($_) for @dedupe[1 .. $#dedupe]; +} diff --git a/plugins/market b/plugins/market index 767d558..7fed980 100644 --- a/plugins/market +++ b/plugins/market @@ -35,21 +35,14 @@ sub command :Tab(market,&tab) ($self, $cart, $command, @) { my $space = parse_amount($product->{ space }) or return NEXT; my $description = $product->{description}; - my @existing = grep { - $_->attribute('plugin') eq $self->id and - $_->attribute('product_id') eq $command - } $cart->entries('plugin'); - - if (@existing) { - $existing[0]->quantity($existing[0]->quantity + 1); - $cart->select($existing[0]); - return ACCEPT; - } - $cart->add( -($seller + $space), "$description (sold by $username)", - { product_id => $command, plugin => $self->id } + { + product_id => $command, + plugin => $self->id, + deduplicate => join("/", $self->id, $command), + } )->add_contra( $username, $seller, diff --git a/plugins/products b/plugins/products index 7fe1ae8..f172960 100644 --- a/plugins/products +++ b/plugins/products @@ -94,17 +94,6 @@ sub command :Tab(&tab) ($self, $cart, $command, @) { my $product = $products->{ $command } or return NEXT; my $price = $product->{price}; - my @existing = grep { - $_->attribute('plugin') eq $self->id and - $_->attribute('product_id') eq $product->{id} - } $cart->entries('plugin'); - - if (@existing) { - $existing[0]->quantity($existing[0]->quantity + 1); - $cart->select($existing[0]); - return ACCEPT; - } - my $contra_desc = "\$you bought $product->{description}"; my @addons = @{ $product->{addons} // [] }; @@ -116,7 +105,12 @@ sub command :Tab(&tab) ($self, $cart, $command, @) { my $entry = $cart->add( -$price, $product->{description}, - { product_id => $product->{id}, plugin => $self->id, product => $product } + { + product_id => $product->{id}, + plugin => $self->id, + product => $product, + deduplicate => join("/", $self->id, $product->{id}), + } ); $entry->add_contra( $product->{contra}, diff --git a/plugins/revspace_barcode b/plugins/revspace_barcode index be2b981..81d1ccd 100644 --- a/plugins/revspace_barcode +++ b/plugins/revspace_barcode @@ -20,7 +20,11 @@ sub data($self, $cart, $input, @) { ->add( -$price, "Barcode <$input>", - { is_barcode => 1, barcode_data => $input } + { + is_barcode => 1, + barcode_data => $input, + deduplicate => join("/", $self->id, $input), + } ) ->add_contra( "+sales/barcodes", diff --git a/plugins/statiegeld b/plugins/statiegeld index 7fd8cc8..d03483d 100644 --- a/plugins/statiegeld +++ b/plugins/statiegeld @@ -76,20 +76,12 @@ sub command { # args via @_ for mutable alias ? "$addon->{description}" : "$addon->{description} ($product->{description})"; - my @existing = grep { - $_->attribute('plugin') eq $invocant->id and - $_->attribute('addon_id') eq $addon->{id} and - $_->{description} eq $d - } $cart->entries('plugin'); - - if (@existing) { - $existing[0]->quantity($existing[0]->quantity + 1); - $cart->select($existing[0]); - next; - } - $cart - ->add(+$addon->{price}, $d, { plugin => $invocant->id, addon_id => $addon->{id} }) + ->add(+$addon->{price}, $d, { + plugin => $invocant->id, + addon_id => $addon->{id}, + deduplicate => join("/", $invocant->id, $addon->{id}), + }) ->add_contra($addon->{contra}, -$addon->{price}, "$d for \$you"); } diff --git a/revbank b/revbank index 1a4c4a5..0d664cd 100755 --- a/revbank +++ b/revbank @@ -18,7 +18,7 @@ use RevBank::Global; use RevBank::Messages; use RevBank::Cart; -our $VERSION = "3.7"; +our $VERSION = "3.8"; our %HELP1 = ( "abort" => "Abort the current transaction", ); diff --git a/revbank.plugins b/revbank.plugins index 927549e..0136215 100644 --- a/revbank.plugins +++ b/revbank.plugins @@ -27,6 +27,7 @@ dinnerbonus # Then, plugins that apply heuristics repeat +deduplicate # wants to be after 'repeat' statiegeld statiegeld_tokens products # matches product IDs (barcodes)