Bug 23991 - Move SearchSuggestion to Koha::Suggestions
Summary: Move SearchSuggestion to Koha::Suggestions
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 23590 29886
Blocks: 23990 31024 34963 35911 35916 31158 31347 32531 33002
  Show dependency treegraph
 
Reported: 2019-11-07 16:39 UTC by Jonathan Druart
Modified: 2024-04-17 09:49 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00


Attachments
Bug 23991: Move SearchSuggestion to Koha::Suggestions (29.22 KB, patch)
2019-11-07 16:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23991: Move SearchSuggestion to Koha::Suggestions (53.70 KB, patch)
2021-01-15 14:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Script for making some suggestions for testing (1.15 KB, application/x-perl)
2021-01-15 14:53 UTC, Nick Clemens
Details
Bug 23991: Move SearchSuggestion to Koha::Suggestions (60.48 KB, patch)
2021-02-10 09:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23991: Move SearchSuggestion to Koha::Suggestions (60.54 KB, patch)
2021-03-18 08:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23991: Move SearchSuggestion to Koha::Suggestions (61.14 KB, patch)
2021-04-07 08:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23991: Move SearchSuggestion to Koha::Suggestions (61.00 KB, patch)
2021-10-28 14:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23991: Move SearchSuggestion to Koha::Suggestions (61.19 KB, patch)
2021-10-29 14:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23991: Move SearchSuggestion to Koha::Suggestions (61.28 KB, patch)
2021-11-29 13:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23991: (follow-up) Fix wrong import (880 bytes, patch)
2021-11-29 13:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23991: Remove SearchSuggestion tests (4.65 KB, patch)
2021-11-29 13:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23991: Add Koha::Suggestions->search_limited POD (1.01 KB, patch)
2021-11-29 13:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23991: (QA follow-up) Save some DB queries (9.99 KB, patch)
2021-11-29 13:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23991: Move SearchSuggestion to Koha::Suggestions (60.43 KB, patch)
2022-01-14 18:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23991: Remove SearchSuggestion tests (4.65 KB, patch)
2022-01-14 18:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23991: (QA follow-up) Save some DB queries (9.99 KB, patch)
2022-01-14 18:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23991: Fix branchcode and budgetid filtering (2.89 KB, patch)
2022-01-17 13:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23991: Fix conflict with bug 28941 (1.08 KB, patch)
2022-02-01 16:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23991: (follow-up) Missing semicolon (754 bytes, patch)
2022-03-18 13:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23991: Fix 'all' libraries (897 bytes, patch)
2022-03-22 14:41 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23991: Move SearchSuggestion to Koha::Suggestions (60.49 KB, patch)
2022-03-24 15:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23991: Remove SearchSuggestion tests (4.71 KB, patch)
2022-03-24 15:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23991: (QA follow-up) Save some DB queries (10.05 KB, patch)
2022-03-24 15:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23991: Fix branchcode and budgetid filtering (2.94 KB, patch)
2022-03-24 15:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23991: Fix conflict with bug 28941 (1.13 KB, patch)
2022-03-24 15:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23991: (follow-up) Missing semicolon (810 bytes, patch)
2022-03-24 15:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23991: Fix 'all' libraries (951 bytes, patch)
2022-03-24 15:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23991: (follow-up) Add value to filter_archived (1.81 KB, patch)
2022-04-26 11:08 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23991: Move SearchSuggestion to Koha::Suggestions (71.16 KB, patch)
2022-05-12 10:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23991: Move SearchSuggestion to Koha::Suggestions (71.45 KB, patch)
2022-06-23 10:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23991: (QA follow-up) Fix tabbing (1.48 KB, patch)
2022-06-23 10:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23991: Don't display empty div if no suggestions exist (1.90 KB, patch)
2022-07-06 09:31 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 Jonathan Druart 2019-11-07 16:39:07 UTC

    
Comment 1 Jonathan Druart 2019-11-07 16:41:18 UTC
Created attachment 95172 [details] [review]
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Comment 2 Jonathan Druart 2019-11-07 16:42:00 UTC
The hardest part is missing, suggestion/suggestion.pl

