Bug 7412 - Pre-filling items in cataloging
Summary: Pre-filling items in cataloging
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: 3.10
Hardware: All All
: P3 enhancement (vote)
Assignee: Matthias Meusburger
QA Contact: Marcel de Rooy
URL:
Keywords:
: 3480 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-01-06 13:57 UTC by Matthias Meusburger
Modified: 2014-05-26 21:04 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
proposed patch (10.76 KB, patch)
2012-01-06 13:59 UTC, Matthias Meusburger
Details | Diff | Splinter Review
proposed patch (10.32 KB, patch)
2012-01-09 15:25 UTC, Matthias Meusburger
Details | Diff | Splinter Review
proposed patch (11.32 KB, patch)
2012-02-16 13:29 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 7412: Pre-filling items in cataloguing (11.86 KB, patch)
2012-03-30 15:08 UTC, Matthias Meusburger
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7412: Pre-filling items in cataloguing (11.44 KB, patch)
2012-06-10 11:16 UTC, Katrin Fischer
Details | Diff | Splinter Review
Rebased patch (11.61 KB, patch)
2012-08-01 14:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7412: Pre-filling items in cataloguing (10.76 KB, patch)
2012-08-21 15:15 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 7412: Follow up: Update preference to sentence (1.62 KB, patch)
2012-09-25 15:51 UTC, Nicole C. Engard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7412: Follow up: Update preference to sentence (1.72 KB, patch)
2012-10-02 14:14 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Meusburger 2012-01-06 13:57:33 UTC
Pre-filling of items: remember what was in the previous item filled, to be able to create multiple items quickly.
Comment 1 Matthias Meusburger 2012-01-06 13:59:36 UTC Comment hidden (obsolete)
Comment 2 Matthias Meusburger 2012-01-06 14:01:26 UTC
To test: 
Enable PrefillItem syspref
Create a new item
Create another item.
Expected result: fields are prefilled with previous values. (except for the following fields: f k u v x z)
Comment 3 Nicole C. Engard 2012-01-06 14:34:47 UTC
This does to appear to apply to master.


nengard@debian:~/kohaclone$ git bz apply 7412
Bug 7412 - Pre-filling items in cataloguing

proposed patch
Apply? [yn] y

Applying: Bug 7412: Pre-filling items in cataloguing
/home/nengard/kohaclone/.git/rebase-apply/patch:85: trailing whitespace.
    # force the use of "add and duplicate" feature, so the form will be filled with 
/home/nengard/kohaclone/.git/rebase-apply/patch:176: trailing whitespace.
$itemrecord = $cookieitemrecord if ($prefillitem and not $justaddeditem and $op ne "edititem"); 
error: patch failed: kohaversion.pl:16
error: kohaversion.pl: patch does not apply
Patch failed at 0001 Bug 7412: Pre-filling items in cataloguing
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Patch left in /tmp/proposed-patch-NyUHpB.patch
nengard@debian:~/kohaclone$
Comment 4 Katrin Fischer 2012-01-06 14:49:44 UTC
Hi Nicole, I was looking at this too - applies to latest master cleanly for me using git bz.
Comment 5 Nicole C. Engard 2012-01-06 14:58:31 UTC
I just did a fetch on master and tried again and still it's not applying - are you on the latest master as of this minute?


nengard@debian:~/kohaclone$ git bz apply 7412
Bug 7412 - Pre-filling items in cataloguing

proposed patch
Apply? [yn] y

Applying: Bug 7412: Pre-filling items in cataloguing
/home/nengard/kohaclone/.git/rebase-apply/patch:85: trailing whitespace.
    # force the use of "add and duplicate" feature, so the form will be filled with 
/home/nengard/kohaclone/.git/rebase-apply/patch:176: trailing whitespace.
$itemrecord = $cookieitemrecord if ($prefillitem and not $justaddeditem and $op ne "edititem"); 
error: patch failed: kohaversion.pl:16
error: kohaversion.pl: patch does not apply
Patch failed at 0001 Bug 7412: Pre-filling items in cataloguing
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Patch left in /tmp/proposed-patch-jUts5f.patch
nengard@debian:~/kohaclone$
Comment 6 Katrin Fischer 2012-01-06 15:02:12 UTC
Nicole, you are right. And the problem is kohaversion (that should not be in this patch).
Comment 7 Matthias Meusburger 2012-01-09 15:25:58 UTC Comment hidden (obsolete)
Comment 8 Matthias Meusburger 2012-01-09 15:27:04 UTC
My bad, sorry. I replaced the proposed patch with an updated version, which does not contain kohaversion.pl modification.
Comment 9 Katrin Fischer 2012-01-15 19:30:57 UTC
I fixed the conflicts in systempreferences.sql and updatedatabase.pl and checked the code.

