Bug 14247 - System preference for news author display
Summary: System preference for news author display
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Persson
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: newsauthor
Blocks: 14248
  Show dependency treegraph
 
Reported: 2015-05-21 16:00 UTC by Martin Persson
Modified: 2016-06-21 21:40 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
DB update script and sysprefs file (1.95 KB, patch)
2015-05-21 16:15 UTC, Martin Persson
Details | Diff | Splinter Review
Adds preferences for news authorship display (2.18 KB, patch)
2015-05-22 14:25 UTC, Martin Persson
Details | Diff | Splinter Review
SQL command file to remove news author preferences (68 bytes, application/sql)
2015-05-22 14:44 UTC, Martin Persson
Details
[Signed-off] Bug 14247: Add system preference for news display (2.36 KB, patch)
2015-05-23 10:26 UTC, Marc Véron
Details | Diff | Splinter Review
Updated version of patch, integrates syspref SQL, atomic update is pure SQL and test plan is in commit message. (4.13 KB, patch)
2015-05-25 14:24 UTC, Martin Persson
Details | Diff | Splinter Review
Optional patch to rename the atomic update to match guidelines (1.73 KB, patch)
2015-05-26 14:03 UTC, Martin Persson
Details | Diff | Splinter Review
Bug 14247: Add system preference for news display (2.27 KB, patch)
2015-10-04 13:52 UTC, Martin Persson
Details | Diff | Splinter Review
Bug 14247: Add system preference for news display (2.40 KB, patch)
2015-10-16 18:39 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14246: Update NewsChannel tests (3.63 KB, patch)
2015-10-16 18:43 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14247: Add system preference for news display (4.31 KB, patch)
2015-10-16 18:53 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14247: Add system preference for news display (4.31 KB, patch)
2015-10-16 18:58 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14247: Add system preference for news display (4.31 KB, patch)
2015-10-16 19:17 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14247: Rename sysprefs atomic update file (1021 bytes, patch)
2015-10-16 19:25 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14247: Add system preference for news display (4.36 KB, patch)
2015-10-22 00:26 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 14247: Rename sysprefs atomic update file (1.05 KB, patch)
2015-10-22 00:26 UTC, M. Tompsett
Details | Diff | Splinter Review
[PASSED QA] Bug 14247: Add system preference for news display (4.43 KB, patch)
2015-10-24 11:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14247: Rename sysprefs atomic update file (1.11 KB, patch)
2015-10-24 11:33 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Persson 2015-05-21 16:00:58 UTC
This patch adds a new binary option under Preferences/Tools called 'NewsAuthorDisplay'; it allows Koha to display the full name of the user who authored a news item. Depends on the patch in bug 14246.
Comment 1 Martin Persson 2015-05-21 16:15:11 UTC Comment hidden (obsolete)
Comment 2 Martin Persson 2015-05-22 07:38:04 UTC
Comment on attachment 39362 [details] [review]
DB update script and sysprefs file

Missing key functionality. Needs ability to switch on/off display for OPAC/Staff separately.
Comment 3 Martin Persson 2015-05-22 14:25:24 UTC Comment hidden (obsolete)
Comment 4 Martin Persson 2015-05-22 14:44:31 UTC
Created attachment 39418 [details]
SQL command file to remove news author preferences

To remove the changes from the database:
mysql -u kohaadmin -p -D koha < remove_news_prefs.sql
Comment 5 Martin Persson 2015-05-22 14:59:38 UTC
Test plan:

* Apply dependant patch 14246 (if not done already).
* Apply current patch.
* Execute the database update file (requires PERL5LIB and KOHA_CONF to be set up):
  $ perl perl installer/data/mysql/atomicupdate/add_news_prefs.pl
