Bug 14246 (newsauthor)

Summary: Add borrowernumber to koha_news
Product: Koha Reporter: Martin Persson <xarragon>
Component: DatabaseAssignee: Martin Persson <xarragon>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: indradg, jonathan.druart, joonas.kylmala, katrin.fischer, mtompset, tomascohen, veron, viktor.sarge, xarragon
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 14247    
Attachments: Adds koha_news column borrowernumber
Adds borrowerid column to news, stores current user id.
SQL command file to revert changes to database
[Signed-off] Bug 14246: Add koha_news.borrowernumber and store
Updated version in accordance wih Wiki rules.
Updated SQL, AFTER qualifier and changed UPDATE action
Updated tests in NewsChannels.t
(fix) correct ALTER TABLE mysql syntax
Bug 14246: Add borrowernumber to koha_news
Bug 14246: Add borrowernumber, altered SQL
Bug 14246: Update NewsChannel tests
(fix) correct ALTER TABLE mysql syntax
Bug 14246: Adding Koha::Schema::Result differences
Bug 14246: Add borrowernumber to koha_news
Bug 14246: Add borrowernumber, altered SQL
Bug 14246: Update NewsChannel tests
(fix) correct ALTER TABLE mysql syntax
Bug 14246: Adding Koha::Schema::Result differences
[PASSED QA] Bug 14246: Add borrowernumber to koha_news
[PASSED QA] Bug 14246: Add borrowernumber, altered SQL
[PASSED QA] Bug 14246: Update NewsChannel tests
[PASSED QA] (fix) correct ALTER TABLE mysql syntax
[PASSED QA] Bug 14246: Adding Koha::Schema::Result differences

Description Martin Persson 2015-05-21 15:44:13 UTC
News items in Koha's OPAC and Staff client are anonymous. This is the first in a set of patches to add authorship tracking to the news items, with optional display preferences maintaining the current functionality.

This first patch changes the database; it adds a column called 'borrowernumber' to the 'koha_news' table, adding a foreign key relationship to the 'borrowers' table. This is how tracking authorship is done. A LEFT JOIN statement is then used to get the title, firstname and surname columns available in the news data set. That is a separate patch.
Comment 1 Martin Persson 2015-05-21 16:14:31 UTC Comment hidden (obsolete)
Comment 2 Martin Persson 2015-05-22 07:35:43 UTC
Comment on attachment 39361 [details] [review]
Adds koha_news column borrowernumber

Missing key functionality.
Comment 3 Martin Persson 2015-05-22 14:23:34 UTC Comment hidden (obsolete)
Comment 4 Martin Persson 2015-05-22 14:30:16 UTC Comment hidden (obsolete)
Comment 5 Martin Persson 2015-05-22 14:42:10 UTC
Test plan:

* Check out clean master, reference sha-1 for this was: f52084df0e395fdf89d80ca2fd77844273a8cf7c
* You need at least one news item, log in and add one from 'Administration' -> 'Tools' -> 'News'.
* Create a branch for the testing: git checkout -b test_news_author
* Apply the patch.
* Execute the database update:
  $ perl installer/data/mysql/atomicupdate/add_news_author.pl
* Go back to Staff interface and post a new news item.
* Use the mysql command line client to connect to the Koha installation's database: mysql -u kohaadmin -p -D koha (change username and database to values appropiate for your system).
* Execute the following query: SELECT title, borrowernumber FROM opac_news;
* Inspect the results, the 'borrowernumber' column should be NULL for all news items created before the patch was applied, and non-NULL for the new item created AFTER the patch was applied.

To remove all the changes (there are dependant patches that you can test before you do this!):

* Execute the supplied SQL script: 
  $ mysql -u kohaadmin -p -D koha < remove_news_author.sql
* Drop the testing branch:
  $ git branch -d test_news_author
Comment 6 Marc Véron 2015-05-23 10:19:49 UTC Comment hidden (obsolete)
Comment 7 Mark Tompsett 2015-05-25 20:48:28 UTC
This is missing a corresponding kohastructure.sql changes, or schema file changes as well.

http://wiki.koha-community.org/wiki/Database_updates
Comment 8 Martin Persson 2015-05-26 13:14:45 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2015-05-26 13:33:51 UTC
Comment on attachment 39521 [details] [review]
Updated version in accordance wih Wiki rules.

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

