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.

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'.

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.

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!

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.

(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.

The last, updated patch has been rewritten to make use of the Koha Preferences system rather than the template variables.

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.

(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.

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.

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.

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.

If you are ok with the tests, signing them off is perfectly fine. Just important you don't sign your own things :)

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>

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>

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.

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.

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.

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.

Patches pushed to master.

Thanks Martin, Mark and Katrina!