In the subscriptions new areas were added. itype for the last numero (995$r) for the last item received itype for the previous numero (995$r) for the previous item received When receiving new features : 1. the Issue number is automatically copied into $ 995 v (field "Issue No." copy of the record). 2. The itype for the last numero in the subscription is assigned to the last subscription copy received 3. the previous copy is changed : will be awarded the itype for the previous numero who is signaled in the subscription 4. the previous copy is changed : will be awarded the code notforloan 0 'Avaible' The last action cited is possible only if the preference system makePreviousSerialAvailable is activated
Test plan: Once the makePreviousSerialAvailable syspref is enabled, receive a serial, and then another, then check that: - the first received itemtype has been set to the "previous item type" value (set in the subscription). - the first received has been made available. - the last received serial itemtype has been set to the "item type" value (set in the subscription). - 995$l is automatically prefilled. Configure the serialsFieldsToAV syspref. When creating or editing a subscription, check that: - the domain and/or origin and/or support fields are correctly displaying the authorized values configured in the syspref.
Created attachment 16339 [details] [review] Bug 7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. It is also made available. You can define the serialsFieldsToAV syspref to associate authorized values to the domain, origin and/or support fields. 995$l is automatically prefilled when receiving.
Created attachment 16425 [details] [review] Bug 7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available. You can define the serialsFieldsToAV syspref to associate authorized values to the domain, origin and/or support fields. 995$l is automatically prefilled when receiving.
This patch adds explanations about how the item-level_itypes works with makePreviousSerialAvailable.
Patch tested with a sandbox, by Frederic Durand <frederic.durand@unilim.fr>
Created attachment 16456 [details] [review] Bug 7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available. You can define the serialsFieldsToAV syspref to associate authorized values to the domain, origin and/or support fields. 995$l is automatically prefilled when receiving. Signed-off-by: Frederic Durand <frederic.durand@unilim.fr>
Comment on attachment 16456 [details] [review] Bug 7677: New areas in subscriptions and new functions when receiving. Review of attachment 16456 [details] [review]: ----------------------------------------------------------------- This feature will have to wait for 3.14, but I did take a look at the code today and wanted to leave some notes. ::: C4/Serials.pm @@ +45,4 @@ > &ReNewSubscription &GetLateIssues &GetLateOrMissingIssues > &GetSerialInformation &AddItem2Serial > &PrepareSerialsData &GetNextExpected &ModNextExpected > + &GetPreviousSerialid Needs a unit test! ::: installer/data/mysql/updatedatabase.pl @@ +6570,5 @@ > + print "Upgrade to $DBversion done (added new syspref: serialsFieldsToAV)\n"; > + SetVersion($DBversion); > +} > + > + Database updates should be merged into 1 update with a nice description. ::: koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt @@ +75,5 @@ > subfieldid = "subfield" + itemid.substr(4) + "h"; > // Setting text field > $("#" + subfieldid + " input[type='text']").val($("#serialseq" + serialId).val()); > + // Also prefilling 995$l > + subfieldid = "subfield" + itemid.substr(4) + "l"; This looks like it's UNIMARC specific? ::: serials/subscription-add.pl @@ +193,5 @@ > + previoustypeloop => \@previoustypearg, > + supportloop => \@supportarg, > + originloop => \@originarg, > + domainloop => \@domainarg, > + DHTMLcalendar_dateformat => C4::Dates->DHTMLcalendar(), DHTMLcalendar is no longer used.
Also I was wondering about the purpose of domain, origin and support. How do they work? :) Failed QA for the missing unit test for now.
Created attachment 20537 [details] [review] Bug 7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available. You can define the serialsFieldsToAV syspref to associate authorized values to the domain, origin and/or support fields. 995$l is automatically prefilled when receiving. Signed-off-by: Frederic Durand <frederic.durand@unilim.fr> http://bugs.koha-community.org/show_bug.cgi?id=8435
Created attachment 20538 [details] [review] Bug 7677: UT: add unit tests for GetPreviousSerialid
attachment 20537 [details] [review] is a rebased patch. attachment 20538 [details] [review] is the unit tests for GetPreviousSerialid Now this patch depends on bug 8435 (in order to avoir conflicts on the unit tests). Needs QA.
Hi Jonathan and Frederic, could you please add some documentation about the use of the new fields? Also the commit message states that 995... will be automatically filled. And this is indeed hardcoded: + // Also prefilling 995$l + subfieldid = "subfield" + itemid.substr(4) + "l"; + $("#" + subfieldid + " input[type='text']").val($("#serialseq" + serialId).val()); + // And prefilling for supplemental issue + $("#subfieldNEWl input[type='text']").val($("#serialseqNEW").val()); +
Hm, I see now that I had already commented in comment 7...
Hi ! The new field previousitemtype is used to automatically set a given item type to the previously received serial (when a new one is received). The fields domain origin and ccode are used to provide statistics, like counting the number of items per domain, origin and ccode. As for the 995l, yes, it should be marc-agnostic.
Could you describe domain and origin in a sentence or two? I am really confused what to put in those fields.
> the commit message states that 995... will be automatically filled. And this is indeed hardcoded The subfield $h was also hardcoded. It has been corrected by Bug 9947. This patch should be corrected the same way.
Created attachment 24099 [details] [review] proposed patch This patch solves the problems previously discussed: - The 3 new fields (origin, domain and ccode) that had an unclear use-case and were too specific were removed from the patch, as Bug 10855 will do the job in a more generic way. - The hardcoding for 995$l was replaced by a marc-agnostic version (as Fridolin suggested in his last comment).
Comment on attachment 24099 [details] [review] proposed patch Review of attachment 24099 [details] [review]: ----------------------------------------------------------------- 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?
::: 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)); and @@ +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); and ::: 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}; -> You're right, this is not needed. ::: C4/Serials.pm @@ +926,5 @@ > + > + return $return; > +} > + > + -> Why do you quote this part specifically? The patch adds the whole GetPreviousSerialid, which return is needed. Am I missing something here? 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. -> done 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? -> when makePreviousSerialAvailable is enabled, when receiving a new serial, notforloan will be set to 0 and the itype will be set to previousitemtype for previously received serial. Are these the points I should make more explicit? Could it be an option to make this pref a bit more configurable, behaving like AcqItemSetSubfieldsWhenReceived? -> If I'm not mistaken, AcqItemSetSubfieldsWhenReceived applies to the serial we just received, as makePreviousSerialAvailable applies to the one that was received before (just to make things clear). But besides that, if there are use-cases where other fields than notforloan and itype have to be used, it might be useful to have something more configurable. Is it so? 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. -> using enumchron for UNIMARC as well makes sense. Done. 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. -> done 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. -> 'NEW' is previously set by koha : $cell->{'serialid'} = 'NEW'; (serials/serials-edit.pl:185)
Created attachment 24635 [details] [review] proposed patch
Jonathan, I have a problem in the QA script I am not sure how to fix - could you take a look please? FAIL serials/serials-edit.pl OK pod OK forbidden patterns FAIL valid "my" variable $dbh masks earlier declaration in same scope OK critic
Created attachment 25980 [details] [review] Bug 7677: UT: add unit tests for GetPreviousSerialid
Created attachment 25981 [details] [review] BZ7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available.
Created attachment 25983 [details] [review] Bug 7677: FIX QA issues FAIL C4/Serials.pm FAIL forbidden patterns forbidden pattern: trailing space char (line 1501) forbidden pattern: trailing space char (line 921) FAIL serials/serials-edit.pl FAIL valid "my" variable $dbh masks earlier declaration in same scope
If we look at the Description http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7677#c0 Points 0, 1, 3 and 4 are OK. But Point 2 KO. Nota : There's a link with BZ10855 http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10855 because some new areas of the subscription must appear in the item record fields.
(In reply to Koha Team AMU from comment #25) > If we look at the Description > http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7677#c0 > Points 0, 1, 3 and 4 are OK. > > But Point 2 KO. Matthias will try to have a look asap. > Nota : There's a link with BZ10855 > http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10855 > because some new areas of the subscription must appear in the item record > fields. I would prefer to see bug 10855 pushed before to add new behaviors.
The sentence describing the point 2 is not very clear. It means that the newly received serial automatically gets its itemtype from what is defined in the subscription in the "itemtype" dropdown list.
Created attachment 27774 [details] [review] Make the newly received serial gets its default itemtype from the subscription.
h1>Something went wrong !</h1>Applying: Bug 7677: UT: add unit tests for GetPreviousSerialid Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Serials_2.t Applying: BZ7677: New areas in subscriptions and new functions when receiving. Using index info to reconstruct a base tree... <stdin>:49: trailing whitespace. ORDER BY serialid DESC LIMIT ,1 <stdin>:97: trailing whitespace. , , , , , <stdin>:198: trailing whitespace. choices: warning: 3 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging C4/Serials.pm Auto-merging installer/data/mysql/kohastructure.sql Auto-merging installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt Auto-merging serials/subscription-add.pl Failed to merge in the changes. Patch failed at 0001 BZ7677: New areas in subscriptions and new functions when receiving.
Created attachment 28950 [details] [review] Bug 7677: UT: add unit tests for GetPreviousSerialid
Created attachment 28951 [details] [review] BZ7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available.
Created attachment 28952 [details] [review] Bug 7677: FIX QA issues FAIL C4/Serials.pm FAIL forbidden patterns forbidden pattern: trailing space char (line 1501) forbidden pattern: trailing space char (line 921) FAIL serials/serials-edit.pl FAIL valid "my" variable $dbh masks earlier declaration in same scope
Created attachment 28953 [details] [review] BZ7677 [Follow-up]: New areas in subscriptions and new functions when receiving. - Make the newly received serial gets its default itemtype from the subscription.
Created attachment 29570 [details] [review] BZ7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available.
Created attachment 29571 [details] [review] Bug 7677: FIX QA issues FAIL C4/Serials.pm FAIL forbidden patterns forbidden pattern: trailing space char (line 1501) forbidden pattern: trailing space char (line 921) FAIL serials/serials-edit.pl FAIL valid "my" variable $dbh masks earlier declaration in same scope
Created attachment 29572 [details] [review] BZ7677 [Follow-up]: New areas in subscriptions and new functions when receiving. - Make the newly received serial gets its default itemtype from the subscription.
rebased patches (updatedatabase conflict).
I saw this is on the list for the testing day :) It would be good to add a test plan and description. I think the feature evolved a bit over time, as this is one of the older bug reports.
I've tried to apply the patches against master 3.19.00.000 The second patch needed some [almost trivial] rebasing: Applying: BZ7677: New areas in subscriptions and new functions when receiving. Using index info to reconstruct a base tree... <stdin>:49: trailing whitespace. ORDER BY serialid DESC LIMIT $nth,1 <stdin>:97: trailing whitespace. $staffdisplaycount, $opacdisplaycount, $graceperiod, $location, $enddate, <stdin>:197: trailing whitespace. choices: warning: 3 lines add whitespace errors. Falling back to patching base and 3-way merge... Auto-merging serials/serials-edit.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/kohastructure.sql Auto-merging C4/Serials.pm Failed to merge in the changes. Patch failed at 0001 BZ7677: New areas in subscriptions and new functions when receiving. I pass the patch to "Patch doesn't apply" status.
Created attachment 34220 [details] [review] Bug 7677: UT: add unit tests for GetPreviousSerialid
Created attachment 34221 [details] [review] BZ7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available.
Created attachment 34222 [details] [review] Bug 7677: FIX QA issues FAIL C4/Serials.pm FAIL forbidden patterns forbidden pattern: trailing space char (line 1501) forbidden pattern: trailing space char (line 921) FAIL serials/serials-edit.pl FAIL valid "my" variable $dbh masks earlier declaration in same scope
Created attachment 34223 [details] [review] BZ7677 [Follow-up]: New areas in subscriptions and new functions when receiving. - Make the newly received serial gets its default itemtype from the subscription.
I've applied the patches against master 3.19.00.002 The first patch requires: prove t/db_dependent/Serials_2.t t/db_dependent/Serials_2.t .. Global symbol "$subscriptionid" requires explicit package name at t/db_dependent/Serials_2.t line 208. Global symbol "$subscriptionid" requires explicit package name at t/db_dependent/Serials_2.t line 209. Global symbol "$subscriptionid" requires explicit package name at t/db_dependent/Serials_2.t line 210. Global symbol "$subscriptionid" requires explicit package name at t/db_dependent/Serials_2.t line 212. Global symbol "$subscriptionid" requires explicit package name at t/db_dependent/Serials_2.t line 213. Global symbol "$subscriptionid" requires explicit package name at t/db_dependent/Serials_2.t line 214. Global symbol "$subscriptionid" requires explicit package name at t/db_dependent/Serials_2.t line 215. Execution of t/db_dependent/Serials_2.t aborted due to compilation errors. # Looks like your test exited with 255 before it could output anything. t/db_dependent/Serials_2.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 36/36 subtests Test Summary Report ------------------- t/db_dependent/Serials_2.t (Wstat: 65280 Tests: 0 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 36 tests but ran 0. Files=1, Tests=0, 1 wallclock secs ( 0.00 usr 0.01 sys + 0.72 cusr 0.05 csys = 0.78 CPU) Result: FAIL I pass the patch to "Failed QA" status.
Created attachment 34665 [details] [review] Bug 7677: Fix tests They don't pass since bug 12098 has been pushed.
I've applied the patches against master 3.19.00.003 [it needed a trivial rebase] I set "makePreviousSerialAvailable" syspref to "Make" as required. I kept the "item-level_itypes" syspref to "specific item". I created a subscription, which didn't create an item record when it received the serial. When I received ("Arrived") issues, no item was created: so this bug is not about this case. So I created another subscription, which created an item record when receiving the serial, and I set its 2 new fields ["Item type" and "Previous item type"] to "Continuing Resources". Then I "Edited" the subscription: the "Item type" and "Previous item type" fields were not set to "Continuing Resources" anymore! (ERROR) Anyway I went on: Was the previously received serial's itemtype set as defined in the subscription? When I received ("Arrived") an issue [in DB: table serial] of the subscription (the first issue), I saw that the default value of the: "y - Koha item type" field of the new item was set to nothing (why?). I didn't change it to "Continuing Resources" [because koha should do it for me, shouldn't it?]; then I filled the other fields (barcode, aso), and I saved the new item. Then I looked at the biblio: In the biblio's Holdings tag, the item was "Available" and its item type is CR as required (RIGHT). I received another issue [the 2-nd] for the same subscription. Now, in the biblio's Holdings, the new (2-nd) item was "Available" and CR, as required. But the previous (1-st) item [still "Available"] lost its type: it had no type anymore! (ERROR) But I could have been wrong: as Katrin wrote in comment 38: >It would be good to add a test plan and description. I think >the feature evolved a bit over time, as this is one of the older bug reports. Anyway: prove t/db_dependent/Serials_2.t t/db_dependent/Serials_2.t .. 3/40 # Failed test 'With IndependentBranches, superserials can edit a subscription from his branch' # at t/db_dependent/Serials_2.t line 106. # got: '0' # expected: '1' # Failed test 'With IndependentBranches, superserials can edit a subscription from another branch' # at t/db_dependent/Serials_2.t line 109. # got: '0' # expected: '1' # Failed test 'With IndependentBranches, superserials can show a subscription from his branch' # at t/db_dependent/Serials_2.t line 112. # got: '0' # expected: '1' # Failed test 'With IndependentBranches, superserials can show a subscription from another branch' # at t/db_dependent/Serials_2.t line 115. # got: '0' # expected: '1' # Failed test 'With IndependentBranches, edit_subscription can edit a subscription from his branch' # at t/db_dependent/Serials_2.t line 121. # got: '0' # expected: '1' # Failed test 'With IndependentBranches, show_subscription can show a subscription from his branch' # at t/db_dependent/Serials_2.t line 127. # got: '0' # expected: '1' # Failed test 'With IndependentBranches, renew_subscription can show a subscription from his branch' # at t/db_dependent/Serials_2.t line 141. # got: '0' # expected: '1' # Failed test 'Without IndependentBranches, superserials can edit a subscription from his branch' # at t/db_dependent/Serials_2.t line 166. # got: '0' # expected: '1' # Failed test 'Without IndependentBranches, superserials can edit a subscription from another branch' # at t/db_dependent/Serials_2.t line 169. # got: '0' # expected: '1' # Failed test 'Without IndependentBranches, superserials can show a subscription from his branch' # at t/db_dependent/Serials_2.t line 172. # got: '0' # expected: '1' # Failed test 'Without IndependentBranches, superserials can show a subscription from another branch' # at t/db_dependent/Serials_2.t line 175. # got: '0' # expected: '1' # Failed test 'Without IndependentBranches, edit_subscription can edit a subscription from his branch' # at t/db_dependent/Serials_2.t line 180. # got: '0' # expected: '1' # Failed test 'Without IndependentBranches, edit_subscription can edit a subscription from another branch' # at t/db_dependent/Serials_2.t line 183. # got: '0' # expected: '1' # Failed test 'Without IndependentBranches, show_subscription can show a subscription from his branch' # at t/db_dependent/Serials_2.t line 186. # got: '0' # expected: '1' # Failed test 'Without IndependentBranches, show_subscription can show a subscription from another branch' # at t/db_dependent/Serials_2.t line 189. # got: '0' # expected: '1' # Failed test 'Without IndependentBranches, renew_subscription cannot show a subscription from his branch' # at t/db_dependent/Serials_2.t line 200. # got: '0' # expected: '1' # Failed test 'Without IndependentBranches, renew_subscription cannot show a subscription from another branch' # at t/db_dependent/Serials_2.t line 203. # got: '0' # expected: '1' # Looks like you failed 17 tests of 40. t/db_dependent/Serials_2.t .. Dubious, test returned 17 (wstat 4352, 0x1100) Failed 17/40 subtests Test Summary Report ------------------- t/db_dependent/Serials_2.t (Wstat: 4352 Tests: 40 Failed: 17) Failed tests: 9-13, 15, 19, 25-32, 35-36 Non-zero exit status: 17 Files=1, Tests=40, 8 wallclock secs ( 0.01 usr 0.04 sys + 0.29 cusr 2.21 csys = 2.55 CPU) Result: FAIL ------------------------------------------------------------------------------ So I pass the patch to "Failed QA" status.
Updated test plan: Enable the makePreviousSerialAvailable syspref. Set the item-level_itypes syspref to 'specific item'. Create a subscription, with the 'create an item record when receiving this serial' option enabled. Select an item-type (let's say ItemType) and a previous item-type (let's say OldItemType). Receive a serial, and then another, then check that: - the first received serial's itemtype has been set to the "previous item type" value which was set in the subscription (OldItemType in our example). - the first received has been made available. - the last received serial itemtype has been set to the "item type" value, which was set in the subscription (ItemType in our example). - enumchron field of the item is automatically prefilled.
Paola, about your remarks: the "Item type" and "Previous item type" fields were not set to "Continuing Resources" anymore! (ERROR) => fixed in a little patch I will submit. When I received ("Arrived") an issue [in DB: table serial] of the suscription (the first issue), I saw that the default value of the: "y - Koha item type" field of the new item was set to nothing (why?). => Is your itemtype field (995$y in your case) using itemtypes authorized values in the bibliographic framework ? It does not work if authorized values are not linked. prove t/db_dependent/Serials_2.t => The tests also fail from test #3 on master, so my patch may have not broken the tests (must be fixed on master to be sure, though).
Created attachment 36389 [details] [review] BZ7677 [Follow-up]: New areas in subscriptions and new functions when receiving. - Fix missing itemtype and previous itemtype when adding a subscription.
About the tests, my bad: they are not broken in master, BUT these are db_dependent tests, and as such, they need to be run with a proper database. This patch allows to create such a database in your environment : http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10337 With this database on master, the tests run fine. With this database and my patches, the tests also run fine: > prove t/db_dependent/Serials_2.t [...] All tests successful. Files=1, Tests=40, 2 wallclock secs ( 0.04 usr 0.00 sys + 1.14 cusr 0.08 csys = 1.26 CPU) Result: PASS
Created attachment 36446 [details] [review] BZ7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available.
patch rebased for updatedatabase.
Created attachment 36448 [details] [review] BZ7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available.
Created attachment 36449 [details] [review] BZ7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available.
Created attachment 36450 [details] [review] Bug 7677: UT: add unit tests for GetPreviousSerialid
Created attachment 36451 [details] [review] Bug 7677: FIX QA issues FAIL C4/Serials.pm FAIL forbidden patterns forbidden pattern: trailing space char (line 1501) forbidden pattern: trailing space char (line 921) FAIL serials/serials-edit.pl FAIL valid "my" variable $dbh masks earlier declaration in same scope
Created attachment 36452 [details] [review] Bug 7677: Fix tests They don't pass since bug 12098 has been pushed.
Created attachment 36453 [details] [review] BZ7677 [Follow-up]: New areas in subscriptions and new functions when receiving. - Fix missing itemtype and previous itemtype when adding a subscription.
Created attachment 36679 [details] [review] BZ7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767
Created attachment 36680 [details] [review] Bug 7677: UT: add unit tests for GetPreviousSerialid Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767
Created attachment 36681 [details] [review] Bug 7677: FIX QA issues FAIL C4/Serials.pm FAIL forbidden patterns forbidden pattern: trailing space char (line 1501) forbidden pattern: trailing space char (line 921) FAIL serials/serials-edit.pl FAIL valid "my" variable $dbh masks earlier declaration in same scope Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767
Created attachment 36682 [details] [review] Bug 7677: Fix tests They don't pass since bug 12098 has been pushed. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767
Created attachment 36683 [details] [review] BZ7677 [Follow-up]: New areas in subscriptions and new functions when receiving. - Fix missing itemtype and previous itemtype when adding a subscription. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767
Wow, first comment here from me was over a year ago :( As this is a bigger feature with a longer history, I will leave some longer testing notes for reference: Automated Testing: - OK - QA script passes on current 5 patches - OK - Unit tests, especially all in t/db_dependent/Serial* pass Code Review: - OK - New sub GetPreviousSerialid has unit tests and is used to get the last arrived serial item. - OK - Database changes in updatedatabase and kohastructure are the same. - (1) serials-edit.pl: Patch adds SQL to the .pl script - please move this out of the pl script. - (2) System preference description currently reads: previous serial automatically available when collecting a new serial. Please note that the item-level_itypes syspref must be set to specific item. -- s/collecting/receiving -- s/serial/serial issue (?) -- It would be nice to add a note that this will also enable you to switch the itemtype of the last issue to another item type on receiving a new issue. Testing: - OK Adding subscription (syspref is OFF) -- System preference not active, only item type shows on form -- Item type is saved - Receiving issue and creating an item -- (3) In the item form, the selected itemtype doesn't preselect - OK Editing subscription -- Changes save correctly - Editing subscription (syspref is now ON) -- "Previous item type:" shows up now -- (4) I think the descrption might be misleading - as it's the final and not really the previous item type of the issue. Not sure I can explain that well. Maybe we could use a longer description? --- Item type for newest issue: / Item type for older issues: - OK receiving issues -- This is ok, as the last issue is set to my defined item type, but (3) is still showing as a problem here I have used numbers to mark some problematic bits. Please take a look!
I think we are not far - please fix so this long story can have a happy end :)
Created attachment 42923 [details] [review] BZ7677 [Follow-up]: New areas in subscriptions and new functions when receiving. - Various fixes.
This last patch fixes the issues you've raised when testing, Katrin.
Created attachment 42924 [details] [review] BZ7677 [Follow-up]: New areas in subscriptions and new functions when receiving. - Various fixes.
tried on a sandbox : <h1>Something went wrong !</h1>Applying: BZ7677: New areas in subscriptions and new functions when receiving. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging serials/subscription-add.pl CONFLICT (content): Merge conflict in serials/subscription-add.pl Auto-merging serials/serials-edit.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-add.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/serials/serials-edit.tt Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/kohastructure.sql CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql Auto-merging C4/Serials.pm Failed to merge in the changes. Patch failed at 0001 BZ7677: New areas in subscriptions and new functions when receiving.
Created attachment 45938 [details] [review] BZ7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767
Created attachment 45939 [details] [review] BZ7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767
Created attachment 45940 [details] [review] Bug 7677: UT: add unit tests for GetPreviousSerialid Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767
Created attachment 45941 [details] [review] Bug 7677: FIX QA issues FAIL C4/Serials.pm FAIL forbidden patterns forbidden pattern: trailing space char (line 1501) forbidden pattern: trailing space char (line 921) FAIL serials/serials-edit.pl FAIL valid "my" variable $dbh masks earlier declaration in same scope Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767
Created attachment 45942 [details] [review] Bug 7677: Fix tests They don't pass since bug 12098 has been pushed. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767
Created attachment 45943 [details] [review] BZ7677 [Follow-up]: New areas in subscriptions and new functions when receiving. - Various fixes.
Rebased. And moved SQL update from updatedatabase to atomic update.
Created attachment 46216 [details] [review] Bug 7677 [Follow-up]: New areas in subscriptions and new functions when receiving. - Various fixes. Test plan: Once the makePreviousSerialAvailable syspref is enabled, receive a serial, and then another, then check that: - the first received itemtype has been set to the "previous item type" value (set in the subscription). - the first received has been made available. - the last received serial itemtype has been set to the "item type" value (set in the subscription). - 995$l is automatically prefilled. Configure the serialsFieldsToAV syspref. When creating or editing a subscription, check that: - the domain and/or origin and/or support fields are correctly displaying the authorized values configured in the syspref. Signed-off-by: Chris <chris@bigballofwax.co.nz>
Created attachment 47475 [details] [review] Bug 7677 [QA Followup] - Replace GetItemnumberFromSerialId with use of Koha::Serial::Item(s) GetItemnumberFromSerialId has no unit tests. It would be better to start using our object system here.
Just starting with this - the QA tools have some complaints. Could you please take a look? FAIL Koha/Serial/Item.pm OK critic OK forbidden patterns OK pod OK spelling 0 FAIL valid Name "Tie::Hash::FIELDS" used only once: possible typo Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo FAIL Koha/Serial/Items.pm OK critic OK forbidden patterns OK pod OK spelling 0 FAIL valid Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo Name "Tie::Hash::FIELDS" used only once: possible typo
(In reply to Katrin Fischer from comment #79) > FAIL valid > Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo > Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo > Name "Tie::Hash::FIELDS" used only once: possible typo See bug 16104.
The subscription form behaviour looks good, although it would be nice if the new settings appeared on the subscription detail page. Location and Call number are also missing, so this could be handled separately. But I have a problem on receiving serial items with and without this patch that stops me from testing this further - see bug 16118.
Ah some note: I wonder about the change to the not for loan status that is built in here. Wouldn't a 'not for loan' itemtype have the same effect? The only difference I can think of is that the nfl in the item gives you the option of having a custom nfl status text.
Working on this!
I am experiencing bug 16118 again - so can't continue testing until I have found the issue for this. Any help appreciated. Meanwhile, can you check my written up test plan for this is correct? Bug 7677 - New area in subscriptions and new function when receiving This patch: - It's now possible to select a default item type in the subscription, similar to the existing default location and call number. - A new system preference makePreviousSerialAvailable is introcuded. This has 2 effects: - It will automatically switch the not for loan status of the last issue to 0 (available) when the next one is received. - It will allow you to pick an item type in the subscription, that the older issue is set to, when the next one is received. To test: - Create a subscription, where you create items on receive. - Leave preference off and test the new default item type field. - Verify new items are generated with the configured item type on receive. - Activate preference. - Receive issue, set not for loan != available/empty - Receive another issue. - Verify that the item is now available (not for loan = 0). - Add another an item type for older issues. - Receive another 2 issues, verify that the itemtype switch happens correctly.
I am sorry, this fails when I try to receive a serial issue: Can't use an undefined value as an ARRAY reference at /home/katrin/kohaclone/serials/serials-collection.pl line 131. And after running the Schema update script: DBIx::Class::Schema::resultset(): resultset() expects a source name at /home/katrin/kohaclone/Koha/Objects.pm line 235 Please take a look!
I tried running misc/devel/update_dbix_class_files.pl, but I still get the error message on trying to save the received issue.
Created attachment 52010 [details] [review] Bug 7677 [QA Followup] - Replace GetItemnumberFromSerialId with use of Koha::Serial::Item(s) - use _type instead of type (see https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15446 )
I've run the database update and created the schema files, but sadly, I am still seeing an error: DBIx::Class::ResultSource::unique_constraint_columns(): Unknown unique constraint primary on 'serialitems' at /home/katrin/kohaclone/Koha/Objects.pm line 83 It happens when I try to receive 2 serial issues at once. To reproduce this: - Search for a subscription - Go to it's serial collection page - Create a next issue - Try to receive both the now late issue and the expected one at once
We are so close on this... please don't give up!
Created attachment 54876 [details] [review] Bug 7677: QA followup Fix 'unique contraint primary' error when receiving serials
Hi Matthias, in testing things looked great, but now the tests are failing for me, hoping it's easy to fix: perl t/db_dependent/Serials_2.t: Can't call method "lastvalue1" on an undefined value at /home/katrin/kohaclone/C4/Serials.pm line 1575. # Looks like your test exited with 2 just after 46.
Created attachment 55000 [details] [review] Bug 7677: QA followup - Fix tests
Matthias, I am sorry, but there is still something odd about the test: rollback ineffective with AutoCommit enabled at t/db_dependent/Serials_2.t line 257.
(In reply to Katrin Fischer from comment #93) > Matthias, I am sorry, but there is still something odd about the test: > rollback ineffective with AutoCommit enabled at t/db_dependent/Serials_2.t > line 257. Caused by a wrong fix conflict on "Bug 7677: UT: add unit tests for GetPreviousSerialid", the rollback should not be added by this patch (was not when I submitted it).
I am not sure I understand how to fix - can you please provide a new patch for the last patch? I think that's the one adding it.
Comment on attachment 45940 [details] [review] Bug 7677: UT: add unit tests for GetPreviousSerialid Review of attachment 45940 [details] [review]: ----------------------------------------------------------------- ::: t/db_dependent/Serials_2.t @@ +255,5 @@ > +is( GetPreviousSerialid( $subscriptionid, 1 ), $serialid2, "get previous serialid with 1" ); > +is( GetPreviousSerialid( $subscriptionid, 2 ), $serialid1, "get previous serialid with 2" ); > +is( GetPreviousSerialid( $subscriptionid, 3 ), undef, "get previous serialid with 3, does not exist" ); > + > +$dbh->rollback; This rollback should not have been added by this patch (added by a wrong merge conflict).
Comment on attachment 54876 [details] [review] Bug 7677: QA followup This patch shouldn't have been obsoleted
Created attachment 55323 [details] [review] Bug 7677: UT: Remove $dbh->rollback
Created attachment 55466 [details] [review] [PASSED QA] BZ7677: New areas in subscriptions and new functions when receiving. When enabling the makePreviousSerialAvailable syspref, the previously received serial's itemtype is set as defined in the subscription. (Please note that the item-level_itypes syspref must be set to specific item.) It is also made available. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767 Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 55467 [details] [review] [PASSED QA] Bug 7677: UT: add unit tests for GetPreviousSerialid Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767 Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 55468 [details] [review] [PASSED QA] Bug 7677: FIX QA issues FAIL C4/Serials.pm FAIL forbidden patterns forbidden pattern: trailing space char (line 1501) forbidden pattern: trailing space char (line 921) FAIL serials/serials-edit.pl FAIL valid "my" variable $dbh masks earlier declaration in same scope Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767 Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 55469 [details] [review] [PASSED QA] Bug 7677: Fix tests They don't pass since bug 12098 has been pushed. Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com> http://bugs.koha-community.org/show_bug.cgi?id=7767 Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 55470 [details] [review] [PASSED QA] Bug 7677 [Follow-up]: New areas in subscriptions and new functions when receiving. - Various fixes. Test plan: Once the makePreviousSerialAvailable syspref is enabled, receive a serial, and then another, then check that: - the first received itemtype has been set to the "previous item type" value (set in the subscription). - the first received has been made available. - the last received serial itemtype has been set to the "item type" value (set in the subscription). - 995$l is automatically prefilled. Configure the serialsFieldsToAV syspref. When creating or editing a subscription, check that: - the domain and/or origin and/or support fields are correctly displaying the authorized values configured in the syspref. Signed-off-by: Chris <chris@bigballofwax.co.nz> Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 55471 [details] [review] [PASSED QA] Bug 7677 [QA Followup] - Replace GetItemnumberFromSerialId with use of Koha::Serial::Item(s) GetItemnumberFromSerialId has no unit tests. It would be better to start using our object system here. Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 55472 [details] [review] [PASSED QA] Bug 7677 [QA Followup] - Replace GetItemnumberFromSerialId with use of Koha::Serial::Item(s) - use _type instead of type (see https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15446 ) Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 55473 [details] [review] [PASSED QA] Bug 7677: QA followup Fix 'unique contraint primary' error when receiving serials Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 55474 [details] [review] [PASSED QA] Bug 7677: QA followup - Fix tests Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Created attachment 55475 [details] [review] [PASSED QA] Bug 7677: UT: Remove $dbh->rollback Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Long story... happy ending? :)
Pushed to master for 16.11, thanks Matthius, Jonathan, Julian!
locations_loop=>$locations_loop, is passed twice in serials/subscription-add.pl