Created attachment 22863 [details] [review]
Bug 7567 - News by Branch

Created attachment 22917 [details] [review]
Bug 7567 - News by Branch

Created attachment 22920 [details] [review]
Bug 7567 - News by Branch

Created attachment 23350 [details] [review]
Bug 7567 - News by Branch

By selecting a particular branch, news specific to a branch
can be showed once the user logs into the opac (or the staff)
client.

This includes adding a non-NULL default '' branchcode field
to the opac_news table.

When a user is logged in they receive both '' and their branch
news. When no user is logged in, they only receive '' news. ''
means all branches.

TEST PLAN
---------
 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Go to Tools->News
    - Are the locations properly display?
 6) Add a News item, for 'All' interfaces for 'All Branches'.
 7) Add a News item, for 'Slip' interfaces at your user branch.
 8) Add a News item, for 'OPAC' interface at your user branch.
 9) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
10) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
11) Find a patron to print a slip for. Print Slip.
    - Does both the 'All' and 'Slip' news show up on the print slip?
12) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
          (Only one of the new things added should be displayed)
13) Log in as a user with the branch you added the news item for.
    - Does that piece of News display now too?
          (Both things added should be displayed)
14) Check the news items added and click the delete button in the staff client.
    - Did it delete them appropriately?

Created attachment 23351 [details] [review]
Bug 7567 - News by Branch

By selecting a particular branch, news specific to a branch
can be showed once the user logs into the opac (or the staff)
client.

This includes adding a non-NULL default '' branchcode field
to the opac_news table.

When a user is logged in they receive both '' and their branch
news. When no user is logged in, they only receive '' news. ''
means all branches.

TEST PLAN
---------
 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Go to Tools->News
    - Are the locations properly display?
 6) Add a News item, for 'All' interfaces for 'All Branches'.
 7) Add a News item, for 'Slip' interfaces at your user branch.
 8) Add a News item, for 'OPAC' interface at your user branch.
 9) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
10) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
11) Find a patron to print a slip for. Print Slip.
    - Does both the 'All' and 'Slip' news show up on the print slip?
12) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
          (Only one of the new things added should be displayed)
13) Log in as a user with the branch you added the news item for.
    - Does that piece of News display now too?
          (Both things added should be displayed)
14) Check the news items added and click the delete button in the staff client.
    - Did it delete them appropriately?

Created attachment 23352 [details] [review]
Bug 7567 - News by Branch

By selecting a particular branch, news specific to a branch
can be showed once the user logs into the opac (or the staff)
client.

This includes adding a non-NULL default '' branchcode field
to the opac_news table.

When a user is logged in they receive both '' and their branch
news. When no user is logged in, they only receive '' news. ''
means all branches.

TEST PLAN
---------
 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Go to Tools->News
    - Are the locations properly display?
 6) Add a News item, for 'All' interfaces for 'All Branches'.
 7) Add a News item, for 'Slip' interfaces at your user branch.
 8) Add a News item, for 'OPAC' interface at your user branch.
 9) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
10) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
11) Find a patron to print a slip for. Print Slip.
    - Does both the 'All' and 'Slip' news show up on the print slip?
12) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
          (Only one of the new things added should be displayed)
13) Log in as a user with the branch you added the news item for.
    - Does that piece of News display now too?
          (Both things added should be displayed)
14) Check the news items added and click the delete button in the staff client.
    - Did it delete them appropriately?

Created attachment 23362 [details] [review]
Bug 7567 - News by Branch

By selecting a particular branch, news specific to a branch
can be showed once the user logs into the opac (or the staff)
client.

This includes adding a non-NULL default '' branchcode field
to the opac_news table.

When a user is logged in they receive both '' and their branch
news. When no user is logged in, they only receive '' news. ''
means all branches.

REVISED TEST PLAN
-----------------
 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Add a new dummy branch (so it has no items or borrowers)
 6) Go to Tools->News
    - Are the locations properly display?
 7) Add a News item, for 'All' interfaces for 'All Branches'.
 8) Add a News item, for 'Librarian interface' interfaces for
     the dummy branch.
 9) Add a News item, for 'Slip' interfaces at your user branch.
10) Add a News item, for 'OPAC' interface at your user branch.
11) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
12) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
13) Find a patron to print a slip for. Print Slip.
    - Does both the 'All' and 'Slip' news show up on the print slip?
14) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
          (Only one of the new things added should be displayed)
15) Log in as yourself on OPAC.
    - Does that piece of News display now too?
          (Both things added should be displayed)
16) In the staff client, delete the dummy branch.
    - Does the 'Librarian interface' news item exist any more?
      It shouldn't.
17) Check the news items added and click the delete button in the
     staff client.
    - Did it delete them appropriately?

Created attachment 23368 [details] [review]
Bug 7567 - News by Branch

By selecting a particular branch, news specific to a branch
can be showed once the user logs into the opac (or the staff)
client.

This includes adding a non-NULL default '' branchcode field
to the opac_news table.

When a user is logged in they receive both '' and their branch
news. When no user is logged in, they only receive '' news. ''
means all branches.

REVISED TEST PLAN
-----------------
 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Add a new dummy branch (so it has no items or borrowers)
 6) Go to Tools->News
    - Are the locations properly display?
 7) Add a News item, for 'All' interfaces for 'All Branches'.
 8) Add a News item, for 'Librarian interface' interfaces for
     the dummy branch.
 9) Add a News item, for 'Slip' interfaces at your user branch.
10) Add a News item, for 'OPAC' interface at your user branch.
11) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
12) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
13) Find a patron to print a slip for. Print Slip.
    - Does both the 'All' and 'Slip' news show up on the print slip?
14) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
          (Only one of the new things added should be displayed)
15) Log in as yourself on OPAC.
    - Does that piece of News display now too?
          (Both things added should be displayed)
16) In the staff client, delete the dummy branch.
    - Does the 'Librarian interface' news item exist any more?
      It shouldn't.
17) Check the news items added and click the delete button in the
     staff client.
    - Did it delete them appropriately?

Created attachment 23378 [details] [review]
Bug 7567 - News by Branch

By selecting a particular branch, news specific to a branch
can be showed once the user logs into the opac (or the staff)
client.

This includes adding a non-NULL default '' branchcode field
to the opac_news table.

When a user is logged in they receive both '' and their branch
news. When no user is logged in, they only receive '' news. ''
means all branches.

REVISED TEST PLAN
-----------------
 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Add a new dummy branch (so it has no items or borrowers)
 6) Go to Tools->News
    - Are the locations properly display?
 7) Add a News item, for 'All' interfaces for 'All Branches'.
 8) Add a News item, for 'Librarian interface' interfaces for
     the dummy branch.
 9) Add a News item, for 'Slip' interfaces at your user branch.
10) Add a News item, for 'OPAC' interface at your user branch.
11) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
12) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
13) Find a patron to print a slip for. Print Slip.
    - Does both the 'All' and 'Slip' news show up on the print slip?
14) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
          (Only one of the new things added should be displayed)
15) Log in as yourself on OPAC.
    - Does that piece of News display now too?
          (Both things added should be displayed)
16) In the staff client, delete the dummy branch.
    - Does the 'Librarian interface' news item exist any more?
      It shouldn't.
17) Check the news items added and click the delete button in the
     staff client.
    - Did it delete them appropriately?

Created attachment 23383 [details] [review]
Bug 7567 - News by Branch

By selecting a particular branch, news specific to a branch
can be showed once the user logs into the opac (or the staff)
client.

This includes adding a non-NULL default '' branchcode field
to the opac_news table.

When a user is logged in they receive both '' and their branch
news. When no user is logged in, they only receive '' news. ''
means all branches.

REVISED TEST PLAN
-----------------
 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Add a new dummy branch (so it has no items or borrowers)
 6) Go to Tools->News
    - Are the locations properly display?
 7) Add a News item, for 'All' interfaces for 'All Branches'.
 8) Add a News item, for 'Librarian interface' interfaces for
     the dummy branch.
 9) Add a News item, for 'Slip' interfaces at your user branch.
10) Add a News item, for 'OPAC' interface at your user branch.
11) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
12) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
13) Find a patron to print a slip for. Print Slip.
    - Does both the 'All' and 'Slip' news show up on the print slip?
14) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
          (Only one of the new things added should be displayed)
15) Log in as yourself on OPAC.
    - Does that piece of News display now too?
          (Both things added should be displayed)
16) In the staff client, delete the dummy branch.
    - Does the 'Librarian interface' news item exist any more?
      It shouldn't.
17) Check the news items added and click the delete button in the
     staff client.
    - Did it delete them appropriately?

Created attachment 23386 [details] [review]
Bug 7567 - News by Branch

By selecting a particular branch, news specific to a branch
can be showed once the user logs into the opac (or the staff)
client.

This includes adding a non-NULL default '' branchcode field
to the opac_news table.

When a user is logged in they receive both '' and their branch
news. When no user is logged in, they only receive '' news. ''
means all branches.

REVISED TEST PLAN
-----------------
 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Add a new dummy branch (so it has no items or borrowers)
 6) Go to Tools->News
    - Are the locations properly display?
 7) Add a News item, for 'All' interfaces for 'All Branches'.
 8) Add a News item, for 'Librarian interface' interfaces for
     the dummy branch.
 9) Add a News item, for 'Slip' interfaces at your user branch.
10) Add a News item, for 'OPAC' interface at your user branch.
11) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
12) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
13) Find a patron to print a slip for. Print Slip.
    - Does both the 'All' and 'Slip' news show up on the print slip?
14) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
          (Only one of the new things added should be displayed)
15) Log in as yourself on OPAC.
    - Does that piece of News display now too?
          (Both things added should be displayed)
16) In the staff client, delete the dummy branch.
    - Does the 'Librarian interface' news item exist any more?
      It shouldn't.
17) Check the news items added and click the delete button in the
     staff client.
    - Did it delete them appropriately?

Created attachment 23474 [details] [review]
Bug 7567 - News by Branch

By selecting a particular branch, news specific to a branch
can be showed once the user logs into the opac (or the staff)
client.

This includes adding a non-NULL default '' branchcode field
to the opac_news table.

When a user is logged in they receive both '' and their branch
news. When no user is logged in, they only receive '' news. ''
means all branches.

REVISED TEST PLAN
-----------------
 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
    - Did you try for something non-English?
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Add a new dummy branch (so it has no items or borrowers)
 6) Go to Tools->News
    - Are the locations properly display?
 7) Add a News item, for 'All' interfaces for 'All Branches'.
 8) Add a News item, for 'Librarian interface' interfaces for
     the dummy branch.
 9) Add a News item, for 'Slip' interfaces at your user branch.
10) Add a News item, for 'OPAC' interface at your user branch.
11) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
12) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
13) Find a patron to print a slip for. Print Slip.
    - Does both the 'All' and 'Slip' news show up on the print slip?
14) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
          (Only one of the new things added should be displayed)
15) Log in as yourself on OPAC.
    - Does that piece of News display now too?
          (Both things added should be displayed)
16) In the staff client, delete the dummy branch.
    - Does the 'Librarian interface' news item exist any more?
      It shouldn't.
17) Check the news items added and click the delete button in the
     staff client.
    - Did it delete them appropriately?

Created attachment 23475 [details] [review]
Bug 7567 - News by Branch

By selecting a particular branch, news specific to a branch
can be showed once the user logs into the opac (or the staff)
client.

This includes adding a NULL default branchcode field
to the opac_news table.

When a user is logged in they receive both NULL and their branch
news. When no user is logged in, they only receive NULL news.
NULL means all branches.

C4::NewsChannels problems that were raised by Katrin Fischer
have been corrected by modifying the template file, and
removing the hardcoded strings.

I have revised all the sample_news.sql files, including adding
setting utf8, disabling and reenabling the foreign keys, and
removing explicit references to the idnew field. The idnew field
is autogenerated. This allowed me to source all the sql files and
get all of them added into my MySQL DB.

And as per Katrin Fischer's suggestion, I added AFTER into the
ALTER command, so that the fresh install and update install should
generate identical field orders.

REVISED TEST PLAN
-----------------
 1) Do a fresh install, to ensure that branchcode is created as a field
     in the opac_news table.
    - Did you try for something non-English?
 2) Do an upgrade to confirm that the branchcode field is added
     to the opac_news table.
 3) prove -v t/db_dependent/NewsChannels.t
     This should test all the changes in C4::NewsChannels functions.
 4) Log into the staff client
    - Does the News display on the main page bust?
 5) Add a new dummy branch (so it has no items or borrowers)
 6) Go to Tools->News
    - Are the locations properly display?
 7) Add a News item, for 'All' interfaces for 'All Branches'.
 8) Add a News item, for 'Librarian interface' interfaces for
     the dummy branch.
 9) Add a News item, for 'Slip' interfaces at your user branch.
10) Add a News item, for 'OPAC' interface at your user branch.
11) Change the Location and Branch filters.
    - Do they show what you select after you click the Filter button?
          (Unpatched the Location does not!)
    - Are the items displayed matching what is selected?
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
12) Click 'Edit' for one of the news items added.
    - Does what comes up match what was displayed?
          ( Unpatched the Location may be 'All' )
    - Are the drop downs containing the expected values?
          (All, Librarian Interface, Slip,
            and OPAC (<lang> -- for each <lang> installed)
          (All Branches, and every branch listed)
    - Does the submitted values match the changes made?
13) Find a patron to print a slip for. Print Slip.
    - Does both the 'All' and 'Slip' news show up on the print slip?
14) In a new tab, go to OPAC
    - Does the News display on the main page bust?
    - Does it display the news item for a specific branch?
          (It shouldn't when patched, but unpatched it would)
          (Only one of the new things added should be displayed)
15) Log in as yourself on OPAC.
    - Does that piece of News display now too?
          (Both things added should be displayed)
