Bug 15572 - Authority creation fails when authid is linked to 001 field
Summary: Authority creation fails when authid is linked to 001 field
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Frédéric Demians
QA Contact: Testopia
URL:
Keywords:
Depends on: 6657
Blocks:
  Show dependency treegraph
 
Reported: 2016-01-13 20:53 UTC by Frédéric Demians
Modified: 2019-06-27 09:24 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 6657: Follow-up to fix error on authority creation (5.81 KB, patch)
2016-01-13 20:56 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 6657: Add tests for TransformHtmlToMarc (7.83 KB, patch)
2016-01-13 20:56 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 6657: TransformHtmlToMarc - rewrite the loop (2.32 KB, patch)
2016-01-13 20:57 UTC, Frédéric Demians
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 6657: Follow-up to fix error on authority creation (5.92 KB, patch)
2016-01-14 04:40 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 6657: Add tests for TransformHtmlToMarc (7.83 KB, patch)
2016-01-14 04:41 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 6657: TransformHtmlToMarc - rewrite the loop (2.32 KB, patch)
2016-01-14 04:41 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 15572: Follow-up to fix error on authority creation (5.93 KB, patch)
2016-01-14 08:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15572: Add tests for TransformHtmlToMarc (7.85 KB, patch)
2016-01-14 08:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15572: TransformHtmlToMarc - rewrite the loop (2.34 KB, patch)
2016-01-14 08:45 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Demians 2016-01-13 20:53:33 UTC
When adding a new authority, authorities.pl run foreever when authid is linked to 001 field. This is related to bug 6657 which modified the way C4::Biblio::TransformHtmlToMarc operates in order to solve an issue occurring during biblio record cataloging. But this function is also used by authorities cataloging, and the code in this case is irrelevant.

Occurs also in 3.20.
Comment 1 Frédéric Demians 2016-01-13 20:56:51 UTC Comment hidden (obsolete)
Comment 2 Frédéric Demians 2016-01-13 20:56:59 UTC Comment hidden (obsolete)
Comment 3 Frédéric Demians 2016-01-13 20:57:18 UTC Comment hidden (obsolete)
Comment 4 Frédéric Demians 2016-01-13 20:58:29 UTC
Patch-1 has to be sign-off. Patch-2 and 3 are already signed-off.
Comment 5 Héctor Eduardo Castro Avalos 2016-01-14 04:40:46 UTC Comment hidden (obsolete)
Comment 6 Héctor Eduardo Castro Avalos 2016-01-14 04:41:24 UTC Comment hidden (obsolete)
Comment 7 Héctor Eduardo Castro Avalos 2016-01-14 04:41:35 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2016-01-14 08:45:46 UTC
Created attachment 46641 [details] [review]
Bug 15572: Follow-up to fix error on authority creation

Bug 6657 modified the way C4::Biblio::TransformHtmlToMarc operates in order to
solve an issue occuring during biblio record cataloguing. But this function is
also used by authorities cataloguing, and the code in this case is irrelevante.
This followup allows to distinguish for which kind of record
TransformHtmlToMarc is called: biblio/authority.

A bug appears in authority creation without this patch in some circunstances:
when authid is linked to 001 field.

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Tested with a new authority record

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Jonathan Druart 2016-01-14 08:45:49 UTC
Created attachment 46642 [details] [review]
Bug 15572: Add tests for TransformHtmlToMarc

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2016-01-14 08:45:52 UTC
Created attachment 46643 [details] [review]
Bug 15572: TransformHtmlToMarc - rewrite the loop

This rewrites the while loop into a for loop, so $i still gets
incremented when we call next

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
  Make sense. Add readability. Infinite loop no more possible.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Jonathan Druart 2016-01-14 08:46:44 UTC
This subroutine is based on the order of the input in the html... It is really dangerous and should be rewrite.
Comment 12 Brendan Gallagher 2016-01-14 17:13:38 UTC
So does this still pass QA?  Or this is a recommendation that we should rewrite in the future?  At this point in the release I am ok - waiting for a rewrite because we still have a couple of months.


(In reply to Jonathan Druart from comment #11)
> This subroutine is based on the order of the input in the html... It is
> really dangerous and should be rewrite.
Comment 13 Jonathan Druart 2016-01-14 18:26:37 UTC
(In reply to Brendan Gallagher from comment #12)
> So does this still pass QA?  Or this is a recommendation that we should
> rewrite in the future?  At this point in the release I am ok - waiting for a
> rewrite because we still have a couple of months.

Yes, it needs a rewrite.
It's how the subroutine has been for years, so no rush :)
Comment 14 Frédéric Demians 2016-01-14 18:59:05 UTC
What is the workflow? This patch is required in 3.22/3.20 branches. Can it be pushed directly without being firstly pushed in master?
Comment 15 Katrin Fischer 2016-01-15 08:17:29 UTC
Hi Frederic, I think the fix should go into master and stable releases and then we can rewrite on top of it in master - Jonathan, do you agree?
Comment 16 Jonathan Druart 2016-01-15 10:31:42 UTC
The patches are for master, 3.22.x, 3.20.x and 3.18.x
It would be good to have it into the next stable releases.
Comment 17 Jesse Weaver 2016-01-19 22:01:10 UTC
Pushed to master. Thanks for your work.
Comment 18 Julian Maurice 2016-01-25 11:28:32 UTC
Patches were not pushed in the right order in master, resulting in a code slightly different from what it should be (empty if body instead of a 'next if' statement)

I think this should be fixed.
Comment 19 Julian Maurice 2016-01-25 11:29:27 UTC
Patches pushed to 3.22.x, will be in 3.22.2
Comment 20 Marcel de Rooy 2016-01-25 12:57:15 UTC
(In reply to Julian Maurice from comment #18)
> Patches were not pushed in the right order in master, resulting in a code
> slightly different from what it should be (empty if body instead of a 'next
> if' statement)
> 
> I think this should be fixed.

Perhaps change the status to attract RM attention again?
Comment 21 Jesse Weaver 2016-01-25 14:15:26 UTC
Patches repushed in correct order.
Comment 22 Frédéric Demians 2016-01-25 16:27:17 UTC
This patch has been pushed to 3.20.x, will be in 3.20.8.
Comment 23 Mirko Tietgen 2016-02-10 10:30:33 UTC
In master, creating authorities from z39.50 runs forever now, see 15715. Is that a side effect of these patches?
Comment 24 Jonathan Druart 2016-02-10 11:19:57 UTC
(In reply to Mirko Tietgen from comment #23)
> In master, creating authorities from z39.50 runs forever now, see 15715. Is
> that a side effect of these patches?

I don't get the issue on 3.22.x
Comment 25 Jonathan Druart 2016-02-10 17:41:18 UTC
(In reply to Mirko Tietgen from comment #23)
> In master, creating authorities from z39.50 runs forever now, see 15715. Is
> that a side effect of these patches?

For the record, bug 15715 is not linked at all to this patchset.