Bug 28220: Add more tests
* Add the transaction when a patron is created * The changes in merge_and_replace_with prevent the creation of the patron and so the attributes if several non-repeatable attributes are passed Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
This commit is contained in:
parent
e755139334
commit
4de341c2d0
3 changed files with 158 additions and 78 deletions
|
|
@ -96,8 +96,9 @@ $new_attributes is an arrayref of hashrefs
|
|||
sub merge_and_replace_with {
|
||||
my ( $self, $new_attributes ) = @_;
|
||||
|
||||
my @merged = @{$self->unblessed};
|
||||
my @existing_attributes = @{$self->unblessed};
|
||||
my $attribute_types = { map { $_->code => $_->unblessed } Koha::Patron::Attribute::Types->search };
|
||||
my @new_attributes;
|
||||
for my $attr ( @$new_attributes ) {
|
||||
|
||||
my $attribute_type = $attribute_types->{$attr->{code}};
|
||||
|
|
@ -107,13 +108,13 @@ sub merge_and_replace_with {
|
|||
|
||||
unless ( $attribute_type->{repeatable} ) {
|
||||
# filter out any existing attributes of the same code
|
||||
@merged = grep {$attr->{code} ne $_->{code}} @merged;
|
||||
@existing_attributes = grep {$attr->{code} ne $_->{code}} @existing_attributes;
|
||||
}
|
||||
|
||||
push @merged, $attr;
|
||||
push @new_attributes, $attr;
|
||||
}
|
||||
|
||||
@merged = map { { code => $_->{code}, attribute => $_->{attribute} } } @merged;
|
||||
my @merged = map { { code => $_->{code}, attribute => $_->{attribute} } } ( @existing_attributes, @new_attributes );
|
||||
|
||||
# WARNING - we would like to return a set, but $new_attributes is not in storage yet
|
||||
# Maybe there is something obvious I (JD) am missing
|
||||
|
|
|
|||
|
|
@ -241,6 +241,7 @@ sub import_patrons {
|
|||
if exists $borrower{$field} and $borrower{$field} eq "";
|
||||
}
|
||||
|
||||
my $success = 1;
|
||||
if ($borrowernumber) {
|
||||
|
||||
# borrower exists
|
||||
|
|
@ -270,9 +271,87 @@ sub import_patrons {
|
|||
}
|
||||
|
||||
my $patron = Koha::Patrons->find( $borrowernumber );
|
||||
eval { $patron->set(\%borrower)->store };
|
||||
if ( $@ ) {
|
||||
try {
|
||||
$schema->storage->txn_do(sub {
|
||||
$patron->set(\%borrower)->store;
|
||||
# Don't add a new restriction if the existing 'combined' restriction matches this one
|
||||
if ( $borrower{debarred} && ( ( $borrower{debarred} ne $member->{debarred} ) || ( $borrower{debarredcomment} ne $member->{debarredcomment} ) ) ) {
|
||||
|
||||
# Check to see if this debarment already exists
|
||||
my $debarrments = GetDebarments(
|
||||
{
|
||||
borrowernumber => $borrowernumber,
|
||||
expiration => $borrower{debarred},
|
||||
comment => $borrower{debarredcomment}
|
||||
}
|
||||
);
|
||||
|
||||
# If it doesn't, then add it!
|
||||
unless (@$debarrments) {
|
||||
AddDebarment(
|
||||
{
|
||||
borrowernumber => $borrowernumber,
|
||||
expiration => $borrower{debarred},
|
||||
comment => $borrower{debarredcomment}
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
if ($patron->category->category_type ne 'S' && $overwrite_passwords && defined $borrower{password} && $borrower{password} ne ''){
|
||||
try {
|
||||
$patron->set_password({ password => $borrower{password} });
|
||||
}
|
||||
catch {
|
||||
if ( $_->isa('Koha::Exceptions::Password::TooShort') ) {
|
||||
push @errors, { passwd_too_short => 1, borrowernumber => $borrowernumber, length => $_->{length}, min_length => $_->{min_length} };
|
||||
}
|
||||
elsif ( $_->isa('Koha::Exceptions::Password::WhitespaceCharacters') ) {
|
||||
push @errors, { passwd_whitespace => 1, borrowernumber => $borrowernumber } ;
|
||||
}
|
||||
elsif ( $_->isa('Koha::Exceptions::Password::TooWeak') ) {
|
||||
push @errors, { passwd_too_weak => 1, borrowernumber => $borrowernumber } ;
|
||||
}
|
||||
elsif ( $_->isa('Koha::Exceptions::Password::Plugin') ) {
|
||||
push @errors, { passwd_plugin_err => 1, borrowernumber => $borrowernumber } ;
|
||||
}
|
||||
else {
|
||||
push @errors, { passwd_unknown_err => 1, borrowernumber => $borrowernumber } ;
|
||||
}
|
||||
}
|
||||
}
|
||||
if ($extended) {
|
||||
if ($ext_preserve) {
|
||||
$patron_attributes = $patron->extended_attributes->merge_and_replace_with( $patron_attributes );
|
||||
}
|
||||
# We do not want to filter by branch, maybe we should?
|
||||
Koha::Patrons->find($borrowernumber)->extended_attributes->delete;
|
||||
$patron->extended_attributes($patron_attributes);
|
||||
}
|
||||
$overwritten++;
|
||||
push(
|
||||
@feedback,
|
||||
{
|
||||
feedback => 1,
|
||||
name => 'lastoverwritten',
|
||||
value => $borrower{'surname'} . ' / ' . $borrowernumber
|
||||
}
|
||||
);
|
||||
});
|
||||
} catch {
|
||||
$invalid++;
|
||||
$success = 0;
|
||||
|
||||
my $patron_id = defined $matchpoint ? $borrower{$matchpoint} : $matchpoint_attr_type;
|
||||
if ( $_->isa('Koha::Exceptions::Patron::Attribute::UniqueIDConstraint') ) {
|
||||
push @errors, { patron_attribute_unique_id_constraint => 1, patron_id => $patron_id, attribute => $_->attribute };
|
||||
} elsif ( $_->isa('Koha::Exceptions::Patron::Attribute::InvalidType') ) {
|
||||
push @errors, { patron_attribute_invalid_type => 1, patron_id => $patron_id, attribute_type_code => $_->type };
|
||||
} elsif ( $_->isa('Koha::Exceptions::Patron::Attribute::NonRepeatable') ) {
|
||||
push @errors, { patron_attribute_non_repeatable => 1, patron_id => $patron_id, attribute => $_->attribute };
|
||||
} else {
|
||||
use Data::Printer colored => 1; warn p $_;
|
||||
push @errors, { unknown_error => 1 };
|
||||
}
|
||||
|
||||
push(
|
||||
@errors,
|
||||
|
|
@ -282,76 +361,7 @@ sub import_patrons {
|
|||
value => $borrower{'surname'} . ' / ' . $borrowernumber
|
||||
}
|
||||
);
|
||||
next LINE;
|
||||
}
|
||||
# Don't add a new restriction if the existing 'combined' restriction matches this one
|
||||
if ( $borrower{debarred} && ( ( $borrower{debarred} ne $member->{debarred} ) || ( $borrower{debarredcomment} ne $member->{debarredcomment} ) ) ) {
|
||||
|
||||
# Check to see if this debarment already exists
|
||||
my $debarrments = GetDebarments(
|
||||
{
|
||||
borrowernumber => $borrowernumber,
|
||||
expiration => $borrower{debarred},
|
||||
comment => $borrower{debarredcomment}
|
||||
}
|
||||
);
|
||||
|
||||
# If it doesn't, then add it!
|
||||
unless (@$debarrments) {
|
||||
AddDebarment(
|
||||
{
|
||||
borrowernumber => $borrowernumber,
|
||||
expiration => $borrower{debarred},
|
||||
comment => $borrower{debarredcomment}
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
if ($patron->category->category_type ne 'S' && $overwrite_passwords && defined $borrower{password} && $borrower{password} ne ''){
|
||||
try {
|
||||
$patron->set_password({ password => $borrower{password} });
|
||||
}
|
||||
catch {
|
||||
if ( $_->isa('Koha::Exceptions::Password::TooShort') ) {
|
||||
push @errors, { passwd_too_short => 1, borrowernumber => $borrowernumber, length => $_->{length}, min_length => $_->{min_length} };
|
||||
}
|
||||
elsif ( $_->isa('Koha::Exceptions::Password::WhitespaceCharacters') ) {
|
||||
push @errors, { passwd_whitespace => 1, borrowernumber => $borrowernumber } ;
|
||||
}
|
||||
elsif ( $_->isa('Koha::Exceptions::Password::TooWeak') ) {
|
||||
push @errors, { passwd_too_weak => 1, borrowernumber => $borrowernumber } ;
|
||||
}
|
||||
elsif ( $_->isa('Koha::Exceptions::Password::Plugin') ) {
|
||||
push @errors, { passwd_plugin_err => 1, borrowernumber => $borrowernumber } ;
|
||||
}
|
||||
else {
|
||||
push @errors, { passwd_unknown_err => 1, borrowernumber => $borrowernumber } ;
|
||||
}
|
||||
}
|
||||
}
|
||||
if ($extended) {
|
||||
if ($ext_preserve) {
|
||||
$patron_attributes = $patron->extended_attributes->merge_and_replace_with( $patron_attributes );
|
||||
}
|
||||
eval {
|
||||
# We do not want to filter by branch, maybe we should?
|
||||
Koha::Patrons->find($borrowernumber)->extended_attributes->delete;
|
||||
$patron->extended_attributes($patron_attributes);
|
||||
};
|
||||
if ($@) {
|
||||
# FIXME This is not an unknown error, we can do better here
|
||||
push @errors, { unknown_error => 1 };
|
||||
}
|
||||
}
|
||||
$overwritten++;
|
||||
push(
|
||||
@feedback,
|
||||
{
|
||||
feedback => 1,
|
||||
name => 'lastoverwritten',
|
||||
value => $borrower{'surname'} . ' / ' . $borrowernumber
|
||||
}
|
||||
);
|
||||
}
|
||||
else {
|
||||
try {
|
||||
|
|
@ -397,6 +407,7 @@ sub import_patrons {
|
|||
});
|
||||
} catch {
|
||||
$invalid++;
|
||||
$success = 0;
|
||||
my $patron_id = defined $matchpoint ? $borrower{$matchpoint} : $matchpoint_attr_type;
|
||||
if ( $_->isa('Koha::Exceptions::Patron::Attribute::UniqueIDConstraint') ) {
|
||||
push @errors, { patron_attribute_unique_id_constraint => 1, patron_id => $patron_id, attribute => $_->attribute };
|
||||
|
|
@ -406,6 +417,7 @@ sub import_patrons {
|
|||
push @errors, { patron_attribute_non_repeatable => 1, patron_id => $patron_id, attribute => $_->attribute };
|
||||
|
||||
} else {
|
||||
use Data::Printer colored => 1; warn p $_;
|
||||
push @errors, { unknown_error => 1 };
|
||||
}
|
||||
push(
|
||||
|
|
@ -418,6 +430,8 @@ sub import_patrons {
|
|||
};
|
||||
}
|
||||
|
||||
next LINE unless $success;
|
||||
|
||||
# Add a guarantor if we are given a relationship
|
||||
if ( $guarantor_id ) {
|
||||
my $relationship = Koha::Patron::Relationships->find(
|
||||
|
|
|
|||
|
|
@ -969,7 +969,7 @@ subtest 'patron_attributes' => sub {
|
|||
}
|
||||
|
||||
subtest 'update existing patron' => sub {
|
||||
plan tests => 6;
|
||||
plan tests => 19;
|
||||
|
||||
my $patron = $builder->build_object(
|
||||
{
|
||||
|
|
@ -1016,8 +1016,7 @@ subtest 'patron_attributes' => sub {
|
|||
# The normal_attribute_type has been replaced with 'my normal attribute 2'
|
||||
compare_patron_attributes($patron->extended_attributes->unblessed, { %$attributes, %$new_attributes } );
|
||||
|
||||
|
||||
# uniqueness
|
||||
# UniqueIDConstraint
|
||||
$patron->extended_attributes->delete; # reset
|
||||
$builder->build_object(
|
||||
{
|
||||
|
|
@ -1041,6 +1040,72 @@ subtest 'patron_attributes' => sub {
|
|||
);
|
||||
|
||||
is( $result->{overwritten}, 0 );
|
||||
my $error = $result->{errors}->[0];
|
||||
is( $error->{patron_attribute_unique_id_constraint}, 1 );
|
||||
is( $error->{patron_id}, $cardnumber );
|
||||
is( $error->{attribute}->code, $unique_attribute_type->code );
|
||||
|
||||
compare_patron_attributes($patron->extended_attributes->unblessed, {}, );
|
||||
|
||||
|
||||
#InvalidType
|
||||
$attributes = {
|
||||
$non_existent_attribute_type_code => ['my non-existent attribute'],
|
||||
$normal_attribute_type->code => ['my attribute 1'],
|
||||
};
|
||||
$fh = build_csv({ %$attributes });
|
||||
|
||||
$result = $patrons_import->import_patrons(
|
||||
{
|
||||
file => $fh,
|
||||
matchpoint => 'cardnumber',
|
||||
overwrite_cardnumber => 1,
|
||||
preserve_extended_attributes => 1
|
||||
}
|
||||
);
|
||||
is( $result->{overwritten}, 0 );
|
||||
|
||||
$error = $result->{errors}->[0];
|
||||
is( $error->{patron_attribute_invalid_type}, 1 );
|
||||
is( $error->{patron_id}, $cardnumber );
|
||||
is( $error->{attribute_type_code}, $non_existent_attribute_type_code );
|
||||
|
||||
# NonRepeatable
|
||||
$attributes = {
|
||||
$repeatable_attribute_type->code => ['my repeatable attribute 1', 'my repeatable attribute 2'],
|
||||
$normal_attribute_type->code => ['my normal attribute 1', 'my normal attribute 2'],
|
||||
};
|
||||
$fh = build_csv({ %$attributes });
|
||||
$result = $patrons_import->import_patrons(
|
||||
{
|
||||
file => $fh,
|
||||
matchpoint => 'cardnumber',
|
||||
overwrite_cardnumber => 1,
|
||||
preserve_extended_attributes => 1
|
||||
}
|
||||
);
|
||||
is( $result->{overwritten}, 0 );
|
||||
|
||||
$error = $result->{errors}->[0];
|
||||
is( $error->{patron_attribute_non_repeatable}, 1 );
|
||||
is( $error->{patron_id}, $cardnumber );
|
||||
is( $error->{attribute}->code, $normal_attribute_type->code );
|
||||
|
||||
# Don't preserve existing attributes
|
||||
$attributes = {
|
||||
$repeatable_attribute_type->code => ['my repeatable attribute 3', 'my repeatable attribute 4'],
|
||||
$normal_attribute_type->code => ['my normal attribute 1'],
|
||||
};
|
||||
$fh = build_csv({ %$attributes });
|
||||
$result = $patrons_import->import_patrons(
|
||||
{
|
||||
file => $fh,
|
||||
matchpoint => 'cardnumber',
|
||||
overwrite_cardnumber => 1,
|
||||
preserve_extended_attributes => 1
|
||||
}
|
||||
);
|
||||
is( $result->{overwritten}, 1 );
|
||||
|
||||
compare_patron_attributes($patron->extended_attributes->unblessed, { %$attributes } );
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue