Bug 16371

Summary: Quote of the Day (QOTD) for the staff interface
Product: Koha Reporter: Olli-Antti Kivilahti <olli-antti.kivilahti>
Component: NoticesAssignee: Emmi Takkinen <emmi.takkinen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: david.roberts, david, emmi.takkinen, indradg, jonathan.druart, katrin.fischer, lucas, martin.renvoize, Niamh.WalkerHeadon, tomascohen, veron
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement lets you choose where the Quote of the Day (QOTD) is displayed: - OPAC: QOTD only appears in the OPAC. - Staff interface: QOTD only appears in the staff interface. - Both [Select all]: QOTD appears in the staff interface and OPAC.
Version(s) released in:
20.11.00
Bug Depends on:    
Bug Blocks: 27466    
Attachments: Bug 16371: Add needed Koha::Exception
Bug 16371: Quote of the Day (QOTD) - Display in different interfaces
Bug 16371: Rework tests
Bug 16371: Use multi-select on syspref and remove 'display_daily_quote' condition
Bug 16371: Use multi-select on syspref and remove 'display_daily_quote' condition
Bug 16371: Add Koha::Quote[s] classes
Bug 16371: Move GetDailyQuote to get_daily_quote
Bug 16371: Rename test file
Bug 16371: Add needed Koha::Exception
Bug 16371: Quote of the Day (QOTD) - Display in different interfaces
Bug 16371: Use multi-select on syspref
Bug 16371: Fix tests
Bug 16371: Add Koha::Quote[s] classes
Bug 16371: Move GetDailyQuote to get_daily_quote
Bug 16371: Rename test file
Bug 16371: Add needed Koha::Exception
Bug 16371: Quote of the Day (QOTD) - Display in different interfaces
Bug 16371: Use multi-select on syspref
Bug 16371: Fix tests
Bug 16371: Fix QA issues, change terminology
Bug 16371: Rewrite get_daily_quote
Bug 16371: Add database updates
Bug 16371: Rewrite get_daily_quote
Bug 16371: Add Koha::Quote[s] classes
Bug 16371: Move GetDailyQuote to get_daily_quote
Bug 16371: Rename test file
Bug 16371: Add needed Koha::Exception
Bug 16371: Quote of the Day (QOTD) - Display in different interfaces
Bug 16371: Use multi-select on syspref
Bug 16371: Fix tests
Bug 16371: Fix QA issues, change terminology
Bug 16371: Add database updates
Bug 16371: Rewrite get_daily_quote
Bug 16371: Add Koha::Quote[s] classes
Bug 16371: Move GetDailyQuote to get_daily_quote
Bug 16371: Rename test file
Bug 16371: Add needed Koha::Exception
Bug 16371: Quote of the Day (QOTD) - Display in different interfaces
Bug 16371: Use multi-select on syspref
Bug 16371: Fix tests
Bug 16371: Fix QA issues, change terminology
Bug 16371: Add database updates
Bug 16371: Rewrite get_daily_quote
Bug 16371: Remove GetDailyQuote tests from Koha.t
Bug 16371: Combine get_daily_quote and get_daily_quote_for_interface
Bug 16371: Fix inconsistencies with timestamp
Bug 16371: (follow-up) DB changes

Description Olli-Antti Kivilahti 2016-04-27 16:21:13 UTC
Quote of the day is a very handy tools to show random "tip-of-the-day"s, or "did-you-know"s for our librarians in the staff client.

Since most of the staff don't read email, this will help to reach a wider audience and teach how to more effectively use Koha.

This patch enables choosing the interface where the QOTD is shown.
Comment 1 Aleisha Amohia 2016-05-12 04:56:22 UTC
No patch has been attached!
Comment 2 Emmi Takkinen 2020-04-07 13:29:26 UTC
Created attachment 102515 [details] [review]
Bug 16371: Add needed Koha::Exception

Add Koha::Exception needed in this bug.

Sponsored-by: Koha-Suomi Oy
Comment 3 Emmi Takkinen 2020-04-07 13:30:15 UTC
Created attachment 102516 [details] [review]
Bug 16371: Quote of the Day (QOTD) - Display in different interfaces

Set 'QuoteOfTheDay'-syspref to 'opac intranet' to allow displaying news in the
opac and the staff client.

