Summary: | Cloned item subfields disappear when editing an item | ||
---|---|---|---|
Product: | Koha | Reporter: | jesus |
Component: | Cataloging | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | critical | ||
Priority: | P5 - low | CC: | fridolin.somers, jesse, jonathan.druart, lucas, m.de.rooy, mtompset, nick, tomascohen, yolanda.marcos |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21800 | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Small patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
Bug Depends on: | 10306 | ||
Bug Blocks: | |||
Attachments: |
Screenshots of steps to reproduce
Bug 21774: Cloned item subfields disappear when editing an item Bug 21774: Cloned item subfields disappear when editing an item Bug 21774: Cloned item subfields disappear when editing an item Bug 21774: Cloned item subfields disappear when editing an item |
Description
jesus
2018-11-06 18:01:38 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? Created attachment 82060 [details]
Screenshots of steps to reproduce
I can confirm the bug happening on Debian 9.5 stock packages 17.11.04 and 18.05.05-1 (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. 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.. Created attachment 82078 [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> 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> > 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 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. (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. Just noting: we are using repeatable subfields for notes and custom item fields. It would be bad for us if this feature was broken. (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. (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. (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 ! Opened bug 21800 Nick we need this one too in 18.11 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> Works well. Very old bug I think. Looks like nearly nobody uses repeatable subfields in items field ;) (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. 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). Thx Tomas (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 (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. (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). Awesome work all! Pushed to master for 18.11 (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. OK We are talking different languages but meaning the same: bug fixed. Backported to 18.05 for 18.05.07 (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" (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 ? 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. (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.. reverted and reapplied cleaner merge Depends on Bug 10306 not in 17.11.x |