Bug 18212 - Move SQL code from aqplan.pl administrative script into Koha directory perl modules
Summary: Move SQL code from aqplan.pl administrative script into Koha directory perl m...
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: 17.05
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-04 21:44 UTC by Alex Buckley
Modified: 2019-03-21 05:25 UTC (History)
2 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 18212 - Created a subroutine GetAuthValuesForCategory() in C4/Acqusition.pm perl module. (2.39 KB, patch)
2017-03-10 22:59 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files (6.73 KB, patch)
2017-03-10 23:00 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18212 - Added a successful unit test for GetAuthValues() function (1.90 KB, patch)
2017-03-10 23:01 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18212 - Four successfully transferred sql queries to Koha::AuthorisedValue, Koha::ItemType, Koha::Libraries from aqplan.pl administrative script (11.21 KB, patch)
2017-03-10 23:01 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files (8.76 KB, patch)
2017-04-24 04:01 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18212 - Four successfully transferred sql queries to Koha::AuthorisedValue, Koha::ItemType, Koha::Libraries from aqplan.pl administrative script (10.36 KB, patch)
2017-04-24 04:01 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files (9.02 KB, patch)
2017-04-24 07:22 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files (9.03 KB, patch)
2017-04-24 07:38 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18212 - Followup: Fixed POD in AuthorisedValue.pm and ItemType.pm (3.38 KB, patch)
2017-08-28 20:43 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files (12.68 KB, patch)
2017-08-28 20:54 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18212 - Followup: Fixed POD in AuthorisedValue.pm and ItemType.pm (3.38 KB, patch)
2017-08-28 20:57 UTC, Alex Buckley
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2017-03-04 21:44:45 UTC
This bug report requires the removal of SQL code out of the administrative script aqplan.pl and placed into a perl module in C4 directory. 

Instead of placing the SQL code directly into a subroutine in a C4 module the database query will be re-written as a DBIx query.
Comment 1 Alex Buckley 2017-03-10 22:59:58 UTC
Created attachment 61006 [details] [review]
Bug 18212 - Created a subroutine GetAuthValuesForCategory() in C4/Acqusition.pm perl module.

I've removed one sql select query from aqplan.pl and rewritten it in DBIx query format in the new GetAuthValuesForCategory subroutine.
aqplan.pl is successfully calling and running this DBIx query
Comment 2 Alex Buckley 2017-03-10 23:00:45 UTC
Created attachment 61007 [details] [review]
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files

The new subroutines in the perl modules contain POD
Comment 3 Alex Buckley 2017-03-10 23:01:28 UTC
Created attachment 61008 [details] [review]
Bug 18212 - Added a successful unit test for GetAuthValues() function
Comment 4 Alex Buckley 2017-03-10 23:01:56 UTC
Created attachment 61009 [details] [review]
Bug 18212 - Four successfully transferred sql queries to Koha::AuthorisedValue, Koha::ItemType, Koha::Libraries from aqplan.pl administrative script

All these queries have been placed inside subroutines in the perl
modules outlined above, each subroutine has POD, and a successful unit test
Comment 5 Alex Buckley 2017-03-10 23:21:30 UTC
Test plan:

1. Restart memcached: sudo service memcached restart
2. Drop and recreate the Koha instance database
3. Go through the web installer, selecting all data to be installed in step 3
4. After the web installer is finished log in with the Koha database administrator credentials
5. Create yourself a patron account 
6. Set the patron account to have superlibrarian privileges 
7. Log out and back in as your newly created patron
8. Navigate to the Acquisitions interface 
9. Create a budget named 'testbudget' 
10. Create a fund named 'testfund' 
11. Navigate to the 'Administrator' interface 
12. Write in the URL 'cgi-bin/koha/admin/aqplan.pl'
13. There should be no records showing 
14. If you click on Planning->Plan by Months testfund will be displayed 
15. Repeat step 14 but click on Planning->Plan by libraries and Planning->Plan by Itemtypes, Both of these pages should also show 'testfund'
16. In your terminal navigate from your Koha instance root directory to t/db_dependent
17. Enter koha shell: sudo koha-shell <instancename>
18. Run AuthorisedValues unit tests: prove -v AuthorisedValues.t
19. The tests should run successful
20. Exit the koha shell
21. cd into the 'koha' directory from where you are
22. Enter the koha shell and run the ItemTypes.t and Libraries.t unit tests by writing in: prove -v filename.t (both of which should be successful)
Comment 6 Alex Buckley 2017-03-12 12:29:52 UTC
Amended Test plan:

