|
Description
Frédéric Demians
2016-01-13 20:53:33 UTC
Created attachment 46619 [details] [review] Bug 6657: 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. Created attachment 46620 [details] [review] Bug 6657: Add tests for TransformHtmlToMarc Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Created attachment 46621 [details] [review] Bug 6657: 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. Patch-1 has to be sign-off. Patch-2 and 3 are already signed-off. Created attachment 46635 [details] [review] [SIGNED-OFF]Bug 6657: 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 Created attachment 46636 [details] [review] Bug 6657: Add tests for TransformHtmlToMarc Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Created attachment 46637 [details] [review] Bug 6657: 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. 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> 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> 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> This subroutine is based on the order of the input in the html... It is really dangerous and should be rewrite. 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. (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 :) 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? 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? 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. Pushed to master. Thanks for your work. 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. Patches pushed to 3.22.x, will be in 3.22.2 (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? Patches repushed in correct order. This patch has been pushed to 3.20.x, will be in 3.20.8. In master, creating authorities from z39.50 runs forever now, see 15715. Is that a side effect of these patches? (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 (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. |