16) In the staff client, delete the dummy branch.
    - Does the 'Librarian interface' news item exist any more?
      It shouldn't.
17) Check the news items added and click the delete button in the
     staff client.
    - Did it delete them appropriately?

Created attachment 23501 [details] [review]
Bug 7567 - Testing for C4/NewsChannel.pm

Testing was lacking, this tests every function call. As a
side effect, this moved the stub of a test file from
t/NewsChannels.t to t/db_dependent/NewsChannels.t, since the
table opac_news must exist.

Created attachment 23544 [details] [review]
Bug 7567 - Update License and use Modern::Perl

Grabbed the current license from
  http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
and changed the use strict; use warnings; into a
use Modern::Perl instead.

TEST PLAN
---------
1) Log into staff client.
    - Does news look okay?
2) Apply patch
3) Refresh staff client.
    - Does news look the same?

Created attachment 23547 [details] [review]
Bug 7567 - Update License tools/koha-news.pl

Updated license text using
    http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
for the base text.

TEST PLAN
---------
1) less tools/koha-news.pl
2) apply patch
3) less tools/koha-news.pl
    - was any contributor information lost?

Created attachment 23552 [details] [review]
Bug 7567 - Clean up sample news files

Some files had truncate. Some files had foreign key checks.
Other files had foreign key checks disabled. Some files gave
the column names expressly, and others assumed the fields in
a particular order. Everything was modified to:
- not truncate
- not say utf8 (because it should already be by default)
- not disable foreign key checks (because it should work)
- expressly name the fields (adding fields shouldn't break it)

TEST PLAN
---------
0) Backup opac_news if you care about your news.
1) open mysql client
2) use the koha database
3) Run the following script
TRUNCATE opac_news;
source ~/kohaclone/installer/data/mysql/uk-UA/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/it-IT/necessari/sample_news.sql
source ~/kohaclone/installer/data/mysql/de-DE/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/nb-NO/2-Valgfritt/sample_news.sql
source ~/kohaclone/installer/data/mysql/en/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/pl-PL/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/es-ES/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/ru-RU/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/fr-FR/2-Optionel/sample_news.sql

- Were there errors?
- Did everything add nicely?
    - Not likely, by expressly stating the idnew field, the news
      items in some files would not be added.

4) apply patch
5) try running the script again

- Were there errors?
- Did everything add nicely?
6) Log into the staff client
7) Tools->News
- Do you see the news items added?

Created attachment 23553 [details] [review]
Bug 7567 - Undefined $lang triggers warning

Changed:
    $template->param(
        $lang           => 1,
        opac_news       => $opac_news,
        opac_news_count => $opac_news_count,
                );
Into:
    $template->param(
        opac_news       => $opac_news,
        opac_news_count => $opac_news_count,
                );
    $template->param( $lang => 1 ) if $lang;

Because $lang is not defined when you come immediately into
Home -> Tools -> News.

TEST PLAN
---------
1) log into staff client
2) Tools -> News
3) Check koha error log
   - Expecting to see an error like:
koha-news.pl: Problem = a value of 1 has been passed to param without key
4) Apply patch
5) Home -> Tools -> News
6) Check koha error log
   - No error generated

Created attachment 23554 [details] [review]
Bug 7567 - Correct Filtering and Default dropdown values

In the staff client, when you went to Home -> Tools -> News
the default 'Display location' is 'All'. Everything is correctly
displayed. However, if you change the filter it filters the
table correctly, but does not set the dropdown value to match.
This patch corrects this.

TEST PLAN
---------
 1) Home -> Tools -> News
 2) Change 'Display location' value and click 'Filter'.
    - Is the 'Display location' value the same now?
      (no)
    - Are the correct values displayed?
 3) Click 'New entry'
    - Is it anything other than 'All'?
      (no, and is this correct?)
 4) Click 'Cancel'
 5) Click 'Edit' for various news items with different locations.
    - Does the 'Display location' get proper set?
      (no)
 6) Apply patch
 7) Change the 'Display location' value and click 'Filter'.
    - Does the 'Display location' value stay the same?
      (this should be correct now)
    - Are the correct values displayed?
 8) Change the 'Display location' value to 'All' and click 'Filter'.
    - Does everything display?
 9) Change the 'Display location' value and click 'Filter'.
10) Click 'New entry'
    - Does it match what the filter was?
      (should match now)
11) Click 'Cancel'
12) Click 'Edit' for various news items with different locations.
    - Does the 'Display location' get proper set?
      (this should be correct now)

Created attachment 23559 [details] [review]
Bug 7567 - Removed finish calls

"When all the data has been fetched from a SELECT statement,
the driver will automatically call finish for you. So you should
not call it explicitly except when you know that you've not
fetched all the data from a statement handle and the handle
won't be destroyed soon."
(http://search.cpan.org/~timb/DBI-1.627/DBI.pm#finish)

All the $sth variables were scoped within the functions,
and would be destroyed immediately. Additionally, there was
one after a SELECT, for only a single idnew, and so it was
not necessary.

TEST PLAN
---------
1) prove -v t/db_dependent/NewsChannels.t
2) apply patch
3) prove -v t/db_dependent/NewsChannels.t

Created attachment 23664 [details] [review]
Bug 7567 - Too many parameters and tiny bugs

Changed the add and update functions to use a hash reference
for the parameter, so that adding or subtracting parameters
should be easier. Added some POD for the add_opac_news and
upd_opac_news functions, so that developers would know how to
call it.

The hashref changes resulted in being able to return 0 for
failure and 1 for success. This meant adding a couple tests
to the test file.

And while testing, there was some sort of logic problem with
the matter of '' being all, but selecting all only showed
things set for all, and excluded particular languages, or other
interfaces.

Created attachment 23703 [details] [review]
Bug 7567 - Added branchcode to opac_news

This patch merely adds branchcode varchar(10) DEFAULT NULL to
the opac_news table.

TEST PLAN
---------
 1) backup your kohadata base if you care about the data.
 2) use the koha database
 3) describe opac_news;
 4) show create table opac_news;
    -- No branchcode constraint will exist.
 5) apply the patch
 6) upgrade the database (either staff client or script)
 7) use the koha database
 8) describe opac_news;
 9) show create table opac_news;
    -- The branchcode constraint should be listed.
10) drop that koha database
11) create the koha database
12) use the koha database
13) source ~/kohaclone/installer/data/mysql/kohastructure.sql
    -- there should be no errors in creating the database.
14) describe opac_news;
15) show create table opac_news;
    -- The branchcode constraint should be listed.
16) restore your koha database if you backed it up.

Created attachment 23704 [details] [review]
Bug 7567 - Use, Display, Filter News by Branch

With the branch parameter added to key functions in
C4/NewsChannels.pm, function calls in C4/Members.pm,
mainpage.pl, opac/opac-main.pl, tools/koha-news.pl, and
t/db_dependent/NewsChannels.t were needed.

Some license texts were updated.