1. Apply all the patches attached to this bug report
2. Restart memcached: sudo service memcached restart
3. Drop and recreate the Koha instance database
4. Go through the web installer, selecting all data to be installed in step 3
5. After the web installer is finished log in with the Koha database administrator credentials
6. Create yourself a patron account 
7. Set the patron account to have superlibrarian privileges 
8. Log out and back in as your newly created patron
9. Navigate to the Acquisitions interface 
10. Create a budget named 'testbudget' 
11. Create a fund named 'testfund' 
12. Navigate to the 'Administrator' interface 
13. Write in the URL 'cgi-bin/koha/admin/aqplan.pl'
14. There should be no records showing 
15. If you click on Planning->Plan by Months testfund will be displayed 
16. Repeat step 14 but click on Planning->Plan by libraries and Planning->Plan by Itemtypes, Both of these pages should also show 'testfund'
17. In your terminal navigate from your Koha instance root directory to t/db_dependent
18. Enter koha shell: sudo koha-shell <instancename>
19. Run AuthorisedValues unit tests: prove -v AuthorisedValues.t
20. The tests should run successful
21. Exit the koha shell
22. cd into the 'koha' directory from where you are
23. Enter the koha shell and run the ItemTypes.t and Libraries.t unit tests by writing in: prove -v filename.t (both of which should be successful)
Comment 7 Marc Véron 2017-04-21 06:02:11 UTC
Patch Bug 18212 - Four successfully transferred sql queries... does not apply:

Wende an: Bug 18212 - Four successfully transferred sql queries to Koha::AuthorisedValue, Koha::ItemType, Koha::Libraries from aqplan.pl administrative script
/home/marc/koha/.git/rebase-apply/patch:316: trailing whitespace.
     my $dbh = C4::Context->dbh; 
fatal: sha1 information is lacking or useless (Koha/ItemType.pm).
Dem Repository fehlen notwendige Blobs um auf einen 3-Wege-Merge zurückzufallen.
Kann nicht zu 3-Wege-Merge zurückfallen.
Comment 8 Alex Buckley 2017-04-24 04:01:10 UTC
Created attachment 62584 [details] [review]
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files

The new subroutines in the perl modules contain POD
Comment 9 Alex Buckley 2017-04-24 04:01:27 UTC
Created attachment 62585 [details] [review]
Bug 18212 - Four successfully transferred sql queries  to Koha::AuthorisedValue, Koha::ItemType, Koha::Libraries from aqplan.pl administrative script
Comment 10 Alex Buckley 2017-04-24 04:03:18 UTC
(In reply to Marc Veron on comment 7)

Hi Marc 

I have gone through and fixed the patches and rebased them down to 2 patches. It should now apply successfully for you on a clean branch.
Comment 11 Marc Véron 2017-04-24 07:22:51 UTC
Created attachment 62587 [details] [review]
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files

The new subroutines in the perl modules contain POD

Tested both patches together, works as expected
Amended with blank line preceding a =cut to make qa tools pass
for first patch
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 12 Marc Véron 2017-04-24 07:29:50 UTC
Comment on attachment 62587 [details] [review]
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files

Obsoleting because there are problems with next patch after my amendement.
Comment 13 Marc Véron 2017-04-24 07:31:42 UTC
Comment on attachment 62584 [details] [review]
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files

