Bug 21774 - Cloned item subfields disappear when editing an item
Summary: Cloned item subfields disappear when editing an item
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords: rel_18_11_candidate
Depends on: 10306
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-06 18:01 UTC by jesus
Modified: 2018-12-19 10:15 UTC (History)
10 users (show)

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


Attachments
Screenshots of steps to reproduce (349.98 KB, application/pdf)
2018-11-08 09:41 UTC, jesus
Details
Bug 21774: Cloned item subfields disappear when editing an item (3.06 KB, patch)
2018-11-08 13:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21774: Cloned item subfields disappear when editing an item (3.04 KB, patch)
2018-11-08 14:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21774: Cloned item subfields disappear when editing an item (3.08 KB, patch)
2018-11-16 09:47 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 21774: Cloned item subfields disappear when editing an item (3.22 KB, patch)
2018-11-20 13:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description jesus 2018-11-06 18:01:38 UTC
When editing an item record that has repetable subfields, these subfields dissapear on the editing screen in those records that have more than one value.

- On the cataloging screen (basic screen), create a new record
- Go to a field with repetable subfields and associated with authorised values CCODE
- Select a value for the subfield
 952$8)
 - Clone the subfield
 - select another value for the new subfield
 - Save the record
 - Edit the record to modify => the clone subfields have disappeared.

Seems to be related to bug 17818

Verified happening on 17.11.04 and 18.05.05-1
Comment 1 Marcel de Rooy 2018-11-07 13:53:28 UTC
I cannot reproduce your problem. I tried on a 505$g with some authval and on 505$8 with CCODE but it works as expected.
Tested on master.
Could you confirm that it works on master for you?
Comment 2 jesus 2018-11-08 09:41:22 UTC
Created attachment 82060 [details]
Screenshots of steps to reproduce
Comment 3 jesus 2018-11-08 09:42:14 UTC
I can confirm the bug happening on Debian 9.5 stock packages 17.11.04 and 18.05.05-1
Comment 4 Marcel de Rooy 2018-11-08 11:28:13 UTC
(In reply to jesus from comment #2)
> Created attachment 82060 [details]
> Screenshots of steps to reproduce

OK Jesus. This is helpful information. Because I did not understand that it was about items. I thought it was about cloning in the cataloging editor. I did not read your first comment well enough, realizing that now.

Will add another comment soon.
Comment 5 Marcel de Rooy 2018-11-08 12:20:58 UTC
Bug 10306 changed behavior here in 18.05. But it was not backported to 17.11.
So I do not understand why the problem comes up in 17.11.
I tried with a repeatable ccode in 16.11 and it works. (But I am not sure if we should recommend it btw.)

Please try the following adjustment in your 18.05 in C4/Items.pm, sub Item2Marc:

Replace
 my $itemmarc = C4::Biblio::TransformKohaToMarc(
        $mungeditem, { no_split => 1},
    );
by 
my $itemmarc = C4::Biblio::TransformKohaToMarc($mungeditem);

In other words: remove the no_split parameter. Since this actually is old behavior (recommended or not), I will submit a patch for it too..
Comment 6 Marcel de Rooy 2018-11-08 13:53:42 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2018-11-08 14:05:47 UTC
Created attachment 82080 [details] [review]
Bug 21774: Cloned item subfields disappear when editing an item

Bug 10306 changed behavior on cloning item subfields by no longer splitting
constructions like 'A | B' in item fields like ccode.

If it is really recommended to clone item subfields, I am not so sure
about. But this patch at least restores the possibility to do so while
we discuss if we should ;)

Test plan:
[1] Run Items.t
[2] Make an item subfield repeatable in framework. And test edit items.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 jesus 2018-11-08 15:54:42 UTC
> Replace
>  my $itemmarc = C4::Biblio::TransformKohaToMarc(
>         $mungeditem, { no_split => 1},
>     );
> by 
> my $itemmarc = C4::Biblio::TransformKohaToMarc($mungeditem);
> 

Making this change solved the bug for us in 18.05.05-1

Thx.
Comment 9 M. Tompsett 2018-11-08 16:30:44 UTC
Comment on attachment 82080 [details] [review]
Bug 21774: Cloned item subfields disappear when editing an item

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

::: C4/Items.pm
@@ +1342,4 @@
>          } keys %{ $itemrecord } 
>      };
>      my $framework = C4::Biblio::GetFrameworkCode( $biblionumber );
> +    my $itemmarc = C4::Biblio::TransformKohaToMarc( $mungeditem ); # Bug 21774: no_split parameter removed to allow cloned subfields