* Log into the Staff client, go to 'Administration' -> 'Global Preferences' -> 'Tools', a new section titled 'News' should appear with a single option named 'NewsAuthorDisplay'. Available values should be 'Not at all', 'OPAC only', 'Staff client only' and 'Both OPAC and staff client'. By default the value should be 'Not at all' (to mimic existing behaviour, important!).
* Change the value to something different and save the changes.
* Use the mysql command line client to connect to the Koha database:
  $ mysql -u kohaadmin -p -D koha
* Enter the following query:
  SELECT * FROM systempreferences WHERE variable = 'NewsAuthorDisplay';
* Check to make sure that the 'value' column has changed to reflect the choice you made in the above step.
Comment 6 Marc Véron 2015-05-23 10:26:41 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2015-05-24 11:54:07 UTC
Martin,
Actually, the atomicupdate file should have been created like:

== installer/data/mysql/atomicupdate/add_news_prefs.sql
-- your comment
INSERT IGNORE INTO systempreferences (variable,type,options,value,explanation) VALUES('NewsAuthorDisplay','Choice','none|opac|staff|both','none','Display the author name for news items.');

And that's all.
On this way, it will be executed by the updatedatabase.pl script.
ATM it does not deal with pl scripts.

This is not blocker, the RM will manage to deal with it.

However you have to add the new syspref in the sysprefs file (installer/data/mysql/sysprefs.sql).

Marked as Failed QA.
Comment 8 Martin Persson 2015-05-25 14:24:21 UTC Comment hidden (obsolete)
Comment 9 Martin Persson 2015-05-25 14:27:49 UTC
The order of the fields used for insertion in the atomic update has been changed to match the order in the main sysprefs.sql insert statement.

The clean install was tested against a new database that had only had the kohastructure.sql applied. No errors and data looked correct.
Comment 10 Martin Persson 2015-05-25 16:10:38 UTC
cait @ IRC informed me that in situations with minor changes, I can simply add another patch on top of the existing one rather than obsoleting it. That might have been more appropiate here. Pardon any inconvenience.
Comment 11 Martin Persson 2015-05-26 14:03:53 UTC Comment hidden (obsolete)
Comment 12 Indranil Das Gupta 2015-06-14 12:56:05 UTC
Comment on attachment 39488 [details] [review]
Updated version of patch, integrates syspref SQL, atomic update is pure SQL and test plan is in commit message.

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

Martin,

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/tools.pref
@@ +13,5 @@
>              - images.
> +    News:
> +        -
> +            - "Show the author for news items:"
> +            - pref: NewsAuthorDisplay

since you are seeding the sql with "none" as the default (which btw is the correct thing to do), do consider adding after 'pref: NewsAuthorDisplay' -

default: none
Comment 13 Indranil Das Gupta 2015-06-14 13:01:52 UTC
setting it back to assigned
Comment 14 Martin Persson 2015-10-04 13:52:31 UTC Comment hidden (obsolete)
Comment 15 Martin Persson 2015-10-04 14:54:51 UTC
Updated as suggested, new patch has default value set.
Comment 16 Marc Véron 2015-10-16 18:39:36 UTC Comment hidden (obsolete)
Comment 17 Marc Véron 2015-10-16 18:43:56 UTC Comment hidden (obsolete)
Comment 18 Marc Véron 2015-10-16 18:53:33 UTC Comment hidden (obsolete)
Comment 19 Marc Véron 2015-10-16 18:58:44 UTC Comment hidden (obsolete)
Comment 20 Marc Véron 2015-10-16 19:17:29 UTC Comment hidden (obsolete)
Comment 21 Marc Véron 2015-10-16 19:25:02 UTC Comment hidden (obsolete)
Comment 22 Marc Véron 2015-10-16 19:30:53 UTC
Comment on attachment 43541 [details] [review]
Bug 14247: Add system preference for news display

Obsoleted because the same content is in second patch. 

@ Martin, can you please verify / confirm that the signed-off patches are the correct ones?
Comment 23 Katrin Fischer 2015-10-21 22:17:39 UTC
There is something wrong here - the 3 patches, even with the one I put back lack the logic in the template/pl files for the display. 

