[Koha-bugs] [Bug 7677] New area in subscriptions and new function when receiving

[bugzilla-daemon at bugs.koha-community.org]

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7677

--- Comment #18 from Katrin Fischer <katrin.fischer at bsz-bw.de> ---
Comment on attachment 24099
  --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=24099
proposed patch

Review of attachment 24099:
 --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=7677&attachment=24099)
-----------------------------------------------------------------

Starting with a code review..., but also ran some tests in Koha.

I really like the idea behind this and have been asked about functionality to
automatically set/unset a status for the newest and previous issue before. But
I think the current implementation is a bit problematic.

Also there are some changes in this patch, that are not related to the main
change and cause a lot of question marks for me. The change for the copynumber
is especially problematic, as MARC21 uses a different field for the information
already.

I think this needs a bit more work - also see comments in splinter review
below.

::: C4/Items.pm
@@ +2782,5 @@
>                              push @authorised_values, $itemtype;
>                              $authorised_lib{$itemtype} = $description;
> +
> +                            # If we have default value named itemtype or itemtypes, we use it
> +                            $defaultvalue = $itemtype if ($defaultvalues and ($defaultvalues->{'itemtypes'} eq $itemtype or $defaultvalues->{'itemtype'} eq $itemtype));

How is this change related to the bug description?

@@ +2814,5 @@
>                              $authorised_lib{$value} = $lib;
> +
> +                            # If we have a default value that has the same name as the authorised value category of the field,
> +                            # we use it
> +                            $defaultvalue = $value if ($defaultvalues and $defaultvalues->{$tagslib->{$tag}->{$subfield}->{authorised_value}} and $defaultvalues->{$tagslib->{$tag}->{$subfield}->{authorised_value}} eq $value);

How is this change related to the bug description?

::: C4/Serials.pm
@@ +926,5 @@
> +
> +    return $return;
> +}
> +
> +

Unit tests included in a separate patch. I had to solve a conflict, but the
tests pass. All good.

::: installer/data/mysql/updatedatabase.pl
@@ +7935,5 @@
> +    );
> +
> +    print "Upgrade to $DBversion done (Add makePreviousSerialAvailable syspref)\n";
> +    SetVersion($DBversion);
> +}

Please combine into one single database update and add an AFTER reneweddate to
ensure that the sequence of fields is the same on new and updated
installations.

Also - 15 :)

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/serials.pref
@@ +54,5 @@
> +        - pref: makePreviousSerialAvailable
> +          choices: 
> +            yes: Make
> +            no: Do not make
> +        - previous serial automatically available when collecting a new serial. Please note that the item-level_itypes syspref must be set to specific item.

I think 'receiving' would be a bit better than 'collecting' here, but probably
better to check wording with a native speaker. It's not clear, which status
will be used from the description - What is the first, and what is the second
status? Which fields will be used? 

Could it be an option to make this pref a bit more configurable, behaving like
AcqItemSetSubfieldsWhenReceived?

::: koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt
@@ +62,5 @@
> +    // Also prefilling copynumber
> +    // Getting subfield with copynumber mapping
> +    subfield_div = $(item_div).find("input[name='kohafield'][value='items.copynumber']").parent();
> +    // Setting text field
> +    $(subfield_div).children("input[type='text'][name='field_value']").val($("#serialseq" + serialId).val());

This is a bigger problem: I think copynumber is used in MARC21 to indicate
different copies of the same book. The enumchron field is used for the
information about issue and year. For MARC21 serialseq is now copied into both
fields. I think we can't leave it that way. My suggestion would be to use
enumchron for UNIMARC as well or make this marcflavour dependent.

::: koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt
@@ +621,5 @@
> +                                    </select>
> +                                </li>
> +                                [%IF makePreviousSerialAvailable %]
> +                                <li>
> +                                    <label for="previousitemtype">Previous serial Item type:</label>

Tiny capitalization issue - "Item".
As this is on the subscription form, we could maybe leave out "serial" and
shorten the description ab it. Also "Serial" is missing from the first field.

::: serials/serials-edit.pl
@@ +243,5 @@
>                  $notes[$i]
>              );
>          }
> +        my $makePreviousSerialAvailable = C4::Context->preference('makePreviousSerialAvailable');
> +        if ($makePreviousSerialAvailable && $serialids[$i] ne "NEW") {

What is 'NEW' referring to? In my tests the change for the itemtype worked, but
I couldn't figure out, how the status change is supposed to work.

::: serials/subscription-add.pl
@@ +153,5 @@
>  # prepare template variables common to all $op conditions:
> +my $shelflocations = GetKohaAuthorisedValues( 'items.location', '' );
> +
> +my @locationarg =
> +  map { { code => $_, value => $shelflocations->{$_}, selected => ( ( $subs->{location} and $_ eq $subs->{location} ) ? "selected=\"selected\"" : "" ), } } sort keys %{$shelflocations};

How is this change related to the bug description?

-- 
You are receiving this mail because:
You are watching all bug changes.