The patch uses subfield codes instead of the Koha to MARC mappings:

+sub removeFieldsForPrefill {
+    #FIXME: this is not generic enough. We could define fields to remove in a syspref.
+    my $item = shift;
+    # Getting item tag
+    my ($tag, $subtag) = GetMarcFromKohaField("items.barcode", '');
+    if ($tag) {
+        my $field = $item->field($tag);
+        if ($field) {
+            $field->delete_subfield(code => 'f');
+            $field->delete_subfield(code => 'k');
+            $field->delete_subfield(code => 'u');
+            $field->delete_subfield(code => 'v');
+            $field->delete_subfield(code => 'x');
+            $field->delete_subfield(code => 'z');
+        }
+    }
+    return $item;
+
+}

I checked and the item subfields between UNIMARC and MARC21 are quite different. Also it should be documented which fields are pre-filled and which not.
Comment 10 Matthias Meusburger 2012-02-03 16:17:06 UTC
Using koha to marc mappings is indeed a good idea. However, considering default unimarc mappings, not all of the removed fields are mapped.

Mapped:

$field->delete_subfield(code => 'f'); # barcode
$field->delete_subfield(code => 'k'); # itemcallnumber
$field->delete_subfield(code => 'u'); # itemnotes
$field->delete_subfield(code => 'v'); # enumchron

Not mapped:

$field->delete_subfield(code => 'x'); # ?
$field->delete_subfield(code => 'z'); # ?


What should we do with those two ?



Documenting which fields are prevented from being prefilled is a good idea too... Where should it be done ? In the page itself ? In the help page ?
Comment 11 Katrin Fischer 2012-02-03 16:19:19 UTC
I think for fields not mapped we will need a UNIMARC/MARC21/NORMARC switch in the code. What is the content of those fields in UNIMARC?

I think the help page is a good idea, but also the commit message is a good starting point.
Comment 12 Matthias Meusburger 2012-02-16 13:28:08 UTC
At first, I thought of keeping only the mapped subfields for subfields that should not be prefilled and get rid of the others. 

By that would not have been generic enough anyway.

Deciding which subfields should be prefilled and which should not has to be the librarian's call.

So I came up with another solution (based on biblibre MT6413, for reference): a syspref has been added, in which we can give a list of the subfields that should not be prefilled.

I also added informations in the commit message.

Proposed patch has been updated.
Comment 13 Matthias Meusburger 2012-02-16 13:29:26 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2012-02-26 16:53:26 UTC
Hi Matthias, 

what does biblibre MT6413 refer to? Is there a bug in bugzilla for that?

The patch does no longer apply:
CONFLICT (content): Merge conflict in cataloguing/additem.pl
Auto-merging installer/data/mysql/sysprefs.sql
CONFLICT (content): Merge conflict in installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Failed to merge in the changes.

updatedatabase.pl has a bad version number, which makes testing a bit harder. Could you fix it to be XXX and resubmit? Thank you!
Comment 15 Katrin Fischer 2012-02-26 16:54:42 UTC
Also a thought - perhaps make it a 'positive' system preference. So people don't enter the fields that should not be prefilled, but those that should be. I think would be a little less confusing.
Comment 16 Matthias Meusburger 2012-03-02 13:50:19 UTC
I'm not sure about that. If you enable a feature, you naturally expect that feature to be working. Having a 'positive' syspref would make the feature disabled even when enabled, which is more confusing, to my opinion.

Isn't it better to have a feature that simply works out of box when enabled, and to which you can perform fine-tuning if you want to, rather than having a feature that doesn't work unless you explicitely configure it ?

What do you think ?
Comment 17 Katrin Fischer 2012-03-02 13:57:40 UTC
Hi Matthias :) 

It's already confusing me :) 

I think it's differnt to how Koha works now and we normally tend to not change Koha's behaviour on update, but make it possible to activate new functionality later. I think for some fields, prefilling might not make sense. 

