|
Description
Savitra Sirohi
2012-02-21 06:13:20 UTC
Since I recently heard that osslabs.biz is no longer in business, I am taking this bug. Though, I am probably going to take it a slightly different direction. The tools->news will have branch fields added. This would allow for every branch to create a news item for their library hours or other specific news, which will be then become visible once a user logs in. Anonymous users will not be able to see branch specific news. Refactored, Enhanced, and Fixed. I'm tempted to rename this to "Overhaul of News". 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 Hi Mark, Do you have a test plan for this? Cheers, Liz I'm still working on the test plan. I have to review if this is complete. This is why the status is "In Discussion" and not "Needs Signoff".
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
6) Add a News item, for something other than 'All Branches'.
7) Add a News item, for OPAC.
8) 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)
9) 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?
10) Check the news items added and click the delete button.
- Did it delete them appropriately?
11) 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)
12) Log in as a user with the branch you added the news item for.
- Does that piece of News display now too?
Cool, thanks. I'm actually quite interested in this patch - it's great functionality. Cheers, Liz Okay, I've reviewed and I think this test plan should be good.
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 'OPAC' interface at your user branch.
8) 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)
9) 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?
10) 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)
11) 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)
12) Check the news items added and click the delete button in the staff client.
- Did it delete them appropriately?
I found another section affected by the GetNewsToDisplay change. This affects the slips, like Liz was asking about. Doing further testing first, and new patch up by next week. 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? This is the latest, but I haven't fully tested it yet, hence the status is not changed. Feel free to test it and then anxiously await my toggling the status if there are no problems. :) 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? Okay, average as compared to low. And, I just cleaned up the sample news issues for a fresh install. Now to actually test with a rolled system. Hm, from the subject of this it looks a bit like it should better be several patches ('Ätell the RM a story' ;) ) than one big patch?
I would, except everything is so interdependent, I don't see where to break it up right now. I suppose I could try to break the "change from (parameter, parameter, parameter,etc.) into hash ref" portion out, but I didn't think to do that early on. I'll see if I can generate some sensible break lines. 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? *** Bug 11370 has been marked as a duplicate of this bug. *** 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? I know it's big, and I didn't split it up, but I finally got through my test plan and it all works. Comment on attachment 23386 [details] [review] Bug 7567 - News by Branch Review of attachment 23386 [details] [review]: ----------------------------------------------------------------- Hi Mark, I did a quick code review - noted some things using splinter review below. Could you please take a look? ::: C4/NewsChannels.pm @@ +124,5 @@ > $data->{$data->{'lang'}} = 1 if defined $data->{lang}; > $data->{expirationdate} = format_date($data->{expirationdate}); > $data->{timestamp} = format_date($data->{timestamp}); > + if (!defined($data->{branchcode}) || $data->{branchcode} eq '') { > + $data->{branchname} = "All Branches"; Suspecting a translation problem here. @@ +166,5 @@ > while (my $row = $sth->fetchrow_hashref) { > if ((($limit) && ($count < $limit)) || (!$limit)) { > $row->{'newdate'} = format_date($row->{'newdate'}); > $row->{'expirationdate'} = format_date($row->{'expirationdate'}); > + $row->{'branchname'} = "All Branches" if !$row->{'branchcode'}; And here. ::: installer/data/mysql/en/optional/sample_news.sql @@ +1,1 @@ > +INSERT INTO `opac_news` ( title, new, Please also correct translated installers to avoid problems when installing Koha in other languages. ::: installer/data/mysql/updatedatabase.pl @@ +7826,5 @@ > > +$DBversion = "3.15.00.XXX"; > +if ( CheckVersion($DBversion) ) { > + $dbh->do(q{ > + ALTER TABLE opac_news ADD branchcode varchar(10) NULL DEFAULT NULL, Please make sure that the new column is added at the same position than it is in kohastructure.sql (using AFTER should work). 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? As per Katrin Fischer's comments in comment #18, I am going to attempt to split this into smaller pieces. This is why the status is still Failed QA. I just wanted to make sure my patch was here for eyeball review, if someone wanted to peek. :) Hm out of interest, why are those 2 lines needed? +SET NAMES utf8; +SET FOREIGN_KEY_CHECKS=0; ? *** Bug 11372 has been marked as a duplicate of this bug. *** 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. http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23501&action=diff is the beginning of the breaking the first patch into smaller pieces. It applies against master, and basically creates the basis for the tests that must continue to succeed at each step. TEST PLAN --------- 1) git checkout -b bug_7567 origin/master 2) cat t/NewsChannels.t 3) cry that only a use was checked. 4) prove -v t/NewsChannels.t 5) git bz apply 7567 6) prove -v t/db_dependent/NewsChannels.t 7) cat t/NewsChannels.t 8) cheer that all the functions in C4/NewsChannels.pm are called at least once. 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. Only last patch to add tomorrow. All the "small" pieces (not the 85.93K patch) so far can apply to 3.14.x and master as "bug fixes". The patch tomorrow will add branch. 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? I've put up my split version now, but I will fully finish testing it and obsoleting the 85.93KB patch later. Comment on attachment 23475 [details] [review] Bug 7567 - News by Branch This mega patch has been replaced by all the smaller patches. I think I even fixed up a couple things from it too. I believe this test plan properly tests everything. However, each smaller one should have it's own test plan in the appropriate comment, for those who don't have time to test everything at once.
And really only the last two patches are enhancements, so I would hope that the other 8 patches could make it into 3.14.x and/or earlier too.
REVISED TEST PLAN
-----------------
0) Test the existing functionality and notice some of the bugs.
Backup the database if you care about your opac_news. :)
Apply all the patches.
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?
- This would require rolling your own distribution, or
being lazy and just source'ing kohastructure.sql after dropping
and recreating the database.
2) Do an upgrade to confirm that the branchcode field is added
to the opac_news table.
- useful MySQL commands: show create table opac_news;
describe opac_news;
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?
For 3.16 we are making more and more use of DBIX::Class .. which means you need to update the schema files too. I will sign off these patches, and when you add a follow up doing the Schema change I will sign that off too and set to signed off. 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! I don't know how to do Schema files. Sorry. Reading links? 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> Mark, could you please rebase the last patch please? fatal: sha1 information is lacking or useless (C4/NewsChannels.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 7567 - Use, Display, Filter News by Branch 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> Switching to 'signed off' since the last two are only a rebase. For the record, it was bug 11736 that triggered this required rebase. QA comment:
1)
+ $template->param( $lang => 1 ) if $lang;
Why ? Seems not used in template. But maybe I am wrong.
2)
- # $query.= "LIMIT 0, " . $limit;
+ # $query.= 'LIMIT 0, ' . $limit;
Could be removed
3)
+ if ($lang && $branchcode) {
+ $query .= " WHERE (opac_news.lang='' OR opac_news.lang=?)";
+ $query .= " AND (opac_news.branchcode IS NULL OR opac_news.branchcode=?)";
+ push @values,$lang;
+ push @values,$branchcode;
+ }
+ elsif ($lang) {
+ $query .= " WHERE (opac_news.lang='' OR opac_news.lang=?)";
+ push @values,$lang;
+ }
+ elsif ($branchcode) {
+ $query .= " WHERE (opac_news.branchcode IS NULL OR opac_news.branchcode=?)";
+ push @values,$branchcode;
+ }
Maybe the following is more readable:
$query .= " WHERE 1 ";
if ( $lang ) {
$query .= "AND (opac_news.lang='' OR opac_news.lang=?)";
push @values, $lang
}
and same for $branchcode
4) s/branch/library in templates
5) in opac-main.tt
- [% IF ( koha_news_count ) %]
<div id="news" class="container">
+ [% IF ( koha_news_count ) %]
Why? It will produce an empty div.
6)
Some QA issues (minor):
FAIL koha-tmpl/intranet-tmpl/prog/en/modules/tools/koha-news.tt
FAIL forbidden patterns
forbidden pattern: tab char (line 233)
forbidden pattern: tab char (line 169)
forbidden pattern: tab char (line 227)
forbidden pattern: tab char (line 229)
forbidden pattern: tab char (line 228)
forbidden pattern: tab char (line 225)
forbidden pattern: tab char (line 167)
forbidden pattern: tab char (line 231)
FAIL koha-tmpl/opac-tmpl/prog/en/modules/opac-main.tt
FAIL forbidden patterns
forbidden pattern: tab char (line 25)
7) It would be great to pass a hashref to get_opac_news
Marked as Failed QA.
(In reply to Jonathan Druart from comment #68) > + $template->param( $lang => 1 ) if $lang; > Why ? Seems not used in template. But maybe I am wrong. Because I was thinking it may exist in an earlier version of the template, and was hoping that this could get backported, not just into master. I didn't search earlier versions, though I think you are correct about it not being necessary in master. I'll add a patch for that. > 2) > - # $query.= "LIMIT 0, " . $limit; > + # $query.= 'LIMIT 0, ' . $limit; > Could be removed However, the parameter $limit was not removed, and so I'd like to leave it to show how it was used. It may come in handy if some libraries go news-happy, and we need to add a system preference to specify the limit on the amount of news displayed. > 3) > Maybe the following is more readable: > > $query .= " WHERE 1 "; > if ( $lang ) { > $query .= "AND (opac_news.lang='' OR opac_news.lang=?)"; > push @values, $lang > } > > and same for $branchcode Agreed. > 4) s/branch/library in templates > > 5) in opac-main.tt > - [% IF ( koha_news_count ) %] > <div id="news" class="container"> > + [% IF ( koha_news_count ) %] > > Why? It will produce an empty div. Empty divs can still be styled without generating errors in javascript. And empty divs should not cause massive screen variation over the existing output. > Some QA issues (minor): > FAIL koha-tmpl/intranet-tmpl/prog/en/modules/tools/koha-news.tt > FAIL forbidden patterns > forbidden pattern: tab char (line 233) > forbidden pattern: tab char (line 169) > forbidden pattern: tab char (line 227) > forbidden pattern: tab char (line 229) > forbidden pattern: tab char (line 228) > forbidden pattern: tab char (line 225) > forbidden pattern: tab char (line 167) > forbidden pattern: tab char (line 231) > > > FAIL koha-tmpl/opac-tmpl/prog/en/modules/opac-main.tt > FAIL forbidden patterns > forbidden pattern: tab char (line 25) Oh fiddlesticks! Patching. > 7) It would be great to pass a hashref to get_opac_news I don't see the number of parameters on get_opac_news growing past 3. Once you hit that fourth parameter, that seems like the point in time to consider making the parameter a hashref. Hope this addresses your concerns. I'll have a revision up shortly. (In reply to M. Tompsett from comment #69) > > 7) It would be great to pass a hashref to get_opac_news > > I don't see the number of parameters on get_opac_news growing past 3. Once > you hit that fourth parameter, that seems like the point in time to consider > making the parameter a hashref. In my opinion, the trigger is when we have a call with undef: + my ( $opac_news_count, $opac_news ) = &get_opac_news( undef, $lang, $branchcode ); It is not blocker for my QA point of view. Maybe for the RM point of view. 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> (In reply to Jonathan Druart from comment #70) > (In reply to M. Tompsett from comment #69) > > > 7) It would be great to pass a hashref to get_opac_news > > > > I don't see the number of parameters on get_opac_news growing past 3. Once > > you hit that fourth parameter, that seems like the point in time to consider > > making the parameter a hashref. > > In my opinion, the trigger is when we have a call with undef: > + my ( $opac_news_count, $opac_news ) = &get_opac_news( undef, $lang, > $branchcode ); > > It is not blocker for my QA point of view. Maybe for the RM point of view. Perhaps the problem isn't undef, but rather it probably should be 0, even though it isn't used. 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. The difficulty of a multi-patch bug fix is changing just parts after a code review. Only the ones with [NEEDS SIGN OFF] are changed compared to earlier versions which Chris Cormack signed off. These changes include: - chopping an unused template parameter (comment #68 - point 1) - simplifying the lang/branchcode query building logic (comment #68 - point 3) - using Library or Libraries instead of Branch or Braches in the template (comment #68 - point 4) - white space fixes (comment #68 - point 6) - renaming a variable from hashref to parameters (comment #55) To recap the other comments by Jonathan Druart: comment #68 - point 2: parameter is still there, and left for historical and future expansion purposes. comment #68 - point 5: empty div's can be styled without breaking javascript. comment #68 - point 7: it probably should be 0, but left undef, so obvious change was only the addition of branch code. I figure 4+ parameters deserve a hashref as a parameter. SIDE COMMENT: Bug 7731 was likely the motivation for comment #68 point 4. :) Created attachment 26631 [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 Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26632 [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 Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26633 [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 Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26634 [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 Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26635 [details] [review] 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. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26636 [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: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26637 [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! Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26638 [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: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26639 [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> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26640 [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: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26641 [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> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26642 [details] [review] Bug 7567: QA followup: FIX SQL query The WHERE clause should not erase $query. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Created attachment 26643 [details] [review] Bug 7567: QA followup: remove useless $parameters hashref This hashref is useless, it is only used once. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> (In reply to M. Tompsett from comment #95) > The difficulty of a multi-patch bug fix is changing just parts after a code > review. Only the ones with [NEEDS SIGN OFF] are changed compared to earlier > versions which Chris Cormack signed off. It is why it is preferable not to modify signed off patches. It is really hard for QAers to know what changes you introduced. Next time, prefer a followup, QAer can squash patches. I squashed 2 patches Bug 7567 - Undefined $lang triggers warning [NEED SIGN OFF] Bug 7567 - Remove excess line into Bug 7567 - Remove excess line And I provided 2 followups: 1/ Fix the sql query on getting rows from DB 2/ remove an unnecessary variable. Marked as Passed QA. 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. Created attachment 26647 [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. Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> (In reply to Owen Leonard from comment #111) > Created attachment 26644 [details] [review] [review] Good catch Owen, thanks! Pushed to master. Thanks, Mark! *** Bug 6745 has been marked as a duplicate of this bug. *** |