I will come back to this one later.
Comment 3 Jonathan Druart 2021-01-15 14:08:39 UTC
Created attachment 115203 [details] [review]
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Comment 4 Nick Clemens 2021-01-15 14:53:04 UTC
Created attachment 115207 [details]
Script for making some suggestions for testing

For making suggestions in various statuses in several branches
Comment 5 Nick Clemens 2021-01-15 14:55:28 UTC
Updating suggestions, applying filters all seem to work as expected - the only thing I note is that if I have selected a library on the dropdown above the table and then clear filters on the left I am defaulted back to my logged in library

Also, on the homepage the count of pending suggestions does not exclude archived suggestions
Comment 6 Nick Clemens 2021-01-15 14:59:08 UTC
Two more things, not necessarily caused by this:
1 - If I add a new suggestion I am returned to the suggestions for that library, but I will be on the first tab (accepted for example), not the pending tab
2 - if I have multiple suggestions in unknown statuses I get multiple tabs when organizing by status, but they only say 'unknown' I can tell what the actual status is - the tabe shoudl include the unknown code
Comment 7 Jonathan Druart 2021-01-18 14:04:22 UTC
(In reply to Nick Clemens from comment #5)
> Updating suggestions, applying filters all seem to work as expected - the
> only thing I note is that if I have selected a library on the dropdown above
> the table and then clear filters on the left I am defaulted back to my
> logged in library

That's the behaviour in master.

> Also, on the homepage the count of pending suggestions does not exclude
> archived suggestions

Same, opened bug 27462.

Maybe we should simplify this view and have it using the REST API (and the datatables wrapper). With correct column filtering we shouldn't need the different "organized by" views. What do you think?
Comment 8 Jonathan Druart 2021-02-10 09:06:12 UTC
Created attachment 116639 [details] [review]
Bug 23991: Move SearchSuggestion to Koha::Suggestions

The C4::Suggestions::SearchSuggestion subroutine is badly written and
can be replaced by calls to Koha::Suggestions->search.
The hard part in this patch is suggestion.pl, the other occurrences have
been replaced easily.

Test plan:
The idea is to test the whole suggestion workflow.
1. Create a suggestion on OPAC
2. Create a suggestion on the staff interface
3. Edit suggestions
4. Filter suggestions (use the different filters and "organize by"
values)
Comment 9 Jonathan Druart 2021-02-10 09:06:43 UTC
This is ready for testing :)
Comment 10 Jonathan Druart 2021-03-18 08:57:22 UTC
Created attachment 118454 [details] [review]
Bug 23991: Move SearchSuggestion to Koha::Suggestions

The C4::Suggestions::SearchSuggestion subroutine is badly written and
can be replaced by calls to Koha::Suggestions->search.
The hard part in this patch is suggestion.pl, the other occurrences have
been replaced easily.

Test plan:
The idea is to test the whole suggestion workflow.
1. Create a suggestion on OPAC
2. Create a suggestion on the staff interface
3. Edit suggestions
4. Filter suggestions (use the different filters and "organize by"
values)
Comment 11 Jonathan Druart 2021-03-18 08:57:40 UTC
Conflicts with bug 26406 fixed.
Comment 12 Séverine Queune 2021-03-31 08:44:26 UTC
Suggestions are hidden on the opac side, on the 'cgi-bin/koha/opac-suggestions.pl' page.
Checked on master without the patch, they are correctly displayed, as well on the intranet side on '/cgi-bin/koha/members/purchase-suggestions.pl' with the patch.
Comment 13 Jonathan Druart 2021-04-07 08:45:59 UTC
Created attachment 119256 [details] [review]
Bug 23991: Move SearchSuggestion to Koha::Suggestions

The C4::Suggestions::SearchSuggestion subroutine is badly written and
can be replaced by calls to Koha::Suggestions->search.
The hard part in this patch is suggestion.pl, the other occurrences have
been replaced easily.

