Bug 13287 - Add a system preference to define the number of days used in purge_suggestions.pl
Summary: Add a system preference to define the number of days used in purge_suggestion...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Rémi Mayrand-Provencher
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 16280
  Show dependency treegraph
 
Reported: 2014-11-18 15:37 UTC by simith.doliveira
Modified: 2018-04-06 15:42 UTC (History)
18 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Apart from introducing the new preference PurgeSuggestionsOlderThan, this report also adds a -confirm flag to the cron job purge_suggestions.pl. Please adjust existing cron tab files and add this flag in order to have the expected results.


Attachments
Add a system preference to define the number of days used in purge_suggestions.pl (5.37 KB, patch)
2014-11-18 16:28 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug13287 - Add a system preference to define the number of days used in purge_suggestions.pl (5.26 KB, patch)
2015-07-28 16:17 UTC, Rémi Mayrand-Provencher
Details | Diff | Splinter Review
Bug13287 - Add a system preference to define the number of days used in purge_suggestions.pl (5.33 KB, patch)
2015-11-03 01:05 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 13287: Follow up (4.98 KB, patch)
2016-02-24 00:38 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 13287: QA Follow up (4.96 KB, patch)
2016-02-24 00:42 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug13287 - Add a system preference to define the number of days used in purge_suggestions.pl (5.47 KB, patch)
2016-04-17 16:30 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 13287: QA Follow up (5.10 KB, patch)
2016-04-17 16:31 UTC, Marc Véron
Details | Diff | Splinter Review
Bug13287 - Add a system preference to define the number of days used in purge_suggestions.pl (5.44 KB, patch)
2017-02-22 18:59 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 13287: QA Follow up (5.05 KB, patch)
2017-02-22 19:00 UTC, Mehdi Hamidi
Details | Diff | Splinter Review
Bug 13287: QA Follow up (5.10 KB, patch)
2017-10-16 17:52 UTC, Jon Knight
Details | Diff | Splinter Review
Bug13287 - Add a system preference to define the number of days used in purge_suggestions.pl (5.48 KB, patch)
2017-11-14 16:29 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 13287: QA Follow up (5.10 KB, patch)
2017-11-14 16:29 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 13287: Add a system preference to define the number of days used in purge_suggestions.pl (5.49 KB, patch)
2018-02-23 10:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13287: (QA follow-up) Add db revision (6.65 KB, patch)
2018-02-23 10:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13287: (QA follow-up) Prepare Suggestions.t for changes (2.55 KB, patch)
2018-02-23 10:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13287: (QA follow-up) Extend Suggestions.t for DelSuggestionsOlderThan (3.79 KB, patch)
2018-02-23 10:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13287: (QA follow-up) Final polishing (3.67 KB, patch)
2018-02-23 10:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 13287: Remove warning if --days is not passed and pref empty (927 bytes, patch)
2018-02-26 16:02 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description simith.doliveira 2014-11-18 15:37:51 UTC
The system preferences value is used whenever purge_suggestions.pl is called without the 'days' parameter.
Comment 1 simith.doliveira 2014-11-18 15:55:22 UTC
Testing:

I Apply the patch
II Run updatedatabase.pl

a) Run purge_suggestion.pl without the days parameter
   - validate error message
b) Run purge_suggestion.pl with the days parameter 
   - validate no error message
c) Insert a number of days in the system variable PurgeSuggestionsOlderThan
d) Run purge_suggestion.pl without the days parameter 
   - validate no error message