bumblebee:~/kohaclone (14247-newsauthor) $ git diff --stat HEAD~3
 installer/data/mysql/atomicupdate/bug_14247-add_news_prefs.sql       |  2 ++
 installer/data/mysql/sysprefs.sql                                    |  1 +
 koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/tools.pref | 10 ++++++++++
 t/db_dependent/NewsChannels.t                                        | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 57 insertions(+), 1 deletion(-)

Also, the tests are not passing - I think because we are missing a dependency - "newsauthor" or bug 14246. We need this to be signed off please asap and this to be put in order.

bumblebee:~/kohaclone (14247-newsauthor) $ perl t/db_dependent/NewsChannels.t
1..11
ok 1 - use C4::NewsChannels;
# Category CAT1 not found, inserting
# Borrower BRWR1 not found, inserting
ok 2
ok 3 - Correctly failed on no parameter!
ok 4 - Successfully added the first dummy news item!
DBD::mysql::st execute failed: Unknown column 'borrowernumber' in 'field list' at /home/katrin/kohaclone/C4/NewsChannels.pm line 69.
DBD::mysql::st execute failed: Unknown column 'borrowernumber' in 'field list' at /home/katrin/kohaclone/C4/NewsChannels.pm line 69.
# Looks like you planned 11 tests but ran 4.
# Looks like your test exited with 255 just after 4.

Please check asap - feature slush is close!)
Comment 24 M. Tompsett 2015-10-22 00:09:35 UTC
Comment on attachment 43539 [details] [review]
Bug 14246: Update NewsChannel tests

This patch is identical to the one on bug 14246. Since we know that this bug depends on it, there is no need to include it here.
Comment 25 M. Tompsett 2015-10-22 00:26:34 UTC Comment hidden (obsolete)
Comment 26 M. Tompsett 2015-10-22 00:26:43 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2015-10-22 11:09:16 UTC
Waiting for bug 14248.
Comment 28 Katrin Fischer 2015-10-24 11:33:01 UTC
Created attachment 43914 [details] [review]
[PASSED QA] Bug 14247: Add system preference for news display

Test plan:

* Apply dependant patch 14246 (if not done already).
* Apply current patch.
* Log into the Staff client, go to 'Administration' -> 'Global Preferences' -> 'Tools', a new section titled 'News' should appear with a single option named 'NewsAuthorDisplay'. Available values should be 'Not at all', 'OPAC only', 'Staff client only' and 'Both OPAC and staff client'. By default the value should be 'Not at all' (to mimic existing behaviour, important!).
* Change the value to something different and save the changes.
* Use the mysql command line client to connect to the Koha database:
  $ mysql -u kohaadmin -p -D koha
* Enter the following query:
  SELECT * FROM systempreferences WHERE variable = 'NewsAuthorDisplay';
* Check to make sure that the 'value' column has changed to reflect the choice you made in the above step.

Separate SQL testing (clean installation of Koha):
* Koha$ mysql -u root < create_koha_test_db.sql
* Koha$ mysql -u koha_test -D koha_test < installer/data/mysql/kohastructure.sql
* Koha$ mysql -u koha_test -D koha_test < installer/data/mysql/sysprefs.sql

Testing the update script works the same way:
* Koha$ echo "DELETE FROM systempreferences WHERE variable = 'NewsAuthorDisplay';" | mysql -u koha_test -D koha_test
* Koha$ mysql -u koha_test -D koha_test < installer/data/mysql/atomicupdate/add_news_prefs.sql

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2015-10-24 11:33:05 UTC
Created attachment 43915 [details] [review]
[PASSED QA] Bug 14247: Rename sysprefs atomic update file

This trivial patch changes the name of the SQL update
file to match the guidelines on the wiki.

Sponsored-by: Halland County Library
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Tomás Cohen Arazi 2015-10-27 19:30:30 UTC
Patches pushed to master.

Thanks Martin!