Restoring previous patch. It does not pass qa tools because of a fail in Koha/ItemType.pm (Apparent command =cut not preceded by blank lins)
Comment 14 Marc Véron 2017-04-24 07:38:02 UTC
Created attachment 62588 [details] [review]
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files

The new subroutines in the perl modules contain POD

There is a minor error in qa tools that can be fixed in next patch.
Followed test plan (comment #6) with both patches together, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 15 Marc Véron 2017-04-24 07:43:10 UTC
Sorr,, Alex, I wanted to rescue a minor issue in the first patch (qa tools complaining about missing blank line preceding a =cut to make), but it did not work out.

Reset to your patches.

Do you have qa tools installed?
Comment 16 Alex Buckley 2017-04-28 07:23:11 UTC
Hi Marc

I do have qa tools installed.

Could you please clarify what the issue with these patches are? I have just run the qa tool over the last 2 patches and all tests  were successful.
Comment 17 Marc Véron 2017-04-28 17:45:09 UTC
(In reply to Alex Buckley from comment #16)
> Hi Marc
> 
> I do have qa tools installed.
> 
> Could you please clarify what the issue with these patches are? I have just
> run the qa tool over the last 2 patches and all tests  were successful.

Just re-tested. QA tools complain about 2 minor things:

- Apply 1st patch
- Run QA tools (koha-qa.pl -v 2 -c 1)
Result:
(...)
 FAIL	Koha/ItemType.pm
(...)
   FAIL	  pod
		 in file Koha/ItemType.pm
		Apparent command =cut not preceded by blank line

-Apply 2nd patch
- QA tools
FAIL	Koha/AuthorisedValue.pm
(...)
   FAIL	  pod
		 in file Koha/AuthorisedValue.pm
		
		Apparent command =cut not preceded by blank line

Setting back to FQA. Will be happy to sign of as soon as QA tools pass :-)
Comment 18 Alex Buckley 2017-08-28 20:43:26 UTC
Created attachment 66567 [details] [review]
Bug 18212 - Followup: Fixed POD in AuthorisedValue.pm and ItemType.pm

Passes QA test now, and all unit tests pass

Test plan:

1. Apply all the patches attached to this bug report
2. Restart memcached: sudo service memcached restart
3. Drop and recreate the Koha instance database
4. Go through the web installer, selecting all data to be installed in
step 3
5. After the web installer is finished log in with the Koha database
administrator credentials
6. Create yourself a patron account
7. Set the patron account to have superlibrarian privileges
8. Log out and back in as your newly created patron
9. Navigate to the Acquisitions interface
10. Create a budget named 'testbudget'
11. Create a fund named 'testfund'
12. Navigate to the 'Administrator' interface
13. Write in the URL 'cgi-bin/koha/admin/aqplan.pl'
14. There should be no records showing
15. If you click on Planning->Plan by Months testfund will be displayed
16. Repeat step 14 but click on Planning->Plan by libraries and
Planning->Plan by Itemtypes, Both of these pages should also show
'testfund'
17. In your terminal navigate from your Koha instance root directory to
t/db_dependent
18. Enter koha shell: sudo koha-shell <instancename>
19. Run AuthorisedValues unit tests: prove -v AuthorisedValues.t
20. The tests should run successful
21. Exit the koha shell
22. cd into the 'koha' directory from where you are
23. Enter the koha shell and run the ItemTypes.t and Libraries.t unit tests by writing in: prove -v filename.t (both of which should be successful)

Sponsored-by: Catalyst IT
Comment 19 Alex Buckley 2017-08-28 20:54:53 UTC
Created attachment 66570 [details] [review]
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files

The new subroutines in the perl modules contain POD

Test plan:
1. Apply all the patches attached to this bug report
2. Restart memcached: sudo service memcached restart
3. Drop and recreate the Koha instance database
4. Go through the web installer, selecting all data to be installed in
step 3
5. After the web installer is finished log in with the Koha database
administrator credentials
6. Create yourself a patron account
7. Set the patron account to have superlibrarian privileges
8. Log out and back in as your newly created patron
9. Navigate to the Acquisitions interface
10. Create a budget named 'testbudget'
11. Create a fund named 'testfund'
12. Navigate to the 'Administrator' interface
13. Write in the URL 'cgi-bin/koha/admin/aqplan.pl'
14. There should be no records showing
15. If you click on Planning->Plan by Months testfund will be displayed
16. Repeat step 14 but click on Planning->Plan by libraries and
Planning->Plan by Itemtypes, Both of these pages should also show
'testfund'
17. In your terminal navigate from your Koha instance root directory to
t/db_dependent
18. Enter koha shell: sudo koha-shell <instancename>
19. Run AuthorisedValues unit tests: prove -v AuthorisedValues.t
20. The tests should run successful
21. Exit the koha shell
22. cd into the 'koha' directory from where you are
23. Enter the koha shell and run the ItemTypes.t and Libraries.t unit
tests by writing in: prove -v filename.t (both of which should be
successful)

Sponsored-by: Catalyst IT
Comment 20 Alex Buckley 2017-08-28 20:57:51 UTC
Created attachment 66571 [details] [review]
Bug 18212 - Followup: Fixed POD in AuthorisedValue.pm and ItemType.pm

Passes QA test now, and all unit tests pass

Test plan:

1. Apply all the patches attached to this bug report
2. Restart memcached: sudo service memcached restart
3. Drop and recreate the Koha instance database
4. Go through the web installer, selecting all data to be installed in
step 3
5. After the web installer is finished log in with the Koha database
administrator credentials
6. Create yourself a patron account
7. Set the patron account to have superlibrarian privileges
8. Log out and back in as your newly created patron
9. Navigate to the Acquisitions interface
10. Create a budget named 'testbudget'
11. Create a fund named 'testfund'
12. Navigate to the 'Administrator' interface
13. Write in the URL 'cgi-bin/koha/admin/aqplan.pl'
14. There should be no records showing
15. If you click on Planning->Plan by Months testfund will be displayed
16. Repeat step 14 but click on Planning->Plan by libraries and
Planning->Plan by Itemtypes, Both of these pages should also show
'testfund'
17. In your terminal navigate from your Koha instance root directory to
t/db_dependent
18. Enter koha shell: sudo koha-shell <instancename>
19. Run AuthorisedValues unit tests: prove -v AuthorisedValues.t
20. The tests should run successful
21. Exit the koha shell
22. cd into the 'koha' directory from where you are
23. Enter the koha shell and run the ItemTypes.t and Libraries.t unit tests by writing in: prove -v filename.t (both of which should be successful)

Sponsored-by: Catalyst IT
Comment 21 Alex Buckley 2017-08-28 21:04:08 UTC
QA test pass now with these two patches:

 OK	C4/Acquisition.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	Koha/AuthorisedValue.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	Koha/ItemType.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	Koha/Libraries.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	admin/aqplan.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	t/db_dependent/AuthorisedValues.t
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	t/db_dependent/Koha/ItemTypes.t
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid

 OK	t/db_dependent/Koha/Libraries.t
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   OK	  valid
Comment 22 Josef Moravec 2017-10-29 21:28:09 UTC
Comment on attachment 66570 [details] [review]
Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files

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

::: C4/Acquisition.pm
@@ +3055,5 @@
> +            });
> +    return $authvalues;
> +}
> +
> +

