Bug 14248

Summary: Optionally display authorship for news items
Product: Koha Reporter: Martin Persson <xarragon>
Component: ToolsAssignee: Martin Persson <xarragon>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, mtompset, tomascohen, veron, viktor.sarge
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14247    
Bug Blocks:    
Attachments: Changes to templates and perl files to enable display
Template/script changes to handle news author display
[Singned-off] Bug 14248: Optionally display the author for news
Bug 14248 - Optionally display authorship for news
Bug 14248 - Optionally display authorship for news
Bug 14248: Regression tests requested
Bug 14248: Regression tests requested
[PASSED QA] Bug 14248 - Optionally display authorship for news
[PASSED QA] Bug 14248: Regression tests requested
Bug 12428: QA Follow-up: Wrap borrower's title in a span to make it possible to hide

Description Martin Persson 2015-05-21 16:06:07 UTC
This patch adds the ability to display the name of the user who authored a specific news item. It will by default work as before, not displaying any information. Enabling it will make Koha more suitable as a simple CMS and/or blog.

It depends on the patches for bugs 14246 and 14247.
Comment 1 Martin Persson 2015-05-21 16:15:50 UTC Comment hidden (obsolete)
Comment 2 Martin Persson 2015-05-22 07:38:38 UTC
Comment on attachment 39363 [details] [review]
Changes to templates and perl files to enable display

Needs to be changed to allow OPAC/Staff display preferences indenpendently.
Comment 3 Martin Persson 2015-05-22 14:26:37 UTC Comment hidden (obsolete)
Comment 4 Martin Persson 2015-05-22 15:10:58 UTC
Test plan:

* Apply both dependant patches, in order: 14246, 14247
* Apply this patch
* Go to Staff client, Select 'Tools' -> 'News'; there should be a new column named 'Author' for news items.
* Add a new news item; the full name of the current user should be displayed in the 'Author' column.
* Go to 'Home' -> 'Administration' -> 'Global system preferences' -> 'Tools', change the display of news author from 'None' to either of the other values.
* Check the OPAC start page, the author should only be shown if set to 'both' or 'opac only'.
* Check the Staff client start page, the author should only be shown if set to 'both' or 'staff client only'.
Comment 5 Marc Véron 2015-05-23 10:32:25 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-05-24 11:49:51 UTC
Martin,

Actually the 3 patches (from bug 14246 and 14247) could have been attached to this bug report. No need to open 3 bug reports for 1 enh :)

I have some remarks concerning this patch:
1/ You add changes to a module file, in most of the cases, you have to provide tests to make sure there won't be regression later.
Here you have to test C4::NewsChannels::get_opac_news, have a look at t/db_dependent/NewsChannels.t

2/ A value for a pref can be retrieved directly from the template.
I would tend not to pass the display_news_author variable to the template, but check/compare the syspref value in the template.
Since this is debatable, it's not blocker.

Marked as Failed QA for 1.
Comment 7 Viktor Sarge 2015-05-25 11:02:49 UTC
Jonathan - glad to hear we could have done all three in one thread. That was my fault. I recalled guidelines being to separate different types of changes in different threads (database change, syspref to use it and display logic in different patches). But that might be for larger patches or all together wrong(?)