I'm not sure no_split logic was the correct solution, so I have no issue with this removal. However, I think they may be tried to use that to split or not split based on the frameworkcode, because the field might not be repeatable, and so it is a little larger than this change alone. Just my thoughts on this. I could be off on a different tangent.
Comment 10 Marcel de Rooy 2018-11-08 18:13:21 UTC
(In reply to M. Tompsett from comment #9)
> I'm not sure no_split logic was the correct solution, so I have no issue
> with this removal. However, I think they may be tried to use that to split
> or not split based on the frameworkcode, because the field might not be
> repeatable, and so it is a little larger than this change alone. Just my
> thoughts on this. I could be off on a different tangent.

Hi Mark,

Split or not based on the frameworkcode or repeatableness of the subfield is imo an enhancement request; this was no part of earlier code. If feel it could be useful though. I could also think of having the option to use another separator than pipe. But they should imo be handled on a new report.
Comment 11 Katrin Fischer 2018-11-08 19:13:45 UTC
Just noting: we are using repeatable subfields for notes and custom item fields. It would be bad for us if this feature was broken.
Comment 12 M. Tompsett 2018-11-08 19:28:20 UTC
(In reply to Marcel de Rooy from comment #10)
> Split or not based on the frameworkcode or repeatableness of the subfield is
> imo an enhancement request; this was no part of earlier code.

It wasn't part of the earlier code, because they did no_split as a stop gap measure to fix it. The fix that made it in was wrong. This wouldn't be an enhancement, because it would actually be fixing the problem correctly.

I could sign this off, but then we haven't really solved the enforcing of non-repeatables. Why do you think frameworkcode was passed before it was removed in bug 19096? It is what would determine if it is repeatable, not the default framework. This patch allows anything and all things to repeat. Perhaps I'm reading the code wrong? *shrug*


> ... I could also think of having the option to use
> another separator than pipe. But they should imo be handled on a new report.

Pipe is already used as a separator in other places. I don't see a need for separator flexibility. But yes, that would be a separate issue.
Comment 13 Marcel de Rooy 2018-11-09 06:46:30 UTC
(In reply to Katrin Fischer from comment #11)
> Just noting: we are using repeatable subfields for notes and custom item
> fields. It would be bad for us if this feature was broken.

I am raising the severity again. Thx for feedback.
Comment 14 Marcel de Rooy 2018-11-09 07:02:55 UTC
(In reply to M. Tompsett from comment #12)

> I could sign this off, but then we haven't really solved the enforcing of
> non-repeatables. Why do you think frameworkcode was passed before it was
> removed in bug 19096? It is what would determine if it is repeatable, not
> the default framework. This patch allows anything and all things to repeat.
> Perhaps I'm reading the code wrong? *shrug*

You make a point, but frameworkcode was unfortunately not used to check if the field was repeatable. There was just a split on pipe.
Report 19096 removed the framework in order to make Koha consistent in its handling of kohafield: always use the default. But it did not look at repeatability just as the former code did not. So now we face the challenge of resolving this issue. The fix here is simple and can still go into 18.11 and yes it might repeat a bit too much (like before). But benefits are far greater. And I would still prefer to solve the repeatability issue on another report, perhaps making its way into 18.11.01 which would be a fix for something that we did never check before. (Just tested this again in 16.11: put a pipe in your barcode field.)

So hope you signoff !
Comment 15 Marcel de Rooy 2018-11-09 07:10:48 UTC
Opened bug 21800
Comment 16 Marcel de Rooy 2018-11-09 07:12:22 UTC
Nick we need this one too in 18.11
Comment 17 Fridolin SOMERS 2018-11-16 09:47:48 UTC
Created attachment 82390 [details] [review]
Bug 21774: Cloned item subfields disappear when editing an item

Bug 10306 changed behavior on cloning item subfields by no longer splitting
constructions like 'A | B' in item fields like ccode.

If it is really recommended to clone item subfields, I am not so sure
about. But this patch at least restores the possibility to do so while
we discuss if we should ;)

Test plan:
[1] Run Items.t
[2] Make an item subfield repeatable in framework. And test edit items.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 18 Fridolin SOMERS 2018-11-16 09:49:42 UTC
Works well.
Very old bug I think.
Looks like nearly nobody uses repeatable subfields in items field ;)
Comment 19 Marcel de Rooy 2018-11-16 10:55:27 UTC
(In reply to Fridolin SOMERS from comment #18)
> Works well.
> Very old bug I think.
> Looks like nearly nobody uses repeatable subfields in items field ;)

Thanks for testing. No old bug btw. See above comments.
Comment 20 Tomás Cohen Arazi 2018-11-20 13:52:19 UTC
Created attachment 82543 [details] [review]
Bug 21774: Cloned item subfields disappear when editing an item

Bug 10306 changed behavior on cloning item subfields by no longer splitting
constructions like 'A | B' in item fields like ccode.

If it is really recommended to clone item subfields, I am not so sure
about. But this patch at least restores the possibility to do so while
we discuss if we should ;)

Test plan:
[1] Run Items.t
[2] Make an item subfield repeatable in framework. And test edit items.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Works as expected. Also fixes the display of collections on the items
table (on editing items).
Comment 21 Marcel de Rooy 2018-11-20 13:55:08 UTC
Thx Tomas
Comment 22 Nick Clemens 2018-11-21 11:05:22 UTC
(In reply to Marcel de Rooy from comment #21)
> Thx Tomas

I just want to make sure this is expected behvaiour:
1 - Make an item field repeatable
2 - Edit item, repeat a field
3 - Save the item
4 - The repeated field is stored as "VALUEA | VALUEB"
5 - Edit the item
6 - Field is loaded as a single pipe delimited field

I worry about the use of pipe in regards to SIP, but can push if this is confirmed as working as expected and it is restoring previous behvaiour
Comment 23 Marcel de Rooy 2018-11-21 12:03:28 UTC
(In reply to Nick Clemens from comment #22)

> I just want to make sure this is expected behvaiour:
> 1 - Make an item field repeatable
> 2 - Edit item, repeat a field
> 3 - Save the item
> 4 - The repeated field is stored as "VALUEA | VALUEB"
> 5 - Edit the item
> 6 - Field is loaded as a single pipe delimited field
No, in step 6 you should see two separate subfields. Check framework?

> I worry about the use of pipe in regards to SIP, but can push if this is
> confirmed as working as expected and it is restoring previous behvaiour
This is not introducing something new. Just what we did before.
Comment 24 Tomás Cohen Arazi 2018-11-21 13:53:14 UTC
(In reply to Nick Clemens from comment #22)
> (In reply to Marcel de Rooy from comment #21)
> > Thx Tomas
> 
> I just want to make sure this is expected behvaiour:
> 1 - Make an item field repeatable
> 2 - Edit item, repeat a field
> 3 - Save the item
> 4 - The repeated field is stored as "VALUEA | VALUEB"
> 5 - Edit the item
> 6 - Field is loaded as a single pipe delimited field
> 
> I worry about the use of pipe in regards to SIP, but can push if this is
> confirmed as working as expected and it is restoring previous behvaiour

The bug this is fixing is that in master, the values are stored as pipe-separated values, but on the way back for rendering they are lost, and then data is lost.

I also noticed that when the item subfields are repeatable they are displayed as pipe-separated codes, and the patch also makes them show the descriptions instead (as happens with single values).
Comment 25 Nick Clemens 2018-11-21 14:03:01 UTC
Awesome work all!

Pushed to master for 18.11
Comment 26 Marcel de Rooy 2018-11-21 14:13:08 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> The bug this is fixing is that in master, the values are stored as
> pipe-separated values, but on the way back for rendering they are lost, and
> then data is lost.
No data is lost with me.


> I also noticed that when the item subfields are repeatable they are
> displayed as pipe-separated codes, and the patch also makes them show the
> descriptions instead (as happens with single values).
The pipe separated codes should not be in the editor ! Not sure what you mean with showing descriptions.
Comment 27 Marcel de Rooy 2018-11-21 14:30:09 UTC
OK We are talking different languages but meaning the same: bug fixed.
Comment 28 Jesse Maseto 2018-12-07 22:06:41 UTC
Backported to 18.05 for 18.05.07
Comment 29 Jonathan Druart 2018-12-12 12:34:05 UTC
(In reply to Jesse Maseto from comment #28)
> Backported to 18.05 for 18.05.07

One test is failing:
koha_1       | #   Failed test 'No tests run for subtest "Check stockrotationitem relationship"'
koha_1       | #   at t/db_dependent/Items.t line 860.
koha_1       | DBIx::Class::Schema::source(): Can't find source for Stockrotationitem at /kohadevbox/koha/t/lib/TestBuilder.pm line 189
koha_1       | # Looks like your test exited with 255 just after 13.
koha_1       | [20:33:35] t/db_dependent/Items.t

In order to make it pass, a follow-up for 18.05 should be provided to use another class than the new "Stockrotationitem"
Comment 30 Marcel de Rooy 2018-12-12 12:35:36 UTC
(In reply to Jonathan Druart from comment #29)
> (In reply to Jesse Maseto from comment #28)
> > Backported to 18.05 for 18.05.07
> 
> One test is failing:
> koha_1       | #   Failed test 'No tests run for subtest "Check
> stockrotationitem relationship"'
> koha_1       | #   at t/db_dependent/Items.t line 860.
> koha_1       | DBIx::Class::Schema::source(): Can't find source for
> Stockrotationitem at /kohadevbox/koha/t/lib/TestBuilder.pm line 189
> koha_1       | # Looks like your test exited with 255 just after 13.
> koha_1       | [20:33:35] t/db_dependent/Items.t
> 
> In order to make it pass, a follow-up for 18.05 should be provided to use
> another class than the new "Stockrotationitem"

Wrong bug ?
Comment 31 Jonathan Druart 2018-12-12 12:52:28 UTC
Nope, see
  commit d6fe41657861666320383904b16165cc4820777a
  Bug 21774: Cloned item subfields disappear when editing an item

Actually my understanding of the problem was wrong, it's more a bad merge conflict.
Comment 32 Marcel de Rooy 2018-12-12 12:55:31 UTC
(In reply to Jonathan Druart from comment #31)
> Nope, see
>   commit d6fe41657861666320383904b16165cc4820777a
>   Bug 21774: Cloned item subfields disappear when editing an item
> 
> Actually my understanding of the problem was wrong, it's more a bad merge
> conflict.

Ah okay. Very messy merge..
Comment 33 Lucas Gass 2018-12-12 17:32:07 UTC
reverted and reapplied cleaner merge
Comment 34 Fridolin SOMERS 2018-12-19 10:15:29 UTC
Depends on Bug 10306 not in 17.11.x