You don't need this, you use Koha::AuthorisedValues module directly

::: Koha/AuthorisedValue.pm
@@ +174,5 @@
> +         order_by => [qw/ lib /]
> +    });
> +    return @Getauthorised_values;
> +};
> +

You don't need this, you use Koha::AuthorisedValues module directly

@@ +187,5 @@
> +    Koha::AuthorisedValues->new()->search({
> +             category => { 'like','%a%'},
> +     });
> +    return @distinctauthorised_values;
> +};

You don't need this, you use Koha::AuthorisedValues module directly

::: Koha/ItemType.pm
@@ +116,5 @@
> +     my $sth   = $dbh->prepare($query);
> +     $sth->execute();
> +     return $sth;
> +}
> +

You don't need this, just use Koha::ItemTypes->search_with_localization

::: Koha/Libraries.pm
@@ +65,5 @@
> +    my $sth   = $dbh->prepare($query);
> +    $sth->execute();
> +    return $sth;
> +}
> +

Use Koha::Libraries->search for getting all libraries.

::: admin/aqplan.pl
@@ +110,4 @@
>  my $budgets_ref = GetBudgetHierarchy( $budget_period_id, $show_mine?$template->{VARS}->{'USER_INFO'}->{'branchcode'}:'', $show_mine?$template->{VARS}->{'USER_INFO'}->{'borrowernumber'}:'' );
>  
>  # build categories list
> +my @DistinctAuthValues = Koha::AuthorisedValue->GetDistinctCat();