Templates were modified to display, allow for entry and editing
of the branches selected.

TEST PLAN
---------
1) Having logged into the staff client, is the news displaying
   correctly? Have you entered a news item which should not
   display for this branch of logged in user?
2) Find a patron (with some items checked out?)
3) Print a slip
   - News which is labelled 'All Branches' or for the same branch
     as the one printing the slip should display on the slip.
   - THIS DOES NOT AFFECT QUICK SLIPS
4) Home -> Tools -> News
   - Can you edit a news item?
   - Does the change save correctly?
   - Can you filter based on location and branch correctly?
   - Can you add a new entry correctly?
   - Can you delete an entry correctly?
5) Open an OPAC client.
   - Does only the news for all branches display?
6) Log into the OPAC client.
   - Does the news for all branches and the specific branch display?
7) prove -v t/db_dependent/NewsChannels.t
   - Does it run and all succeed?
   - Does the code seem to catch the required cases?
8) Comparing the patched and unpatched versions of files affected,
   are the license changes missing anything?

Created attachment 23705 [details] [review]
Bug 7567 - Use, Display, Filter News by Branch

With the branch parameter added to key functions in
C4/NewsChannels.pm, function calls in C4/Members.pm,
mainpage.pl, opac/opac-main.pl, tools/koha-news.pl, and
t/db_dependent/NewsChannels.t were needed.

Some license texts were updated.

Templates were modified to display, allow for entry and editing
of the branches selected.

TEST PLAN
---------
1) Having logged into the staff client, is the news displaying
   correctly? Have you entered a news item which should not
   display for this branch of logged in user?
2) Find a patron (with some items checked out?)
3) Print a slip
   - News which is labelled 'All Branches' or for the same branch
     as the one printing the slip should display on the slip.
   - THIS DOES NOT AFFECT QUICK SLIPS
4) Home -> Tools -> News
   - Can you edit a news item?
   - Does the change save correctly?
   - Can you filter based on location and branch correctly?
   - Can you add a new entry correctly?
   - Can you delete an entry correctly?
5) Open an OPAC client.
   - Does only the news for all branches display?
6) Log into the OPAC client.
   - Does the news for all branches and the specific branch display?
7) prove -v t/db_dependent/NewsChannels.t
   - Does it run and all succeed?
   - Does the code seem to catch the required cases?
8) Comparing the patched and unpatched versions of files affected,
   are the license changes missing anything?

Created attachment 25316 [details] [review]
Bug 7567 - Testing for C4/NewsChannel.pm

Testing was lacking, this tests every function call. As a
side effect, this moved the stub of a test file from
t/NewsChannels.t to t/db_dependent/NewsChannels.t, since the
table opac_news must exist.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Passes all 8 tests

Created attachment 25317 [details] [review]
Bug 7567 - Update License and use Modern::Perl

Grabbed the current license from
  http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
and changed the use strict; use warnings; into a
use Modern::Perl instead.

TEST PLAN
---------
1) Log into staff client.
    - Does news look okay?
2) Apply patch
3) Refresh staff client.
    - Does news look the same?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Safe no op action

Created attachment 25318 [details] [review]
Bug 7567 - Update License tools/koha-news.pl

Updated license text using
    http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
for the base text.

TEST PLAN
---------
1) less tools/koha-news.pl
2) apply patch
3) less tools/koha-news.pl
    - was any contributor information lost?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Safe no op action

Created attachment 25319 [details] [review]
Bug 7567 - Clean up sample news files

Some files had truncate. Some files had foreign key checks.
Other files had foreign key checks disabled. Some files gave
the column names expressly, and others assumed the fields in
a particular order. Everything was modified to:
- not truncate
- not say utf8 (because it should already be by default)
- not disable foreign key checks (because it should work)
- expressly name the fields (adding fields shouldn't break it)

TEST PLAN
---------
0) Backup opac_news if you care about your news.
1) open mysql client
2) use the koha database
3) Run the following script
TRUNCATE opac_news;
source ~/kohaclone/installer/data/mysql/uk-UA/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/it-IT/necessari/sample_news.sql
source ~/kohaclone/installer/data/mysql/de-DE/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/nb-NO/2-Valgfritt/sample_news.sql
source ~/kohaclone/installer/data/mysql/en/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/pl-PL/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/es-ES/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/ru-RU/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/fr-FR/2-Optionel/sample_news.sql

- Were there errors?
- Did everything add nicely?
    - Not likely, by expressly stating the idnew field, the news
      items in some files would not be added.

4) apply patch
5) try running the script again

- Were there errors?
- Did everything add nicely?
6) Log into the staff client
7) Tools->News
- Do you see the news items added?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Much needed clean up, making for much more robust and less error prone
sql

Created attachment 25320 [details] [review]
Bug 7567 - Undefined $lang triggers warning

Changed:
    $template->param(
        $lang           => 1,
        opac_news       => $opac_news,
        opac_news_count => $opac_news_count,
                );
Into:
    $template->param(
        opac_news       => $opac_news,
        opac_news_count => $opac_news_count,
                );
    $template->param( $lang => 1 ) if $lang;

Because $lang is not defined when you come immediately into
Home -> Tools -> News.

TEST PLAN
---------
1) log into staff client
2) Tools -> News
3) Check koha error log
   - Expecting to see an error like:
koha-news.pl: Problem = a value of 1 has been passed to param without key
4) Apply patch
5) Home -> Tools -> News
6) Check koha error log
   - No error generated

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 25321 [details] [review]
Bug 7567 - Correct Filtering and Default dropdown values

In the staff client, when you went to Home -> Tools -> News
the default 'Display location' is 'All'. Everything is correctly
displayed. However, if you change the filter it filters the
table correctly, but does not set the dropdown value to match.
This patch corrects this.

TEST PLAN
---------
 1) Home -> Tools -> News
 2) Change 'Display location' value and click 'Filter'.
    - Is the 'Display location' value the same now?
      (no)
    - Are the correct values displayed?
 3) Click 'New entry'
    - Is it anything other than 'All'?
      (no, and is this correct?)
 4) Click 'Cancel'
 5) Click 'Edit' for various news items with different locations.
    - Does the 'Display location' get proper set?
      (no)
 6) Apply patch
 7) Change the 'Display location' value and click 'Filter'.
    - Does the 'Display location' value stay the same?
      (this should be correct now)
    - Are the correct values displayed?
 8) Change the 'Display location' value to 'All' and click 'Filter'.
    - Does everything display?
 9) Change the 'Display location' value and click 'Filter'.
10) Click 'New entry'
    - Does it match what the filter was?
      (should match now)
11) Click 'Cancel'
12) Click 'Edit' for various news items with different locations.
    - Does the 'Display location' get proper set?
      (this should be correct now)

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 25322 [details] [review]
Bug 7567 - Removed finish calls