Comment 2 simith.doliveira 2014-11-18 16:28:51 UTC Comment hidden (obsolete)
Comment 3 simith.doliveira 2014-11-19 19:27:29 UTC
Sponsored-by: CCSR ( http://www.ccsr.qc.ca )
Comment 4 Marcel de Rooy 2015-01-28 13:43:04 UTC
Hi Simith,
Thanks for writing your patch, but I do have a question:

Although we have some cron jobs that use or even fall back to preferences, it is not the default solution. Normally, cronjobs should just get their parameters from the command line. If you do not pass a command line parameter and you do not expect the cronjob to use a pref as fallback, you could suddenly have lost your suggestions unwillingly. So, could you give a very good reason why the script still should fall back to the pref value? Isn't it true that you set this cmd line parameter once in your crontab and do not touch it again?

Marcel
Comment 5 Eric Bégin 2015-01-29 19:08:20 UTC
Thanks Marcel for the opportunity to clarify why a syspref is within scripts executed via a cronjobs.

The main reason is to give the libraries a control to those parameters without having to constantly get back to their service provider in order to change a parameter on the cronjob command line.
Comment 6 Mirko Tietgen 2015-02-25 22:26:59 UTC
How often is a value like this changed? I would expect this to be set once and probably never touched again?
Comment 7 Katrin Fischer 2015-02-25 22:29:34 UTC
I think there is a use case for making the options easy to configure from the gui - multiple instances hosted on one server. Then every instance can decide on their defaults and you don't have to touch the cronjobs to make it happen.
Comment 8 Mirko Tietgen 2015-02-25 22:49:27 UTC
Right, I did not think about that.
Comment 9 Marc Véron 2015-03-21 06:01:27 UTC
I agree with Eric and Katrin.

The patch works as expected, however I think the explanation in syspref needs some changes:

--Original-------
Keep purchase suggestions for a period of [   ] days.
WARNING - Leave this field empty if you don't want to activate this automatic feature.
Example: [30] Sets purgation of suggestions for those older than 30 days.
(Used when the purge_suggestions.pl script is called with a specific number of days)
------------------

--Proposition-----
Keep accepted or rejected purchase suggestions for a period of [   ] days.
WARNING - Leave this field empty if you don't want to activate this automatic feature.
Example: [30] Sets purgation of suggestions for those older than 30 days.
(Used when the cronjob purge_suggestions.pl is active and called without a specific number of days)
------------------

Additionally, I would like to see some more explanation in the manual
http://manual.koha-community.org/latest/en/cronjobsch.html#cronjobs
1.9.1. Clean up old suggestions
Comment 10 M. Tompsett 2015-03-30 04:49:18 UTC
Comment on attachment 33661 [details] [review]
Add a system preference to define the number of days used in purge_suggestions.pl

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

::: misc/cronjobs/purge_suggestions.pl
@@ +47,5 @@
> +-help|? This message
> +-days TTT to define the age of suggestions to delete
> +
> +Example:
> +$PERL5LIB/misc/cronjobs/purge_suggestions.pl -days 30

This line change seems wrong to me.
It doesn't interpret $PERL5LIB, and this is a drastic change from what was already there. And though what was there is a little unclear to the average user, perhaps it is better?

@@ +68,4 @@
>      DelSuggestionsOlderThan($days);
>  }
> +elsif(defined($days) && $days == 0) {
> +    print << 'ERROR';

Seriously, why? There is no need to << 'LABEL ... LABEL this. It's a waste of vertical space.

@@ +72,5 @@
> +    This script is not executed with 0 days. Aborted.
> +ERROR
> +}
> +else {
> +    print << 'ERROR';

Why not use:
    print STDERR "This script requires a positive number of days. Aborted.";
Comment 11 M. Tompsett 2015-03-30 04:56:50 UTC
Comment on attachment 33661 [details] [review]
Add a system preference to define the number of days used in purge_suggestions.pl

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
@@ +561,5 @@
> +              class: integer
> +            - days.
> +            - <br>WARNING - Leave this field empty if you don't want to activate this automatic feature.
> +            - "<br>Example: [30] Sets purgation of suggestions for those older than 30 days."
> +            - <br>(Used when the purge_suggestions.pl script is called with a specific number of days)

As per comment #9. This is unclear. This feature is only used if the --days ### command line parameter is not passed. This last line makes it sound like it is used instead of the command line value.
Comment 12 M. Tompsett 2015-03-30 04:59:41 UTC
Marking as 'Failed QA', since 'In Discussion' tends to head into 'Never get looked at again' land.
Comment 13 Rémi Mayrand-Provencher 2015-07-28 16:17:35 UTC Comment hidden (obsolete)
Comment 14 Liz Rea 2015-11-03 01:05:21 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2015-11-08 21:36:00 UTC
Changing this to enh - it's a good one, but not a bug.
Comment 16 Kyle M Hall 2015-11-20 15:58:08 UTC
I would like to see this script die with errors if both the command line switch and the system preference are set. I can see a scenario where a sysadmin changes the value on the switch but it doesn't change the purge days!
Comment 17 Rémi Mayrand-Provencher 2016-02-11 15:19:01 UTC
I think the aim of this patch was to allow sysadmins to set a default value to the number of days that should be used in the script while still allowing them to call a custom number of days directly from the command line. Right now, this is exactly what it's doing. It only changes the "days" parameter if the system preference is set, and if both are set, the script will use the value received from the command line.

My point is that I am not sure that I understand the problem in your scenario, could you tell me whats wrong?

Thank you!
Comment 18 Katrin Fischer 2016-02-23 22:21:22 UTC
I think maybe the question here is what should have priority - the command line option or the system preference setting. Both make sense to me in some circumstance. 

A suggestion for consistency: the prefs are only be used if the script is called without the parameters - like it was done for the longoverdue script in bug 8337.
Comment 19 M. Tompsett 2016-02-24 00:38:36 UTC Comment hidden (obsolete)
Comment 20 M. Tompsett 2016-02-24 00:42:24 UTC Comment hidden (obsolete)
Comment 21 Marc Véron 2016-04-17 14:26:13 UTC
misc/cronjobs/purge_suggestions.pl -days -1
OK

misc/cronjobs/purge_suggestions.pl -days 0
OK

misc/cronjobs/purge_suggestions.pl -days 10

Result:

Name "Tie::Hash::FIELDS" used only once: possible typo at /usr/share/perl/5.14/fields.pm line 135.
Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo at /usr/share/perl/5.14/fields.pm line 135.
Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo at /usr/share/perl/5.14/fields.pm line 135.
Use of uninitialized value $_[0] in lc at /usr/share/perl5/Cache.pm line 485.
Comment 22 Marc Véron 2016-04-17 14:30:13 UTC
(In reply to Marc Véron from comment #21)
> misc/cronjobs/purge_suggestions.pl -days -1
> OK
> 
> misc/cronjobs/purge_suggestions.pl -days 0
> OK
> 
> misc/cronjobs/purge_suggestions.pl -days 10
> 
> Result:
> 
> Name "Tie::Hash::FIELDS" used only once: possible typo at
> /usr/share/perl/5.14/fields.pm line 135.
> Name "Cache::RemovalStrategy::FIELDS" used only once: possible typo at
> /usr/share/perl/5.14/fields.pm line 135.
> Name "Cache::RemovalStrategy::LRU::FIELDS" used only once: possible typo at
> /usr/share/perl/5.14/fields.pm line 135.
> Use of uninitialized value $_[0] in lc at /usr/share/perl5/Cache.pm line 485.

Ah, it happens with and without patch...
Comment 23 Marc Véron 2016-04-17 15:18:32 UTC
Related to Bug 16104 Warnings "used only once: possible typo" should be removed
Comment 24 Marc Véron 2016-04-17 16:30:09 UTC
Created attachment 50296 [details] [review]
Bug13287 - Add a system preference to define the number of days used in purge_suggestions.pl

    The system preferences value is used whenever purge_suggestions.pl is called without the 'days' parameter.
    This patch uses the preference description suggested by comment #9.
    This version should now be cleanly applicable.

    I Apply the patch
    II Run updatedatabase.pl

    a) Run purge_suggestions.pl without the days parameter
       - validate that there is an error message
    b) Run purge_suggestions.pl with the days parameter
       - validate that there is no error message
    c) Insert a number of days in the system variable PurgeSuggestionsOlderThan
    d) Run purge_suggestions.pl without the days parameter
       - validate that there is no error message

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Tested per plan, all tests pass.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 25 Marc Véron 2016-04-17 16:31:00 UTC
Created attachment 50297 [details] [review]
Bug 13287: QA Follow up