Test plan:
The idea is to test the whole suggestion workflow.
1. Create a suggestion on OPAC
2. Create a suggestion on the staff interface
3. Edit suggestions
4. Filter suggestions (use the different filters and "organize by"
values)
Comment 14 Jonathan Druart 2021-04-07 08:47:18 UTC
Thanks Séverine, indeed there were a couple of mistakes in the patch. It should work correctly now.
Comment 15 Jonathan Druart 2021-10-28 14:43:59 UTC
This has been forgotten!
Comment 16 Jonathan Druart 2021-10-28 14:54:17 UTC
Created attachment 127058 [details] [review]
Bug 23991: Move SearchSuggestion to Koha::Suggestions

The C4::Suggestions::SearchSuggestion subroutine is badly written and
can be replaced by calls to Koha::Suggestions->search.
The hard part in this patch is suggestion.pl, the other occurrences have
been replaced easily.

Test plan:
The idea is to test the whole suggestion workflow.
1. Create a suggestion on OPAC
2. Create a suggestion on the staff interface
3. Edit suggestions
4. Filter suggestions (use the different filters and "organize by"
values)
Comment 17 Jonathan Druart 2021-10-28 14:54:37 UTC
Patch rebased.
Comment 18 Owen Leonard 2021-10-28 16:32:36 UTC
I get an error when I try to access the suggestions page in the OPAC:

Could not compile /kohadevbox/koha/opac/opac-suggestions.pl: "SearchSuggestion" is not exported by the C4::Suggestions module
Can't continue after import errors at /kohadevbox/koha/opac/opac-suggestions.pl line 27.
Comment 19 Jonathan Druart 2021-10-29 14:08:50 UTC
Created attachment 127100 [details] [review]
Bug 23991: Move SearchSuggestion to Koha::Suggestions

The C4::Suggestions::SearchSuggestion subroutine is badly written and
can be replaced by calls to Koha::Suggestions->search.
The hard part in this patch is suggestion.pl, the other occurrences have
been replaced easily.

Test plan:
The idea is to test the whole suggestion workflow.
1. Create a suggestion on OPAC
2. Create a suggestion on the staff interface
3. Edit suggestions
4. Filter suggestions (use the different filters and "organize by"
values)
Comment 20 Tomás Cohen Arazi 2021-11-29 12:55:03 UTC
Looking. I'm investigating this query log differences:

before:

		   242 Query	select DISTINCT(STATUS) as value, count(*) as cnt from suggestions group by value order by STATUS
		   242 Query	SELECT COUNT( * ) FROM `suggestions` `me` WHERE ( ( `STATUS` = 'ASKED' AND `archived` = '0' AND `branchcode` = 'CPL' ) )
		   242 Query	select DISTINCT(managedby) as value, count(*) as cnt from suggestions group by value order by managedby
		   242 Query	select DISTINCT(acceptedby) as value, count(*) as cnt from suggestions group by value order by acceptedby
		   242 Query	select DISTINCT(suggestedby) as value, count(*) as cnt from suggestions group by value order by suggestedby
		   242 Query	select DISTINCT(budgetid) as value, count(*) as cnt from suggestions group by value order by budgetid