"When all the data has been fetched from a SELECT statement,
the driver will automatically call finish for you. So you should
not call it explicitly except when you know that you've not
fetched all the data from a statement handle and the handle
won't be destroyed soon."
(http://search.cpan.org/~timb/DBI-1.627/DBI.pm#finish)

All the $sth variables were scoped within the functions,
and would be destroyed immediately. Additionally, there was
one after a SELECT, for only a single idnew, and so it was
not necessary.

TEST PLAN
---------
1) prove -v t/db_dependent/NewsChannels.t
2) apply patch
3) prove -v t/db_dependent/NewsChannels.t

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

ok 1 - use C4::NewsChannels;
ok 2 - Successfully added the first dummy news item!
ok 3 - Successfully added the second dummy news item!
ok 4 - Successfully updated second dummy news item!
ok 5 - Successfully tested get_opac_new id1!
ok 6 - Successfully tested get_opac_new id2!
ok 7 - Successfully tested get_opac_news!
ok 8 - Successfully tested GetNewsToDisplay!

Created attachment 25323 [details] [review]
Bug 7567 - Too many parameters and tiny bugs

Changed the add and update functions to use a hash reference
for the parameter, so that adding or subtracting parameters
should be easier. Added some POD for the add_opac_news and
upd_opac_news functions, so that developers would know how to
call it.

The hashref changes resulted in being able to return 0 for
failure and 1 for success. This meant adding a couple tests
to the test file.

And while testing, there was some sort of logic problem with
the matter of '' being all, but selecting all only showed
things set for all, and excluded particular languages, or other
interfaces.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

1..10
ok 1 - use C4::NewsChannels;
ok 2 - Correctly failed on no parameter!
ok 3 - Successfully added the first dummy news item!
ok 4 - Successfully added the second dummy news item!
ok 5 - Correctly failed on no parameter!
ok 6 - Successfully updated second dummy news item!
ok 7 - Successfully tested get_opac_new id1 (0)!
ok 8 - Successfully tested get_opac_new id2 (0)!
ok 9 - Successfully tested get_opac_news!
ok 10 - Successfully tested GetNewsToDisplay!

My only nitpick here is the variable naming ... hashref is a really
bad name for a variable. But I will sign off, because apart from that
it is a good clean up

Created attachment 25324 [details] [review]
Bug 7567 - Added branchcode to opac_news

This patch merely adds branchcode varchar(10) DEFAULT NULL to
the opac_news table.

TEST PLAN
---------
 1) backup your kohadata base if you care about the data.
 2) use the koha database
 3) describe opac_news;
 4) show create table opac_news;
    -- No branchcode constraint will exist.
 5) apply the patch
 6) upgrade the database (either staff client or script)
 7) use the koha database
 8) describe opac_news;
 9) show create table opac_news;
    -- The branchcode constraint should be listed.
10) drop that koha database
11) create the koha database
12) use the koha database
13) source ~/kohaclone/installer/data/mysql/kohastructure.sql
    -- there should be no errors in creating the database.
14) describe opac_news;
15) show create table opac_news;
    -- The branchcode constraint should be listed.
16) restore your koha database if you backed it up.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 25325 [details] [review]
Bug 7567 - Use, Display, Filter News by Branch

With the branch parameter added to key functions in
C4/NewsChannels.pm, function calls in C4/Members.pm,
mainpage.pl, opac/opac-main.pl, tools/koha-news.pl, and
t/db_dependent/NewsChannels.t were needed.

Some license texts were updated.

Templates were modified to display, allow for entry and editing
of the branches selected.

TEST PLAN
---------
1) Having logged into the staff client, is the news displaying
   correctly? Have you entered a news item which should not
   display for this branch of logged in user?
2) Find a patron (with some items checked out?)
3) Print a slip
   - News which is labelled 'All Branches' or for the same branch
     as the one printing the slip should display on the slip.
   - THIS DOES NOT AFFECT QUICK SLIPS
4) Home -> Tools -> News
   - Can you edit a news item?
   - Does the change save correctly?
   - Can you filter based on location and branch correctly?
   - Can you add a new entry correctly?
   - Can you delete an entry correctly?
5) Open an OPAC client.
   - Does only the news for all branches display?
6) Log into the OPAC client.
   - Does the news for all branches and the specific branch display?
7) prove -v t/db_dependent/NewsChannels.t
   - Does it run and all succeed?
   - Does the code seem to catch the required cases?
8) Comparing the patched and unpatched versions of files affected,
   are the license changes missing anything?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

1..10
ok 1 - use C4::NewsChannels;
ok 2 - Correctly failed on no parameter!
ok 3 - Successfully added the first dummy news item!
ok 4 - Successfully added the second dummy news item!
ok 5 - Correctly failed on no parameter!
ok 6 - Successfully updated second dummy news item!
ok 7 - Successfully tested get_opac_new id1 (0)!
ok 8 - Successfully tested get_opac_new id2 (0)!
ok 9 - Successfully tested get_opac_news for LIB1!
ok 10 - Successfully tested GetNewsToDisplay for LIB1!

Created attachment 25331 [details] [review]
Bug 7567 - Schema update for opac_news

This was generated by:
Reset hard to master.
Restore old DB
Apply the patches for 7567
./installer/data/mysql/updatedatabase.pl
./misc/devel/update_dbix_class_files.pl -db_name=... -db_user=... -db_passwd=...
git add Koha/Schema/Results/OpacNews.pm

This should provide the necessary changes to the schema files for
Koha 3.16 and forward.

Created attachment 25332 [details] [review]
Bug 7567 - Schema update for opac_news

This was generated by:
Reset hard to master.
Restore old DB
Apply the patches for 7567
./installer/data/mysql/updatedatabase.pl
./misc/devel/update_dbix_class_files.pl -db_name=... -db_user=... -db_passwd=...
git add Koha/Schema/Results/OpacNews.pm

This should provide the necessary changes to the schema files for
Koha 3.16 and forward.

Had to generate under Debian, because Ubuntu's was broken by comparison.

Created attachment 25333 [details] [review]
Bug 7567 - Schema update for opac_news

This was generated by:
Reset hard to master.
Restore old DB
Apply the patches for 7567
./installer/data/mysql/updatedatabase.pl
./misc/devel/update_dbix_class_files.pl -db_name=... -db_user=... -db_passwd=...
git add Koha/Schema/Results/OpacNews.pm

This should provide the necessary changes to the schema files for
Koha 3.16 and forward.

Had to generate under Debian, because Ubuntu's was broken by comparison.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 26510 [details] [review]
Bug 7567 - Use, Display, Filter News by Branch

With the branch parameter added to key functions in
C4/NewsChannels.pm, function calls in C4/Members.pm,
mainpage.pl, opac/opac-main.pl, tools/koha-news.pl, and
t/db_dependent/NewsChannels.t were needed.

Some license texts were updated.

Templates were modified to display, allow for entry and editing
of the branches selected.

TEST PLAN
---------
1) Having logged into the staff client, is the news displaying
   correctly? Have you entered a news item which should not
   display for this branch of logged in user?