Added missing upgrade SQL system preference.
Corrected system preference screen message
Fixes on purge_suggestions.pl
- perlcritic friendlier
- address $PERL5LIB comment by using $PROGRAM_NAME (comment #10)
- used STDERR (comment #10)
- perltidy

TEST PLAN
---------
$ ./installer/data/mysql/updatedatabase.pl
-- should run upgrade and generate new systempreference in table
$ ./misc/cronjobs/purge_suggestions.pl --help
-- should give help with a real path used instead of $PERL5LIB.
$ ./misc/cronjobs/purge_suggestions.pl -days -1
-- should give error message as expected
$ ./misc/cronjobs/purge_suggestions.pl -days 0
-- should give error message as expected

Go to OPAC system preferences tab and check the
PurgeSuggestionsOlderThan system preference
-- message should be as expected (see comment #9)

run koha qa test tools
-- all should pass

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 26 Jonathan Druart 2016-04-18 09:58:09 UTC
Suggestions:
What about moving all the cronjob prefs to a new "cronjob" tab?
What about a new pref to turn all of these prefs on/off?


Review:
1/ Change in the behavior if the script is called without parameter (help is not displayed anymore).
I think we should add a confirm flag if the pref will be used and --days is not given.
2/ +use English qw( -no_match_vars );
why that?
3/ It would have been good to use Pod2usage

Failing QA for 1.
Comment 27 Katrin Fischer 2016-04-18 20:26:01 UTC
Just a note here - please think if systems with multiple instances. I have run into several problems with other cronjobs lately :( It would be nice if a move to sysprefs allows you to schedule it for all instances, while it will only be active for ones with the pref set. Not sure if it's doable without behaviour change... but you can make wishes, right? :)
Comment 28 Mehdi Hamidi 2017-02-22 18:59:56 UTC
Created attachment 60569 [details] [review]
Bug13287 - Add a system preference to define the number of days used in purge_suggestions.pl

The system preferences value is used whenever purge_suggestions.pl is called without the 'days' parameter.
    This patch uses the preference description suggested by comment #9.
    This version should now be cleanly applicable.

    I Apply the patch
    II Run updatedatabase.pl

    a) Run purge_suggestions.pl without the days parameter
       - validate that there is an error message
    b) Run purge_suggestions.pl with the days parameter
       - validate that there is no error message
    c) Insert a number of days in the system variable PurgeSuggestionsOlderThan
    d) Run purge_suggestions.pl without the days parameter
       - validate that there is no error message

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Tested per plan, all tests pass.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 29 Mehdi Hamidi 2017-02-22 19:00:07 UTC
Created attachment 60570 [details] [review]
Bug 13287: QA Follow up