The other thing was, that it seems a bit backward for me to think "I have to add the fields that I don't want to be prefilled". So if I only want itemtype, callnumber, branchcodes and location (for example) I will have to check the frameworks and find all other fields where I perhaps don't want it to happen. 
If it was a positive pref I could add the fields I am using to be prefilled. Shorter list (probably) and easier to figure out. But perhaps it's safe to assume you want to copy all fields you entered? 

I am not really sure here - It would be great to get some more opinions on this.
Comment 18 Matthias Meusburger 2012-03-20 13:24:33 UTC
Anybody have an opinion on this ?
Comment 19 Matthias Meusburger 2012-03-30 15:08:14 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2012-06-10 10:21:10 UTC
Comment on attachment 7685 [details] [review]
proposed patch

>From f80b4d1115262d2cdace9eb7494da7c22116ebf0 Mon Sep 17 00:00:00 2001
>From: Matthias Meusburger <matthias.meusburger@biblibre.com>
>Date: Fri, 6 Jan 2012 14:55:45 +0100
>Subject: [PATCH] Bug 7412: Pre-filling items in cataloguing
>
>Pre-filling of items: remember what was in the previous item filled, to be able to create multiple items quickly.
>
>By default, all subfields are prefilled when the PrefillItem syspref is on.
>If you want some subfields not to be prefilled, you can use the SubfieldsToDiscardWhenPrefill syspref.
>With a value of "f u v", the $f, $u and $v won't be prefilled, for example.
>---
> cataloguing/additem.pl                             |  123 +++++++++++++++++---
> installer/data/mysql/sysprefs.sql                  |    2 +
> installer/data/mysql/updatedatabase.pl             |   16 +++-
> .../en/modules/admin/preferences/cataloguing.pref  |   10 ++
> 4 files changed, 134 insertions(+), 17 deletions(-)
>
>diff --git a/cataloguing/additem.pl b/cataloguing/additem.pl
>index a734d0f..b10c986 100755
>--- a/cataloguing/additem.pl
>+++ b/cataloguing/additem.pl
>@@ -32,6 +32,10 @@ use C4::Branch; # XXX subfield_is_koha_internal_p
> use C4::ClassSource;
> use C4::Dates;
> use List::MoreUtils qw/any/;
>+use Storable qw(thaw freeze);
>+use URI::Escape;
>+
>+
> 
> use MARC::File::XML;
> use URI::Escape;
>@@ -275,6 +279,29 @@ sub generate_subfield_form {
>         return \%subfield_data;
> }
> 
>+# Removes some subfields (defined in the SubfieldsToDiscardWhenPrefill syspref) when prefilling items
>+sub removeFieldsForPrefill {
>+
>+    my $item = shift;
>+
>+    # Getting item tag
>+    my ($tag, $subtag) = GetMarcFromKohaField("items.barcode", '');
>+
>+    # Getting list of subfields to remove
>+    my $subfieldsToDiscardWhenPrefill = C4::Context->preference('SubfieldsToDiscardWhenPrefill');
>+
>+    # Removing subfields
>+    if ($tag && $subfieldsToDiscardWhenPrefill) {
>+        my $field = $item->field($tag);
>+        my @subfieldsToDiscard= split(/ /,$subfieldsToDiscardWhenPrefill);
>+        foreach my $subfieldsDiscard(@subfieldsToDiscard) {
>+            $field->delete_subfield(code => $subfieldsDiscard);
>+        }
>+    }
>+
>+    return $item;
>+
>+}
> 
> my $input        = new CGI;
> my $error        = $input->param('error');
>@@ -315,9 +342,26 @@ my $oldrecord = TransformMarcToKoha($dbh,$record);
> my $itemrecord;
> my $nextop="additem";
> my @errors; # store errors found while checking data BEFORE saving item.
>+
>+# Getting last created item cookie
>+my $prefillitem = C4::Context->preference('PrefillItem');
>+my $justaddeditem;
>+my $cookieitemrecord;
>+if ($prefillitem) {
>+    my $lastitemcookie = $input->cookie('LastCreatedItem');
>+    if ($lastitemcookie) {
>+	$lastitemcookie = uri_unescape($lastitemcookie);
>+	if ( thaw($lastitemcookie) ) {
>+	    $cookieitemrecord = thaw($lastitemcookie) ;
>+	    $cookieitemrecord = removeFieldsForPrefill($cookieitemrecord);
>+	}
>+    }
>+}
>+
> #-------------------------------------------------------------------------------
> if ($op eq "additem") {
>-#-------------------------------------------------------------------------------
>+
>+    #-------------------------------------------------------------------------------
>     # rebuild
>     my @tags      = $input->param('tag');
>     my @subfields = $input->param('subfield');
>@@ -334,26 +378,69 @@ if ($op eq "additem") {
>     my $add_multiple_copies_submit = $input->param('add_multiple_copies_submit');
>     my $number_of_copies           = $input->param('number_of_copies');
> 
>+    # This is a bit tricky : if there is a cookie for the last created item and
>+    # we just added an item, the cookie value is not correct yet (it will be updated
>+    # next page). To prevent the form from being filled with outdated values, we
>+    # force the use of "add and duplicate" feature, so the form will be filled with 
>+    # correct values.
>+    $add_duplicate_submit = 1 if ($prefillitem);
>+    $justaddeditem = 1;
>+
>+    # if autoBarcode is set to 'incremental', calculate barcode...
>+    # NOTE: This code is subject to change in 3.2 with the implemenation of ajax based autobarcode code
>+    # NOTE: 'incremental' is the ONLY autoBarcode option available to those not using javascript
>+    if ( C4::Context->preference('autoBarcode') eq 'incremental' ) {
>+        my ( $tagfield, $tagsubfield ) = &GetMarcFromKohaField( "items.barcode", $frameworkcode );
>+        unless ( $record->field($tagfield)->subfield($tagsubfield) ) {
>+            my $sth_barcode = $dbh->prepare("select max(abs(barcode)) from items");
>+            $sth_barcode->execute;
>+            my ($newbarcode) = $sth_barcode->fetchrow;
>+            $newbarcode++;
>+
>+            # OK, we have the new barcode, now create the entry in MARC record
>+            my $fieldItem = $record->field($tagfield);
>+            $record->delete_field($fieldItem);
>+            $fieldItem->add_subfields( $tagsubfield => $newbarcode );
>+            $record->insert_fields_ordered($fieldItem);
>+        }
>+    }
>+
>+
>     if (C4::Context->preference('autoBarcode') eq 'incremental') {
>         $record = _increment_barcode($record, $frameworkcode);
>     }
> 
>-    my $addedolditem = TransformMarcToKoha($dbh,$record);
>+    my $addedolditem = TransformMarcToKoha( $dbh, $record );
> 
>     # If we have to add or add & duplicate, we add the item
>-    if ($add_submit || $add_duplicate_submit) {
>-	# check for item barcode # being unique
>-	my $exist_itemnumber = get_item_from_barcode($addedolditem->{'barcode'});
>-	push @errors,"barcode_not_unique" if($exist_itemnumber);
>-	# if barcode exists, don't create, but report The problem.
>-    unless ($exist_itemnumber) {
>-	    my ($oldbiblionumber,$oldbibnum,$oldbibitemnum) = AddItemFromMarc($record,$biblionumber);
>-        set_item_default_location($oldbibitemnum);
>-    }
>-	$nextop = "additem";
>-	if ($exist_itemnumber) {
>-	    $itemrecord = $record;
>-	}
>+    if ( $add_submit || $add_duplicate_submit ) {
>+
>+        # check for item barcode # being unique
>+        my $exist_itemnumber = get_item_from_barcode( $addedolditem->{'barcode'} );
>+        push @errors, "barcode_not_unique" if ($exist_itemnumber);
>+
>+        # if barcode exists, don't create, but report The problem.
>+        unless ($exist_itemnumber) {
>+            my ( $oldbiblionumber, $oldbibnum, $oldbibitemnum ) = AddItemFromMarc( $record, $biblionumber );
>+            set_item_default_location($oldbibitemnum);
>+
>+	  # Pushing the last created item cookie back
>+	  if ($prefillitem && defined $record) {
>+                my $itemcookie = $input->cookie(
>+                    -name => 'LastCreatedItem',
>+                    # We uri_escape the whole freezed structure so we're sure we won't have any encoding problems
>+                    -value   => uri_escape( freeze( $record ) ),
>+                    -expires => ''
>+                );
>+
>+		$cookie = ( $cookie, $itemcookie );
>+	    }
>+
>+        }
>+        $nextop = "additem";
>+        if ($exist_itemnumber) {
>+            $itemrecord = $record;
>+        }
>     }
> 
>     # If we have to add & duplicate
>@@ -370,6 +457,7 @@ if ($op eq "additem") {
>             $fieldItem->delete_subfields($tagsubfield);
>             $itemrecord->insert_fields_ordered($fieldItem);
>         }
>+	$itemrecord = removeFieldsForPrefill($itemrecord) if ($prefillitem);
>     }
> 
>     # If we have to add multiple copies
>@@ -698,6 +786,11 @@ if($itemrecord){
>             }
>     # and now we add fields that are empty
> 
>+# Using last created item if it exists
>+
>+$itemrecord = $cookieitemrecord if ($prefillitem and not $justaddeditem and $op ne "edititem"); 
>+
>+# We generate form, and fill with values if defined
> foreach my $tag ( keys %{$tagslib}){
>     foreach my $subtag (keys %{$tagslib->{$tag}}){
>         next if subfield_is_koha_internal_p($subtag);
>diff --git a/installer/data/mysql/sysprefs.sql b/installer/data/mysql/sysprefs.sql
>index 128c9b2..e9070a8 100755
>--- a/installer/data/mysql/sysprefs.sql
>+++ b/installer/data/mysql/sysprefs.sql
>@@ -328,4 +328,6 @@ INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES('
> INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES ('OpacKohaUrl','1',"Show 'Powered by Koha' text on OPAC footer.",NULL,NULL);
> INSERT INTO `systempreferences` (variable,value,explanation,options,type) VALUES('EasyAnalyticalRecords','0','If on, display in the catalogue screens tools to easily setup analytical record relationships','','YesNo');
> INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES('OpacShowRecentComments',0,'If ON a link to recent comments will appear in the OPAC masthead',NULL,'YesNo');
>+INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES('PrefillItem','0','When a new item is added, should it be prefilled with last created item values?','','YesNo');
>+INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES ('SubfieldsToDiscardWhenPrefill','','Define a list of subfields to discard when prefilling items (separated by space)','','Free');
> 
>diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl
>index 67708fa..014b96e 100755
>--- a/installer/data/mysql/updatedatabase.pl
>+++ b/installer/data/mysql/updatedatabase.pl
>@@ -4585,6 +4585,20 @@ if (C4::Context->preference("Version") < TransformToNum($DBversion)) {
>     SetVersion($DBversion);
> }
> 
>+$DBversion = "3.06.03.002";
>+if ( C4::Context->preference("Version") < TransformToNum($DBversion) ) {
>+    $dbh->do("
>+    INSERT IGNORE INTO `systempreferences` (variable,value,explanation,options,type) VALUES('PrefillItem','0','When a new item is added, should it be prefilled with last created item values?','','YesNo');
>+    ");
>+
>+    $dbh->do(
>+            "INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES ('SubfieldsToDiscardWhenPrefill','','Define a list of subfields to discard when prefilling items (separated by space)','','Free');"
>+    );
>+
>+    print "Upgrade to $DBversion done (Adding PrefillItem and SubfieldsToDiscardWhenPrefill sysprefs)\n";
>+    SetVersion($DBversion);
>+}
>+
> =head1 FUNCTIONS
> 
> =head2 DropAllForeignKeys($table)
>@@ -4592,8 +4606,6 @@ if (C4::Context->preference("Version") < TransformToNum($DBversion)) {
> Drop all foreign keys of the table $table
> 
> =cut
>-
>-
> sub DropAllForeignKeys {
>     my ($table) = @_;
>     # get the table description
>diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref
>index 10e1d13..885c73d 100644
>--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref
>+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/cataloguing.pref
>@@ -90,6 +90,16 @@ Cataloging:
>                   annual: generated in the form &lt;year&gt;-0001, &lt;year&gt;-0002.
>                   hbyymmincr: generated in the form &lt;branchcode&gt;yymm0001.
>                   "OFF": not generated automatically.
>+        -
>+            - When a new item is added, should it be prefilled with last created item values?
>+            - pref: PrefillItem
>+              choices:
>+                  yes: the new item is prefilled with last created item values
>+                  no: the new item is not prefilled with last created item values
>+        -
>+            - Define a list of subfields to discard when prefilling items (separated by space)
>+            - pref: SubfieldsToDiscardWhenPrefill
>+
>     Display:
>         -
>             - 'Separate multiple displayed authors, series or subjects with '
>-- 
>1.7.5.4
>
Comment 21 Katrin Fischer 2012-06-10 10:22:20 UTC
Oups.
Comment 22 Katrin Fischer 2012-06-10 11:15:41 UTC
Ok, here are my testing comments:

I like the new way better - a positive system preference to add all fields I want to have prefilled. I am wondering if we really need 2 new preferences for that. Perhaps turning the feature off could be done by leaving the field empty instead?

That said, turning the feature off/on works. 
Only fields I wanted to be copied to the next item get copied.
Adding items, duplicating items and adding multiple copies of an item seem to work nicely.
Adding items in serials seems not to be affected and worked ok for me.
Comment 23 Katrin Fischer 2012-06-10 11:16:12 UTC Comment hidden (obsolete)
Comment 24 Marcel de Rooy 2012-06-28 13:30:54 UTC
Just some questions before starting QA on this one:
Why do you need two new prefs?
At first glance through code, I see a lot of testing on barcode and read about tricky cookie handling: Could you please explain? If you do not want to copy barcode, you should just not put it in the prefill? How exactly do you use this new cookie?
Is the commit message outdated?
Since we also have default values in framework, we have Add & Duplicate, what exacly make this feature so needed?
Comment 25 Matthias Meusburger 2012-07-06 14:23:18 UTC
Why do you need two new prefs?

-> Maybe one would have been enough. On the other hand, with two sysprefs, you can enable/disable the feature and keep your settings.

At first glance through code, I see a lot of testing on barcode and read about tricky cookie handling: Could you please explain? If you do not want to copy barcode, you should just not put it in the prefill? 

-> I'm not sure I understand... The code about barcode is for barcode autoincrement management.

How exactly do you use this new cookie?

-> The cookie is used to store the values to prefill. The tricky part is only because, when you send a cookie, you don't get to read its value right away. You have to wait for a refresh. Is this clear or does it need more explanations?

Is the commit message outdated?

-> Why would you say that?

Since we also have default values in framework, we have Add & Duplicate, what exacly make this feature so needed?

-> This feature keeps track of what to prefill from one session to another (both in time and user: if the user disconnects and reconnects, the values are still prefilled. If a user disconnects and another user logs in, the values are still prefilled.). Add & Duplicate does not.
Comment 26 Marcel de Rooy 2012-07-18 14:40:41 UTC
I see that this report lost its signoff. Could you clarify, mtj?
IMO it is still at signed off and I will continue with this report soon.
Comment 27 Marcel de Rooy 2012-08-01 13:33:55 UTC
(In reply to comment #26)
> I see that this report lost its signoff. Could you clarify, mtj?
> IMO it is still at signed off and I will continue with this report soon.

No response: resetting status to Signed off.
Comment 28 Marcel de Rooy 2012-08-01 14:53:04 UTC
QA Comment:

General
One pref instead of two prefs would probably be better in terms of managing the number of prefs. But this may be just easier in use. (No blocker)

sub removeFieldsForPrefill
Somewhat theoretical, but if you would not use barcode and disconnected that field from 952, the function will not work. (No blocker for me)

Cookie
> This feature keeps track of what to prefill from one session to another (both in time and user: 
> if the user disconnects and reconnects, the values are still prefilled. 
> If a user disconnects and another user logs in, the values are still prefilled.).
You assume that the user runs under the same OS credentials. But if he logs out from the OS and another user logs in, the situation is different of course.
The line 439 is intriguing:
$cookie = ( $cookie, $itemcookie );
Does that work? (Apparently..) It probably should be = [ .. ]; The cookie parameter of CGI accepts an arrayref. Please correct. 

URI::Escape
Just a question, did not examine this further. But should you use uri_escape_utf8 instead of uri_escape?
Please test on chars in 128..255 and above, and report back.

Indentation: additem.pl lines 356 to 360; the same for segment 426-442 (followup..)

Barcode
You are adding this comment somewhere: This code is subject to change in 3.2
That sounds a little bit old now?
In the new code you add code for incrementing barcode. Note that additem already contains subroutine _increment_barcode. Why not use that? Followup?

Conclusion
In general. Looks good to me, but needs some further attention. 

Note for RM:
Paul, I am setting status to Passed QA for this one. It was quite long time in the queue. But it needs some clarification and followups. Matthias is close to you :)
Comment 29 Marcel de Rooy 2012-08-01 14:54:09 UTC Comment hidden (obsolete)
Comment 30 Paul Poulain 2012-08-01 15:57:36 UTC
(In reply to comment #28)
> Paul, I am setting status to Passed QA for this one. It was quite long time
> in the queue. But it needs some clarification and followups. Matthias is
> close to you :)

I think Matts is closer to you than to me, because he live near Strasbourg ;-)

Thx for the passed QA. There's not need to hurry to push it, Matts, pls provide follow-up to Marcel's comments
Comment 31 Matthias Meusburger 2012-08-21 15:15:47 UTC
Created attachment 11723 [details] [review]
Bug 7412: Pre-filling items in cataloguing

Pre-filling of items: remember what was in the previous item filled, to be able to create multiple items quickly.

By default, all subfields are prefilled when the PrefillItem syspref is on, unless you specify which fields
you want to prefill in the SubfieldsToUseWhenPrefill syspref.
With a value of "f u v", only the $f, $u and $v will be prefilled, for example.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Rebased and QAed.
Comment 32 Matthias Meusburger 2012-08-21 15:17:23 UTC
It probably should be = [ .. ]; The cookie parameter of CGI accepts an arrayref. Please correct. 

-> fixed

Just a question, did not examine this further. But should you use uri_escape_utf8 instead of uri_escape?
Please test on chars in 128..255 and above, and report back.

-> I've tested with the following chars:
É À œ Œ ñ … as well as with some cyrillic: Советский Союз
And it does behave correctly. But it also behave correctly with uri_escape_utf8. I changed to code so it uses uri_escape_utf8, which seems safer.

Indentation: additem.pl lines 356 to 360; the same for segment 426-442 (followup..)

-> fixed


You are adding this comment somewhere: This code is subject to change in 3.2
That sounds a little bit old now?
In the new code you add code for incrementing barcode. Note that additem already contains subroutine _increment_barcode. Why not use that? Followup?

-> fixed
Comment 33 Paul Poulain 2012-09-03 15:24:09 UTC
(In reply to comment #25)
> Why do you need two new prefs?
> 
> -> Maybe one would have been enough. On the other hand, with two sysprefs,
> you can enable/disable the feature and keep your settings.
Well, I agree with Marcel that one syspref would have been enough = if you want to disable the feature for a short time, just copy/paste it somewhere. However, I won't reject your patch for that, because it is 9 months old, it would not be fair.

> Since we also have default values in framework, we have Add & Duplicate,
> what exacly make this feature so needed?
> 
> -> This feature keeps track of what to prefill from one session to another
> (both in time and user: if the user disconnects and reconnects, the values
> are still prefilled. If a user disconnects and another user logs in, the
> values are still prefilled.). Add & Duplicate does not.

During my tests, I discovered that the cookie last only for the session. Is it one of my specific FF setup ?
The cookie is created using:
+                my $itemcookie = $input->cookie(
+                    -name => 'LastCreatedItem',
+                    # We uri_escape the whole freezed structure so we're sure we won't have any encoding problems
+                    -value   => uri_escape_utf8( freeze( $record ) ),
+                    -expires => ''
+                );
isn't it meaning "session" ?
However, I'm OK with a duration of a browser session (I even think I prefer that to a forever lasting prefill), so ...

... patch pushed
Comment 34 Nicole C. Engard 2012-09-25 15:51:52 UTC Comment hidden (obsolete)
Comment 35 Nicole C. Engard 2012-09-25 15:52:20 UTC
I have submitted a follow up to fix the preference to be a sentence like other preferences.

Nicole
Comment 36 Nicole C. Engard 2012-09-25 15:52:34 UTC
Follow up needs sign off.
Comment 37 Owen Leonard 2012-10-02 14:14:01 UTC
Created attachment 12640 [details] [review]
[SIGNED-OFF] Bug 7412: Follow up: Update preference to sentence

This patch updates the preference question to a sentence
to follow the standard set with previous preferences.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 38 Nicole C. Engard 2012-10-09 10:39:20 UTC
Changing from enhancement to 'normal' because this is a patch to an enhancement that was put in 3.10.

Nicole
Comment 39 Paul Poulain 2012-10-12 10:28:49 UTC
follow-up string (syspref) only, passed QA
Comment 40 Paul Poulain 2012-10-12 10:29:26 UTC
follow-up pushed
(back to ENH to have this feature in the release notes)
Comment 41 Katrin Fischer 2013-04-13 21:56:11 UTC
*** Bug 3480 has been marked as a duplicate of this bug. ***