This would be very helpful for a number of libraries I've talked with about purchase suggestions. 
Lisette

Created attachment 95940 [details] [review]
Bug 22774: New systempreferences for suggestions

Created attachment 95941 [details] [review]
Bug 22774: Limit Purchase Suggestion in a specified Time period

Test plan:
1- Apply this patch
2- Run updatedatabase.pl
3- By default you may have max. 3 suggestions in last 30 days.
4- Go to purchase suggestion page from OPAC
5- If you exceed limits of suggestions in step 3 you will not able to add new suggestions.
6- If you want to change the limits go to systempreferences and change  MaxTotalSuggestions and NumberOfSuggestionDays parameters.
7- Test again from step 4.

Hi,
Upon testing this, I received this error when trying to place a hold on the OPAC side.

Could not compile /kohadevbox/koha/opac/opac-suggestions.pl: Global symbol "$suggestions_loop" requires explicit package name (did you forget to declare "my $suggestions_loop"?) at /kohadevbox/koha/opac/opac-suggestions.pl line 144

Created attachment 97061 [details] [review]
Bug 22774: (follow-up) Limit Purchase Suggestion in a specified Time period

This was resolved. Fail occured because of some changes after we commit the patch

Created attachment 97070 [details] [review]
Bug 22774: New systempreferences for suggestions

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Created attachment 97071 [details] [review]
Bug 22774: (follow-up) Limit Purchase Suggestion in a specified Time period

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

I do not think it is correct to have the condition check in the controller. I am pretty sure Tomas will disagree with that as we will want the API taking into account those new sysprefs.

CCing Tomas to get his opinion on this.

Created attachment 99045 [details] [review]
Bug 22774: New systempreferences for suggestions

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov>

Created attachment 99046 [details] [review]
Bug 22774: (follow-up) Limit Purchase Suggestion in a specified Time period

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov>

1) The database update will limit the number of suggestions for existing installations:

+    $dbh->do( q{
+            INSERT IGNORE INTO systempreferences (variable,value,explanation,type) VALUES
+                ('MaxTotalSuggestions','3','Number of total suggestions','free'),
+                ('NumberOfSuggestionDays','30','days','free')
+            });

This is against the "rules" as we always aim for having no change of behaviour on update. We should set them to empty (unlimited)

2) For the same reason I am wondering about the default settings in the YAML file, I don't think they are needed.

3) This patch set misses the change to sysprefs.sql for setting up new installations. While we could do the 3 and 30 there, I wonder if we should be a little less restrictive there as well?

4) Please (re)add descriptions of the changes and test plans to the commit messages. I've seen quite a lot of them showing here but not being part of the attached files. Make sure you write them up in your git commit step, not when using git bz attach, as the latter won't change the files.

Created attachment 99381 [details] [review]
Bug 22774: New systempreferences for suggestions added to sysprefs.sql

New patch added for new installations

Created attachment 100193 [details] [review]
Bug 22774: New systempreferences for suggestions

Test plan:
1- Apply this patch
2- Run updatedatabase.pl
3- By default you may have max. 3 suggestions in last 30 days.
4- Go to purchase suggestion page from OPAC
5- If you exceed limits of suggestions in step 3 you will not able to add new suggestions.
6- If you want to change the limits go to systempreferences and change  MaxTotalSuggestions and NumberOfSuggestionDays parameters.
7- Test again from step 4.

Created attachment 100912 [details] [review]
Bug 22774: New systempreferences for suggestions added to sysprefs.sql

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 100913 [details] [review]
Bug 22774: Limit Purchase Suggestion in a specified Time period

Test plan:
 1 - Apply this patch
 2 - Run updatedatabase.pl
 3 - By default the preferences are blank and do not limit.
 4 - Set the limits to 3 in 30 days
 5 - Go to purchase suggestion page from OPAC as a logged in patron
 6 - Place 3 suggestions and confirm you cannot place any more
 7 - Alter one of the suggestions to have been made more than 30 days ago
    UPDATE suggestions SET suggesteddate = '2020-01-01' WHERE suggestionid=3;
 8 - Confirm you can place another suggestion
 9 - Log out of OPAC