Added missing upgrade SQL system preference.
Corrected system preference screen message
Fixes on purge_suggestions.pl
- perlcritic friendlier
- address $PERL5LIB comment by using $PROGRAM_NAME (comment #10)
- used STDERR (comment #10)
- perltidy

TEST PLAN
---------
$ ./installer/data/mysql/updatedatabase.pl
-- should run upgrade and generate new systempreference in table
$ ./misc/cronjobs/purge_suggestions.pl --help
-- should give help with a real path used instead of $PERL5LIB.
$ ./misc/cronjobs/purge_suggestions.pl -days -1
-- should give error message as expected
$ ./misc/cronjobs/purge_suggestions.pl -days 0
-- should give error message as expected

Go to OPAC system preferences tab and check the
PurgeSuggestionsOlderThan system preference
-- message should be as expected (see comment #9)

run koha qa test tools
-- all should pass

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 30 Mehdi Hamidi 2017-02-22 19:07:37 UTC
The patch has been rebased on the current master.
I made some changed to base on Jonathan comments.

(In reply to Jonathan Druart from comment #26)
> Suggestions:
> What about moving all the cronjob prefs to a new "cronjob" tab?
> What about a new pref to turn all of these prefs on/off?
> 
> 
> Review:
> 1/ Change in the behavior if the script is called without parameter (help is
> not displayed anymore).
> I think we should add a confirm flag if the pref will be used and --days is
> not given.
> 2/ +use English qw( -no_match_vars );
> why that?
> 3/ It would have been good to use Pod2usage
> 
> Failing QA for 1.

1- The script now show help if no parameter is passed and the system preference is not set. 
2- I removed this line, apparently $PROGRAM_NAME needed it, so I changed the variable to $0 to display the name (relative path) of the script. 
3- Maybe another patch.


(In reply to Katrin Fischer from comment #27)
> Just a note here - please think if systems with multiple instances. I have
> run into several problems with other cronjobs lately :( It would be nice if
> a move to sysprefs allows you to schedule it for all instances, while it
> will only be active for ones with the pref set. Not sure if it's doable
> without behaviour change... but you can make wishes, right? :)

We'll keep this in mind :) This was just a rebase and minor changes.
Comment 31 Dominic Pichette 2017-09-29 20:17:34 UTC
Just installed the patch and did step 1 and 2. though running the command at step 2 gives me this:

export PERL5LIB=/path/to/koha;export KOHA_CONF=/etc/koha/koha-conf.xml;./purge_suggestions.pl -days 30

I understand it is not the result you want?
Comment 32 Jon Knight 2017-10-16 17:52:53 UTC
Created attachment 68187 [details] [review]
Bug 13287: QA Follow up

Added missing upgrade SQL system preference.
Corrected system preference screen message
Fixes on purge_suggestions.pl
- perlcritic friendlier
- address $PERL5LIB comment by using $PROGRAM_NAME (comment #10)
- used STDERR (comment #10)
- perltidy

TEST PLAN
---------
$ ./installer/data/mysql/updatedatabase.pl
-- should run upgrade and generate new systempreference in table
$ ./misc/cronjobs/purge_suggestions.pl --help
-- should give help with a real path used instead of $PERL5LIB.
$ ./misc/cronjobs/purge_suggestions.pl -days -1
-- should give error message as expected
$ ./misc/cronjobs/purge_suggestions.pl -days 0
-- should give error message as expected

Go to OPAC system preferences tab and check the
PurgeSuggestionsOlderThan system preference
-- message should be as expected (see comment #9)

run koha qa test tools
-- all should pass

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 33 Katrin Fischer 2017-10-16 22:29:09 UTC
Jon, did you mean to sign off? Switching status.
Comment 34 David Bourgault 2017-11-14 16:29:14 UTC
Created attachment 69142 [details] [review]
Bug13287 - Add a system preference to define the number of days used in purge_suggestions.pl

    The system preferences value is used whenever purge_suggestions.pl is called without the 'days' parameter.
    This patch uses the preference description suggested by comment #9.
    This version should now be cleanly applicable.

    I Apply the patch
    II Run updatedatabase.pl

    a) Run purge_suggestions.pl without the days parameter
       - validate that there is an error message
    b) Run purge_suggestions.pl with the days parameter
       - validate that there is no error message
    c) Insert a number of days in the system variable PurgeSuggestionsOlderThan
    d) Run purge_suggestions.pl without the days parameter
       - validate that there is no error message

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Tested per plan, all tests pass.

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 35 David Bourgault 2017-11-14 16:29:19 UTC
Created attachment 69143 [details] [review]
Bug 13287: QA Follow up

Added missing upgrade SQL system preference.
Corrected system preference screen message
Fixes on purge_suggestions.pl
- perlcritic friendlier
- address $PERL5LIB comment by using $PROGRAM_NAME (comment #10)
- used STDERR (comment #10)
- perltidy

TEST PLAN
---------
$ ./installer/data/mysql/updatedatabase.pl
-- should run upgrade and generate new systempreference in table
$ ./misc/cronjobs/purge_suggestions.pl --help
-- should give help with a real path used instead of $PERL5LIB.
$ ./misc/cronjobs/purge_suggestions.pl -days -1
-- should give error message as expected
$ ./misc/cronjobs/purge_suggestions.pl -days 0
-- should give error message as expected

Go to OPAC system preferences tab and check the
PurgeSuggestionsOlderThan system preference
-- message should be as expected (see comment #9)

run koha qa test tools
-- all should pass

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 36 David Bourgault 2017-11-14 16:29:44 UTC
Rebased.
Comment 37 Marcel de Rooy 2018-02-23 07:57:32 UTC
QA: Looking here now
Comment 38 Marcel de Rooy 2018-02-23 10:44:50 UTC
Created attachment 72105 [details] [review]
Bug 13287: Add a system preference to define the number of days used in purge_suggestions.pl

The system preferences value is used whenever purge_suggestions.pl is called without the 'days' parameter.
    This patch uses the preference description suggested by comment #9.
    This version should now be cleanly applicable.

    I Apply the patch
    II Run updatedatabase.pl

    a) Run purge_suggestions.pl without the days parameter
       - validate that there is an error message
    b) Run purge_suggestions.pl with the days parameter
       - validate that there is no error message
    c) Insert a number of days in the system variable PurgeSuggestionsOlderThan
    d) Run purge_suggestions.pl without the days parameter
       - validate that there is no error message

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Tested per plan, all tests pass.

Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Marcel de Rooy 2018-02-23 10:44:55 UTC
Created attachment 72106 [details] [review]
Bug 13287: (QA follow-up) Add db revision

Added missing upgrade SQL system preference.
Corrected system preference screen message
Fixes on purge_suggestions.pl
- perlcritic friendlier
- address $PERL5LIB comment by using $PROGRAM_NAME (comment #10)
- used STDERR (comment #10)
- perltidy

TEST PLAN
---------
$ ./installer/data/mysql/updatedatabase.pl
-- should run upgrade and generate new systempreference in table
$ ./misc/cronjobs/purge_suggestions.pl --help
-- should give help with a real path used instead of $PERL5LIB.
$ ./misc/cronjobs/purge_suggestions.pl -days -1
-- should give error message as expected
$ ./misc/cronjobs/purge_suggestions.pl -days 0
-- should give error message as expected

Go to OPAC system preferences tab and check the
PurgeSuggestionsOlderThan system preference
-- message should be as expected (see comment #9)

run koha qa test tools
-- all should pass

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Amended: Moved new pref from OPAC to Acquisitions preferences.
Comment 40 Marcel de Rooy 2018-02-23 10:45:01 UTC
Created attachment 72107 [details] [review]
Bug 13287: (QA follow-up) Prepare Suggestions.t for changes

Just rearranging some modules here.
Adding Koha::Database and schema calls.

Test plan:
Run t/db_dependent/Suggestions.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2018-02-23 10:45:06 UTC
Created attachment 72108 [details] [review]
Bug 13287: (QA follow-up) Extend Suggestions.t for DelSuggestionsOlderThan

Adding a days>0 test in the sub with a POD line.
Specific subtest for this sub added in Suggestions.t.

Test plan:
Run t/db_dependent/Suggestions.t again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2018-02-23 10:45:11 UTC
Created attachment 72109 [details] [review]
Bug 13287: (QA follow-up) Final polishing

Cosmetic changes.
And: adding a confirm flag (see earlier comment too). Without this flag but
with having a filled pref, the script would purge when you do not pass any
parameter. This might not be appreciated.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Marcel de Rooy 2018-02-23 10:46:56 UTC
This waited long time but I wonder if the purge job should look at date or suggesteddate. And I am also having doubts about status<>ASKED. But these are strictly speaking out of scope (existing conditions). Note that the job also deletes CHECKED suggestions.

Passed QA
Comment 44 Jonathan Druart 2018-02-26 15:53:40 UTC
Please add a release notes for this change. We need to tell people the cronjob line has to be updated (with --confirm) otherwise it will do nothing.
Comment 45 Jonathan Druart 2018-02-26 15:56:45 UTC
Ideally we should provide a verbose mode to display how many suggestions will be removed.
Comment 46 Jonathan Druart 2018-02-26 16:00:15 UTC
Dependency on bug 16104 removed (??)
Comment 47 Jonathan Druart 2018-02-26 16:02:34 UTC
Created attachment 72217 [details] [review]
Bug 13287: Remove warning if --days is not passed and pref empty
Comment 48 Jonathan Druart 2018-02-26 16:28:50 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 49 Marcel de Rooy 2018-02-27 13:04:11 UTC
(In reply to Jonathan Druart from comment #44)
> Please add a release notes for this change. We need to tell people the
> cronjob line has to be updated (with --confirm) otherwise it will do nothing.

I preliminary added a few lines and invite the author (Remy or one of his colleagues) to check or extend.
Comment 50 Nick Clemens 2018-04-06 15:42:38 UTC
Awesome work all!

Enhancement, not backported for 17.11.Z