Thanks for swift testing Marc & Jonathan!
Comment 8 Martin Persson 2015-05-25 11:25:09 UTC
Thanks for the feedback. Tests will be added; omitting them was an oversight, albeit a greivous one. I apologzie for that. I saw the other way of handling data to the template but was not sure which one was better; I realize that simple display options really have no business poking through to the business logic in the Perl file itself.
Comment 9 Jonathan Druart 2015-05-26 08:20:46 UTC
(In reply to Viktor Sarge from comment #7)
> Jonathan - glad to hear we could have done all three in one thread. That was
> my fault. I recalled guidelines being to separate different types of changes
> in different threads (database change, syspref to use it and display logic
> in different patches). But that might be for larger patches or all together
> wrong(?)

Not really, we split big feature/enh in small bug reports if we can split it, and if it makes sense.
Most of the time the DB and sysprefs changes are attached to the same bug report. I would prefer to see them managed in a separate patch, but not everybody does it.
Comment 10 Martin Persson 2015-10-02 07:31:06 UTC Comment hidden (obsolete)
Comment 11 Martin Persson 2015-10-02 09:50:14 UTC
The last, updated patch has been rewritten to make use of the Koha Preferences system rather than the template variables.
Comment 12 Martin Persson 2015-10-04 15:27:39 UTC
There are tests included in the original bug, 14246. They cover the changes in NewsChannel.pm, ar they enough to pass QA? Changing to 'Needs Signoff', please reset if not appropiate.
Comment 13 Marc Véron 2015-10-15 09:57:01 UTC Comment hidden (obsolete)
Comment 14 Mark Tompsett 2015-10-22 00:36:17 UTC
(In reply to Jonathan Druart from comment #6)
> Martin,
> 
> Actually the 3 patches (from bug 14246 and 14247) could have been attached
> to this bug report. No need to open 3 bug reports for 1 enh :)

I agree. However, 14246 and 14247 are passable in my opinion. This one is not.


> I have some remarks concerning this patch:
> 1/ You add changes to a module file, in most of the cases, you have to
> provide tests to make sure there won't be regression later.
> Here you have to test C4::NewsChannels::get_opac_news, have a look at
> t/db_dependent/NewsChannels.t

There are no tests added.
Suggestion: test to see that the title, firstname, and surname keys exist in at least the first row of results.

This is why I'm leaving it failed QA.
Comment 15 Katrin Fischer 2015-10-22 05:33:48 UTC
Mark, thx for clearing this up - I see now how the 3 patches go together. I think all we are missing is the tests asked for on this bug report.

Martin - small steps are great - this just went a little too far :) It would have been ok to do the database changes and the feature in one go in this case, as this is a nice contained feature.
Comment 16 Martin Persson 2015-10-22 12:41:45 UTC
Yes, thanks. This was the first ticket I created of any complexity and it got messy. I am sorry, I have since learned. I am now starting to complement it to have it pass QA.
Comment 17 Mark Tompsett 2015-10-22 12:54:15 UTC Comment hidden (obsolete)
Comment 18 Martin Persson 2015-10-22 14:30:12 UTC Comment hidden (obsolete)
Comment 19 Martin Persson 2015-10-22 14:31:31 UTC
Not sure if it is enough to sign off on tests for my own code made by a third party, but there you go. Not touching status. All tests passes on a clean test system.
Comment 20 Katrin Fischer 2015-10-22 14:32:50 UTC
If you are ok with the tests, signing them off is perfectly fine. Just important you don't sign your own things :)
Comment 21 Katrin Fischer 2015-10-24 11:34:16 UTC
Created attachment 43916 [details] [review]
[PASSED QA] Bug 14248 - Optionally display authorship for news

Test plan:

* Apply dependant patches 14246 and 14247.
* 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.
* Go to 'Tools' -> 'News' and add some news items (at least 2).
* Use the MySQL command line tool to edit the opac_news table,
  removing the user id from one of the news items:
  $ mysql -u kohaadmin -p -D koha
  > SELECT `idnew`, `title`, `borrowernumber` FROM `opac_news`;
  * Make a mental note of one of the 'newid' numbers to change.
  > UPDATE `opac_news` SET `borrowernumber` = NULL WHERE `idnew` = <your-id-here>;
  * Now one of the news items will have no user id, and no author should be shown for it.
* Go to OPAC and Staff client and make sure the display modes works as intended.
  * Repeat for all values, making sure that the settings works as intended.
  * The changed news item should have no author info shown at any time.
  * The unchanged news item should show your own user name if the
    settings match the section where it is displayed.

Sponsored-By: Halland County Library

Tested on top of Bugs 1246 and 1247, works as advertised.
Note: Some whitespaces changed / removed in koha-news.tt to pass qa tools.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2015-10-24 11:34:20 UTC
Created attachment 43917 [details] [review]
[PASSED QA] Bug 14248: Regression tests requested

TEST PLAN
---------
1) apply patch
2) prove -v t/db_dependent/NewsChannels.t
3) run koha qa test tools.

Signed-off-by: Martin Persson <xarragon@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2015-10-24 11:34:23 UTC
Created attachment 43918 [details] [review]
Bug 12428: QA Follow-up: Wrap borrower's title in a span to make it possible to hide

I think showing the title with the user name is quite unusual for
Koha. Wrapping the title in a span with a unique class makes it
possible to hide it.
Comment 24 Katrin Fischer 2015-10-24 11:52:24 UTC
There is just one small thing that is irritating to me about this patch set - the display of the news author's name: <title> <firstname> <surname>.

Thinking of blogs and similar I think the <title> is not needed. I think we could maybe remove it. I provided a follow-up wrapping the title into a span with a unique class so now you can hide it if you want to.
Comment 25 Viktor Sarge 2015-10-26 06:33:40 UTC
Thanks Katrin! 

We don't use title either, but we solved that by removing the field in the "add patron" form and without any value in the field things display ok without having to mod other areas/reports that includes title. I like the span approach.
Comment 26 Katrin Fischer 2015-10-26 06:38:10 UTC
I was not sure if it was from your specification that it showed up in the patch - so I didn't want to remove it totally. We still could I guess - it will show now by default until you remove it.
Comment 27 Tomás Cohen Arazi 2015-10-27 19:31:11 UTC
Patches pushed to master.

Thanks Martin, Mark and Katrina!