2) Find a patron (with some items checked out?)
3) Print a slip
   - News which is labelled 'All Branches' or for the same branch
     as the one printing the slip should display on the slip.
   - THIS DOES NOT AFFECT QUICK SLIPS
4) Home -> Tools -> News
   - Can you edit a news item?
   - Does the change save correctly?
   - Can you filter based on location and branch correctly?
   - Can you add a new entry correctly?
   - Can you delete an entry correctly?
5) Open an OPAC client.
   - Does only the news for all branches display?
6) Log into the OPAC client.
   - Does the news for all branches and the specific branch display?
7) prove -v t/db_dependent/NewsChannels.t
   - Does it run and all succeed?
   - Does the code seem to catch the required cases?
8) Comparing the patched and unpatched versions of files affected,
   are the license changes missing anything?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

1..10
ok 1 - use C4::NewsChannels;
ok 2 - Correctly failed on no parameter!
ok 3 - Successfully added the first dummy news item!
ok 4 - Successfully added the second dummy news item!
ok 5 - Correctly failed on no parameter!
ok 6 - Successfully updated second dummy news item!
ok 7 - Successfully tested get_opac_new id1 (0)!
ok 8 - Successfully tested get_opac_new id2 (0)!
ok 9 - Successfully tested get_opac_news for LIB1!
ok 10 - Successfully tested GetNewsToDisplay for LIB1!

Created attachment 26511 [details] [review]
Bug 7567 - Schema update for opac_news

This was generated by:
Reset hard to master.
Restore old DB
Apply the patches for 7567
./installer/data/mysql/updatedatabase.pl
./misc/devel/update_dbix_class_files.pl -db_name=... -db_user=... -db_passwd=...
git add Koha/Schema/Results/OpacNews.pm

This should provide the necessary changes to the schema files for
Koha 3.16 and forward.

Had to generate under Debian, because Ubuntu's was broken by comparison.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 26590 [details] [review]
Bug 7567 - Testing for C4/NewsChannel.pm

Testing was lacking, this tests every function call. As a
side effect, this moved the stub of a test file from
t/NewsChannels.t to t/db_dependent/NewsChannels.t, since the
table opac_news must exist.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Passes all 8 tests

Created attachment 26591 [details] [review]
Bug 7567 - Update License and use Modern::Perl

Grabbed the current license from
  http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
and changed the use strict; use warnings; into a
use Modern::Perl instead.

TEST PLAN
---------
1) Log into staff client.
    - Does news look okay?
2) Apply patch
3) Refresh staff client.
    - Does news look the same?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Safe no op action

Created attachment 26592 [details] [review]
Bug 7567 - Update License tools/koha-news.pl

Updated license text using
    http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
for the base text.

TEST PLAN
---------
1) less tools/koha-news.pl
2) apply patch
3) less tools/koha-news.pl
    - was any contributor information lost?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Safe no op action

Created attachment 26593 [details] [review]
Bug 7567 - Clean up sample news files

Some files had truncate. Some files had foreign key checks.
Other files had foreign key checks disabled. Some files gave
the column names expressly, and others assumed the fields in
a particular order. Everything was modified to:
- not truncate
- not say utf8 (because it should already be by default)
- not disable foreign key checks (because it should work)
- expressly name the fields (adding fields shouldn't break it)

TEST PLAN
---------
0) Backup opac_news if you care about your news.
1) open mysql client
2) use the koha database
3) Run the following script
TRUNCATE opac_news;
source ~/kohaclone/installer/data/mysql/uk-UA/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/it-IT/necessari/sample_news.sql
source ~/kohaclone/installer/data/mysql/de-DE/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/nb-NO/2-Valgfritt/sample_news.sql
source ~/kohaclone/installer/data/mysql/en/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/pl-PL/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/es-ES/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/ru-RU/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/fr-FR/2-Optionel/sample_news.sql

- Were there errors?
- Did everything add nicely?
    - Not likely, by expressly stating the idnew field, the news
      items in some files would not be added.

4) apply patch
5) try running the script again

- Were there errors?
- Did everything add nicely?
6) Log into the staff client
7) Tools->News
- Do you see the news items added?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Much needed clean up, making for much more robust and less error prone
sql

Created attachment 26594 [details] [review]
Bug 7567 - Undefined $lang triggers warning

Changed:
    $template->param(
        $lang           => 1,
        opac_news       => $opac_news,
        opac_news_count => $opac_news_count,
                );
Into:
    $template->param(
        opac_news       => $opac_news,
        opac_news_count => $opac_news_count,
                );
    $template->param( $lang => 1 ) if $lang;

Because $lang is not defined when you come immediately into
Home -> Tools -> News.

TEST PLAN
---------
1) log into staff client
2) Tools -> News
3) Check koha error log
   - Expecting to see an error like:
koha-news.pl: Problem = a value of 1 has been passed to param without key
4) Apply patch
5) Home -> Tools -> News
6) Check koha error log
   - No error generated

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 26595 [details] [review]
Bug 7567 - Correct Filtering and Default dropdown values

In the staff client, when you went to Home -> Tools -> News
the default 'Display location' is 'All'. Everything is correctly
displayed. However, if you change the filter it filters the
table correctly, but does not set the dropdown value to match.
This patch corrects this.

TEST PLAN
---------
 1) Home -> Tools -> News
 2) Change 'Display location' value and click 'Filter'.
    - Is the 'Display location' value the same now?
      (no)
    - Are the correct values displayed?
 3) Click 'New entry'
    - Is it anything other than 'All'?
      (no, and is this correct?)
 4) Click 'Cancel'
 5) Click 'Edit' for various news items with different locations.
    - Does the 'Display location' get proper set?
      (no)
 6) Apply patch
 7) Change the 'Display location' value and click 'Filter'.
    - Does the 'Display location' value stay the same?
      (this should be correct now)
    - Are the correct values displayed?
 8) Change the 'Display location' value to 'All' and click 'Filter'.
    - Does everything display?
 9) Change the 'Display location' value and click 'Filter'.
10) Click 'New entry'
    - Does it match what the filter was?
      (should match now)
11) Click 'Cancel'
12) Click 'Edit' for various news items with different locations.
    - Does the 'Display location' get proper set?
      (this should be correct now)

Created attachment 26596 [details] [review]
Bug 7567 - Removed finish calls

"When all the data has been fetched from a SELECT statement,
the driver will automatically call finish for you. So you should
not call it explicitly except when you know that you've not
fetched all the data from a statement handle and the handle
won't be destroyed soon."
(http://search.cpan.org/~timb/DBI-1.627/DBI.pm#finish)

All the $sth variables were scoped within the functions,
and would be destroyed immediately. Additionally, there was
one after a SELECT, for only a single idnew, and so it was
not necessary.

TEST PLAN
---------
1) prove -v t/db_dependent/NewsChannels.t
2) apply patch
3) prove -v t/db_dependent/NewsChannels.t

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

