http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7677
--- Comment #18 from Katrin Fischer <[email protected]> --- 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. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