10 - Make sure AnonSyggestions is set to 'Allow' and AnonymousPatron is set
11 - Confirm anonymous suggestions are not limited by the syspref
12 - Confirm that a blank value in either  MaxTotalSuggestions  or  NumberOfSuggestionDays  does not limit suggestions

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 101448 [details] [review]
Bug 22774: (follow-up) Limit Purchase Suggestion in a specified Time period

We changed the code to satisfy new methods:
we used Koha::Suggestions->search()->count instead of scalar search

1) QA test tools

Looks good, only got what looks like a false positive:

Processing additional checks
	* Email's author 'kohadevinim@devinim.com.tr' seems invalid

2) Capitalization

(Bug 22774 - Limit Purchase Suggestion in a specified Time period)

Please stick with our capitalization rules everywhere! :)

Still testing here, but won't finish tonight. Will fix 2) myself.

(In reply to Katrin Fischer from comment #20)
> 
> 1) QA test tools
> 
> Looks good, only got what looks like a false positive:
> 
> Processing additional checks
> 	* Email's author 'kohadevinim@devinim.com.tr' seems invalid

Hi Katrin,

I didn't understand that. It is one of our valid email address.

> 
> I didn't understand that. It is one of our valid email address.

Yep, was just noting it - we can safely ignore it in my opinion. Maybe it's not aware of com.tr?

You can ignore it, IIRC I added a check on "*koha*", to catch example developer emails (like the default from kohadevbox, etc.)

Oh, sorry, it looks like I forgot to continue here - will do it now.

Created attachment 102882 [details] [review]
Bug 22774: New systempreferences for suggestions added to sysprefs.sql

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 102883 [details] [review]
Bug 22774: Limit Purchase Suggestion in a specified Time period

Test plan:
 1 - Apply this patch
 2 - Run updatedatabase.pl
 3 - By default the preferences are blank and do not limit.
 4 - Set the limits to 3 in 30 days
 5 - Go to purchase suggestion page from OPAC as a logged in patron
 6 - Place 3 suggestions and confirm you cannot place any more
 7 - Alter one of the suggestions to have been made more than 30 days ago
    UPDATE suggestions SET suggesteddate = '2020-01-01' WHERE suggestionid=3;
 8 - Confirm you can place another suggestion
 9 - Log out of OPAC
10 - Make sure AnonSyggestions is set to 'Allow' and AnonymousPatron is set
11 - Confirm anonymous suggestions are not limited by the syspref
12 - Confirm that a blank value in either  MaxTotalSuggestions  or  NumberOfSuggestionDays  does not limit suggestions

Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com>

Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 102884 [details] [review]
Bug 22774: (follow-up) Limit purchase suggestion in a specified time period

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 102885 [details] [review]
Bug 22774: (QA follow-up) Add syspref descriptions to database update and sysprefs.sql

Moves the updatedatabase entry to the new format and
adds information for the explanation column of the
systempreferences table.

Also slightly rephrases the description in the system preference
editor.

To test:
- Verify database update still works the same with addition
  of explanations
- Verify the sys pref description makes sense

Created attachment 102886 [details] [review]
Bug 22774: (QA follow-up) Rephrase OPAC notes

I've changed the OPAC notes a little bit, main change is
the removal of the <b> tags used to highlight single words
in one sentence as they would result in splitting up the
sentence into multiple parts which makes proper translations really
hard.

RM, as a native speaker, please check the follow-ups make sense :)

Thanks Katrin

(In reply to Jonathan Druart from comment #8)
> I do not think it is correct to have the condition check in the controller.
> I am pretty sure Tomas will disagree with that as we will want the API
> taking into account those new sysprefs.
> 
> CCing Tomas to get his opinion on this.

Discussed with Tomas and we agreed to push this and followup with an API fix later.. a bigger discussion on how to handle things like needs to happen in a broader scope.

(In reply to Katrin Fischer from comment #30)
> RM, as a native speaker, please check the follow-ups make sense :)

Looks good to me :)

Nice work everyone!

Pushed to master for 20.05

Wrong bug, sorry

enhancement not backported to 19.11.x

Thanks folks for all your work, we just discovered this enhancement and our collection development librarians are looking forward to this feature.