From 3979b8f7da29ca67441c3b9b540b079eaf0b8634 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20B=C3=B6hmer?= Date: Sun, 20 Jul 2025 22:53:09 +0200 Subject: [PATCH 1/3] Refactor code for adjusting dish ingredients Closes #370. Change started as bugfix for that but I soon realized the old code was somewhat broken and entangled. - reduce code duplication - clearly split code for handling dish ingredient/item value - name methods more precisely - improve error handling - pass UnitConversionGraph object around even more to avoid rebuilding it --- .../Controller/Ajax/IngredientsEditor.pm | 4 +- lib/Coocook/Controller/Item.pm | 5 +- lib/Coocook/Schema/Result/DishIngredient.pm | 99 +++--- lib/Coocook/Schema/Result/Item.pm | 306 +++++++++--------- lib/Coocook/Schema/Result/PurchaseList.pm | 2 +- lib/Coocook/Schema/ResultSet/Dish.pm | 2 +- lib/Coocook/Schema/ResultSet/Item.pm | 32 +- t/controller_Item.t | 18 ++ t/schema_DishIngredient.t | 14 +- t/schema_Item.t | 132 ++++++-- 10 files changed, 366 insertions(+), 248 deletions(-) diff --git a/lib/Coocook/Controller/Ajax/IngredientsEditor.pm b/lib/Coocook/Controller/Ajax/IngredientsEditor.pm index e5173f0a..4637c5dc 100644 --- a/lib/Coocook/Controller/Ajax/IngredientsEditor.pm +++ b/lib/Coocook/Controller/Ajax/IngredientsEditor.pm @@ -148,8 +148,8 @@ sub delete_ingredient : POST Chained('base') PathPart('ingredients/delete') my $ingredient = $c->stash->{dish_or_recipe}->ingredients->find( $ajax_request->{id} ) or $c->detach('/error/object_not_found'); - if ( $ingredient->is_dish_ingredient and my $item = $ingredient->item ) { - $item->remove_ingredients( $c->project->unit_conversion_graph, $ingredient ); + if ( $ingredient->is_dish_ingredient ) { + $ingredient->delete_update_item(); } else { $ingredient->delete(); diff --git a/lib/Coocook/Controller/Item.pm b/lib/Coocook/Controller/Item.pm index 8dc821d7..2c146308 100644 --- a/lib/Coocook/Controller/Item.pm +++ b/lib/Coocook/Controller/Item.pm @@ -47,10 +47,11 @@ sub convert : POST Chained('base') Args(0) RequiresCapability('edit_project') { # can't use numerical equivalence with == because of floating point math! if ( $new_total <= 0 or $abs_difference > 0.0001 ) { - $c->detach('/error/bad_request'); + $c->detach( '/error/bad_request', + ["New total doesn’t fit to conversion. Conversions might have been modified ."] ); } - my $new_item = $item->change_value_offset_unit( + my $new_item = $item->set_value_offset_unit( $item->value * $factor, #perltidy $item->offset * $factor, $unit->id diff --git a/lib/Coocook/Schema/Result/DishIngredient.pm b/lib/Coocook/Schema/Result/DishIngredient.pm index 7f901082..98301774 100644 --- a/lib/Coocook/Schema/Result/DishIngredient.pm +++ b/lib/Coocook/Schema/Result/DishIngredient.pm @@ -63,7 +63,7 @@ __PACKAGE__->has_many( __PACKAGE__->meta->make_immutable; -sub assign_to_purchase_list ( $self, $list_id ) { +sub assign_to_purchase_list ( $self, $list_id, $ucg = undef ) { my $item; $self->txn_do( @@ -74,6 +74,7 @@ sub assign_to_purchase_list ( $self, $list_id ) { article_id => $self->article_id, unit_id => $self->unit_id, value => $self->value, + ucg => $ucg, } ); @@ -86,97 +87,69 @@ sub assign_to_purchase_list ( $self, $list_id ) { =head2 delete_update_item() -=cut - -sub delete_update_item ($self) { - my $retval = $self->item_id && $self->set_value_update_item(0); - $self->delete(); - return $retval; -} - -=head2 set_value_update_item( $value, $ucg? ) +Returns C if still existing, otherwise empty list. =cut -sub set_value_update_item ( $self, $new_value, $ucg = undef ) { +sub delete_update_item ( $self, $ucg = undef ) { $self->txn_do( sub { - my $item = $self->item - or return $self->update( { value => $new_value } ); - - my $old_value = $self->value; - - if ( $old_value == 0 ) { - $self->update( { item_id => undef, value => $new_value } ); - - my $purchase_list_id = $item->purchase_list_id; + $self->delete(); - if ( $item->ingredients->count == 0 ) { # TODO use results_exist() - $item->delete(); - $item = undef; + if ( my $item = $self->item ) { + if ( $item->ingredients->results_exist ) { + return $item->delta_to_value_or_recalculate( $self->value => 0, $self->unit_id => undef, $ucg ); } - if ( $new_value > 0 ) { - return $self->assign_to_purchase_list($purchase_list_id); - } - else { - return $item; - } - } - - if ( $new_value == 0 and not $self->other_ingredients_on_item->results_exist ) { - $self->update( { item_id => undef, value => 0 } ); $item->delete(); - return; } - $self->update( { value => $new_value } ); + return; + } + ); +} + +=head2 set_value_update_item( $value, $ucg? ) - $ucg ||= $item->purchase_list->project->unit_conversion_graph; +Returns the updated C or +an empty list if it didn’t exist or was deleted. - if ( my $factor = $ucg->factor_between_units( $self->unit_id => $item->unit_id ) ) { - if ( $self->other_ingredients_on_item->results_exist ) { - return $item->delta_to_value_offset( ( $new_value - $old_value ) * $factor ); - } - else { - return $item->set_value_offset( $new_value * $factor ); - } - } +=cut - # no factor to item unit -> whole item needs to be rebuilt - return $item->update_from_ingredients(); +sub set_value_update_item ( $self, $new_value, $ucg = undef ) { + $self->txn_do( + sub { + my $old_value = $self->value; + $self->update( { value => $new_value } ); + $self->item or return; + return $self->item->delta_to_value_or_recalculate( + $old_value => $new_value, + ( $self->unit_id ) x 2, + $ucg + ); } ); } =head2 set_value_unit_update_item( $value, $unit, $ucg? ) +Returns the updated C or an empty list if it doesn't exist. + =cut sub set_value_unit_update_item ( $self, $new_value, $new_unit_id, $ucg = undef ) { $self->txn_do( sub { - my $item = $self->item - or return $self->update( { value => $new_value, unit_id => $new_unit_id } ); - - $ucg ||= $item->purchase_list->project->unit_conversion_graph; - my $old_value = $self->value; my $old_unit_id = $self->unit_id; - my $factor_ingredient = $ucg->factor_between_units( $old_unit_id => $new_unit_id ); - my $factor_item = $ucg->factor_between_units( $old_unit_id => $item->unit_id ); - - if ( $factor_ingredient and $factor_item ) { - $self->update( { value => $new_value, unit_id => $new_unit_id } ); - - my $delta = ( $new_value / $factor_ingredient - $old_value ) * $factor_item; - return $item->delta_to_value_offset($delta); - } - - $self->set_value_update_item(0); $self->update( { value => $new_value, unit_id => $new_unit_id } ); - $self->assign_to_purchase_list( $item->purchase_list_id ); + $self->item or return; + return $self->item->delta_to_value_or_recalculate( + $old_value => $new_value, + $old_unit_id => $new_unit_id, + $ucg + ); } ); } diff --git a/lib/Coocook/Schema/Result/Item.pm b/lib/Coocook/Schema/Result/Item.pm index e80c8d2d..388fab70 100644 --- a/lib/Coocook/Schema/Result/Item.pm +++ b/lib/Coocook/Schema/Result/Item.pm @@ -61,97 +61,105 @@ __PACKAGE__->meta->make_immutable; =encoding utf8 -=head2 delta_to_value_offset($delta) - -Adds a delta to the item’s value and adjusts the offset. -The delta needs to be given in the item’s unit. -Might delete the item if the new value will be zero. -Does B change any dish ingredients. -Returns the C object itself unless -it was deleted (then returns nothing). +=head2 delta_to_value_or_recalculate( $value1 => $value2, $unit1_id => $unit2_id, $ucg? ) + +Calculate the difference from C<$value1 $unit1> to C<$value2 $unit2> and +add/subtract the difference to/from the item’s value. +If the item has a positive offset (rounding difference) and the value is increased +or the item has a negative offset and the value is decreased, +the offset is reduced until the delta exceeds the offset. +If values cannot be converted to the item’s unit, +the item’s value is recalculated from its dish ingredients instead. +Does B change any dish ingredients on its own—any modifications to the dish ingredients must be done beforehand. +Returns the C if it still exists. +In case of recalculation the item might get deleted. =cut -sub delta_to_value_offset ( $self, $delta ) { - return if $delta == 0; - - my $value = $self->value + $delta; - - return - $value < 0 ? croak "Delta leads to negative value" - : $value == 0 ? $self->delete() - : $self->_set_value_offset( $delta => $value ); -} - -=head2 set_value_offset($value) +sub delta_to_value_or_recalculate ( $self, $value1, $value2, $unit1_id, $unit2_id, $ucg = undef ) { + defined $value1 or croak "Value 1 must be defined"; + defined $value2 or croak "Value 2 must be defined"; + $unit1_id or $unit2_id or croak "Unit 1 and 2 must not be both undefined"; + ( $value1 == 0 or $unit1_id ) or croak "Unit for value 1 must be defined"; + ( $value2 == 0 or $unit2_id ) or croak "Unit for value 2 must be defined"; -Sets the item’s value to C<$value> and adjusts the offset. -The new value needs to be given in the item’s unit. -Might delete the item if the new value will be zero. -Does B change any dish ingredients. + if ( ( $value1 and $unit1_id != $self->unit_id ) + or ( $value2 and $unit2_id != $self->unit_id ) ) + { + $ucg ||= $self->purchase_list->project->unit_conversion_graph; -=cut + my $factor1 = + ( $value1 and $unit1_id != $self->unit_id ) + ? $ucg->factor_between_units( $unit1_id => $self->unit_id ) + : 1; -sub set_value_offset ( $self, $value ) { - $value >= 0 or croak "Negative value"; - my $delta = $value - $self->value; - return $self if $delta == 0; - return $self->_set_value_offset( $delta => $value ); -} + my $factor2 = + ( $value2 and $unit2_id != $self->unit_id ) + ? $ucg->factor_between_units( $unit2_id => $self->unit_id ) + : 1; -sub _set_value_offset ( $self, $delta, $value ) { # TODO order of methods in this file - $self->set_column( value => $value ); + if ( !$factor1 or !$factor2 ) { + my $factor1_2 = + ( $unit1_id and $unit2_id ) ? $ucg->factor_between_units( $unit1_id => $unit2_id ) : undef; - my $offset = $self->offset; + if ( $factor1_2 and $value1 * $factor1_2 == $value2 ) { + return $self; # value1/2 cannot be converted to item unit but are equal + } - if ( ( $delta < 0 and $offset < 0 ) - or ( $delta > 0 and $offset > 0 ) ) # both are negative or both positive - { - if ( abs($delta) <= abs($offset) ) { # fill up offset if possible - $offset -= $delta; - return $self->update( { offset => $offset } ); + # difference cannot be converted to item's unit + return $self->calculate_value_from_ingredients(); # might remove item } + + $value1 *= $factor1; + $value2 *= $factor2; } - $self->update( { offset => 0 } ); # otherwise clear offset + $self->_delta_to_value_adjust_offset( $value2 - $value1 ); + return $self; } -sub change_value_offset_unit ( $self, $value, $offset, $unit_id ) { +=head2 set_value_offset_unit( $value, $offset, $unit_id ) + +Set value, offset and unit_id of the item. Useful for conversion by 1 click. +Returns the original C or a different one if the original was merged into that. + +=cut + +sub set_value_offset_unit ( $self, $value, $offset, $unit_id ) { $self->txn_do( sub { - my $existing_item = $self->result_source->resultset->find( - { - purchase_list_id => $self->purchase_list_id, - article_id => $self->article_id, - unit_id => $unit_id, - } - ); - - if ($existing_item) { - $existing_item->update( + if ( $unit_id != $self->unit_id ) { + my $existing_item = $self->result_source->resultset->find( { - value => $existing_item->value + $value, - offset => $existing_item->offset + $offset, + purchase_list_id => $self->purchase_list_id, + article_id => $self->article_id, + unit_id => $unit_id, } ); - $self->ingredients->update( { item_id => $existing_item->id } ); - - $self->delete; + if ($existing_item) { + $existing_item->update( + { + value => $existing_item->value + $value, + offset => $existing_item->offset + $offset, + } + ); - return $existing_item; + $self->ingredients->update( { item_id => $existing_item->id } ); + $self->delete; + return $existing_item; + } } - else { - $self->update( - { - unit_id => $unit_id, - value => $value, - offset => $offset, - } - ); - return $self; - } + $self->update( + { + unit_id => $unit_id, + value => $value, + offset => $offset, + } + ); + + return $self; } ); } @@ -159,6 +167,7 @@ sub change_value_offset_unit ( $self, $value, $offset, $unit_id ) { =head2 remove_ingredients( $union_conversion_graph, @ingredient_results ) Returns the updated C or C if it has been deleted. +The dish ingredients are detached from the item but B deleted. =cut @@ -166,7 +175,6 @@ sub remove_ingredients ( $self, $ucg, @ingredients_to_remove ) { $self->txn_do( sub { my $recalculate_value; - my $value = $self->value; for my $ingredient (@ingredients_to_remove) { $ingredient->item_id == $self->id @@ -176,71 +184,32 @@ sub remove_ingredients ( $self, $ucg, @ingredients_to_remove ) { next if $recalculate_value; - if ( defined( my $factor = $ucg->factor_between_units( $ingredient->unit_id => $self->unit_id ) ) ) - { - my $delta = $ingredient->value * $factor; - $delta >= 0 or croak "Negative dish ingredient value"; - - if ( my $offset = $self->offset ) { - if ( $offset < 0 ) { - if ( -1 * $offset <= $delta ) { - $offset += $delta; # reduce negative (!) offset by delta - } - else { - $offset = 0; - } - } - elsif ( $self->offset > 0 ) { # decreasing value makes positive offset invalid - $self->set_column( offset => 0 ); - } - else { die "code broken" } - } - - $value -= $delta; - $value < 0 and $recalculate_value = 1; - } - else { - $recalculate_value = 1; + if ( $ingredient->value == 0 ) { + next; # nothing to do } - } - if ($recalculate_value) { - $value = 0; - my $item_keeps_ingredients; + # same unit (factor == 1) or convertible unit + if ( my $factor = $ucg->factor_between_units( $ingredient->unit_id => $self->unit_id ) ) { + my $delta = -1 * $ingredient->value * $factor; - for my $ingredient ( $self->ingredients->all ) { # remaining ingredients - if ( defined( my $factor = $ucg->factor_between_units( $ingredient->unit_id => $self->unit_id ) ) ) - { - $value += $ingredient->value * $factor; - $item_keeps_ingredients = 1; - } - else { # ingredient is added to other/new item of same unit - my $item = $self->other_items->add_or_create( - { - purchase_list_id => $self->purchase_list_id, - article_id => $ingredient->article_id, - unit_id => $ingredient->unit_id, - value => $ingredient->value, - } - ); - $ingredient->update( { item_id => $item->id } ); + if ( $self->value + $delta >= 0 ) { # new value < 0 is implausible + $self->_delta_to_value_adjust_offset($delta); + next; } } - if ($item_keeps_ingredients) { - $self->set_column( offset => 0 ); - } - else { - $self->delete(); - return; - } + $recalculate_value = 1; + } + + if ($recalculate_value) { + return $self->calculate_value_from_ingredients($ucg); } elsif ( not $self->ingredients->results_exist ) { $self->delete(); return; } - return $self->update( { value => $value } ); + return $self; } ); } @@ -253,27 +222,34 @@ Returns total, i.e. sum of the item's value and offset. sub total ($self) { return $self->value + $self->offset } -sub update_from_ingredients ( $self, $ucg = undef ) { +=head2 calculate_value_from_ingredients($ucg?) + +Updates this item’s value from its ingredients’ values. Always resets offset to zero. +Might assign ingredients to other existing or new purchase list items if they cannot +be converted to this item’s unit. +Returns the C itself if it still exists or an empty list +if no ingredients remained assigned to this item and the item has been deleted. + +=cut + +sub calculate_value_from_ingredients ( $self, $ucg = undef ) { $self->txn_do( sub { - my $item_value = 0; - my $remaining_items = 0; - my @dangling_ingredients = (); + my $remaining_item_value = 0; + my $remaining_ingredients = 0; + my @dangling_ingredients = (); $ucg ||= $self->purchase_list->project->unit_conversion_graph; for my $ingredient ( $self->ingredients->all ) { - - my $ingredient_value = $ingredient->value; - if ( $ingredient->value == 0 ) { - $remaining_items++; + $remaining_ingredients++; next; } if ( my $factor = $ucg->factor_between_units( $ingredient->unit_id => $self->unit_id ) ) { - $item_value += $ingredient->value * $factor; - $remaining_items++; + $remaining_item_value += $ingredient->value * $factor; + $remaining_ingredients++; next; } @@ -281,32 +257,64 @@ sub update_from_ingredients ( $self, $ucg = undef ) { push @dangling_ingredients, $ingredient; } - if ( $remaining_items == 0 ) { + if (@dangling_ingredients) { + my @other_items = $self->other_items->all; + + # add dangling ingredients existing/new items with minimal number of new items + INGREDIENT: for my $ingredient (@dangling_ingredients) { + for my $other_item (@other_items) { + if ( my $factor = $ucg->factor_between_units( $ingredient->unit_id => $other_item->unit_id ) ) { + $ingredient->update( { item_id => $other_item->id } ); + $other_item->_delta_to_value_adjust_offset( $ingredient->value * $factor ); + next INGREDIENT; + } + } + + my $new_item = $ingredient->assign_to_purchase_list( $self->purchase_list_id, $ucg ); + push @other_items, $new_item; + } + } + + if ( $remaining_ingredients > 0 ) { + $self->update( { value => $remaining_item_value, offset => 0 } ); + return $self; + } + else { $self->delete(); return; } + } + ); +} - my @items; +=head2 _delta_to_value_adjust_offset($delta) - # add dangling ingredients as new items with minimal number of new items - INGREDIENT: for my $ingredient (@dangling_ingredients) { - for my $item (@items) { - if ( my $factor = $ucg->factor_between_units( $ingredient->unit_id => $item->unit_id ) ) { - $ingredient->update( { item_id => $item->id } ); - $item->update( { value => $item->value + $ingredient->value * $factor } ); - last INGREDIENT; +Adds/subtracts C<$delta> to/from the item’s value and adjusts the item’s offset. +The delta must be given in the item’s unit. +Does B change any dish ingredients. - } - } +=cut - my $item = $ingredient->assign_to_purchase_list( $self->purchase_list_id ); - push @items, $item; - } +sub _delta_to_value_adjust_offset ( $self, $delta ) { + return $self if $delta == 0; - $self->update( { value => $item_value, offset => 0 } ); - return $self; + my $value = $self->value + $delta; + $value >= 0 or croak "Delta creates negative value"; + + $self->set_column( value => $value ); + + my $offset = $self->offset; + + if ( ( $delta < 0 and $offset < 0 ) + or ( $delta > 0 and $offset > 0 ) ) # both are negative or both positive + { + if ( abs($delta) <= abs($offset) ) { # fill up offset if possible + $offset -= $delta; + return $self->update( { offset => $offset } ); } - ); + } + + $self->update( { offset => 0 } ); # otherwise clear offset } 1; diff --git a/lib/Coocook/Schema/Result/PurchaseList.pm b/lib/Coocook/Schema/Result/PurchaseList.pm index 3052f4bc..2e44629b 100644 --- a/lib/Coocook/Schema/Result/PurchaseList.pm +++ b/lib/Coocook/Schema/Result/PurchaseList.pm @@ -146,7 +146,7 @@ sub move_items_ingredients ( $self, %args ) { or $items{$item_id}->remove_ingredients( $args{ucg}, @$ingredients ); for my $ingredient (@$ingredients) { - $ingredient->assign_to_purchase_list( $args{target_purchase_list}->id ); + $ingredient->assign_to_purchase_list( $args{target_purchase_list}->id, $args{ucg} ); } } diff --git a/lib/Coocook/Schema/ResultSet/Dish.pm b/lib/Coocook/Schema/ResultSet/Dish.pm index 22797692..64055152 100644 --- a/lib/Coocook/Schema/ResultSet/Dish.pm +++ b/lib/Coocook/Schema/ResultSet/Dish.pm @@ -37,7 +37,7 @@ sub from_recipe ( $self, %args ) { ); if ( my $list_id = $recipe->project->default_purchase_list_id ) { - $dish_ingredient->assign_to_purchase_list($list_id); + $dish_ingredient->assign_to_purchase_list( $list_id, $args{ucg} ); } } diff --git a/lib/Coocook/Schema/ResultSet/Item.pm b/lib/Coocook/Schema/ResultSet/Item.pm index 6a1948ce..723641a3 100644 --- a/lib/Coocook/Schema/ResultSet/Item.pm +++ b/lib/Coocook/Schema/ResultSet/Item.pm @@ -4,7 +4,26 @@ use Coocook::Base qw(Moose); extends 'Coocook::Schema::ResultSet'; -# find item and add value or create new item with value +=head2 add_or_create( { ... } ) + +Find item with given unit and add value +or create new item with given value. +Syntax with hashref like e.g. C from L. +Returns found or created C. + + # add 42 kg of apples to existing or new item on purchase list + my $item = $purchase_list->search_related('items')->add_or_create( + { + purchase_list_id => $purchase_list->id, + value => 42, + unit_id => $kg->id, + article_id => $apples->id, + ucg => $project->ucg, # optional + } + ); + +=cut + sub add_or_create ( $self, $args ) { my $item = $self->find_or_new( { @@ -15,19 +34,20 @@ sub add_or_create ( $self, $args ) { ); if ( $item->in_storage ) { - $item->value( $item->value + $args->{value} ); + $item->delta_to_value_or_recalculate( 0 => $args->{value}, undef => $item->unit_id, $args->{ucg} ) + or die "found item with unit but delta_to_value_or_recalculate() returned undef"; + + return $item; } else { $item->set_columns( { value => $args->{value}, - comment => "", # no argument because existing items have comments - offset => 0, # instantly apply default_value + comment => "", # not an argument because existing items might already have comments } ); + return $item->insert(); } - - $item->update_or_insert and return $item; } __PACKAGE__->meta->make_immutable; diff --git a/t/controller_Item.t b/t/controller_Item.t index ab65b726..8325c861 100644 --- a/t/controller_Item.t +++ b/t/controller_Item.t @@ -55,6 +55,24 @@ subtest "remove offset" => sub { }; subtest "convert item" => sub { + $t->post('/project/1/Test-Project/items/999/convert'); + $t->status_is(404); + + $t->post( '/project/1/Test-Project/items/1/convert', { unit => 2 } ); # 'total' missing + $t->status_is(400); + + $t->post( '/project/1/Test-Project/items/1/convert', { total => 42, unit => 999 } ); + $t->status_is(404); + + $t->post( '/project/1/Test-Project/items/1/convert', { total => 42, unit => 3 } ); + $t->status_is(400); + $t->text_contains("No conversion factor"); + + $t->post( '/project/1/Test-Project/items/1/convert', { total => 999, unit => 2 } ); + $t->status_is(400); + $t->text_contains("New total doesn’t fit to conversion"); + + $t->get_ok('/project/1/Test-Project/purchase_list/1'); $t->text_contains("14.5\N{THIN SPACE}kilograms"); $t->content_contains( my $old_value = 'value="14.5"' ); $t->content_lacks( my $new_value = 'value="14500"' ); diff --git a/t/schema_DishIngredient.t b/t/schema_DishIngredient.t index 9905b0d7..4de9c047 100644 --- a/t/schema_DishIngredient.t +++ b/t/schema_DishIngredient.t @@ -105,15 +105,15 @@ subtest "update ingredient value" => sub { "exception for negative value"; }; -subtest "update ingredient value and unit" => sub { +subtest set_value_unit_update_item => sub { my ( $g, $kg, $l, $p ) = map { $units->find( { short_name => $_ } )->id } qw( g kg l p ); - t "1g", sub { $_->set_value_unit_update_item( 2, $kg ) } => "2kg", "ingredient without item"; + t "1g", sub { !$_->set_value_unit_update_item( 2, $kg ) } => "2kg", "ingredient without item"; t "1+0kg: 1kg", sub { $_->set_value_unit_update_item( 500, $g ) } => "0.5+0kg: 500g", "basic item with conversion"; - t "1+0kg: 1kg", sub { $_->set_value_unit_update_item( 0.5, $l ) } => "0.5+0l: 0.5l", + t "1+0kg: 1kg", sub { !$_->set_value_unit_update_item( 0.5, $l ) } => "0.5+0l: 0.5l", "basic item without conversion"; t "2+0kg: 1kg 1000g", sub { $_->set_value_unit_update_item( 500, $g ) } => "1.5+0kg: 500g 1000g", @@ -123,8 +123,8 @@ subtest "update ingredient value and unit" => sub { sub { $_->set_value_unit_update_item( 0.5, $l ) } => [ "1+0kg: 1000g", "0.5+0l: 0.5l" ], "kg to liter"; - t "2+0kg: 1l 1000g", - sub { $_->set_value_unit_update_item( 500, $g ) } => [ "1+0kg: 1000g", "500+0g: 500g" ], + t "2+0kg: 1l 1000pcs", + sub { $_->set_value_unit_update_item( 500, $g ) } => [ "0.5+0kg: 500g", "1000+0pcs: 1000pcs" ], "liter to grams"; t "2+0kg: 1l 1000g", @@ -251,6 +251,10 @@ sub set_items_ingredients (@strings) { return @ingredients; } +# short function name for tests on dish ingredients: +# - sets dish ingredients to $input +# - executes $coderef on dish ingredient(s) which must return true +# - compares dish ingredients to $expected sub t ( $input, $coderef, $expected, $name = undef ) { local $Test::Builder::Level = $Test::Builder::Level + 1; diff --git a/t/schema_Item.t b/t/schema_Item.t index 94783231..2a7c47bd 100644 --- a/t/schema_Item.t +++ b/t/schema_Item.t @@ -4,12 +4,12 @@ use Test2::V0; use lib "t/lib"; use TestDB qw(txn_do_and_rollback); -plan( tests => 10 ); +plan( tests => 13 ); my $db = TestDB->new(); -# this is a SQL query useful debugging -# is it only run to assert that it matches the current schema +# this is a SQL query useful for debugging +# it is only run to assert that it works with the current schema $db->storage->dbh_do( sub ( $storage, $dbh ) { $dbh->do(<resultset('Project')->find(1); my $items = $project->purchase_lists->find(1)->items; my $ucg = $project->unit_conversion_graph(); -my $l = $project->units->find( { short_name => 'l' } ); -my $t = $project->units->find( { short_name => 't' } ); -{ +my ( $g, $kg, $t, $ml, $l ) = + map { $project->units->find( { short_name => $_ } ) } qw( g kg t ml l ); + +subtest delta_to_value_or_recalculate => sub { my $item = $items->find(1); # repeatedly a block variable to avoid caching in Result object + like dies { $item->delta_to_value_or_recalculate( undef, undef, undef, undef ) } => qr/value 1/i; + like dies { $item->delta_to_value_or_recalculate( 0, undef, undef, undef ) } => qr/value 2/i; + like dies { $item->delta_to_value_or_recalculate( 0, 0, undef, undef ) } => qr/unit 1 and 2/i; + like dies { $item->delta_to_value_or_recalculate( 1, 1, undef, 1 ) } => qr/unit for value 1/i; + like dies { $item->delta_to_value_or_recalculate( 1, 1, 1, undef ) } => qr/unit for value 2/i; + + # test cases without exception but actual results are tested in schema_DishIngredient.t +}; + +subtest "set_value_offset_unit()", txn_do_and_rollback $db, sub { + my $item = $items->find(2); + my $value = $item->value; + my $offset = $item->offset || die "want to test with offset"; + my $unit_id = $item->unit_id; + my $ingredients = $item->ingredients->count; + + subtest "with unchanged values" => sub { + is $item->set_value_offset_unit( $value, $offset, $unit_id ) => exact_ref($item); + $item->discard_changes(); + is $item => object { + call value => $value; + call offset => $offset; + call unit_id => $unit_id; + call sub { shift->ingredients->count } => $ingredients; + }; + }; + + subtest "with same unit but different values" => sub { + is $item->set_value_offset_unit( 90, 10, $unit_id ) => exact_ref($item); + $item->discard_changes(); + is $item => object { + call value => 90; + call offset => 10; + call unit_id => $unit_id; + call sub { shift->ingredients->count } => $ingredients; + }; + }; + + subtest "with different values and previously unused unit" => sub { + is $item->set_value_offset_unit( 900, 100, $l->id ) => exact_ref($item); + $item->discard_changes(); + is $item => object { + call value => 900; + call offset => 100; + call unit_id => $l->id; + call sub { shift->ingredients->count } => $ingredients; + }; + }; + + subtest "merge into existing item" => sub { + my $other_item = $items->create( + { + value => 42, + offset => 23, + unit_id => 1, + article_id => $item->article_id, + comment => __FILE__, + } + ); + $item->unit_id != $other_item->unit_id or die "test broken"; + + isnt my $new_item = + $item->set_value_offset_unit( 900, 100, $other_item->unit_id ) => exact_ref($item); + + $item->discard_changes(); + ok !$item->in_storage, "old item has been deleted"; + + $new_item->discard_changes(); + is $new_item => object { + prop isa => ref $item; + call in_storage => T(); + call id => $other_item->id; + call value => 900 + 42; + call offset => 100 + 23; + call unit_id => $other_item->unit_id; + call sub { shift->ingredients->count } => $ingredients; + }; + }; + +}; + +{ + my $item = $items->find(1); + like dies { $item->remove_ingredients() } => qr/arguments/, "remove_ingredients() without UnitConversionGraph"; @@ -43,7 +128,7 @@ my $t = $project->units->find( { short_name => 't' } ); is $item->value => 14.5, "... value unchanged"; } -subtest "remove 1kg", txn_do_and_rollback $db, sub { +subtest "remove ingredient with same unit: 1kg from kg", txn_do_and_rollback $db, sub { my $item = $items->find(1); my $ingredient = $item->ingredients->find(7); is $item->remove_ingredients( $ucg, $ingredient ) => exact_ref($item); @@ -53,7 +138,7 @@ subtest "remove 1kg", txn_do_and_rollback $db, sub { is $ingredient->item_id => undef; }; -subtest "remove 500g", txn_do_and_rollback $db, sub { +subtest "remove ingredient with direct conversion: 500g from kg", txn_do_and_rollback $db, sub { my $item = $items->find(1); my $ingredient = $item->ingredients->find(1); is $item->remove_ingredients( $ucg, $ingredient ) => exact_ref($item); @@ -63,7 +148,7 @@ subtest "remove 500g", txn_do_and_rollback $db, sub { is $ingredient->item_id => undef; }; -subtest "remove g from t", txn_do_and_rollback $db, sub { +subtest "remove ingredient with indirect conversion: g from t", txn_do_and_rollback $db, sub { my $item = $items->find(2); my $ingredient = $item->ingredients->find(2); @@ -105,7 +190,7 @@ my $ingredient = $item->ingredients->create( } ); -subtest "remove l from kg (unit without conversion)", txn_do_and_rollback $db, sub { +subtest "remove unit without conversion: l from kg", txn_do_and_rollback $db, sub { is $item->remove_ingredients( $ucg, $ingredient ) => exact_ref($item); $item->discard_changes(); $ingredient->discard_changes(); @@ -113,16 +198,24 @@ subtest "remove l from kg (unit without conversion)", txn_do_and_rollback $db, s is $ingredient->item_id => undef; }; -subtest "handling of l (unit without conversion) during removal", txn_do_and_rollback $db, sub { - my $ingredient2 = $item->ingredients->find(1); - $ingredient2->update( { unit_id => $l->id } ); # set to l to force recalculation - $item->ingredients->update( { value => 0 } ); # value 0 must is valid, item must not be removed - is $item->remove_ingredients( $ucg, $ingredient2 ) => exact_ref($item); - $item->discard_changes(); +subtest "calculate_value_from_ingredients()", txn_do_and_rollback $db, sub { + is $item->calculate_value_from_ingredients() => exact_ref($item); + is $item->ingredients->count => 4; + is $item->value => 14.5; + $ingredient->discard_changes(); - is $item->value => 0.0, "item value recalculated"; - isnt $ingredient->item_id => $item->id, "item_id of l ingredient changed"; - is $ingredient->item->ingredients->count => 1, "new item has only 1 ingredient"; + isnt $ingredient->item_id => $item->id, "new item ID"; + is $ingredient->item->value => 1, "value of 5th ingredient"; + is $ingredient->unit_id => $l->id, "same unit ID"; +}; + +subtest "calculate_value_from_ingredients() with zero value", txn_do_and_rollback $db, sub { + $item->ingredients->find(1)->update( { unit_id => $l->id } ); # set to l to force recalculation + $item->ingredients->update( { value => 0 } ); # value 0 is valid, item must not be removed + is $item->calculate_value_from_ingredients() => exact_ref($item); + $item->discard_changes(); + is $item->value => 0.0, "item value recalculated"; + is $item->ingredients->count => $_, "item still has $_ ingredients" for 5; }; subtest "remove all ingredients", txn_do_and_rollback $db, sub { @@ -132,6 +225,7 @@ subtest "remove all ingredients", txn_do_and_rollback $db, sub { ok !$item->in_storage, "item was deleted"; for (@ingredients) { $_->discard_changes(); + ok $_->in_storage; is $_->item_id => undef; } }; -- GitLab From 3baadf67f74e77c6fa0adf4a269b25513da12504 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20B=C3=B6hmer?= Date: Fri, 28 Nov 2025 19:44:23 +0100 Subject: [PATCH 2/3] Remove outdated inventory key 'unassigned_items' --- lib/Coocook/Schema/Result/Project.pm | 22 +++++++++------------- root/templates/project/show.tt | 3 --- t/schema_Project.t | 19 +++++++++---------- 3 files changed, 18 insertions(+), 26 deletions(-) diff --git a/lib/Coocook/Schema/Result/Project.pm b/lib/Coocook/Schema/Result/Project.pm index 7776746b..91d87e0f 100644 --- a/lib/Coocook/Schema/Result/Project.pm +++ b/lib/Coocook/Schema/Result/Project.pm @@ -159,19 +159,15 @@ sub inventory ($self) { undef, { columns => { - articles => $self_rs->search_related('articles')->count_rs->as_query, - dishes => $self_rs->search_related('meals')->search_related('dishes')->count_rs->as_query, - meals => $self_rs->search_related('meals')->count_rs->as_query, - purchase_lists => $self_rs->search_related('purchase_lists')->count_rs->as_query, - recipes => $self_rs->search_related('recipes')->count_rs->as_query, - shop_sections => $self_rs->search_related('shop_sections')->count_rs->as_query, - tags => $self_rs->search_related('tags')->count_rs->as_query, - tag_groups => $self_rs->search_related('tag_groups')->count_rs->as_query, - units => $self_rs->search_related('units')->count_rs->as_query, - unassigned_items => $self_rs->search_related('meals') - ->search_related('dishes') - ->search_related('ingredients') - ->unassigned->count_rs->as_query, + articles => $self_rs->search_related('articles')->count_rs->as_query, + dishes => $self_rs->search_related('meals')->search_related('dishes')->count_rs->as_query, + meals => $self_rs->search_related('meals')->count_rs->as_query, + purchase_lists => $self_rs->search_related('purchase_lists')->count_rs->as_query, + recipes => $self_rs->search_related('recipes')->count_rs->as_query, + shop_sections => $self_rs->search_related('shop_sections')->count_rs->as_query, + tags => $self_rs->search_related('tags')->count_rs->as_query, + tag_groups => $self_rs->search_related('tag_groups')->count_rs->as_query, + units => $self_rs->search_related('units')->count_rs->as_query, }, } )->hri->single; diff --git a/root/templates/project/show.tt b/root/templates/project/show.tt index cc97c5bb..7682e081 100644 --- a/root/templates/project/show.tt +++ b/root/templates/project/show.tt @@ -41,9 +41,6 @@ show_donate_infobox = 0 %] [% END %] [% ELSIF inventory.purchase_lists == 0 %] This project lacks any purchase lists. You can create some. - [% ELSIF inventory.unassigned_items > 0 %] - There are some ingredient items not yet assigned to a purchase list. - You can assign items to purchase lists now. [% ELSIF project.is_stale; show_donate_infobox = 1 %]
diff --git a/t/schema_Project.t b/t/schema_Project.t index c4e483d9..949a18a1 100644 --- a/t/schema_Project.t +++ b/t/schema_Project.t @@ -14,16 +14,15 @@ subtest inventory => sub { my $inventory = $db->resultset('Project')->find(1)->inventory(); is $inventory => { - articles => 5, - dishes => 4, - meals => 4, - purchase_lists => 2, - recipes => 1, - shop_sections => 2, - tags => 3, - tag_groups => 1, - units => 8, - unassigned_items => 0, + articles => 5, + dishes => 4, + meals => 4, + purchase_lists => 2, + recipes => 1, + shop_sections => 2, + tags => 3, + tag_groups => 1, + units => 8, }; }; -- GitLab From 775dfa11edf0d35d31ebadaff2a8fd296dae5f81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20B=C3=B6hmer?= Date: Fri, 28 Nov 2025 22:23:28 +0100 Subject: [PATCH 3/3] Implement automatic fix for unassigned dish ingredients --- lib/Coocook/Schema/Result/Project.pm | 35 ++++++++++++++++++ lib/Coocook/Schema/ResultSet/Project.pm | 36 +++++++++++++++++++ lib/Coocook/Script/Dbck.pm | 29 +++++---------- .../27-28/unassigned_dish_ingredients.pl | 22 ++---------- t/postgresql.t | 9 ++--- t/script_dbck.t | 13 +++++-- t/sqlite.t | 6 ++-- 7 files changed, 101 insertions(+), 49 deletions(-) diff --git a/lib/Coocook/Schema/Result/Project.pm b/lib/Coocook/Schema/Result/Project.pm index 91d87e0f..8fb591a1 100644 --- a/lib/Coocook/Schema/Result/Project.pm +++ b/lib/Coocook/Schema/Result/Project.pm @@ -136,6 +136,41 @@ sub articles_cached_units ($self) { } } +=head2 assign_unassigned_dish_ingredients() + +Assign dish ingredients with no C to a new purchase list +created on demand with the name I. +If there is a purchase list with that name already, the name +is suffixed with the lowest integer possible to create a unique name. + +Returns the C object if one was created. + +=cut + +sub assign_unassigned_dish_ingredients ($self) { + my $unassigned_ingredients = $self->dish_ingredients->unassigned; + + $self->purchase_lists->results_exist or return; + $unassigned_ingredients->results_exist or return; + + my $i = 1; + my $name = sub { "Previously unassigned dish ingredients" . ( $i > 1 ? " ($i)" : "" ) }; + while ( $self->purchase_lists->results_exist( { name => $name->() } ) ) { $i++ } + + my $list = $self->create_related( + purchase_lists => { + date => $self->purchase_lists->default_date, + name => $name->(), + } + ); + + while ( my $ingredient = $unassigned_ingredients->next ) { + $ingredient->assign_to_purchase_list( $list->id ); + } + + return $list; +} + =head2 dish_ingredients() Shortcut. diff --git a/lib/Coocook/Schema/ResultSet/Project.pm b/lib/Coocook/Schema/ResultSet/Project.pm index 7224d30e..83e444b3 100644 --- a/lib/Coocook/Schema/ResultSet/Project.pm +++ b/lib/Coocook/Schema/ResultSet/Project.pm @@ -40,4 +40,40 @@ sub stale ( $self, $pivot_date = undef ) { { -not_bool => [ map { -exists => $_->search( undef, { select => [ \1 ] } )->as_query } @rs ] } ); } +=head2 with_unassigned_dish_ingredients() + +Returns a new resultset with projects +that have dish ingredients and purchase list +but some dish ingredient is not assigned to any purchase list. + +The result objects returned from the resultset will have +the additional column C. + + my $invalid_projects = $projects->with_unassigned_dish_ingredients(); + print $invalid_projects->first->get_column('unassigned_dish_ingredients_count'); + +=cut + +sub with_unassigned_dish_ingredients ($self) { + return $self->search( + { + -and => [ + $self->correlate('purchase_lists')->results_exist_as_query, + $self->correlate('meals') + ->search_related('dishes') + ->search_related('ingredients') + ->unassigned->results_exist_as_query, + ] + }, + { + '+columns' => { + unassigned_dish_ingredients_count => $self->correlate('meals') + ->search_related('dishes') + ->search_related('ingredients') + ->unassigned->count_rs->as_query, + }, + } + ); +} + 1; diff --git a/lib/Coocook/Script/Dbck.pm b/lib/Coocook/Script/Dbck.pm index 4bb5830f..5f867038 100644 --- a/lib/Coocook/Script/Dbck.pm +++ b/lib/Coocook/Script/Dbck.pm @@ -319,30 +319,17 @@ sub check_missing_default_purchase_lists ($self) { } sub check_unassigned_dish_ingredients ($self) { - my $projects = $self->_schema->resultset('Project'); - my $invalid_projects = $projects->search( - { - -and => [ - $projects->correlate('purchase_lists')->results_exist_as_query, - $projects->correlate('meals') - ->search_related('dishes') - ->search_related('ingredients') - ->unassigned->results_exist_as_query, - ] - }, - { - '+columns' => { - unassigned_items_count => $projects->correlate('meals') - ->search_related('dishes') - ->search_related('ingredients') - ->unassigned->count_rs->as_query, - }, - } - ); + my $invalid_projects = $self->_schema->resultset('Project')->with_unassigned_dish_ingredients; while ( my $project = $invalid_projects->next ) { warn sprintf "Project %i has purchase list(s) but also %i unassigned dish ingredient(s)\n", - $project->id, $project->get_column('unassigned_items_count'); + $project->id, $project->get_column('unassigned_dish_ingredients_count'); + + $self->fix or next; + + $project->assign_unassigned_dish_ingredients(); + + warn "... Fixed!\n"; } } diff --git a/share/ddl/_common/upgrade/27-28/unassigned_dish_ingredients.pl b/share/ddl/_common/upgrade/27-28/unassigned_dish_ingredients.pl index 784e81e8..972d7f42 100644 --- a/share/ddl/_common/upgrade/27-28/unassigned_dish_ingredients.pl +++ b/share/ddl/_common/upgrade/27-28/unassigned_dish_ingredients.pl @@ -1,27 +1,9 @@ use Coocook::Base; sub ( $schema, $versions ) { - my $projects = $schema->resultset('Project'); + my $projects = $schema->resultset('Project')->with_unassigned_dish_ingredients; while ( my $project = $projects->next ) { - my $unassigned_ingredients = $project->dish_ingredients->unassigned; - - $project->purchase_lists->results_exist or next; - $unassigned_ingredients->results_exist or next; - - my $i = 1; - my $name = sub { "Previously unassigned items" . ( $i > 1 ? " ($i)" : "" ) }; - while ( $project->purchase_lists->find( { name => $name->() } ) ) { $i++ } - - my $list = $project->create_related( - purchase_lists => { - date => $project->purchase_lists->default_date, - name => $name->(), - } - ); - - while ( my $ingredient = $unassigned_ingredients->next ) { - $ingredient->assign_to_purchase_list( $list->id ); - } + $project->assign_unassigned_dish_ingredients(); } }; diff --git a/t/postgresql.t b/t/postgresql.t index 12b5843e..457d651e 100644 --- a/t/postgresql.t +++ b/t/postgresql.t @@ -329,7 +329,7 @@ subtest "migration 24->25 (issue #266 order of meals/dishes)" => sub { "dishes in order of insertion into database"; }; -subtest "migration 27->28 (issue #292 unassigned items to purchase list)" => sub { +subtest "migration 27->28 (issue #292 unassigned dish ingredients to purchase list)" => sub { my $schema = TestDB->new( deploy => 0 ); install_ok $schema, 24; # latest schema version that works with v21_install.sql ok TestDB->execute_test_data( $schema, 't/test_data_v21_install.sql' ), @@ -344,8 +344,8 @@ subtest "migration 27->28 (issue #292 unassigned items to purchase list)" => sub $purchase_lists->populate( [ [ 'date', 'name' ], - map { [ $purchase_lists->default_date, $_ ] } 'Previously unassigned items', - 'Previously unassigned items (2)' + map { [ $purchase_lists->default_date, $_ ] } 'Previously unassigned dish ingredients', + 'Previously unassigned dish ingredients (2)' ] ); @@ -371,7 +371,8 @@ subtest "migration 27->28 (issue #292 unassigned items to purchase list)" => sub is $project1->dish_ingredients->unassigned->count => 0, "project 1 has no more unassigned dish ingredients"; - ok $schema->resultset('PurchaseList')->find( { name => "Previously unassigned items (3)" } ), + ok $schema->resultset('PurchaseList') + ->find( { name => "Previously unassigned dish ingredients (3)" } ), "created purchase list"; is $project2->purchase_lists->count => 0, diff --git a/t/script_dbck.t b/t/script_dbck.t index 5fe95341..1037ec5c 100644 --- a/t/script_dbck.t +++ b/t/script_dbck.t @@ -224,7 +224,7 @@ subtest "project with purchase list but unassigned dish ingredients", txn_do_and "SQL query returns empty"; note "Creating another dish ingredient without assigning it ..."; - $db->resultset('DishIngredient')->create( + my $ingredient = $db->resultset('DishIngredient')->create( { dish_id => 1, prepare => 0, @@ -235,11 +235,20 @@ subtest "project with purchase list but unassigned dish ingredients", txn_do_and } ); - like warning { $app->run } => qr/unassigned/, "warns"; + is warning { $app->run } => + "Project 1 has purchase list(s) but also 1 unassigned dish ingredient(s)\n", + "warns"; is $db->storage->dbh_do( sub ( $storage, $dbh ) { $dbh->selectall_arrayref($sql) } ) => array { item T(); etc() }, "SQL query with relevant data"; + $app->fix(1); + is warnings { $app->run() } => + [ "Project 1 has purchase list(s) but also 1 unassigned dish ingredient(s)\n", "... Fixed!\n" ], + "with --fix"; + ok !warns { $app->run() }, "doesn't warn anymore"; + $app->fix(0); + note "Deleting all purchase lists ..."; $db->resultset('Project')->update( { default_purchase_list_id => undef } ); $db->resultset('PurchaseList')->delete(); diff --git a/t/sqlite.t b/t/sqlite.t index 7155fc13..3de3a1c7 100644 --- a/t/sqlite.t +++ b/t/sqlite.t @@ -129,7 +129,7 @@ subtest "migration 24->25 (issue #266 order of meals/dishes)" => sub { "dishes in order of insertion into database"; }; -subtest "migration 27->28 (issue #292 unassigned items to purchase list)" => sub { +subtest "migration 27->28 (issue #292 unassigned dish ingredients to purchase list)" => sub { my $schema = TestDB->new( deploy => 0 ); install_ok $schema, 24; # latest schema version that works with v21_install.sql ok TestDB->execute_test_data( $schema, 't/test_data_v21_install.sql' ), @@ -142,7 +142,9 @@ subtest "migration 27->28 (issue #292 unassigned items to purchase list)" => sub is $schema->resultset('DishIngredient')->unassigned->count => 0, "has no more unassigned dish ingredients"; - ok $schema->resultset('PurchaseList')->search( { name => "Previously unassigned items" } )->count, + ok $schema->resultset('PurchaseList') + ->search( { name => "Previously unassigned dish ingredients" } ) + ->count, "created purchase lists"; }; -- GitLab