::: installer/data/mysql/atomicupdate/bug_14246-add_news_author.sql
@@ +1,1 @@
> +ALTER TABLE `opac_news` ADD `borrowernumber` INT(11) default NULL;

Please add an AFTER clause, to ensure that an upgrade and a fresh install will have the same field order.
https://dev.mysql.com/doc/refman/5.7/en/alter-table.html
Comment 10 Martin Persson 2015-05-26 15:48:53 UTC
Okay, will do. The tests probably also needs to be updated as they seem to compare the database content, which I changed. Thanks for the feedback.
Comment 11 Martin Persson 2015-06-01 13:51:26 UTC Comment hidden (obsolete)
Comment 12 Martin Persson 2015-06-01 13:58:11 UTC Comment hidden (obsolete)
Comment 13 Mark Tompsett 2015-06-05 23:34:24 UTC
Comment on attachment 39745 [details] [review]
Updated SQL, AFTER qualifier and changed UPDATE action

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

::: installer/data/mysql/atomicupdate/bug_14246-add_news_author.sql
@@ +1,1 @@
> +ALTER TABLE `opac_news` ADD `borrowernumber` int(11) AFTER `number` default NULL;

The AFTER clause should follow the definition of the column, which would include 'default NULL'. This is a MySQL syntax error.
Comment 14 Mark Tompsett 2015-06-05 23:35:00 UTC
Setting back to ASSIGNED, so Martin can correct issue raised in comment #13.
Comment 15 Indranil Das Gupta 2015-06-14 12:15:07 UTC Comment hidden (obsolete)
Comment 16 Joonas Kylmälä 2015-10-13 11:20:25 UTC
I guess this feature should be also be used by OPAC/Staff side. Now the user itself cannot use the feature (only people with access to the db).
Comment 17 Marc Véron 2015-10-15 09:43:17 UTC
(In reply to Joonas Kylmälä from comment #16)
> I guess this feature should be also be used by OPAC/Staff side. Now the user
> itself cannot use the feature (only people with access to the db).

This patch simply adds a new column as a base for further development.
It does what is advertised in comment #1:
"it adds a column called 'borrowernumber' to the 'koha_news' table, adding a foreign key relationship to the 'borrowers' table."

With patch, creating a news item fills in the borrowernumber. This information can later be used, it opens up a lot of possibilities. 

My opinion is that the patch should be switched back to "Needs sign-off".
Comment 18 Katrin Fischer 2015-10-21 22:15:28 UTC
This patch is needed to continue with 14247. Please sign-off asap!
Comment 19 Mark Tompsett 2015-10-21 23:11:07 UTC
(In reply to Katrin Fischer from comment #18)
> This patch is needed to continue with 14247. Please sign-off asap!

Koha Dev folks... can we sign off without Koha::Schema::Result::Opacnews being updated?
Comment 20 Katrin Fischer 2015-10-21 23:23:35 UTC
Including a Schema changes is still optional I'd say, see:
http://wiki.koha-community.org/wiki/Database_updates
...if you _want_ to...
Comment 21 Mark Tompsett 2015-10-21 23:33:58 UTC
(In reply to Katrin Fischer from comment #20)
> Including a Schema changes is still optional I'd say, see:
> http://wiki.koha-community.org/wiki/Database_updates
> ...if you _want_ to...

I asked, because I discovered issues with AuthorisedValuesBranch and MarcModificationTemplateAction and isnullable, when I tried the koha schema building thing. Totally unrelated to this. Back to testing.
Comment 22 Nick Clemens 2015-10-21 23:40:49 UTC Comment hidden (obsolete)
Comment 23 Nick Clemens 2015-10-21 23:41:03 UTC Comment hidden (obsolete)
Comment 24 Nick Clemens 2015-10-21 23:41:11 UTC Comment hidden (obsolete)
Comment 25 Nick Clemens 2015-10-21 23:41:17 UTC Comment hidden (obsolete)
Comment 26 Mark Tompsett 2015-10-21 23:58:32 UTC Comment hidden (obsolete)
Comment 27 Mark Tompsett 2015-10-21 23:59:39 UTC Comment hidden (obsolete)
Comment 28 Mark Tompsett 2015-10-21 23:59:44 UTC Comment hidden (obsolete)
Comment 29 Mark Tompsett 2015-10-21 23:59:49 UTC Comment hidden (obsolete)
Comment 30 Mark Tompsett 2015-10-21 23:59:54 UTC Comment hidden (obsolete)
Comment 31 Mark Tompsett 2015-10-21 23:59:59 UTC Comment hidden (obsolete)
Comment 32 Mark Tompsett 2015-10-22 00:05:54 UTC
I tested:
- Cascading when a borrower is deleted, the borrowernumber is set to NULL.
- Adding is the only time a borrowernumber is set.
- t/db_dependent/NewsChannels.t

A couple remarks:
- I don't like diags added in the tests.
- I would have gone further and allowed
  setting and changing the borrower number, but I think that is beyond scope.
Comment 33 Marc Véron 2015-10-22 07:35:29 UTC
(In reply to M. Tompsett from comment #32)
(...)
> - I would have gone further and allowed
>   setting and changing the borrower number, but I think that is beyond scope.

I look forward to see that as well - in a new Bug on top of and as enhancement to this one.
Comment 34 Jonathan Druart 2015-10-22 11:09:09 UTC
Waiting for bug 14248.
Comment 35 Katrin Fischer 2015-10-24 11:31:21 UTC
Created attachment 43909 [details] [review]
[PASSED QA] Bug 14246: Add borrowernumber to koha_news

This patch adds a new column to koha_news that links a
borrowernumber to each item. This allows Koha to display the
author for each entry which makes it suitable as a simple CMS.

Changes (from rejected patch):
* Added missing kohastructure.sql changes.
* Turned the atomic update file into a SQL file and changed name
  in accordance with wiki guidelines.
* Changed SQL syntax and naming to be consistent with existing code.
* Attached test plan to commit message.

Test plan:

* You need at least one news item, log in and add one from
  'Administration' -> 'Tools' -> 'News'.
* Apply the patch.
* Apply database upgrade (directly or indirectly).
* Go back to Staff interface and post a new news item.
* Use the mysql command line client to connect to the Koha installation's database:
  mysql -u kohaadmin -p -D koha (change username and database to values
  appropiate for your system).
* Execute the following query: SELECT title, borrowernumber FROM opac_news;
* Inspect the results, the 'borrowernumber' column should be NULL for all news items
  created before the patch was applied, and non-NULL for the new item created AFTER
  the patch was applied.

Optional: Remove database changes via:
  ALTER TABLE opac_news DROP FOREIGN KEY borrowernumber_fk;
  ALTER TABLE opac_news DROP COLUMN borrowernumber;

Sponsored-By: Halland County Library

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 36 Katrin Fischer 2015-10-24 11:31:25 UTC
Created attachment 43910 [details] [review]
[PASSED QA] Bug 14246: Add borrowernumber, altered SQL

As suggested by kind reviewers, an AFTER statement was added to the atomic
upgrade script to ensure that the final field order is consistent is both
fresh and upgraded databases.

Also:
* UPDATE action changed to CASCADE to improve robustness.

Test plan:
* Apply first patch in this set.
* Apply this patch.
* Perform a database upgrade.
* Use mysql client to inspect field order.
  It should be identical to the kohastructure.sql order.

Sponsored-by: Halland County Library

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 37 Katrin Fischer 2015-10-24 11:31:28 UTC
Created attachment 43911 [details] [review]
[PASSED QA] Bug 14246: Update NewsChannel tests

This commit adds the new 'borrowernumber' field to the existing
test framework to make it pass. It does not include new tests.

Sponsored-by: Halland County Library

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 38 Katrin Fischer 2015-10-24 11:31:31 UTC
Created attachment 43912 [details] [review]
[PASSED QA] (fix) correct ALTER TABLE mysql syntax

Address bug 14246 comment 13 remark - correcting MySQL ALTER TABLE
syntax.

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 39 Katrin Fischer 2015-10-24 11:31:35 UTC
Created attachment 43913 [details] [review]
[PASSED QA] Bug 14246: Adding Koha::Schema::Result differences

Ran misc/devel/update_dbix_class_files.pl as shown on
wiki/Database_updates#updatedatabase.pl
However, only git added Borrower and Opacnews, since
those were the two affected by this patch.

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

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

Thanks Martin!