From 10e8f532620a4ec9733e87af71a111a5fca06f3f Mon Sep 17 00:00:00 2001 From: Petro Vashchuk Date: Wed, 12 Nov 2025 13:07:09 +0200 Subject: [PATCH] Bug 32767: New feature that trims attributes (if enabled) to avoid duplicates Added a new "Trim identifier:" checkbox for attribute editor page, that if enabled, will trim the value from any trailing whitespaces. The use case for this feature is to avoid unintentionally creating duplicate attribute values with trailing whitespaces around of it. If this checkbox is grayed for you, that means that you have some values surrounded with whitespaces, and Koha can't decide for you which you want to remove and when (because that as well will lead to creating duplicated entries). So in order to use this feature you first need to clean up your database from already existing values with trailing whitespaces. This SQL query might help you find all untrimmed attributes & borrowers: ```SELECT * FROM borrower_attributes WHERE code=%Attribute Name% AND attribute REGEXP "^\\s+|\\s+$";``` Steps to reproduce: 1. Go to the attribute editor page. Check if the "Trim identifier:" checkbox is not grayed. (if it is grayed, it means you have some values surrounded with whitespaces, in that case you need to fix them manually) 2. Check the checkbox and save settings. 3. Add that attribute for a preferred patron, enter some value with a trailing whitespaces around of it. 4. Check that the trailing whitespaces were trimmed correctly. --- Koha/Patron/Attribute.pm | 19 +++++ Koha/Patron/Attribute/Type.pm | 23 ++++++ Koha/Schema/Result/BorrowerAttributeType.pm | 11 +++ admin/patron-attr-types.pl | 10 +++ .../bug_32767-trim-attr-feature.pl | 14 ++++ installer/data/mysql/kohastructure.sql | 1 + .../en/modules/admin/patron-attr-types.tt | 21 ++++++ members/memberentry.pl | 1 + opac/opac-memberentry.pl | 1 + t/db_dependent/Koha/Patron/Attribute.t | 75 ++++++++++++++++++- 10 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 installer/data/mysql/atomicupdate/bug_32767-trim-attr-feature.pl diff --git a/Koha/Patron/Attribute.pm b/Koha/Patron/Attribute.pm index 7728dc1a24a..a891ff38555 100644 --- a/Koha/Patron/Attribute.pm +++ b/Koha/Patron/Attribute.pm @@ -71,6 +71,8 @@ sub store { Koha::Exceptions::Patron::Attribute::NonRepeatable->throw( attribute => $self ) unless $self->repeatable_ok(); + $self->do_trim_value_if_needed(); + Koha::Exceptions::Patron::Attribute::UniqueIDConstraint->throw( attribute => $self ) unless $self->unique_ok(); @@ -235,6 +237,23 @@ sub value_ok { return $ok; } +=head3 do_trim_value_if_needed + +--- + +=cut + +sub do_trim_value_if_needed { + + my ($self) = @_; + + if ( $self->type->trim_value ) { + my $value_to_trim = $self->attribute; + $value_to_trim =~ s/^\s+|\s+$//g; + $self->attribute($value_to_trim); + } +} + =head2 Internal methods =head3 _type diff --git a/Koha/Patron/Attribute/Type.pm b/Koha/Patron/Attribute/Type.pm index 31218020e99..caea0d35c4c 100644 --- a/Koha/Patron/Attribute/Type.pm +++ b/Koha/Patron/Attribute/Type.pm @@ -46,6 +46,7 @@ sub store { $self->check_repeatables; $self->check_unique_ids; + $self->check_untrimmed_values; return $self->SUPER::store(); } @@ -112,6 +113,28 @@ sub check_unique_ids { return $self; } +=head3 check_untrimmed_values + +=cut + +sub check_untrimmed_values { + my ($self) = @_; + + return $self unless $self->trim_value; + + my $count = $self->attributes->search( + { + attribute => { -regexp => '^\s+|\s+$' } + } + )->count; + + Koha::Exceptions::Patron::Attribute::Type::CannotChangeProperty->throw( + property => 'trim_value' ) + if $count; + + return $self; +} + =head3 _type =cut diff --git a/Koha/Schema/Result/BorrowerAttributeType.pm b/Koha/Schema/Result/BorrowerAttributeType.pm index 36bf42d4c91..5851672b1d0 100644 --- a/Koha/Schema/Result/BorrowerAttributeType.pm +++ b/Koha/Schema/Result/BorrowerAttributeType.pm @@ -55,6 +55,14 @@ defines whether one patron/borrower can have multiple values for this custom fie defines if this value needs to be unique (1 for yes, 0 for no) +=head2 trim_value + + data_type: 'tinyint' + default_value: 0 + is_nullable: 0 + +defines if this value needs to be trimmed of whitespaces (1 for yes, 0 for no) + =head2 is_date data_type: 'tinyint' @@ -164,6 +172,8 @@ __PACKAGE__->add_columns( { data_type => "tinyint", default_value => 0, is_nullable => 0 }, "unique_id", { data_type => "tinyint", default_value => 0, is_nullable => 0 }, + "trim_value", + { data_type => "tinyint", default_value => 0, is_nullable => 0 }, "is_date", { data_type => "tinyint", default_value => 0, is_nullable => 0 }, "opac_display", @@ -260,6 +270,7 @@ __PACKAGE__->belongs_to( __PACKAGE__->add_columns( '+display_checkout' => { is_boolean => 1 }, + '+trim_value' => { is_boolean => 1 }, '+is_date' => { is_boolean => 1 }, '+keep_for_pseudonymization' => { is_boolean => 1 }, '+mandatory' => { is_boolean => 1 }, diff --git a/admin/patron-attr-types.pl b/admin/patron-attr-types.pl index 47c16306a8a..df7377f9957 100755 --- a/admin/patron-attr-types.pl +++ b/admin/patron-attr-types.pl @@ -109,6 +109,7 @@ sub add_update_attribute_type { my $description = $input->param('description'); my $repeatable = $input->param('repeatable') ? 1 : 0; my $unique_id = $input->param('unique_id') ? 1 : 0; + my $trim_value = $input->param('trim_value') ? 1 : 0; my $is_date = $input->param('is_date') ? 1 : 0; my $opac_display = $input->param('opac_display') ? 1 : 0; my $opac_editable = $input->param('opac_editable') ? 1 : 0; @@ -146,6 +147,7 @@ sub add_update_attribute_type { { repeatable => $repeatable, unique_id => $unique_id, + trim_value => $trim_value, is_date => $is_date, opac_display => $opac_display, opac_editable => $opac_editable, @@ -237,12 +239,20 @@ sub edit_attribute_type_form { $can_be_set_to_unique = 0 if $@; $attr_type->unique_id(0); } + my $can_be_trimmed = 1; + if ( $attr_type->trim_value == 0 ) { + $attr_type->trim_value(1); + eval {$attr_type->check_untrimmed_values}; + $can_be_trimmed = 0 if $@; + $attr_type->trim_value(0); + } $template->param( attribute_type => $attr_type, attribute_type_form => 1, edit_attribute_type => 1, can_be_set_to_nonrepeatable => $can_be_set_to_nonrepeatable, can_be_set_to_unique => $can_be_set_to_unique, + can_be_trimmed => $can_be_trimmed, confirm_op => 'cud-edit_attribute_type_confirmed', categories => $patron_categories, ); diff --git a/installer/data/mysql/atomicupdate/bug_32767-trim-attr-feature.pl b/installer/data/mysql/atomicupdate/bug_32767-trim-attr-feature.pl new file mode 100644 index 00000000000..f387e8831fe --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug_32767-trim-attr-feature.pl @@ -0,0 +1,14 @@ +use Modern::Perl; + +return { + bug_number => "32767", + description => "New feature that trims attributes (if enabled) to avoid duplicates", + up => sub { + my ($args) = @_; + my ($dbh, $out) = @$args{qw(dbh out)}; + + unless ( column_exists( 'borrower_attribute_types', 'trim_value' ) ) { + $dbh->do( "ALTER TABLE `borrower_attribute_types` ADD COLUMN `trim_value` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'defines if this value needs to be trimmed of whitespaces (1 for yes, 0 for no)' AFTER unique_id" ); + } + }, +}; \ No newline at end of file diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql index d3b36dca871..37a0ebeebac 100644 --- a/installer/data/mysql/kohastructure.sql +++ b/installer/data/mysql/kohastructure.sql @@ -1253,6 +1253,7 @@ CREATE TABLE `borrower_attribute_types` ( `description` varchar(255) NOT NULL COMMENT 'description for each custom field', `repeatable` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'defines whether one patron/borrower can have multiple values for this custom field (1 for yes, 0 for no)', `unique_id` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'defines if this value needs to be unique (1 for yes, 0 for no)', + `trim_value` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'defines if this value needs to be trimmed of whitespaces (1 for yes, 0 for no)', `is_date` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'defines if this field is displayed as a date', `opac_display` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'defines if this field is visible to patrons on their account in the OPAC (1 for yes, 0 for no)', `opac_editable` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'defines if this field is editable by patrons on their account in the OPAC (1 for yes, 0 for no)', diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt index 631a41867b8..124ca013608 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/patron-attr-types.tt @@ -139,6 +139,27 @@ [% END %] If checked, attribute will be a unique identifier. If a value is given to a patron record, the same value cannot be given to a different record. + [% IF attribute_type AND attribute_type.trim_value %] +
  • + [% ELSE %] +
  • + [% END %] + + [% IF attribute_type %] + [% IF NOT can_be_trimmed %] + + + At least one patron has attribute with whitespaces at the start or/end of it. Please fix that. + [% ELSIF attribute_type.trim_value %] + + [% ELSE %] + + [% END %] + [% ELSE %] + + [% END %] + Check to always trim whitespaces around the identifier to avoid creating duplicates. +
  • [% IF attribute_type AND attribute_type.is_date %] diff --git a/members/memberentry.pl b/members/memberentry.pl index 50343bcc04d..c9b80697e25 100755 --- a/members/memberentry.pl +++ b/members/memberentry.pl @@ -413,6 +413,7 @@ if ( $op eq 'cud-save' || $op eq 'cud-insert' ) { for my $attr (@$extended_patron_attributes) { $attr->{borrowernumber} = $borrowernumber if $borrowernumber; my $attribute = Koha::Patron::Attribute->new($attr); + $attribute->do_trim_value_if_needed; if ( !$attribute->unique_ok ) { push @errors, "ERROR_extended_unique_id_failed"; my $attr_type = Koha::Patron::Attribute::Types->find( $attr->{code} ); diff --git a/opac/opac-memberentry.pl b/opac/opac-memberentry.pl index 9517e8f1c55..700c5f2dfbb 100755 --- a/opac/opac-memberentry.pl +++ b/opac/opac-memberentry.pl @@ -117,6 +117,7 @@ my $conflicting_attribute = 0; foreach my $attr (@$attributes) { my $attribute = Koha::Patron::Attribute->new($attr); + $attribute->do_trim_value_if_needed; if ( !$attribute->unique_ok ) { my $attr_type = Koha::Patron::Attribute::Types->find( $attr->{code} ); $template->param( diff --git a/t/db_dependent/Koha/Patron/Attribute.t b/t/db_dependent/Koha/Patron/Attribute.t index baaf564ef42..2e55367ba67 100755 --- a/t/db_dependent/Koha/Patron/Attribute.t +++ b/t/db_dependent/Koha/Patron/Attribute.t @@ -37,7 +37,7 @@ my $builder = t::lib::TestBuilder->new; subtest 'store() tests' => sub { - plan tests => 6; + plan tests => 7; subtest 'Update an attribute should update the patron "updated_on" field' => sub { @@ -282,6 +282,79 @@ subtest 'store() tests' => sub { $schema->storage->txn_rollback; }; +subtest 'trim identifier tests (Bug 32767)' => sub { + + plan tests => 5; + + $schema->storage->txn_begin; + + my $patron_1 = $builder->build( { source => 'Borrower' } )->{borrowernumber}; + my $patron_2 = $builder->build( { source => 'Borrower' } )->{borrowernumber}; + + my $attribute_type_1 = $builder->build( + { source => 'BorrowerAttributeType', + value => { unique_id => 1, trim_value => 0 } + } + ); + Koha::Patron::Attribute->new( + { borrowernumber => $patron_1, + code => $attribute_type_1->{code}, + attribute => '123' + } + )->store; + Koha::Patron::Attribute->new( + { borrowernumber => $patron_2, + code => $attribute_type_1->{code}, + attribute => ' 123' + } + )->store; + my $attr_count + = Koha::Patron::Attributes->search( + { code => $attribute_type_1->{code} } ) + ->count; + is( $attr_count, 2, + 'Two identical attributes stored and retrieved correctly because one of them had whitespace at the start' ); + + my $attribute_type_2 = $builder->build( + { source => 'BorrowerAttributeType', + value => { unique_id => 1, trim_value => 1 } + } + ); + + Koha::Patron::Attribute->new( + { borrowernumber => $patron_1, + code => $attribute_type_2->{code}, + attribute => '123' + } + )->store; + throws_ok { + Koha::Patron::Attribute->new( + { borrowernumber => $patron_2, + code => $attribute_type_2->{code}, + attribute => ' 123' + } + )->store; + } + 'Koha::Exceptions::Patron::Attribute::UniqueIDConstraint', + 'Exception thrown trying to store more than one unique attribute now that we enabled trim_value'; + + is( + "$@", + "Your action breaks a unique constraint on the attribute. type=" + . $attribute_type_2->{code} + . " value=123", + 'Exception stringified correctly, attribute passed correctly' + ); + + my $attributes = Koha::Patron::Attributes->search( + { borrowernumber => $patron_1, code => $attribute_type_2->{code} } ); + is( $attributes->count, 1, '1 unique attribute stored' ); + is( $attributes->next->attribute, + '123', 'our unique attribute remains unchanged' ); + + $schema->storage->txn_rollback; + }; + subtest 'invalid type tests' => sub { plan tests => 2; -- 2.51.1