after:

		   242 Query	select DISTINCT(STATUS) as value, count(*) as cnt from suggestions group by value order by STATUS
		   242 Query	SELECT COUNT( * ) FROM `suggestions` `me` WHERE ( ( `STATUS` = 'ASKED' AND `archived` = '0' AND `branchcode` = 'CPL' ) )
		   242 Query	select DISTINCT(managedby) as value, count(*) as cnt from suggestions group by value order by managedby
		   242 Query	select DISTINCT(acceptedby) as value, count(*) as cnt from suggestions group by value order by acceptedby
		   242 Query	select DISTINCT(suggestedby) as value, count(*) as cnt from suggestions group by value order by suggestedby
		   242 Query	select DISTINCT(budgetid) as value, count(*) as cnt from suggestions group by value order by budgetid
		   242 Query	SELECT COUNT( * ) FROM `suggestions` `me` WHERE ( ( `STATUS` = 'ASKED' AND `archived` = '0' AND `branchcode` = 'CPL' ) )
		   242 Query	SELECT COUNT( * ) FROM `suggestions` `me` WHERE ( ( `STATUS` = 'ASKED' AND `archived` = '0' AND `branchcode` = 'CPL' ) )
		   242 Query	SELECT `me`.`suggestionid`, `me`.`suggestedby`, `me`.`suggesteddate`, `me`.`managedby`, `me`.`manageddate`, `me`.`acceptedby`, `me`.`accepteddate`, `me`.`rejectedby`, `me`.`rejecteddate`, `me`.`lastmodificationby`, `me`.`lastmodificationdate`, `me`.`STATUS`, `me`.`archived`, `me`.`note`, `me`.`author`, `me`.`title`, `me`.`copyrightdate`, `me`.`publishercode`, `me`.`date`, `me`.`volumedesc`, `me`.`publicationyear`, `me`.`place`, `me`.`isbn`, `me`.`biblionumber`, `me`.`reason`, `me`.`patronreason`, `me`.`budgetid`, `me`.`branchcode`, `me`.`collectiontitle`, `me`.`itemtype`, `me`.`quantity`, `me`.`currency`, `me`.`price`, `me`.`total` FROM `suggestions` `me` WHERE ( ( `STATUS` = 'ASKED' AND `archived` = '0' AND `branchcode` = 'CPL' ) )
		   242 Query	SELECT `me`.`module`, `me`.`page`, `me`.`tablename`, `me`.`columnname`, `me`.`cannot_be_toggled`, `me`.`is_hidden` FROM `columns_settings` `me` WHERE ( ( `module` = 'acqui' AND `page` = 'suggestions' AND `tablename` = 'suggestions' ) )
		   242 Query	SELECT COUNT( * ) FROM `suggestions` `me` WHERE ( ( `STATUS` = 'ASKED' AND `archived` = '0' AND `branchcode` = 'CPL' ) )
Comment 21 Tomás Cohen Arazi 2021-11-29 13:53:20 UTC
Created attachment 128071 [details] [review]
Bug 23991: Move SearchSuggestion to Koha::Suggestions

The C4::Suggestions::SearchSuggestion subroutine is badly written and
can be replaced by calls to Koha::Suggestions->search.
The hard part in this patch is suggestion.pl, the other occurrences have
been replaced easily.

Test plan:
The idea is to test the whole suggestion workflow.
1. Create a suggestion on OPAC
2. Create a suggestion on the staff interface
3. Edit suggestions
4. Filter suggestions (use the different filters and "organize by"
values)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 23991: Remove unused variable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2021-11-29 13:53:36 UTC
Created attachment 128072 [details] [review]
Bug 23991: (follow-up) Fix wrong import

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2021-11-29 13:53:51 UTC
Created attachment 128073 [details] [review]
Bug 23991: Remove SearchSuggestion tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2021-11-29 13:54:03 UTC
Created attachment 128074 [details] [review]
Bug 23991: Add Koha::Suggestions->search_limited POD

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2021-11-29 13:54:16 UTC
Created attachment 128075 [details] [review]
Bug 23991: (QA follow-up) Save some DB queries

This patch makes the suggestion-related pages rely on array size instead
of querying the DB each time they need to. In the case of
suggestion/suggestion.pl it goes from 4 COUNT(*) to 1.

To test, with KTD:
1. Run on the host machine:
    $ docker exec -ti koha_db_1 bash
    $ mysql -ppassword
    > SET GLOBAL general_log_file='/var/log/mysql/mycustom.log';
    > SET GLOBAL log_output = 'FILE';
    > SET GLOBAL general_log = 'ON';
    > \q
    $ tail -f /var/log/mysql/mycustom.log | grep suggestions
2. Visit the different pages changed on this bug
=> SUCCESS: Some queries
3. Apply this patch
4. Repeat 2
=> SUCCESS: Less queries!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Nick Clemens 2022-01-14 15:54:36 UTC
bug 23991 - I ran the script to generate random suggesstions - I can see them - apply patch, I don't see them


If I load from library link on homepage, I see them
but if i change library using pulldown I can't 

the 'all libraries' link form homepage does not work either


http://localhost:8081/cgi-bin/koha/suggestion/suggestion.pl?branchcode=FFL&displayb[…]eptedby=&accepteddate_from=&accepteddate_to=&budgetid=__ANY__
that shows nothing