Add 'commandline' to allow news in the commandline (where applicable)

Accepts all values accepted by the C4::Context->interface()

Test plan:
1. Set 'QuoteOfTheDay' syspref as 'intranet' or 'opac' or both
2. Check that quote is visible on mainpage

Sponsored-by: Koha-Suomi Oy
Comment 4 Emmi Takkinen 2020-04-07 13:31:06 UTC
Created attachment 102517 [details] [review]
Bug 16371: Rework tests

Use TestBuilder and 'mock_preferences' in tests.

To test, prove t/db_dependent/Koha/GetDailyQuoteForInterface.t

Sponsored-by: Koha-Suomi Oy
Comment 5 David Roberts 2020-04-07 21:08:08 UTC
The QOTD certainly displays in the staff client with this patch in place, and the tests pass OK:

t/db_dependent/Koha/GetDailyQuoteForInterface.t .. ok
All tests successful.
Files=1, Tests=3,  3 wallclock secs ( 0.03 usr  0.00 sys +  1.76 cusr  0.30 csys =  2.09 CPU)
Result: PASS


However, with this patch in place, the QOTD doesn't display in the OPAC. I also think that the system preference to display the Quote in the different interfaces would be better if it was a drop down (Off, Intranet Only, OPAC Only, Both), rather than "free text". 

