Bug 6875 - de-nesting C4 packages
Summary: de-nesting C4 packages
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P1 - high major (vote)
Assignee: Paul Poulain
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 7025 7034 7035 7036 7037 7051 7119 7188 7546 7685
Blocks: 7545
  Show dependency treegraph
 
Reported: 2011-09-16 14:48 UTC by Paul Poulain
Modified: 2015-06-04 23:23 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
script to check which package used in a package is useless (3.00 KB, text/x-perl)
2011-09-16 14:48 UTC, Paul Poulain
Details
log of the 1st script run before doing anythin (38.93 KB, text/plain)
2011-09-16 14:52 UTC, Paul Poulain
Details
Bug 6875 de-nesting Auth.pm (1.29 KB, patch)
2011-09-21 15:20 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 de-nesting C4/Branch.pm (2.94 KB, patch)
2011-09-21 15:21 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 de-nesting C4::Koha.pm (1.52 KB, patch)
2011-09-21 15:21 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 de-nesting C4/Serials (2.56 KB, patch)
2011-09-21 15:22 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 de-nesting C4::ClassSort & ClassSortRoutine (1.01 KB, patch)
2011-09-21 15:22 UTC, Paul Poulain
Details | Diff | Splinter Review
[NOT_APPLY][PATCH 01]Bug 6875 de nesting C4::Biblio (36.38 KB, patch)
2011-09-21 15:23 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 de-nesting C4::Heading (1.01 KB, patch)
2011-09-21 15:23 UTC, Paul Poulain
Details | Diff | Splinter Review
[NOT_APPLY][PATCH 02]Bug 6875 de-nesting C4::Items (5.45 KB, patch)
2011-09-21 15:23 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 de-nesting C4::VirtualShelves (1.60 KB, patch)
2011-09-21 15:23 UTC, Paul Poulain
Details | Diff | Splinter Review
[FAILED_QA][PATCH 03]bug 6875 de-nesting C4::Member (15.99 KB, patch)
2011-09-21 15:23 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 de-nesting C4::Circulation (645 bytes, patch)
2011-09-21 15:24 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 de-nesting C4/Print.pm (621 bytes, patch)
2011-09-21 15:24 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 de-nesting C4::Reserve, 1st step (627 bytes, patch)
2011-09-21 15:24 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 cleaning mainpage.pl (1.35 KB, patch)
2011-09-21 15:24 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 cleaning opac-main.pl (894 bytes, patch)
2011-09-21 15:24 UTC, Paul Poulain
Details | Diff | Splinter Review
[NOT_APPLY][PATCH 04]Bug 6875 cleaning opac-detail.pl (4.21 KB, patch)
2011-09-21 15:25 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 C4::Context cleaning (1.28 KB, patch)
2011-09-21 15:25 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 de-nesting Auth.pm (1.43 KB, patch)
2012-01-28 14:00 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[PASSED_QA][PATCH 06]Bug 6875 de-nesting C4::Koha.pm (1.61 KB, patch)
2012-01-28 14:06 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[PASSED_QA][PATCH 07]Bug 6875 cleaning mainpage.pl (1.58 KB, patch)
2012-01-28 14:11 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[PASSED_QA][PATCH 08]Bug 6875 cleaning opac-main.pl (1001 bytes, patch)
2012-01-28 14:13 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[PASSED_QA][PATCH 09]Bug 6875 de-nesting C4/Serials (2.67 KB, patch)
2012-01-28 14:21 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[PASSED_QA][PATCH 10]Bug 6875 de-nesting C4::Heading (1.88 KB, patch)
2012-01-28 14:26 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[PASSED_QA][PATCH 11]Bug 6875 de-nesting C4::VirtualShelves (1.70 KB, patch)
2012-01-28 14:31 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
[PASSED_QA][PATCH 05]Bug 6875 de-nesting Auth.pm (1.57 KB, patch)
2012-02-15 12:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6875 de nesting C4::Biblio (36.33 KB, patch)
2012-02-15 14:45 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6875 de-nesting C4::Items (5.45 KB, patch)
2012-02-15 14:45 UTC, Paul Poulain
Details | Diff | Splinter Review
[HAS_SOME_PROBLEM]bug 6875 de-nesting C4::Member (15.96 KB, patch)
2012-02-15 14:45 UTC, Paul Poulain
Details | Diff | Splinter Review
[FAILED_QA]Bug 6875 cleaning opac-detail.pl (4.17 KB, patch)
2012-02-15 14:45 UTC, Paul Poulain
Details | Diff | Splinter Review
C4::Members follow-up (958 bytes, patch)
2012-02-15 14:46 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED_QA]Bug 6875 Biblio module (36.53 KB, patch)
2012-02-20 11:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[PASSED_QA]Bug 6875 Items module (5.60 KB, patch)
2012-02-20 11:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6875 follow-up for Items/Biblio de-nesting (3.36 KB, patch)
2012-02-22 16:07 UTC, Paul Poulain
Details | Diff | Splinter Review
[PASSED_QA] Bug 6875 follow-up for Items/Biblio de-nesting (3.55 KB, patch)
2012-02-24 06:08 UTC, Katrin Fischer
Details | Diff | Splinter Review
C4::Members follow-up (1.07 KB, patch)
2012-03-08 16:23 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 6875 Followup: De-nesting C4::Items breaks bulkmarcimport (2.05 KB, patch)
2012-03-10 13:06 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 6875 Followup: De-nesting C4::Items breaks bulkmarcimport (2.24 KB, patch)
2012-03-10 13:41 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6875 Followup: De-nesting C4::Items breaks bulkmarcimport (2.24 KB, patch)
2012-03-10 13:42 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2011-09-16 14:48:37 UTC
Created attachment 5442 [details]
script to check which package used in a package is useless