ok 1 - use C4::NewsChannels;
ok 2 - Successfully added the first dummy news item!
ok 3 - Successfully added the second dummy news item!
ok 4 - Successfully updated second dummy news item!
ok 5 - Successfully tested get_opac_new id1!
ok 6 - Successfully tested get_opac_new id2!
ok 7 - Successfully tested get_opac_news!
ok 8 - Successfully tested GetNewsToDisplay!

Created attachment 26597 [details] [review]
Bug 7567 - Too many parameters and tiny bugs

Changed the add and update functions to use a hash reference
for the parameter, so that adding or subtracting parameters
should be easier. Added some POD for the add_opac_news and
upd_opac_news functions, so that developers would know how to
call it.

The hashref changes resulted in being able to return 0 for
failure and 1 for success. This meant adding a couple tests
to the test file.

And while testing, there was some sort of logic problem with
the matter of '' being all, but selecting all only showed
things set for all, and excluded particular languages, or other
interfaces.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

1..10
ok 1 - use C4::NewsChannels;
ok 2 - Correctly failed on no parameter!
ok 3 - Successfully added the first dummy news item!
ok 4 - Successfully added the second dummy news item!
ok 5 - Correctly failed on no parameter!
ok 6 - Successfully updated second dummy news item!
ok 7 - Successfully tested get_opac_new id1 (0)!
ok 8 - Successfully tested get_opac_new id2 (0)!
ok 9 - Successfully tested get_opac_news!
ok 10 - Successfully tested GetNewsToDisplay!

My only nitpick here is the variable naming ... hashref is a really
bad name for a variable. But I will sign off, because apart from that
it is a good clean up

Created attachment 26598 [details] [review]
Bug 7567 - Added branchcode to opac_news

This patch merely adds branchcode varchar(10) DEFAULT NULL to
the opac_news table.

TEST PLAN
---------
 1) backup your kohadata base if you care about the data.
 2) use the koha database
 3) describe opac_news;
 4) show create table opac_news;
    -- No branchcode constraint will exist.
 5) apply the patch
 6) upgrade the database (either staff client or script)
 7) use the koha database
 8) describe opac_news;
 9) show create table opac_news;
    -- The branchcode constraint should be listed.
10) drop that koha database
11) create the koha database
12) use the koha database
13) source ~/kohaclone/installer/data/mysql/kohastructure.sql
    -- there should be no errors in creating the database.
14) describe opac_news;
15) show create table opac_news;
    -- The branchcode constraint should be listed.
16) restore your koha database if you backed it up.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 26599 [details] [review]
Bug 7567 - Use, Display, Filter News by Branch

With the branch parameter added to key functions in
C4/NewsChannels.pm, function calls in C4/Members.pm,
mainpage.pl, opac/opac-main.pl, tools/koha-news.pl, and
t/db_dependent/NewsChannels.t were needed.

Some license texts were updated.

Templates were modified to display, allow for entry and editing
of the branches selected.

TEST PLAN
---------
1) Having logged into the staff client, is the news displaying
   correctly? Have you entered a news item which should not
   display for this branch of logged in user?
2) Find a patron (with some items checked out?)
3) Print a slip
   - News which is labelled 'All Branches' or for the same branch
     as the one printing the slip should display on the slip.
   - THIS DOES NOT AFFECT QUICK SLIPS
4) Home -> Tools -> News
   - Can you edit a news item?
   - Does the change save correctly?
   - Can you filter based on location and branch correctly?
   - Can you add a new entry correctly?
   - Can you delete an entry correctly?
5) Open an OPAC client.
   - Does only the news for all branches display?
6) Log into the OPAC client.
   - Does the news for all branches and the specific branch display?
7) prove -v t/db_dependent/NewsChannels.t
   - Does it run and all succeed?
   - Does the code seem to catch the required cases?
8) Comparing the patched and unpatched versions of files affected,
   are the license changes missing anything?

Created attachment 26600 [details] [review]
Bug 7567 - Schema update for opac_news

This was generated by:
Reset hard to master.
Restore old DB
Apply the patches for 7567
./installer/data/mysql/updatedatabase.pl
./misc/devel/update_dbix_class_files.pl -db_name=... -db_user=... -db_passwd=...
git add Koha/Schema/Results/OpacNews.pm

This should provide the necessary changes to the schema files for
Koha 3.16 and forward.

Had to generate under Debian, because Ubuntu's was broken by comparison.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 26601 [details] [review]
Bug 7567 - Testing for C4/NewsChannel.pm

Testing was lacking, this tests every function call. As a
side effect, this moved the stub of a test file from
t/NewsChannels.t to t/db_dependent/NewsChannels.t, since the
table opac_news must exist.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Passes all 8 tests

Created attachment 26602 [details] [review]
Bug 7567 - Update License and use Modern::Perl

Grabbed the current license from
  http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
and changed the use strict; use warnings; into a
use Modern::Perl instead.

TEST PLAN
---------
1) Log into staff client.
    - Does news look okay?
2) Apply patch
3) Refresh staff client.
    - Does news look the same?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Safe no op action

Created attachment 26603 [details] [review]
Bug 7567 - Update License tools/koha-news.pl

Updated license text using
    http://wiki.koha-community.org/wiki/Coding_Guidelines#Licence
for the base text.

TEST PLAN
---------
1) less tools/koha-news.pl
2) apply patch
3) less tools/koha-news.pl
    - was any contributor information lost?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Safe no op action

Created attachment 26604 [details] [review]
Bug 7567 - Clean up sample news files

Some files had truncate. Some files had foreign key checks.
Other files had foreign key checks disabled. Some files gave
the column names expressly, and others assumed the fields in
a particular order. Everything was modified to:
- not truncate
- not say utf8 (because it should already be by default)
- not disable foreign key checks (because it should work)
- expressly name the fields (adding fields shouldn't break it)

TEST PLAN
---------
0) Backup opac_news if you care about your news.
1) open mysql client
2) use the koha database
3) Run the following script
TRUNCATE opac_news;
source ~/kohaclone/installer/data/mysql/uk-UA/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/it-IT/necessari/sample_news.sql
source ~/kohaclone/installer/data/mysql/de-DE/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/nb-NO/2-Valgfritt/sample_news.sql
source ~/kohaclone/installer/data/mysql/en/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/pl-PL/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/es-ES/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/ru-RU/optional/sample_news.sql
source ~/kohaclone/installer/data/mysql/fr-FR/2-Optionel/sample_news.sql

- Were there errors?
- Did everything add nicely?
    - Not likely, by expressly stating the idnew field, the news
      items in some files would not be added.

4) apply patch
5) try running the script again

- Were there errors?
- Did everything add nicely?
6) Log into the staff client
7) Tools->News
- Do you see the news items added?

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Much needed clean up, making for much more robust and less error prone
sql

Created attachment 26605 [details] [review]
Bug 7567 - Undefined $lang triggers warning

Changed:
    $template->param(
        $lang           => 1,
        opac_news       => $opac_news,
        opac_news_count => $opac_news_count,
                );
Into:
    $template->param(
        opac_news       => $opac_news,
        opac_news_count => $opac_news_count,
                );
    $template->param( $lang => 1 ) if $lang;