should be something like:

my $acq_categories = Koha::AuthorisedValueCategories->search( { category => { like => 'A%' } } );

@@ +116,5 @@
>  my @category_list;
>  
>  # a hash, to check that some hardcoded categories exist.
>  my %categories;
> +while ( my ($category) = each @DistinctAuthValues ) {

while ( my $category = $acq_categories->next ) {

@@ +191,4 @@
>  }
>  # ------------------------------------------------------------
>  if ( $authcat =~ m/^Asort/ ) {
> +    my @AuthValues = Koha::AuthorisedValue->GetAuthValues($authcat);

my $AuthValues = Koha::AuthorisedValues->search ( { category => $authcat } );

@@ +225,4 @@
>  }
>  
>  elsif ( $authcat eq 'ITEMTYPES' ) {
> +     my $sth = Koha::ItemType->GetItemTypes($dbh);

Koha::ItemTypes->search_with_localization

@@ +237,4 @@
>  
>  } elsif ( $authcat eq 'BRANCHES' ) {
>  
> +    my $sth = Koha::Libraries->GetBranches($dbh);

Koha::Libraries->search

@@ +249,3 @@
>  } elsif ($authcat) {
> +
> +    my $sth = Koha::AuthorisedValue->GetAuthValues($authcat,$dbh);

Koha::AuthorisedValues->search ( { category => $authcat } );

::: t/db_dependent/AuthorisedValues.t
@@ +100,5 @@
> +   {
> +        order_by => [qw/ lib /]
> +   });
> +};
> +

If you want to test some subroutine, you can't just redefine it in the test. You need to use it from the place where it is defined... but you do not need this sub routine at all, so no need to add this test

@@ +111,5 @@
> +   Koha::AuthorisedValues->new()->search({
> +      category => { 'like','%a%'},
> +   });
> +}
> +

If you want to test some subroutine, you can't just redefine it in the test. You need to use it from the place where it is defined... but you do not need this sub routine at all, so no need to add this test

::: t/db_dependent/Koha/ItemTypes.t
@@ +126,5 @@
> +    my $sth   = $dbh->prepare($query);
> +    $sth->execute();
> +    return $sth->rows;
> +}
> +

If you want to test some subroutine, you can't just redefine it in the test. You need to use it from the place where it is defined... but you do not need this sub routine at all, so no need to add this test

::: t/db_dependent/Koha/Libraries.t
@@ +92,5 @@
> +    my $sth   = $dbh->prepare($query);
> +    $sth->execute();
> +    return $sth->rows;
> +}
> +

If you want to test some subroutine, you can't just redefine it in the test. You need to use it from the place where it is defined... but you do not need this sub routine at all, so no need to add this test