Bug 7369

Created attachment 37352 [details]
duplicating fields it shouldn't

2 things.

1 - we should put the edit, duplicate, delete options in a pull down like on other pages

2 - much bigger - the hidden fields are duplicating like number of checkouts and last checkout (see attached)


Other QA tests passed:

Processing files before patches
|========================>| 2 / 2 (100.00%)
Processing files after patches
|========================>| 2 / 2 (100.00%)

 OK	cataloguing/additem.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  pod
   OK	  valid

 OK	koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
   OK	  forbidden patterns
   OK	  tt_valid
   OK	  valid_template

Leaving as ASSIGNED, getting a valid_template error with qa tools

passes qa-test-tools and uses dropdown for item actions

Comment on attachment 37496 [details] [review]
Bug 7369 - duplicate item

Review of attachment 37496 [details] [review]:
-----------------------------------------------------------------

::: cataloguing/additem.pl
@@ +581,5 @@
> +        $fieldItem->delete_subfields($tagsubfield);
> +        $itemrecord->insert_fields_ordered($fieldItem);
> +    }
> +
> +    #check for hidden subfield and remove them for the duplicated item

Why? Duplicate means duplicate everything (except itemnumber and barcode), right?

BTW, I love the button dropdown on the Edit/Duplicate/Delete. Such a pretty improvement!

Now for the question: Why is the delete an else on whether there are analytics or not?

Moving to 'In Discussion' to reflect that I have asked some questions before signing off.

> Why? Duplicate means duplicate everything (except itemnumber and barcode),
> right?

The hidden fields include issues and on loan etc., I think duplicating fields you can't manually touch or edit is not helpful.

Essentially it's an extension of Add+duplicate, giving you the option to add additional copies after initial cataloging.

(In reply to M. Tompsett from comment #8)
> 
> Now for the question: Why is the delete an else on whether there are
> analytics or not?
> 
I honestly wondered that myself when editing but not knowing the answer I assumed whoever did that had intention and was better to leave as is than to bug fix someone's feature

(In reply to Nick Clemens from comment #10)
> (In reply to M. Tompsett from comment #8)
> > 
> > Now for the question: Why is the delete an else on whether there are
> > analytics or not?
> > 

Ah, okay, when adding an analytical record (say a separate record for an article in a journal) you attach it to a specific item on the parent record.  This check ensures that if the record has an analytical/'child' record you don't delete it

Can u please rebase it ASAP?

(In reply to Tomás Cohen Arazi from comment #20)
> Can u please rebase it ASAP?

The rebase is required because of bug 13943.
Also noticed that ampersand parameters were added in poor HTML form.

Comment on attachment 44523 [details] [review]
Bug 7369 - Use dropdown buttons for item actions

Review of attachment 44523 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/additem.tt
@@ +188,2 @@
>                            [% ELSE %]
> +                              <li><a class="delete" href="/cgi-bin/koha/cataloguing/additem.pl?op=delitem&amp;biblionumber=[% biblionumber %]&amp;itemnumber=[% item_loo.itemnumber %]&amp;searchid=[% searchid %]" onclick="confirm_deletion([% biblionumber %],[% item_loo.itemnumber %]); return false;">Delete</a></li>

This is the only point I'd question in the rebase: confirm_deletion has parameters here, but not in master.

Created attachment 44524 [details] [review]
Bug 7369 - duplicate item and add dropdown buttons for item actions

This patch adds a link to duplicate existing items on the edit items screen

To test:
1 - Apply patch
2 - navigate to the edit items screen for a record with existing items
3 - click the duplicate link and ensure item info but not barcode are carried over
4 - save the item and ensure it is added correctly
5 - check that all previous functionality on page works as before

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Thanks Mark!  Rebased and squashed

> This is the only point I'd question in the rebase: confirm_deletion has
> parameters here, but not in master.

Patch pushed to master.

Thanks Nick!