http://localhost:8081/cgi-bin/koha/suggestion/suggestion.pl?branchcode=FFL
That shows things


Logs show this often:
[2022/01/14 15:21:34] [WARN] Use of uninitialized value in string eq at /kohadevbox/koha/suggestion/suggestion.pl line 113.
[2022/01/14 15:21:34] [WARN] Use of uninitialized value $v in string eq at /kohadevbox/koha/suggestion/suggestion.pl line 115.
Comment 27 Tomás Cohen Arazi 2022-01-14 18:41:53 UTC
Created attachment 129511 [details] [review]
Bug 23991: Move SearchSuggestion to Koha::Suggestions

The C4::Suggestions::SearchSuggestion subroutine is badly written and
can be replaced by calls to Koha::Suggestions->search.
The hard part in this patch is suggestion.pl, the other occurrences have
been replaced easily.

Test plan:
The idea is to test the whole suggestion workflow.
1. Create a suggestion on OPAC
2. Create a suggestion on the staff interface
3. Edit suggestions
4. Filter suggestions (use the different filters and "organize by"
values)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2022-01-14 18:41:58 UTC
Created attachment 129512 [details] [review]
Bug 23991: Remove SearchSuggestion tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 29 Tomás Cohen Arazi 2022-01-14 18:42:04 UTC
Created attachment 129513 [details] [review]
Bug 23991: (QA follow-up) Save some DB queries

This patch makes the suggestion-related pages rely on array size instead
of querying the DB each time they need to. In the case of
suggestion/suggestion.pl it goes from 4 COUNT(*) to 1.

To test, with KTD:
1. Run on the host machine:
    $ docker exec -ti koha_db_1 bash
    $ mysql -ppassword
    > SET GLOBAL general_log_file='/var/log/mysql/mycustom.log';
    > SET GLOBAL log_output = 'FILE';
    > SET GLOBAL general_log = 'ON';
    > \q
    $ tail -f /var/log/mysql/mycustom.log | grep suggestions
2. Visit the different pages changed on this bug
=> SUCCESS: Some queries
3. Apply this patch
4. Repeat 2
=> SUCCESS: Less queries!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Tomás Cohen Arazi 2022-01-14 18:47:51 UTC
Nick, I stripped the Koha::Suggestions->search_limited and moved to 29886. I found issues when writing unit tests for it, and they might be responsible for the problems you found.