Sorry :(
Comment 6 Katrin Fischer 2020-04-07 21:43:23 UTC
It could also be a multi-select with 2 checkboxes for staff and OPAC showing up.
Comment 7 Emmi Takkinen 2020-04-15 08:01:06 UTC
Created attachment 102969 [details] [review]
Bug 16371: Use multi-select on syspref and remove 'display_daily_quote' condition

This patch replaces textbox with multi-select dropdown on
syspref 'QuoteOfTheDay' and changes tests to match this change.

Also removes condition 'display_daily_quite' from opac-main.tt
since it's no longer needed and prevents quotes showing in OPAC.

Sponsored-by: Koha-Suomi Oy
Comment 8 David Roberts 2020-04-15 16:23:05 UTC
This hadn't been changed back to needs signoff, but I tested it anyway because the patches had been replaced. All looks good:

t/db_dependent/Koha/GetDailyQuoteForInterface.t .. ok
All tests successful.
Files=1, Tests=3,  3 wallclock secs ( 0.04 usr  0.00 sys +  2.18 cusr  0.20 csys =  2.42 CPU)
Result: PASS

The functionality is working as I would expect in the clients as well, so happy to sign off as it is.
Comment 9 David Roberts 2020-04-15 16:23:40 UTC
Created attachment 103019 [details] [review]
Bug 16371: Use multi-select on syspref and remove 'display_daily_quote' condition

This patch replaces textbox with multi-select dropdown on
syspref 'QuoteOfTheDay' and changes tests to match this change.

Also removes condition 'display_daily_quite' from opac-main.tt
since it's no longer needed and prevents quotes showing in OPAC.

Sponsored-by: Koha-Suomi Oy
Signed-off-by: David Roberts <david@koha-ptfs.co.uk>
Comment 10 Jonathan Druart 2020-04-16 11:59:29 UTC
Don't you want to take the opportunity to move existing code to new Koha::QuoteOfTheDay[s] objects?

It does not sound a good idea to add new subroutines to C4::Koha
Comment 11 Emmi Takkinen 2020-04-17 07:20:25 UTC
(In reply to Jonathan Druart from comment #10)
> Don't you want to take the opportunity to move existing code to new
> Koha::QuoteOfTheDay[s] objects?
> 
> It does not sound a good idea to add new subroutines to C4::Koha

Sure I would. When I fetched original patch here I left that subroutine to C4::Koha since there wasn't any other logical place to put it. But if it's ok to add these new classes, I'll look into it. 

I'm setting this bug as ASSIGNED.
Comment 12 Emmi Takkinen 2020-04-21 07:27:37 UTC
Created attachment 103329 [details] [review]
Bug 16371: Add Koha::Quote[s] classes
Comment 13 Emmi Takkinen 2020-04-21 07:28:13 UTC
Created attachment 103330 [details] [review]
Bug 16371: Move GetDailyQuote to get_daily_quote

This patch moves subroutine 'GetDailyQuote' to new
Koha::Quote object and adjusts tests.

To test:
1. Set 'QuoteOfTheDay' as 'enable'
2. Check that quote is displayed on OPAC mainpage

Prove t/db_dependent/Koha/GetDailyQuote.t

Sponsored-by: Koha-Suomi Oy
Comment 14 Emmi Takkinen 2020-04-21 07:28:43 UTC
Created attachment 103331 [details] [review]
Bug 16371: Rename test file
Comment 15 Emmi Takkinen 2020-04-21 07:29:36 UTC
Created attachment 103332 [details] [review]
Bug 16371: Add needed Koha::Exception

Add Koha::Exception needed in this bug.

Sponsored-by: Koha-Suomi Oy
Comment 16 Emmi Takkinen 2020-04-21 07:31:38 UTC
Created attachment 103333 [details] [review]
Bug 16371: Quote of the Day (QOTD) - Display in different  interfaces

Set 'QuoteOfTheDay'-syspref to 'opac intranet' to allow displaying news in the
opac and the staff client.

Add 'commandline' to allow news in the commandline (where applicable)

Accepts all values accepted by the C4::Context->interface()

Test plan:
1. Set 'QuoteOfTheDay' syspref as 'intranet' or 'opac' or both
2. Check that quote is visible on both mainpages

Prove t/db_dependent/Koha/Quotes.t

Sponsored-by: Koha-Suomi Oy

---

NOTE: Tests already reworked in this patch so previous patch is now obsolete
Comment 17 Emmi Takkinen 2020-04-21 07:36:23 UTC
Created attachment 103334 [details] [review]
Bug 16371: Use multi-select on syspref

This patch replaces textbox with multi-select dropdown on
syspref 'QuoteOfTheDay' and changes tests to match this change.

To test:
1. Check 'OPAC', 'staff client' or both on 'QuoteOfTheDay'.
2. Make sure that quote is displayed on (both) mainpage(s).

Prove t/db_dependent/Koha/Quotes.t

Sponsored-by: Koha-Suomi Oy

---

NOTE: Noticed that 'display_daily_quote' should have been dropped on original code. Fixed that so this patch just adds multi-select and adjusts tests.
Comment 18 David Nind 2020-04-22 08:22:34 UTC
Sorry, he tests fail 8-(:

root@b749e4f21e73:koha(bz16371)$ prove t/db_dependent/Koha/Quotes.t
t/db_dependent/Koha/Quotes.t .. 1/13 DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Duplicate entry '6' for key 'PRIMARY' [for Statement "INSERT INTO `quotes` ( `id`, `source`, `text`, `timestamp`) VALUES ( ?, ?, ?, ? )" with ParamValues: 0=6, 1='George Washington', 2='To be prepared for war is one of the most effectual means of preserving peace.', 3='2020-04-22 08:19:26'] at /kohadevbox/koha/Koha/Object.pm line 164
Duplicate ID# Looks like your test exited with 255 just after 2.
t/db_dependent/Koha/Quotes.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 11/13 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Quotes.t (Wstat: 65280 Tests: 2 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 13 tests but ran 2.
Files=1, Tests=2,  1 wallclock secs ( 0.02 usr  0.01 sys +  1.53 cusr  0.13 csys =  1.69 CPU)
Result: FAIL
Comment 19 Emmi Takkinen 2020-04-27 06:18:50 UTC
Created attachment 103732 [details] [review]
Bug 16371: Fix tests

Tests failed due ids already existing in 'quotes' table.
This patch removes hardcoded ids from tests.

Sponsored-by: Koha-Suomi Oy
Comment 20 David Nind 2020-04-29 18:04:05 UTC
Created attachment 103973 [details] [review]
Bug 16371: Add Koha::Quote[s] classes

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>
Comment 21 David Nind 2020-04-29 18:04:08 UTC
Created attachment 103974 [details] [review]
Bug 16371: Move GetDailyQuote to get_daily_quote

This patch moves subroutine 'GetDailyQuote' to new
Koha::Quote object and adjusts tests.

To test:
1. Set 'QuoteOfTheDay' as 'enable'
2. Check that quote is displayed on OPAC mainpage

Prove t/db_dependent/Koha/GetDailyQuote.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>
Comment 22 David Nind 2020-04-29 18:04:12 UTC
Created attachment 103975 [details] [review]
Bug 16371: Rename test file

Signed-off-by: David Nind <david@davidnind.com>
Comment 23 David Nind 2020-04-29 18:04:16 UTC
Created attachment 103976 [details] [review]
Bug 16371: Add needed Koha::Exception

Add Koha::Exception needed in this bug.

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>
Comment 24 David Nind 2020-04-29 18:04:19 UTC
Created attachment 103977 [details] [review]
Bug 16371: Quote of the Day (QOTD) - Display in different interfaces

Set 'QuoteOfTheDay'-syspref to 'opac intranet' to allow displaying news in the
opac and the staff client.

Add 'commandline' to allow news in the commandline (where applicable)

Accepts all values accepted by the C4::Context->interface()

Test plan:
1. Set 'QuoteOfTheDay' syspref as 'intranet' or 'opac' or both
2. Check that quote is visible on both mainpages

Prove t/db_dependent/Koha/Quotes.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2020-04-29 18:04:23 UTC
Created attachment 103978 [details] [review]
Bug 16371: Use multi-select on syspref

This patch replaces textbox with multi-select dropdown on
syspref 'QuoteOfTheDay' and changes tests to match this change.

To test:
1. Check 'OPAC', 'staff client' or both on 'QuoteOfTheDay'.
2. Make sure that quote is displayed on (both) mainpage(s).

Prove t/db_dependent/Koha/Quotes.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 David Nind 2020-04-29 18:04:26 UTC
Created attachment 103979 [details] [review]
Bug 16371: Fix tests

Tests failed due ids already existing in 'quotes' table.
This patch removes hardcoded ids from tests.

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>
Comment 27 Katrin Fischer 2020-06-22 23:03:05 UTC
I am sorry Emmi, but this does no longer apply - can you please rebase?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 16371: Quote of the Day (QOTD) - Display in different interfaces
.git/rebase-apply/patch:208: trailing whitespace.
    
error: sha1 information is lacking or useless (Koha/Quote.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 16371: Quote of the Day (QOTD) - Display in different interfaces
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-16371-Quote-of-the-Day-QOTD---Display-in-diffe-AWZo8w.patch
Comment 28 Emmi Takkinen 2020-06-23 06:34:04 UTC
 (In reply to Katrin Fischer from comment #27)
> I am sorry Emmi, but this does no longer apply - can you please rebase?

Patch should now apply. Attachment 'Bug 16371: Quote of the Day (QOTD) - Display in different interfaces' hadn't been made obsolete at some point and was applied first. That's why whole thing failed.
Comment 29 Katrin Fischer 2020-07-04 20:51:22 UTC
Hi Emmi,

patches apply now, sorry for missing the patch double up.

0) There are some problems highlighted by the QA tools:

FAIL	Koha/Quote.pm
   FAIL	  critic
		"return" statement with explicit "undef" at line 129, column 9. See page 199 of PBP.

 FAIL	Koha/Quotes.pm
   FAIL	  pod coverage
		POD is missing for 'object_class'

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/intranet-main.tt
   FAIL	  filters
		missing_filter at line 41 (                            <span id="daily-quote-text">[% daily_quote.text %]</span><span id="daily-quote-sep"> ~ </span><span id="daily-quote-source">[% daily_quote.source %]</span>)
		missing_filter at line 41 (                            <span id="daily-quote-text">[% daily_quote.text %]</span><span id="daily-quote-sep"> ~ </span><span id="daily-quote-source">[% daily_quote.source %]</span>)


1) I see that you moved the routine to the new namespace, but it appears to be unchanged. Especially the SQL parts I think should be replaced/rewritten.

2) I like making the old sys pref multiple choice :) Can you please use staff interface? 
https://wiki.koha-community.org/wiki/Terminology

3) I think we need a database update to change the type of the system preference in the database from YesNo to multi.
Comment 30 Emmi Takkinen 2020-07-13 06:31:36 UTC
Created attachment 106818 [details] [review]
Bug 16371: Fix QA issues, change terminology
Comment 31 Emmi Takkinen 2020-07-13 06:32:05 UTC
Created attachment 106819 [details] [review]
Bug 16371: Rewrite get_daily_quote
Comment 32 Emmi Takkinen 2020-07-13 06:33:48 UTC
Created attachment 106820 [details] [review]
Bug 16371: Add database updates
Comment 33 David Nind 2020-07-13 11:02:45 UTC
Everything works as expected, however the tests fail! 8-(

prove t/db_dependent/Koha/Quotes.t
t/db_dependent/Koha/Quotes.t .. 1/13 Can't use string ("Koha::Quote") as a HASH ref while "strict refs" in use at /kohadevbox/koha/Koha/Object.pm line 735.
# Looks like your test exited with 255 just after 2.
t/db_dependent/Koha/Quotes.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 11/13 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Quotes.t (Wstat: 65280 Tests: 2 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 13 tests but ran 2.
Files=1, Tests=2,  2 wallclock secs ( 0.03 usr  0.00 sys +  1.63 cusr  0.16 csys =  1.82 CPU)
Result: FAIL

An updated test plan:
1. Apply the patch.
2. Run the database update (updatedatabase on koha-testing-docker).
3. Check that the 'QuoteOfTheDay' system preference options
   work as expected:
   - OPAC: QOTD only appears in the OAPC
   - Staff interface: QOTD only appears in the staff interface
   - Both (Select all): QOTD appears in the staff interface and OPAC
4. Run the tests and make sure they pass:
   prove t/db_dependent/Koha/Quotes.t
5. Sign off!
Comment 34 Emmi Takkinen 2020-07-13 12:10:50 UTC
(In reply to David Nind from comment #33)
> Everything works as expected, however the tests fail! 8-(
> 
> prove t/db_dependent/Koha/Quotes.t
> t/db_dependent/Koha/Quotes.t .. 1/13 Can't use string ("Koha::Quote") as a
> HASH ref while "strict refs" in use at /kohadevbox/koha/Koha/Object.pm line
> 735.
> # Looks like your test exited with 255 just after 2.
> t/db_dependent/Koha/Quotes.t .. Dubious, test returned 255 (wstat 65280,
> 0xff00)
> Failed 11/13 subtests 

Sorry, I was so focused on getting random quote work that testing changes I made to get quote with id slipped my mind (and also actually checking if tests still work). I'll provide patch with fix as soon as possible.

And thanks for new test plan!
Comment 35 Emmi Takkinen 2020-07-14 07:08:25 UTC
Created attachment 106858 [details] [review]
Bug 16371: Rewrite get_daily_quote

An updated test plan:
1. Apply the patch(es).
2. Run the database update (updatedatabase on koha-testing-docker).
3. Check that the 'QuoteOfTheDay' system preference options
   work as expected:
   - OPAC: QOTD only appears in the OAPC
   - Staff interface: QOTD only appears in the staff interface
   - Both (Select all): QOTD appears in the staff interface and OPAC
4. Run the tests and make sure they pass:
   prove t/db_dependent/Koha/Quotes.t
5. Sign off!

----
Fixed errors on get_daily_quote and adjusted tests.
Comment 36 David Nind 2020-07-14 08:33:02 UTC
Created attachment 106859 [details] [review]
Bug 16371: Add Koha::Quote[s] classes

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 37 David Nind 2020-07-14 08:33:07 UTC
Created attachment 106860 [details] [review]
Bug 16371: Move GetDailyQuote to get_daily_quote

This patch moves subroutine 'GetDailyQuote' to new
Koha::Quote object and adjusts tests.

To test:
1. Set 'QuoteOfTheDay' as 'enable'
2. Check that quote is displayed on OPAC mainpage

Prove t/db_dependent/Koha/GetDailyQuote.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 38 David Nind 2020-07-14 08:33:11 UTC
Created attachment 106861 [details] [review]
Bug 16371: Rename test file

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 39 David Nind 2020-07-14 08:33:15 UTC
Created attachment 106862 [details] [review]
Bug 16371: Add needed Koha::Exception

Add Koha::Exception needed in this bug.

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 40 David Nind 2020-07-14 08:33:19 UTC
Created attachment 106863 [details] [review]
Bug 16371: Quote of the Day (QOTD) - Display in different interfaces

Set 'QuoteOfTheDay'-syspref to 'opac intranet' to allow displaying news in the
opac and the staff client.

Add 'commandline' to allow news in the commandline (where applicable)

Accepts all values accepted by the C4::Context->interface()

Test plan:
1. Set 'QuoteOfTheDay' syspref as 'intranet' or 'opac' or both
2. Check that quote is visible on both mainpages

Prove t/db_dependent/Koha/Quotes.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 41 David Nind 2020-07-14 08:33:23 UTC
Created attachment 106864 [details] [review]
Bug 16371: Use multi-select on syspref

This patch replaces textbox with multi-select dropdown on
syspref 'QuoteOfTheDay' and changes tests to match this change.

To test:
1. Check 'OPAC', 'staff client' or both on 'QuoteOfTheDay'.
2. Make sure that quote is displayed on (both) mainpage(s).

Prove t/db_dependent/Koha/Quotes.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 42 David Nind 2020-07-14 08:33:27 UTC
Created attachment 106865 [details] [review]
Bug 16371: Fix tests

Tests failed due ids already existing in 'quotes' table.
This patch removes hardcoded ids from tests.

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 43 David Nind 2020-07-14 08:33:31 UTC
Created attachment 106866 [details] [review]
Bug 16371: Fix QA issues, change terminology

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>
Comment 44 David Nind 2020-07-14 08:33:35 UTC
Created attachment 106867 [details] [review]
Bug 16371: Add database updates

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>
Comment 45 David Nind 2020-07-14 08:33:39 UTC
Created attachment 106868 [details] [review]
Bug 16371: Rewrite get_daily_quote

An updated test plan:
1. Apply the patch(es).
2. Run the database update (updatedatabase on koha-testing-docker).
3. Check that the 'QuoteOfTheDay' system preference options
   work as expected:
   - OPAC: QOTD only appears in the OAPC
   - Staff interface: QOTD only appears in the staff interface
   - Both (Select all): QOTD appears in the staff interface and OPAC
4. Run the tests and make sure they pass:
   prove t/db_dependent/Koha/Quotes.t
5. Sign off!

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>
Comment 46 Katrin Fischer 2020-07-15 06:34:43 UTC
Created attachment 106894 [details] [review]
Bug 16371: Add Koha::Quote[s] classes

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 47 Katrin Fischer 2020-07-15 06:34:48 UTC
Created attachment 106895 [details] [review]
Bug 16371: Move GetDailyQuote to get_daily_quote

This patch moves subroutine 'GetDailyQuote' to new
Koha::Quote object and adjusts tests.

To test:
1. Set 'QuoteOfTheDay' as 'enable'
2. Check that quote is displayed on OPAC mainpage

Prove t/db_dependent/Koha/GetDailyQuote.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 48 Katrin Fischer 2020-07-15 06:34:53 UTC
Created attachment 106896 [details] [review]
Bug 16371: Rename test file

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 49 Katrin Fischer 2020-07-15 06:34:58 UTC
Created attachment 106897 [details] [review]
Bug 16371: Add needed Koha::Exception

Add Koha::Exception needed in this bug.

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 50 Katrin Fischer 2020-07-15 06:35:02 UTC
Created attachment 106898 [details] [review]
Bug 16371: Quote of the Day (QOTD) - Display in different interfaces

Set 'QuoteOfTheDay'-syspref to 'opac intranet' to allow displaying news in the
opac and the staff client.

Add 'commandline' to allow news in the commandline (where applicable)

Accepts all values accepted by the C4::Context->interface()

Test plan:
1. Set 'QuoteOfTheDay' syspref as 'intranet' or 'opac' or both
2. Check that quote is visible on both mainpages

Prove t/db_dependent/Koha/Quotes.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 51 Katrin Fischer 2020-07-15 06:35:07 UTC
Created attachment 106899 [details] [review]
Bug 16371: Use multi-select on syspref

This patch replaces textbox with multi-select dropdown on
syspref 'QuoteOfTheDay' and changes tests to match this change.

To test:
1. Check 'OPAC', 'staff client' or both on 'QuoteOfTheDay'.
2. Make sure that quote is displayed on (both) mainpage(s).

Prove t/db_dependent/Koha/Quotes.t

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 52 Katrin Fischer 2020-07-15 06:35:11 UTC
Created attachment 106900 [details] [review]
Bug 16371: Fix tests

Tests failed due ids already existing in 'quotes' table.
This patch removes hardcoded ids from tests.

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 53 Katrin Fischer 2020-07-15 06:35:15 UTC
Created attachment 106901 [details] [review]
Bug 16371: Fix QA issues, change terminology

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 54 Katrin Fischer 2020-07-15 06:35:20 UTC
Created attachment 106902 [details] [review]
Bug 16371: Add database updates

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 55 Katrin Fischer 2020-07-15 06:35:24 UTC
Created attachment 106903 [details] [review]
Bug 16371: Rewrite get_daily_quote

An updated test plan:
1. Apply the patch(es).
2. Run the database update (updatedatabase on koha-testing-docker).
3. Check that the 'QuoteOfTheDay' system preference options
   work as expected:
   - OPAC: QOTD only appears in the OAPC
   - Staff interface: QOTD only appears in the staff interface
   - Both (Select all): QOTD appears in the staff interface and OPAC
4. Run the tests and make sure they pass:
   prove t/db_dependent/Koha/Quotes.t
5. Sign off!

Sponsored-by: Koha-Suomi Oy

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 56 Jonathan Druart 2020-07-20 13:24:31 UTC
I think Koha::Quote[s] is too generic, I am asking a couple of other QAers for feedback.
Comment 57 Tomás Cohen Arazi 2020-07-20 15:10:06 UTC
(In reply to Jonathan Druart from comment #56)
> I think Koha::Quote[s] is too generic, I am asking a couple of other QAers
> for feedback.

I think if we had some feature to place a RFQ or quote, it would be placed inside the Koha::Acquisitions namespace, and so we can move forward with this naming.

I'd wait for some native users feedback, though.

Well done everyone!
Comment 58 Jonathan Druart 2020-07-20 16:37:30 UTC
Also, we don't need the weird "Koha::Exceptions::UnknownProgramState" exception, C4::Context->interface() is always returning something.
Comment 59 Martin Renvoize 2020-07-20 18:52:16 UTC
"Cite" as in "Citations" is the only real synonym I can come up with.. but it's not especially clear to me.. I find that more confusing the "Quote" I think...

Not read the patches yet so, just seen the comment with the question.
Comment 60 Katrin Fischer 2020-07-20 20:40:57 UTC
(In reply to Martin Renvoize from comment #59)
> "Cite" as in "Citations" is the only real synonym I can come up with.. but
> it's not especially clear to me.. I find that more confusing the "Quote" I
> think...
> 
> Not read the patches yet so, just seen the comment with the question.

I think having another mismatch between GUI and code would not be idea - for this reason I think
Comment 61 Katrin Fischer 2020-07-20 20:41:28 UTC
(In reply to Martin Renvoize from comment #59)
> "Cite" as in "Citations" is the only real synonym I can come up with.. but
> it's not especially clear to me.. I find that more confusing the "Quote" I
> think...
> 
> Not read the patches yet so, just seen the comment with the question.

I think having another mismatch between GUI and code would not be idea - for this reason I think Quote woudl be best actually.
Comment 62 Jonathan Druart 2020-07-21 08:36:31 UTC
Quote vs DailyQuote vs QuoteOfTheDay?

I think Koha::DailyQuote could be more meaningful.
Comment 63 Katrin Fischer 2020-07-21 11:35:42 UTC
More meaningful, but the daily could change at some point. I'd still be ok with that as a compromise.
Comment 64 Martin Renvoize 2020-07-21 12:59:46 UTC
Personally.. I think Koha::Quote is fine.. for other uses I'm envisaging they're more nested.. i.e Acquisition::Quotes as Tomas highlighted.
Comment 65 Jonathan Druart 2020-07-21 14:50:28 UTC
Hum, there are still occurrences of GetDailyQuote in t/db_dependent/Koha.t
Comment 66 Jonathan Druart 2020-07-21 14:58:26 UTC
Better in only one comment:

1. We don't need the weird "Koha::Exceptions::UnknownProgramState" exception, C4::Context->interface() is always returning something.

2. There are still occurrences of GetDailyQuote in t/db_dependent/Koha.t (the tests are failing)

3. The methods from Koha::Quote must be in Koha::Quotes

4. +use DBI qw(:sql_types);
We don't need that

5. You are adding raw SQL queries in Koha namespace, that is not needed.

6. After 5, DateTime::Format::MySQL become useless

7. I don't think we need the 2 methods in Koha::Quotes, could be only 1, get_daily_quote
Comment 67 Emmi Takkinen 2020-08-11 06:15:29 UTC
(In reply to Jonathan Druart from comment #66)

> 2. There are still occurrences of GetDailyQuote in t/db_dependent/Koha.t
> (the tests are failing)
> 
Didn't notice these until now. But looking them now it seems they work exactly like tests in Quotes.t. Could these be removed at this point?

> 5. You are adding raw SQL queries in Koha namespace, that is not needed.
> 
> 6. After 5, DateTime::Format::MySQL become useless
> 
Sorry for being dense, but I'm failing to see what queries you mean.
Comment 68 Jonathan Druart 2020-08-11 08:31:18 UTC
(In reply to Emmi Takkinen from comment #67)
> (In reply to Jonathan Druart from comment #66)
> 
> > 2. There are still occurrences of GetDailyQuote in t/db_dependent/Koha.t
> > (the tests are failing)
> > 
> Didn't notice these until now. But looking them now it seems they work
> exactly like tests in Quotes.t. Could these be removed at this point?

Yes sure.

> > 5. You are adding raw SQL queries in Koha namespace, that is not needed.
> > 
> > 6. After 5, DateTime::Format::MySQL become useless
> > 
> Sorry for being dense, but I'm failing to see what queries you mean.

Looks like I forgot to review with the last patch!

Then 5 is not valid anymore, however:
 70                 timestamp => { -like => "$dt%" },

This is a bit unusual to me, but I cannot think of something else (apart from using \'CAST() that is not better). I will ask a couple of QAers how we should do that.

 94         # update the timestamp for that quote
 95         my $dt = DateTime::Format::MySQL->format_datetime(dt_from_string());
 96         $quote->update({ timestamp => $dt });

You should not use DateTime::Format::MySQL directly. If you need to use a date in a search you should use $dtf->format_date[time] instead
Comment 69 Jonathan Druart 2020-08-11 09:34:54 UTC
(In reply to Jonathan Druart from comment #68)
> (In reply to Emmi Takkinen from comment #67)
> > (In reply to Jonathan Druart from comment #66)
>  70                 timestamp => { -like => "$dt%" },
> 
> This is a bit unusual to me, but I cannot think of something else (apart
> from using \'CAST() that is not better). I will ask a couple of QAers how we
> should do that.

We got an answer from #dbix-class, we should use -between => [ "$dt 00:00:00", "$dt 23:59:59" ], to be DB agnostic and allow use of indexes.
$dt must be generated using $dtf->format_date.
Comment 70 Emmi Takkinen 2020-08-12 11:45:11 UTC
Created attachment 108097 [details] [review]
Bug 16371: Remove GetDailyQuote tests from Koha.t

Tests in Koha.t failed due GetDailyQuote tests. This patch
removes these tests since we test quotes in Quotes.t instead.

To test prove t/db_dependent/Koha.t

Sponsored-by: Koha-Suomi Oy
Comment 71 Emmi Takkinen 2020-08-12 11:47:44 UTC
Created attachment 108098 [details] [review]
Bug 16371: Combine get_daily_quote and get_daily_quote_for_interface

This patch combines get_daily_quote and get_daily_quote_for_interface
methods and moves them from Koha::Quote to Koha::Quotes. Also removes
some unused code and adjusts datetime parsing.

To test apply this patch and confirm 'QuoteOfTheDay' syspref still
works as expected (quote is shown in correct mainpage(s)).

Also prove t/db_dependent/Koha/Quotes.t

Sponsored-by: Koha-Suomi Oy
Comment 72 Jonathan Druart 2020-08-13 07:59:35 UTC
Created attachment 108160 [details] [review]
Bug 16371: Fix inconsistencies with timestamp

The timestamp value needs to be updated for each quote returned by
get_daily_quote (even when there is a match)
Comment 73 Jonathan Druart 2020-08-13 08:19:08 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 74 Jonathan Druart 2020-08-13 08:29:00 UTC
Created attachment 108163 [details] [review]
Bug 16371: (follow-up) DB changes

We need to keep "opac" is the pref was set before the upgrade
Comment 75 Lucas Gass 2020-08-17 15:47:56 UTC
enhancement will not be backported to 20.05.x