Description
Joy Nelson
2019-04-25 15:36:07 UTC
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 99380 [details] [review] New systempreferences for suggestions added to sysprefs.sql New patch added for new installations Created attachment 99381 [details] [review] Bug 22774: New systempreferences for suggestions added to sysprefs.sql New patch added for new installations Created attachment 99382 [details] [review] Bug 22774: (follow-up) New systempreferences for suggestions Created attachment 100192 [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 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 Got this while upgrading: kohadev-koha@156b126f55d3:/kohadevbox/koha$ perl installer/data/mysql/updatedatabase.pl Upgrade to 19.12.00.068 done [19:57:44]: Bug 21190 - Add prefs AuthFailureLog and AuthSuccessLog Upgrade to 19.12.00.069 done [19:57:44]: Bug 22784 - Add a new suggestions.archived column Upgrade to 19.12.00.070 done [19:57:44]: Bug 22774 - Limit purchase suggestion in a specified time period DBD::mysql::db selectall_arrayref failed: Non-grouping field 'c' is used in HAVING clause [for Statement " SELECT category, authorised_value, COUNT(concat(category, ':', authorised_value)) AS c FROM authorised_values GROUP BY category, authorised_value HAVING c > 1 "] at installer/data/mysql/updatedatabase.pl line 21576. Can't use an undefined value as an ARRAY reference at installer/data/mysql/updatedatabase.pl line 21582. 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. |