Bug 23991

Summary: Move SearchSuggestion to Koha::Suggestions
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: lucas, m.de.rooy, martin.renvoize, nick, severine.queune, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=30017
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00
Bug Depends on: 23590, 29886    
Bug Blocks: 23990, 31024, 34963, 35911, 35916, 31158, 31347, 32531, 33002    
Attachments: Bug 23991: Move SearchSuggestion to Koha::Suggestions
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Script for making some suggestions for testing
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Bug 23991: (follow-up) Fix wrong import
Bug 23991: Remove SearchSuggestion tests
Bug 23991: Add Koha::Suggestions->search_limited POD
Bug 23991: (QA follow-up) Save some DB queries
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Bug 23991: Remove SearchSuggestion tests
Bug 23991: (QA follow-up) Save some DB queries
Bug 23991: Fix branchcode and budgetid filtering
Bug 23991: Fix conflict with bug 28941
Bug 23991: (follow-up) Missing semicolon
Bug 23991: Fix 'all' libraries
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Bug 23991: Remove SearchSuggestion tests
Bug 23991: (QA follow-up) Save some DB queries
Bug 23991: Fix branchcode and budgetid filtering
Bug 23991: Fix conflict with bug 28941
Bug 23991: (follow-up) Missing semicolon
Bug 23991: Fix 'all' libraries
Bug 23991: (follow-up) Add value to filter_archived
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Bug 23991: Move SearchSuggestion to Koha::Suggestions
Bug 23991: (QA follow-up) Fix tabbing
Bug 23991: Don't display empty div if no suggestions exist

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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 2022-03-18 13:28:07 UTC
Created attachment 131896 [details] [review]
Bug 23991: (follow-up) Missing semicolon
Comment 39 Nick Clemens (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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 (kidclamp) 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.