I have attached a new patchset, which is the same as before, without that. But given the fact I fixed some issues on how ->search_limited worked, it might be good if you could re-test. Thanks!
Comment 31 Jonathan Druart 2022-01-17 13:21:21 UTC
Created attachment 129523 [details] [review]
Bug 23991: Fix branchcode and budgetid filtering
Comment 32 Jonathan Druart 2022-01-17 13:22:21 UTC
(In reply to Nick Clemens from comment #26)
> bug 23991 - I ran the script to generate random suggesstions - I can see
> them - apply patch, I don't see them
> 
> 
> If I load from library link on homepage, I see them
> but if i change library using pulldown I can't 
> 
> the 'all libraries' link form homepage does not work either
> 
> 
> http://localhost:8081/cgi-bin/koha/suggestion/suggestion.
> pl?branchcode=FFL&displayb[…]eptedby=&accepteddate_from=&accepteddate_to=&bud
> getid=__ANY__
> that shows nothing
> 
> http://localhost:8081/cgi-bin/koha/suggestion/suggestion.pl?branchcode=FFL
> That shows things
> 
> 
> Logs show this often:
> [2022/01/14 15:21:34] [WARN] Use of uninitialized value in string eq at
> /kohadevbox/koha/suggestion/suggestion.pl line 113.
> [2022/01/14 15:21:34] [WARN] Use of uninitialized value $v in string eq at
> /kohadevbox/koha/suggestion/suggestion.pl line 115.

Should be fixed with the last patch.

There is certainly more to do to clean this controller and template, but prefer to not rewrite them here and focus on the SearchSuggestion move only.
Comment 33 Tomás Cohen Arazi 2022-01-31 18:11:39 UTC
This error is suspicious:

"my" variable $suggestion masks earlier declaration in same scope

there's effectively a variable overwrite at the top level.
Comment 34 Jonathan Druart 2022-02-01 16:03:54 UTC
About patch "Bug 23991: (QA follow-up) Save some DB queries". I am not sure about it, I always privileged iterator over retrieving the whole set.
If this is what we really want it needs to be deeply investigated and that we agree on it.
Comment 35 Jonathan Druart 2022-02-01 16:09:13 UTC
Created attachment 130049 [details] [review]
Bug 23991: Fix conflict with bug 28941

Well, this patchset fixed the security bug...
Redoing on top of bug 28941
Comment 36 Tomás Cohen Arazi 2022-02-01 18:25:39 UTC
(In reply to Jonathan Druart from comment #34)
> About patch "Bug 23991: (QA follow-up) Save some DB queries". I am not sure
> about it, I always privileged iterator over retrieving the whole set.
> If this is what we really want it needs to be deeply investigated and that
> we agree on it.

Another option would've been to calculate the count and add a new template param. I decided to minimize the changes to your patchset.
Comment 37 Jonathan Druart 2022-03-18 08:45:19 UTC
Can we get this moving please?
Comment 38 Nick Clemens 2022-03-18 13:28:07 UTC
Created attachment 131896 [details] [review]
Bug 23991: (follow-up) Missing semicolon
Comment 39 Nick Clemens 2022-03-18 13:31:27 UTC
Everything works well, one issue I found:
If I add a new suggestion with acquisition library 'Any' I am returned to view for logged in branch, but no suggestions are loaded
Comment 40 Jonathan Druart 2022-03-22 14:41:15 UTC
Created attachment 132026 [details] [review]
Bug 23991: Fix 'all' libraries
Comment 41 Jonathan Druart 2022-03-22 14:42:45 UTC
(In reply to Nick Clemens from comment #39)
> Everything works well, one issue I found:
> If I add a new suggestion with acquisition library 'Any' I am returned to
> view for logged in branch, but no suggestions are loaded

I don't recreate that, can you provide more detail please?

(In reply to Nick Clemens from comment #39)
> Everything works well, one issue I found:
> If I add a new suggestion with acquisition library 'Any' I am returned to
> view for logged in branch, but no suggestions are loaded

Thanks!

I found another weirdness but it's in master as well.
If you have 2 suggestions, branchcodes are 'CPL' and NULL, organize by "library".
You will see CPL (1) and Any (2).

Will fix on a follow-up bug report if it's needed.
Comment 42 Nick Clemens 2022-03-24 15:21:46 UTC
Created attachment 132169 [details] [review]
Bug 23991: Move SearchSuggestion to Koha::Suggestions

The C4::Suggestions::SearchSuggestion subroutine is badly written and
can be replaced by calls to Koha::Suggestions->search.
The hard part in this patch is suggestion.pl, the other occurrences have
been replaced easily.

Test plan:
The idea is to test the whole suggestion workflow.
1. Create a suggestion on OPAC
2. Create a suggestion on the staff interface
3. Edit suggestions
4. Filter suggestions (use the different filters and "organize by"
values)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 43 Nick Clemens 2022-03-24 15:21:50 UTC
Created attachment 132170 [details] [review]
Bug 23991: Remove SearchSuggestion tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 44 Nick Clemens 2022-03-24 15:21:57 UTC
Created attachment 132171 [details] [review]
Bug 23991: (QA follow-up) Save some DB queries

This patch makes the suggestion-related pages rely on array size instead
of querying the DB each time they need to. In the case of
suggestion/suggestion.pl it goes from 4 COUNT(*) to 1.

To test, with KTD:
1. Run on the host machine:
    $ docker exec -ti koha_db_1 bash
    $ mysql -ppassword
    > SET GLOBAL general_log_file='/var/log/mysql/mycustom.log';
    > SET GLOBAL log_output = 'FILE';
    > SET GLOBAL general_log = 'ON';
    > \q
    $ tail -f /var/log/mysql/mycustom.log | grep suggestions
2. Visit the different pages changed on this bug
=> SUCCESS: Some queries
3. Apply this patch
4. Repeat 2
=> SUCCESS: Less queries!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 45 Nick Clemens 2022-03-24 15:22:01 UTC
Created attachment 132172 [details] [review]
Bug 23991: Fix branchcode and budgetid filtering

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 46 Nick Clemens 2022-03-24 15:22:05 UTC
Created attachment 132173 [details] [review]
Bug 23991: Fix conflict with bug 28941

Well, this patchset fixed the security bug...
Redoing on top of bug 28941

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 47 Nick Clemens 2022-03-24 15:22:09 UTC
Created attachment 132174 [details] [review]
Bug 23991: (follow-up) Missing semicolon

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 48 Nick Clemens 2022-03-24 15:22:13 UTC
Created attachment 132175 [details] [review]
Bug 23991: Fix 'all' libraries

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 49 Nick Clemens 2022-03-24 15:23:11 UTC
(In reply to Jonathan Druart from comment #41)
> I found another weirdness but it's in master as well.
> If you have 2 suggestions, branchcodes are 'CPL' and NULL, organize by
> "library".
> You will see CPL (1) and Any (2).
> 
> Will fix on a follow-up bug report if it's needed.

Yeah, 'Any' looks like 'All libraries' - we could add a new tab for 'Any' - but another report indeed
Comment 50 Tomás Cohen Arazi 2022-04-20 16:10:05 UTC
I already tested this one. Will do a second round to check the rebase fixes and Nick's follow-up
Comment 51 Katrin Fischer 2022-04-25 13:51:44 UTC
(In reply to Nick Clemens from comment #49)
> (In reply to Jonathan Druart from comment #41)
> > I found another weirdness but it's in master as well.
> > If you have 2 suggestions, branchcodes are 'CPL' and NULL, organize by
> > "library".
> > You will see CPL (1) and Any (2).
> > 
> > Will fix on a follow-up bug report if it's needed.
> 
> Yeah, 'Any' looks like 'All libraries' - we could add a new tab for 'Any' -
> but another report indeed

I believe it was supposed to work that way... but I don't remember why anymore :) So possibly a rename would do?
Comment 52 Katrin Fischer 2022-04-25 15:10:17 UTC
I have 2 suggestions for Centerville, both are Pending and archived.

On master, I can make them display by choosing "Include archived" and starting the search:

http://localhost:8081/cgi-bin/koha/suggestion/suggestion.pl?branchcode=CPL&displayby=branchcode&title=&author=&isbn=&publishercode=&copyrightdate=&collectiontitle=&filter_archived=on&STATUS=&suggestedby=&suggesteddate_from=&suggesteddate_to=&managedby=&manageddate_from=&manageddate_to=&acceptedby=&accepteddate_from=&accepteddate_to=&budgetid=__ANY__

This doesn't work with the patch set applied. I can't get archived suggestions to display.
Comment 53 Nick Clemens 2022-04-26 11:08:20 UTC
Created attachment 133874 [details] [review]
Bug 23991: (follow-up) Add value to filter_archived
Comment 54 Jonathan Druart 2022-05-12 10:15:00 UTC
Created attachment 134922 [details] [review]
Bug 23991: Move SearchSuggestion to Koha::Suggestions

The C4::Suggestions::SearchSuggestion subroutine is badly written and
can be replaced by calls to Koha::Suggestions->search.
The hard part in this patch is suggestion.pl, the other occurrences have
been replaced easily.

Test plan:
The idea is to test the whole suggestion workflow.
1. Create a suggestion on OPAC
2. Create a suggestion on the staff interface
3. Edit suggestions
4. Filter suggestions (use the different filters and "organize by"
values)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Bug 23991: Remove SearchSuggestion tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Bug 23991: (QA follow-up) Save some DB queries

This patch makes the suggestion-related pages rely on array size instead
of querying the DB each time they need to. In the case of
suggestion/suggestion.pl it goes from 4 COUNT(*) to 1.

To test, with KTD:
1. Run on the host machine:
    $ docker exec -ti koha_db_1 bash
    $ mysql -ppassword
    > SET GLOBAL general_log_file='/var/log/mysql/mycustom.log';
    > SET GLOBAL log_output = 'FILE';
    > SET GLOBAL general_log = 'ON';
    > \q
    $ tail -f /var/log/mysql/mycustom.log | grep suggestions
2. Visit the different pages changed on this bug
=> SUCCESS: Some queries
3. Apply this patch
4. Repeat 2
=> SUCCESS: Less queries!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Bug 23991: Fix branchcode and budgetid filtering

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

Bug 23991: Fix conflict with bug 28941

Well, this patchset fixed the security bug...
Redoing on top of bug 28941

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

Bug 23991: (follow-up) Missing semicolon

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

Bug 23991: Fix 'all' libraries

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

Bug 23991: (follow-up) Add value to filter_archived
Comment 55 Jonathan Druart 2022-05-12 10:15:46 UTC
I had to squash the patches, sorry for the authorship.
This one is becoming painful to rebase...
Comment 56 Martin Renvoize 2022-06-23 10:25:58 UTC
Created attachment 136441 [details] [review]
Bug 23991: Move SearchSuggestion to Koha::Suggestions

The C4::Suggestions::SearchSuggestion subroutine is badly written and
can be replaced by calls to Koha::Suggestions->search.
The hard part in this patch is suggestion.pl, the other occurrences have
been replaced easily.

Test plan:
The idea is to test the whole suggestion workflow.
1. Create a suggestion on OPAC
2. Create a suggestion on the staff interface
3. Edit suggestions
4. Filter suggestions (use the different filters and "organize by"
values)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Bug 23991: Remove SearchSuggestion tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Bug 23991: (QA follow-up) Save some DB queries

This patch makes the suggestion-related pages rely on array size instead
of querying the DB each time they need to. In the case of
suggestion/suggestion.pl it goes from 4 COUNT(*) to 1.

To test, with KTD:
1. Run on the host machine:
    $ docker exec -ti koha_db_1 bash
    $ mysql -ppassword
    > SET GLOBAL general_log_file='/var/log/mysql/mycustom.log';
    > SET GLOBAL log_output = 'FILE';
    > SET GLOBAL general_log = 'ON';
    > \q
    $ tail -f /var/log/mysql/mycustom.log | grep suggestions
2. Visit the different pages changed on this bug
=> SUCCESS: Some queries
3. Apply this patch
4. Repeat 2
=> SUCCESS: Less queries!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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

Bug 23991: Fix branchcode and budgetid filtering

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

Bug 23991: Fix conflict with bug 28941

Well, this patchset fixed the security bug...
Redoing on top of bug 28941

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

Bug 23991: (follow-up) Missing semicolon

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

Bug 23991: Fix 'all' libraries

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

Bug 23991: (follow-up) Add value to filter_archived

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 57 Martin Renvoize 2022-06-23 10:26:03 UTC
Created attachment 136442 [details] [review]
Bug 23991: (QA follow-up) Fix tabbing

It looks like during one of the many rebases we lost some of the tabn
handling, likely due to the conversion from jquery-ui to bootstrap tabs.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 58 Martin Renvoize 2022-06-23 10:27:39 UTC
Everything working as far as I can tell... Code is looking good and with my minor follow-up fixing a rebase issue somewhere along the line I think we're good to go.

Passing QA.

PS.. I would love to see a switch to API driven tables etc here as suggested earlier in the bug..  the UX is a bit wierd.
Comment 59 Tomás Cohen Arazi 2022-06-27 16:25:37 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 60 Jonathan Druart 2022-07-06 09:31:32 UTC
Created attachment 137201 [details] [review]
Bug 23991: Don't display empty div if no suggestions exist

If no suggestions exist we should not display an empty tab div
Comment 61 Tomás Cohen Arazi 2022-07-06 14:21:24 UTC
Follow-up pushed to master. Thanks Jonathan!
Comment 62 Lucas Gass 2022-07-29 16:48:13 UTC
Doesn't apply cleanly to 22.05.x, if needed please rebase
Comment 63 Katrin Fischer 2022-07-29 20:55:53 UTC
Please don't backport until the depending bug 31158 has been resolved.
Comment 64 Jonathan Druart 2022-08-01 06:17:10 UTC
This change shouldn't be backported.