|
Description
Olli-Antti Kivilahti
2016-04-27 16:21:13 UTC
No patch has been attached! Created attachment 102515 [details] [review] Bug 16371: Add needed Koha::Exception Add Koha::Exception needed in this bug. Sponsored-by: Koha-Suomi Oy 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 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 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 :( It could also be a multi-select with 2 checkboxes for staff and OPAC showing up. 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 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. 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> 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 (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. Created attachment 103329 [details] [review] Bug 16371: Add Koha::Quote[s] classes 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 Created attachment 103331 [details] [review] Bug 16371: Rename test file Created attachment 103332 [details] [review] Bug 16371: Add needed Koha::Exception Add Koha::Exception needed in this bug. Sponsored-by: Koha-Suomi Oy 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 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. 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 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 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> 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> Created attachment 103975 [details] [review] Bug 16371: Rename test file Signed-off-by: David Nind <david@davidnind.com> 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> 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> 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> 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> 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 (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. 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. Created attachment 106818 [details] [review] Bug 16371: Fix QA issues, change terminology Created attachment 106819 [details] [review] Bug 16371: Rewrite get_daily_quote Created attachment 106820 [details] [review] Bug 16371: Add database updates 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!
(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! 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. 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 106867 [details] [review] Bug 16371: Add database updates Sponsored-by: Koha-Suomi Oy Signed-off-by: David Nind <david@davidnind.com> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> I think Koha::Quote[s] is too generic, I am asking a couple of other QAers for feedback. (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! Also, we don't need the weird "Koha::Exceptions::UnknownProgramState" exception, C4::Context->interface() is always returning something. "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. (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 (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. Quote vs DailyQuote vs QuoteOfTheDay? I think Koha::DailyQuote could be more meaningful. More meaningful, but the daily could change at some point. I'd still be ok with that as a compromise. Personally.. I think Koha::Quote is fine.. for other uses I'm envisaging they're more nested.. i.e Acquisition::Quotes as Tomas highlighted. Hum, there are still occurrences of GetDailyQuote in t/db_dependent/Koha.t 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 (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. (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 (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. 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 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 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) Pushed to master for 20.11, thanks to everybody involved! Created attachment 108163 [details] [review] Bug 16371: (follow-up) DB changes We need to keep "opac" is the pref was set before the upgrade enhancement will not be backported to 20.05.x |