C4 packages are using use C4::another package a lot. It result in all packages being loaded (& compiled) on each page.
This bugzilla entry will contain the work to remove as much as possible the dependancies

That will be achieved through :
* removing use C4::otherpackage and using require C4::otherpackage in the subs that need C4::otherpackage. Require is loaded at run time only. Most of the time, the C4::otherpackage is loaded everytime, just to be used once, in one sub (that is rarely used)
* removing useless subs that use a C4::otherpackage::sub, because there are some

Cleaning for each package will result in a patch, with a lot of comments/details/feedback.
To discover which sub is used by which script, I'll use the attached script.
Comment 1 Paul Poulain 2011-09-16 14:52:53 UTC
Created attachment 5443 [details]
log of the 1st script run before doing anythin
Comment 2 Colin Campbell 2011-09-16 15:34:40 UTC
Moving compilation of the module code from compile to runtime seems a bit counter intuitive. Certainly if we start running the code in any kind of persistent framework we want to do the opposite.
One thing that aids maintenenance is to only pollute (I mean import into ) your namespace those subroutines that you need. i.e, use Module qw( function1 function2);
It also flags up to those who come after you how much of that module is used.
Comment 3 Paul Poulain 2011-09-21 15:20:29 UTC Comment hidden (obsolete)
Comment 4 Paul Poulain 2011-09-21 15:21:31 UTC Comment hidden (obsolete)
Comment 5 Paul Poulain 2011-09-21 15:21:57 UTC Comment hidden (obsolete)
Comment 6 Paul Poulain 2011-09-21 15:22:11 UTC Comment hidden (obsolete)
Comment 7 Paul Poulain 2011-09-21 15:22:58 UTC Comment hidden (obsolete)
Comment 8 Paul Poulain 2011-09-21 15:23:09 UTC Comment hidden (obsolete)
Comment 9 Paul Poulain 2011-09-21 15:23:23 UTC Comment hidden (obsolete)
Comment 10 Paul Poulain 2011-09-21 15:23:33 UTC Comment hidden (obsolete)
Comment 11 Paul Poulain 2011-09-21 15:23:44 UTC Comment hidden (obsolete)
Comment 12 Paul Poulain 2011-09-21 15:23:55 UTC Comment hidden (obsolete)
Comment 13 Paul Poulain 2011-09-21 15:24:05 UTC Comment hidden (obsolete)
Comment 14 Paul Poulain 2011-09-21 15:24:14 UTC Comment hidden (obsolete)
Comment 15 Paul Poulain 2011-09-21 15:24:25 UTC Comment hidden (obsolete)
Comment 16 Paul Poulain 2011-09-21 15:24:36 UTC Comment hidden (obsolete)
Comment 17 Paul Poulain 2011-09-21 15:24:48 UTC Comment hidden (obsolete)
Comment 18 Paul Poulain 2011-09-21 15:25:05 UTC Comment hidden (obsolete)
Comment 19 Paul Poulain 2011-09-21 15:25:22 UTC Comment hidden (obsolete)
Comment 20 Paul Poulain 2011-09-21 15:36:33 UTC
(In reply to comment #2)
> Moving compilation of the module code from compile to runtime seems a bit
> counter intuitive. Certainly if we start running the code in any kind of
> persistent framework we want to do the opposite.
> One thing that aids maintenenance is to only pollute (I mean import into ) your
> namespace those subroutines that you need. i.e, use Module qw( function1
> function2);

I agree it seems counter intuitive (well, it's illogic in fact ;-) ) However, as long as we don't have persistency, it's good to have those tweakings, as my tests shows.

I've attached the 17 patches i've done, and here are some results :

* Biblio.pm is well isolated and load only what's needed
* Members is correctly isolated and doesn't load only what's needed
* Reserves and Circulation still require a lot of investigations. They load almost everything and it can't be simplified easily. Circulation & Reserve are loading recursively

Test on mainpage.pl :
Profile of mainpage.pl for 1.08s (of 1.43s), executing 118729 statements and 29826 subroutine calls in 132 source files and 41 string evals.

To compare with the same page before the de-nesting:
Profile of mainpage.pl for 1.73s (of 2.18s), executing 155108 statements and 41041 subroutine calls in 249 source files and 64 string evals.
==> -36379 statements (-24% !)

Test on opac-main.pl
Before de-nesting & cleaning = 
Profile of opac/opac-main.pl for 1.96s (of 2.61s), executing 218971 statements and 51466 subroutine calls in 248 source files and 61 string evals.
After de-nesting & cleaning =
Profile of opac/opac-main.pl for 1.29s (of 1.88s), executing 182574 statements and 40411 subroutine calls in 138 source files and 38 string evals.
==> -36397 statements (-16%)


The timings are not fully relevants, they depend on too much things (like mysql having the data in cache or no).
All tests have been done without any tweaking (ie : no memcache, basic mysql/apache config...)

> It also flags up to those who come after you how much of that module is used.

??? I don't understand what you mean here ?
Comment 21 Ian Walls 2011-10-15 18:55:34 UTC
Adding link to 7025, which I had filed without reading the the C4/Items.pm patch attached here.

I'm not marking 7025 as a duplicate, however, since it's a more direct fix to one small aspect of this overall bug report.  We can safely integrate the fix in 7025 without making all the other C4/Items.pm changes in the patch attached here.
Comment 22 Ian Walls 2011-10-15 19:13:28 UTC
Comment on attachment 5497 [details] [review]
Bug 6875 de-nesting C4/Branch.pm

Marking C4/Branch.pm patch as obsolete, since I've moved it to 7034 (and signed off/Passed QA)
Comment 23 Ian Walls 2011-10-15 19:33:40 UTC
Comment on attachment 5500 [details] [review]
Bug 6875 de-nesting C4::ClassSort & ClassSortRoutine

Marking C4::ClassSource and C4::ClassSortRoutine patch as obsolete here, since they are moved to 7035.
Comment 24 Ian Walls 2011-10-15 19:54:12 UTC
Comment on attachment 5506 [details] [review]
Bug 6875 de-nesting C4::Circulation

Marking patch obsolete here, as it's been moved to 7036
Comment 25 Ian Walls 2011-10-15 19:59:51 UTC
Comment on attachment 5507 [details] [review]
Bug 6875 de-nesting C4/Print.pm

Obsoleting, as this is moved to 7036 (with signoff)
Comment 26 Ian Walls 2011-10-18 13:49:29 UTC
Comment on attachment 5508 [details] [review]
Bug 6875 de-nesting C4::Reserve, 1st step

Marking obsolete, since I'm moving this patch to it's specific bug report (7051)
Comment 27 Paul Poulain 2011-10-24 11:37:59 UTC
Updating Version : This ENH will be for Koha 3.8
Comment 28 Paul Poulain 2011-10-25 15:05:36 UTC
Bug versionned for master. entries will be made against rel_3_8 once the patch has been applied (see thread about that on koha-devel yesterday)
Comment 29 Ian Walls 2011-11-06 11:36:06 UTC
Comment on attachment 5512 [details] [review]
Bug 6875 C4::Context cleaning

Moving C4::Context cleaning patch to bug 7188
Comment 30 Paul Poulain 2011-12-02 17:31:43 UTC
I tested the new benchmark_staff.pl script that does some load testing the results are great ( results compared with master, see http://wiki.koha-community.org/wiki/Benchmark_for_3.8)
1st column is with those patches, 2nd column is master
15311 vs 23709 -35% for the mainpage.pl !!!

Some of the attached patches does not apply anymore, i've rebased on my laptop, but not uploaded them for instance.
Comment 31 Katrin Fischer 2011-12-21 07:32:33 UTC
1) Changes to Auth.pm do not apply:
Bug 6875 de-nesting Auth.pm (1.29 KB, patch) 
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 6875 de-nesting Auth.pm
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Auth.pm
CONFLICT (content): Merge conflict in C4/Auth.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 6875 de-nesting Auth.pm
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

2) Changes to C4:Biblio do not apply:
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 6875 de nesting C4::Biblio
/home/katrin/kohaclone/.git/rebase-apply/patch:292: trailing whitespace.
    
/home/katrin/kohaclone/.git/rebase-apply/patch:425: trailing whitespace.
        
fatal: sha1 information is lacking or useless (C4/Serials.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 6875 de nesting C4::Biblio
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

3) Also failing: Items.pm
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 6875 de-nesting C4::Items
fatal: sha1 information is lacking or useless (C4/Items.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 6875 de-nesting C4::Items
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

4) C4:Member
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: bug 6875 de-nesting C4::Member
/home/katrin/kohaclone/.git/rebase-apply/patch:81: trailing whitespace.
                                FROM borrowers 
/home/katrin/kohaclone/.git/rebase-apply/patch:89: trailing whitespace.
                                FROM borrowers 
/home/katrin/kohaclone/.git/rebase-apply/patch:90: trailing whitespace.
                                LEFT JOIN categories ON borrowers.categorycode=categories.categorycode 
/home/katrin/kohaclone/.git/rebase-apply/patch:271: trailing whitespace.
                      GetMemberFlags 
warning: 4 lines add whitespace errors.
Using index info to reconstruct a base tree...
<stdin>:81: trailing whitespace.
                                FROM borrowers 
<stdin>:89: trailing whitespace.
                                FROM borrowers 
<stdin>:90: trailing whitespace.
                                LEFT JOIN categories ON borrowers.categorycode=categories.categorycode 
<stdin>:271: trailing whitespace.
                      GetMemberFlags 
warning: 4 lines applied after fixing whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging C4/ILSDI/Services.pm
Auto-merging C4/Members.pm
CONFLICT (content): Merge conflict in C4/Members.pm
Auto-merging circ/circulation.pl
Auto-merging circ/returns.pl
Auto-merging opac/sco/sco-main.pl
Auto-merging t/db_dependent/lib/KohaTest/Members.pm
Failed to merge in the changes.
Patch failed at 0001 bug 6875 de-nesting C4::Member
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".

5) opac-detail.pl
Apply? [y]es/[n]o/[e]dit/[v]iew patch/[a]ccept all y
Applying: Bug 6875 cleaning opac-detail.pl
fatal: sha1 information is lacking or useless (opac/opac-detail.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 6875 cleaning opac-detail.pl
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".

Other patches still apply.
Comment 32 Jared Camins-Esakov 2012-01-28 14:00:34 UTC Comment hidden (obsolete)
Comment 33 Jared Camins-Esakov 2012-01-28 14:06:58 UTC Comment hidden (obsolete)
Comment 34 Jared Camins-Esakov 2012-01-28 14:11:45 UTC Comment hidden (obsolete)
Comment 35 Jared Camins-Esakov 2012-01-28 14:13:45 UTC Comment hidden (obsolete)
Comment 36 Jared Camins-Esakov 2012-01-28 14:21:02 UTC Comment hidden (obsolete)
Comment 37 Jared Camins-Esakov 2012-01-28 14:26:45 UTC Comment hidden (obsolete)
Comment 38 Jared Camins-Esakov 2012-01-28 14:31:36 UTC Comment hidden (obsolete)
Comment 39 Jared Camins-Esakov 2012-01-28 14:44:46 UTC
I tested and signed off on the following patches:
De-nesting Auth.pm
De-nesting C4::Koha.pm
Cleaning mainpage.pl
Cleaning opac-main.pl
De-nesting C4/Serials
De-nesting C4::Heading
De-nesting C4::VirtualShelves

The code in those patches is pretty clean.

Three patches would not apply:
De-nesting C4::Biblio
De-nesting C4::Items
Cleaning opac-detail.pl

One patch fails QA on account of breaking circulation:
De-nesting C4::Member

I would like to encourage the QA team to review the patches that I signed off on so that they can be pushed to Master, rather than waiting for revised patches for the four that I didn't sign off on. Removing unnecessary dependencies is a result greatly to be desired.
Comment 40 Paul Poulain 2012-01-31 08:47:12 UTC
(In reply to comment #39)
> I would like to encourage the QA team to review the patches that I signed off
> on so that they can be pushed to Master, rather than waiting for revised
> patches for the four that I didn't sign off on.
Agreed, as all those patches are independant from each other, and change nothing to the Koha behaviour (at least if they're not bugguy ;-) )

I'll ask Marcel & Ian by pm : they are small patches, should be easy to QA
Comment 41 Marcel de Rooy 2012-02-15 12:12:54 UTC
Testing the first one:
Applying: Bug 6875 de nesting C4::Biblio
/usr/share/koha/testclone/.git/rebase-apply/patch:292: trailing whitespace.

/usr/share/koha/testclone/.git/rebase-apply/patch:425: trailing whitespace.

error: patch failed: C4/Items.pm:78
error: C4/Items.pm: patch does not apply
fatal: sha1 information is lacking or useless (C4/Serials.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001.
Comment 42 Marcel de Rooy 2012-02-15 12:14:17 UTC
Testing the second one:
Applying: Bug 6875 de-nesting C4::Items
error: patch failed: C4/Items.pm:1218
error: C4/Items.pm: patch does not apply
fatal: sha1 information is lacking or useless (C4/Items.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001.
Comment 43 Marcel de Rooy 2012-02-15 12:16:14 UTC
Testing the third one:

Applying: bug 6875 de-nesting C4::Member
/usr/share/koha/testclone/.git/rebase-apply/patch:81: trailing whitespace.
                                FROM borrowers
/usr/share/koha/testclone/.git/rebase-apply/patch:89: trailing whitespace.
                                FROM borrowers
/usr/share/koha/testclone/.git/rebase-apply/patch:90: trailing whitespace.
                                LEFT JOIN categories ON borrowers.categorycode=categories.categorycode
/usr/share/koha/testclone/.git/rebase-apply/patch:271: trailing whitespace.
                      GetMemberFlags
error: patch failed: C4/Members.pm:27
error: C4/Members.pm: patch does not apply
error: patch failed: opac/sco/sco-main.pl:95
error: opac/sco/sco-main.pl: patch does not apply
error: patch failed: t/db_dependent/lib/KohaTest/Members.pm:14
error: t/db_dependent/lib/KohaTest/Members.pm: patch does not apply
Using index info to reconstruct a base tree...
<stdin>:81: trailing whitespace.
                                FROM borrowers
<stdin>:89: trailing whitespace.
                                FROM borrowers
<stdin>:90: trailing whitespace.
                                LEFT JOIN categories ON borrowers.categorycode=categories.categorycode
<stdin>:271: trailing whitespace.
                      GetMemberFlags
warning: 4 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merged C4/ILSDI/Services.pm
Auto-merged C4/Members.pm
CONFLICT (content): Merge conflict in C4/Members.pm
Auto-merged C4/SIP/ILS/Patron.pm
Auto-merged circ/circulation.pl
Auto-merged circ/returns.pl
Auto-merged members/deletemem.pl
Auto-merged opac/sco/sco-main.pl
Auto-merged t/db_dependent/lib/KohaTest/Members.pm
Failed to merge in the changes.
Patch failed at 0001.
Comment 44 Marcel de Rooy 2012-02-15 12:17:56 UTC
Additional comment on the third one:
I see reference to t/db_dependent/lib/KohaTest/Members.pm.
I already sent Paul a request to delete those failing tests in that subdir. They are not working (anymore). Please do not include it here.
Comment 45 Marcel de Rooy 2012-02-15 12:19:17 UTC
Testing the 4th:
Applying: Bug 6875 cleaning opac-detail.pl
error: patch failed: opac/opac-detail.pl:302
error: opac/opac-detail.pl: patch does not apply
fatal: sha1 information is lacking or useless (opac/opac-detail.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001.
Comment 46 Marcel de Rooy 2012-02-15 12:24:43 UTC
Testing the 5th:
Reacting on this line:
* still to do = work on C4::VirtualShelves, that can be optimized, definetly !

I would propose to wait with that. I have a patch forthcoming for bug 7310 (large one) with a lot of work for virtual shelves.
Comment 47 Marcel de Rooy 2012-02-15 12:36:12 UTC Comment hidden (obsolete)
Comment 48 Marcel de Rooy 2012-02-15 12:42:08 UTC
Patch 06 for Koha.pm marked as Passed QA too.
Comment 49 Marcel de Rooy 2012-02-15 12:46:40 UTC
Patch 7 (cleaning mainpage.pl)
Removing useless code. Great! Passed QA too.
Comment 50 Marcel de Rooy 2012-02-15 12:51:09 UTC
Patch 08 for opac-main.pl: Passed QA
Comment 51 Marcel de Rooy 2012-02-15 13:01:15 UTC
Comments on 9th patch for Serials.pm: 
Does take out 6 use statements, adds 2 requires. Due to circular module dependency between Serials and Biblio, compiling Serials still results in many subroutine redefinition warnings. But this is the case already now (just observing it).

Subroutine GetSuppliersWithLateIssues redefined at C4/Serials.pm line 87.
Subroutine GetLateIssues redefined at C4/Serials.pm line 110.
[etc][etc]

Since this patch makes a move in the right direction, marking it as Passed QA.
Comment 52 Marcel de Rooy 2012-02-15 13:04:47 UTC
Patch 10 for Heading.pm: Passed QA
Comment 53 Marcel de Rooy 2012-02-15 13:11:14 UTC
Patch 11 for VirtualShelves.pm: Passed QA (will rebase my work for 7310)
Comment 54 Paul Poulain 2012-02-15 14:45:25 UTC Comment hidden (obsolete)
Comment 55 Paul Poulain 2012-02-15 14:45:41 UTC Comment hidden (obsolete)
Comment 56 Paul Poulain 2012-02-15 14:45:54 UTC
Created attachment 7656 [details] [review]
[HAS_SOME_PROBLEM]bug 6875 de-nesting C4::Member

* working on GetMemberDetails.
This sub must be optimized, as it is called on each page by C4::Auth.
The sub call everytime patronflags, that need to load Reserves and Overdues. The patronflag returns a hash with various flags (overdues, fines, ...). It's not related to permissionflags, that are loaded elsewhere. Most of the time, we do nothing with those flags.
The strategy is to split the GetMemberDetails in 2 subs :
* GetMemberDetails, same as previously, without the flags
* GetMemberFlags, that will return just the flags when needed (and will load when needed Reserves package)
The following scripts call GetMemberDetails :
  * admin/aqbudget.pl = no use is done with flags, the splitting changes nothing
  * C4/SIP/ILS/Patron.pm = the flags are used later, so we call GetMemberFlags as well
  * C4/SIP/interactive_members_dump.pl = nothing is done with the flags, we change nothing
  * C4/Print.pm = used by printslip, that don't do anything with flags, we change nothing
  * C4/VirtualShelves = used to check permission to access to a virtualshelf, nothing done with flags, we change nothing
  * C4/Reserves = used in CheckReserves. With the result, we call _GetCircControlBranch only, that does nothing with flags, we change nothing
  * C4/Auth = used in get_template_and_user. The return from GetMemberDetails is stored in the session, I don't see any change, flags are useless
  * C4/Circulation.pm = used many times
    * in CanBookBeIssued, just to retrieve the name/cardnumber/borrowernumber of a book already issued to someone else
    * in CanBookBeIssued, just to retrieve the name/cardnumber/borrowernumber of a book already reserved by someone else
    * in AddReturn, the result of GetMemberDetails is returned. It's then used in return.pl, to display patron flags. Retrieving flags with GetMemberFlags in return.pl
    * in AddRenewal, to retrieve some information, nothing done with flags, we change nothing
    * in GetRenewCount, to retrieve name & categorycode, nothing done with flags, we change nothing
  * in ILSDI/Services.pm, GetMemberDetails is called many times. Only once we check for charges, that is in the flags, so retrieving flags with GetMemberFlags
  * in catalogue/detail.pl, GetMemberDetails is called to retrieve the name of a patron that has placed a hold on a book, we change nothing
  * in circ/transferstoreceive, GetMemberDetails is called to retrieve the name of a patron, (and address), we change nothing
  * in circ/circulation.pl, we must retrieve the flags to display them (like for return.pl)
  * in circ/waitingreserves.pl, GetMemberDetails is called to retrieve the name of the patron, nothing to change
  * in members/moremember.pl, GetMemberDetails is called to retrieve name, surname, borrowernumber, we change nothing
  * in members/deletemem.pl, GetMemberDetails is followed by a $flag = $bor->{flags}, replacing by a call to GetMemberDetails
  * in members/messaging.pl, GetMemberDetails is called to retrieve name, category, nothing to change
  * members/member-flags.pl is here for permissions, nothing to deal with patron flags, nothing to change
  * opac/opac-passwd.pl, nothing related to flags, nothing to change
  * opac/opac-ics.pl,  nothing related to flags, nothing to change
  * opac/opac-renew.pl, used to retrieve only branchcode, nothing to change
  * opac/opac-account.pl, nothing to change, the core part of this script is to display fines that are retrieved by GetMemberAccountRecords
  * opac/opac-reserve.pl, nothing related to flags, nothing to change
  * opac/opac-userdetail.pl, script that retrieve only a few dates and other infos from GetMemberDetails, nothing to change
  * opac/opac-showreviews.pl, retrieve only name & email from GetMemberDetails, nothing to change
  * opac/SCO/sco-main.pl, calls canbookbeissued, that need flags, so retrieving them
  * opac/opac-readingrecord.pl, use GetMemberDetails to retrieve patron name, address... nothing to change
  * opac/opac-messaging.pl, use GetMemberDetails to retrieve sms informations, nothing to change
  * opac/opac-userupdate.pl, use GetMemberDetails to display member information that can be updated by the patron, nothing related to flags, nothing to change
  * opac/opac-privacy.pl, loaded just to retrieve name & privacy flag info, nothing to change
  * opac/opac-user.pl, use GetMemberDetails to display patron information, but not flags one, nothing to change
  * opac/opac-suggestion.pl use GetMemberDetails to retrieve the branchcode, nothing to change
  * reserve/request.pl use GetMemberDetails to retrieve many informations displayed (name, branchcode,...), but none related to flags. Nothing to change

GetMemberDetails also launches 2 SQL queries, the 2nd being superfluous : the enrolmentperiod is already available in the 1st query, just return it !
=> C4::Reserves & C4::Overdues are now used only in GetMemberFlags sub, that is not always called, so switching from a use to a require inside the sub

C4::Accounts is used only in AddMember and ExtendMemberSubscriptionTo, and only if there is a fee for subscription, switching from use to require

sub GetMemberAccountRecords the SQL query has been completed. With a LEFT JOIN, we retrieve directly the title & biblionumber. Thus the call to GetBiblioByItemNumber is now useless and the use C4::Biblio can be removed
Comment 57 Paul Poulain 2012-02-15 14:45:59 UTC
Created attachment 7657 [details] [review]
[FAILED_QA]Bug 6875 cleaning opac-detail.pl

C4::Tags and C4::Reviews used only if the feature is activated.
Changing the use to a require inside the if (after testing it's activated)
For libraries that have activated both, that means no change, but for others, it's an improvement
Comment 58 Paul Poulain 2012-02-15 14:46:05 UTC Comment hidden (obsolete)
Comment 59 Paul Poulain 2012-02-15 14:56:08 UTC
Patches that do no apply have been rebased and re-submitted.
Patches that have been pushed have been obsoleted

Please signoff/QA again on those patches.

I haven't addressed comment #44:
> Additional comment on the third one:
> I see reference to t/db_dependent/lib/KohaTest/Members.pm.
> I already sent Paul a request to delete those failing tests in that subdir.
> They are not working (anymore). Please do not include it here.

I still have to remove the tests, so, until it's done, those lines are useless but harmless.

I've seen comment #39
> One patch fails QA on account of breaking circulation:
> De-nesting C4::Member
but could not figure what's happening for now. I made a follow-up that fixes a part of the problem (execution error), but the page seems broken, with many informations empty (1 issue, not displayed, patron surname empty...)

Please ignore this patch if you're testing them. You can signoff/QA the others though
Comment 60 Marcel de Rooy 2012-02-20 11:04:58 UTC Comment hidden (obsolete)
Comment 61 Marcel de Rooy 2012-02-20 11:41:23 UTC Comment hidden (obsolete)
Comment 62 Marcel de Rooy 2012-02-20 12:00:09 UTC
QA Comment on patch cleaning up opac-detail.pl:
Use of C4::Tags is replaced by later require. Introduces following problem:
[error] opac-detail.pl: Too late to run INIT block at /usr/share/koha/testclone/C4/Tags.pm line 64.
Marked this one as Failed QA in name of patch attachment.
Comment 63 Marcel de Rooy 2012-02-20 12:01:58 UTC
Updating status to Passed QA for the two patches marked as such. Please set status back afterwards.
Comment 64 Paul Poulain 2012-02-20 15:45:27 UTC
Comment on attachment 7770 [details] [review]
[PASSED_QA]Bug 6875 Biblio module

this patch has been pushed
Comment 65 Paul Poulain 2012-02-20 15:45:53 UTC
Comment on attachment 7771 [details] [review]
[PASSED_QA]Bug 6875 Items module

this patch has been pushed
Comment 66 Srdjan Jankovic 2012-02-22 02:22:25 UTC
The patch pushed to master has a bug: C4::Items calls _find_value() that is in Biblio.
Comment 67 Marcel de Rooy 2012-02-22 12:27:51 UTC
(In reply to comment #66)
> The patch pushed to master has a bug: C4::Items calls _find_value() that is in
> Biblio.
Good catch, Srdjan! Anyone already working on this one? Paul perhaps??
Comment 68 Paul Poulain 2012-02-22 12:37:19 UTC
(In reply to comment #67)
> (In reply to comment #66)
> > The patch pushed to master has a bug: C4::Items calls _find_value() that is in
> > Biblio.
> Good catch, Srdjan! Anyone already working on this one? Paul perhaps??

yep. I plan to work on this in the next hours
Comment 69 Paul Poulain 2012-02-22 16:07:01 UTC Comment hidden (obsolete)
Comment 70 Paul Poulain 2012-02-22 16:10:10 UTC
How to test the attachment 7808 [details] [review] :
before applying the patch:
* reach a serial that has "serial recieve create items"
* reach serials-collection.pl page
* check a serial that has already be recieved, and click on "Edit Serials"

=> Perl error

After applying the patch = works fine.
Comment 71 Katrin Fischer 2012-02-24 06:08:12 UTC Comment hidden (obsolete)
Comment 72 Katrin Fischer 2012-02-24 06:45:58 UTC
Changing status so last patch can go through QA.
Comment 73 Marcel de Rooy 2012-02-27 07:57:57 UTC
Changing status to Passed QA for Follow-up for Items/Biblio de-nesting
Comment 74 Paul Poulain 2012-02-27 10:13:21 UTC
Comment on attachment 7844 [details] [review]
[PASSED_QA] Bug 6875 follow-up for Items/Biblio de-nesting

this patch has been pushed
Comment 75 Liz Rea 2012-03-08 16:23:31 UTC Comment hidden (obsolete)
Comment 76 Liz Rea 2012-03-08 16:24:33 UTC
setting to signed off for C4::Members follow-up.
Comment 77 Marcel de Rooy 2012-03-09 15:47:27 UTC
(In reply to comment #76)
> setting to signed off for C4::Members follow-up.

It is only useful in connection with the first Members patch. And that appears to be still in construction. And could be done in one pass then. 
Changing status to reflect that.
Comment 78 Ian Walls 2012-03-10 13:01:59 UTC
De-nesting C4::Items has resulted in bulkmarcimport.pl throwing the error: 

ERROR: Adding items to bib 1 failed: Undefined subroutine &C4::Items::GetBranchName called at /home/sekjal/kohaclone/C4/Items.pm line 656, <GEN13> line 435.

GetBranchName is not scoped properly.  Adding the C4::Branch:: prefix will fix this.  Patch forthcoming.
Comment 79 Ian Walls 2012-03-10 13:06:29 UTC Comment hidden (obsolete)
Comment 80 Jared Camins-Esakov 2012-03-10 13:41:35 UTC Comment hidden (obsolete)
Comment 81 Jared Camins-Esakov 2012-03-10 13:42:07 UTC Comment hidden (obsolete)
Comment 82 Paul Poulain 2012-03-14 15:16:30 UTC
Comment on attachment 8135 [details] [review]
Bug 6875 Followup: De-nesting C4::Items breaks bulkmarcimport

this patch has been pushed
Comment 83 Ian Walls 2012-03-19 02:42:53 UTC
C4::Members follow-up patch passed QA.
Comment 84 Paul Poulain 2012-03-19 16:50:02 UTC
Comment on attachment 8095 [details] [review]
C4::Members follow-up

this patch has been pushed (on master branch directly)
Comment 85 Chris Cormack 2012-08-24 02:23:00 UTC
Why is this assigned? Are their still problems with it?
Comment 86 Chris Cormack 2012-08-24 02:23:29 UTC
Also, are THERE problems with it.
Comment 87 Owen Leonard 2014-07-11 12:34:07 UTC
Time to ask again! There are no more open bugs on which this one depends. Can it be considered resolved?
Comment 88 Katrin Fischer 2014-07-12 11:08:25 UTC
+1 closing