Because $lang is not defined when you come immediately into
Home -> Tools -> News.

TEST PLAN
---------
1) log into staff client
2) Tools -> News
3) Check koha error log
   - Expecting to see an error like:
koha-news.pl: Problem = a value of 1 has been passed to param without key
4) Apply patch
5) Home -> Tools -> News
6) Check koha error log
   - No error generated

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 26606 [details] [review]
[NEEDS SIGN OFF] Bug 7567 - Correct Filtering and Default dropdown values

In the staff client, when you went to Home -> Tools -> News
the default 'Display location' is 'All'. Everything is correctly
displayed. However, if you change the filter it filters the
table correctly, but does not set the dropdown value to match.
This patch corrects this.

TEST PLAN
---------
 1) Home -> Tools -> News
 2) Change 'Display location' value and click 'Filter'.
    - Is the 'Display location' value the same now?
      (no)
    - Are the correct values displayed?
 3) Click 'New entry'
    - Is it anything other than 'All'?
      (no, and is this correct?)
 4) Click 'Cancel'
 5) Click 'Edit' for various news items with different locations.
    - Does the 'Display location' get proper set?
      (no)
 6) Apply patch
 7) Change the 'Display location' value and click 'Filter'.
    - Does the 'Display location' value stay the same?
      (this should be correct now)
    - Are the correct values displayed?
 8) Change the 'Display location' value to 'All' and click 'Filter'.
    - Does everything display?
 9) Change the 'Display location' value and click 'Filter'.
10) Click 'New entry'
    - Does it match what the filter was?
      (should match now)
11) Click 'Cancel'
12) Click 'Edit' for various news items with different locations.
    - Does the 'Display location' get proper set?
      (this should be correct now)

Created attachment 26607 [details] [review]
Bug 7567 - Removed finish calls

"When all the data has been fetched from a SELECT statement,
the driver will automatically call finish for you. So you should
not call it explicitly except when you know that you've not
fetched all the data from a statement handle and the handle
won't be destroyed soon."
(http://search.cpan.org/~timb/DBI-1.627/DBI.pm#finish)

All the $sth variables were scoped within the functions,
and would be destroyed immediately. Additionally, there was
one after a SELECT, for only a single idnew, and so it was
not necessary.

TEST PLAN
---------
1) prove -v t/db_dependent/NewsChannels.t
2) apply patch
3) prove -v t/db_dependent/NewsChannels.t

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

ok 1 - use C4::NewsChannels;
ok 2 - Successfully added the first dummy news item!
ok 3 - Successfully added the second dummy news item!
ok 4 - Successfully updated second dummy news item!
ok 5 - Successfully tested get_opac_new id1!
ok 6 - Successfully tested get_opac_new id2!
ok 7 - Successfully tested get_opac_news!
ok 8 - Successfully tested GetNewsToDisplay!

Created attachment 26608 [details] [review]
[NEEDS SIGN OFF] Bug 7567 - Too many parameters and tiny bugs

Changed the add and update functions to use a hash reference
for the parameter, so that adding or subtracting parameters
should be easier. Added some POD for the add_opac_news and
upd_opac_news functions, so that developers would know how to
call it.

The hashref changes resulted in being able to return 0 for
failure and 1 for success. This meant adding a couple tests
to the test file.

And while testing, there was some sort of logic problem with
the matter of '' being all, but selecting all only showed
things set for all, and excluded particular languages, or other
interfaces.

Created attachment 26609 [details] [review]
Bug 7567 - Added branchcode to opac_news

This patch merely adds branchcode varchar(10) DEFAULT NULL to
the opac_news table.

TEST PLAN
---------
 1) backup your kohadata base if you care about the data.
 2) use the koha database
 3) describe opac_news;
 4) show create table opac_news;
    -- No branchcode constraint will exist.
 5) apply the patch
 6) upgrade the database (either staff client or script)
 7) use the koha database
 8) describe opac_news;
 9) show create table opac_news;
    -- The branchcode constraint should be listed.
10) drop that koha database
11) create the koha database
12) use the koha database
13) source ~/kohaclone/installer/data/mysql/kohastructure.sql
    -- there should be no errors in creating the database.
14) describe opac_news;
15) show create table opac_news;
    -- The branchcode constraint should be listed.
16) restore your koha database if you backed it up.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 26610 [details] [review]
[NEEDS SIGN OFF] Bug 7567 - Use, Display, Filter News by Branch

With the branch parameter added to key functions in
C4/NewsChannels.pm, function calls in C4/Members.pm,
mainpage.pl, opac/opac-main.pl, tools/koha-news.pl, and
t/db_dependent/NewsChannels.t were needed.

Some license texts were updated.

Templates were modified to display, allow for entry and editing
of the branches selected.

TEST PLAN
---------
1) Having logged into the staff client, is the news displaying
   correctly? Have you entered a news item which should not
   display for this branch of logged in user?
2) Find a patron (with some items checked out?)
3) Print a slip
   - News which is labelled 'All Branches' or for the same branch
     as the one printing the slip should display on the slip.
   - THIS DOES NOT AFFECT QUICK SLIPS
4) Home -> Tools -> News
   - Can you edit a news item?
   - Does the change save correctly?
   - Can you filter based on location and branch correctly?
   - Can you add a new entry correctly?
   - Can you delete an entry correctly?
5) Open an OPAC client.
   - Does only the news for all branches display?
6) Log into the OPAC client.
   - Does the news for all branches and the specific branch display?
7) prove -v t/db_dependent/NewsChannels.t
   - Does it run and all succeed?
   - Does the code seem to catch the required cases?
8) Comparing the patched and unpatched versions of files affected,
   are the license changes missing anything?

Created attachment 26611 [details] [review]
Bug 7567 - Schema update for opac_news

This was generated by:
Reset hard to master.
Restore old DB
Apply the patches for 7567
./installer/data/mysql/updatedatabase.pl
./misc/devel/update_dbix_class_files.pl -db_name=... -db_user=... -db_passwd=...
git add Koha/Schema/Results/OpacNews.pm

This should provide the necessary changes to the schema files for
Koha 3.16 and forward.

Had to generate under Debian, because Ubuntu's was broken by comparison.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Created attachment 26612 [details] [review]
[NEED SIGN OFF] Bug 7567 - Remove excess line

The template parameter { $lang => 1 } does not seem used in
master. It is unconfirmed whether this is used in earlier
versions of Koha.

Created attachment 26644 [details] [review]
Bug 7567 - Modify DataTables configuration

This follow-up corrects a JavaScript error by converting the sorting
configuration to be class-based
(http://wiki.koha-community.org/wiki/DataTables_HowTo#Sorting_dates_regardless_of_date_format_preference).

I have also changed the DataTables configuration to allow for pagination
and JS-based filtering.

To test, apply the patch and view the list of News items. Your browser
should report no JavaScript errors. Table sorting, pagination, and
filtering should work correctly.