Bug 25033 - Counts of suggestions are confusing
Summary: Counts of suggestions are confusing
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 24370 26833 27462
  Show dependency treegraph
 
Reported: 2020-04-01 12:37 UTC by Nick Clemens
Modified: 2021-01-18 14:02 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
This patch adds to the simple count of all suggestions in the system with a count filtered by the users branch. This means that on the homepage and other areas a user will see a count of local suggestions and total suggestions in the system. Previously clicking the link to suggestions would take the user to a page showing fewer suggestions that counted in the link. Now these numbers should be more consistent.
Version(s) released in:
20.11.00


Attachments
Bug 25033: Add filter_by_user_branch to Koha/Suggestions.pm (5.61 KB, patch)
2020-04-01 13:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Display both local and all pending suggestions counts if the numbers differ (6.83 KB, patch)
2020-04-01 13:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Remove CountSuggestion (4.73 KB, patch)
2020-04-01 13:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Move branchfilter to top of page (4.45 KB, patch)
2020-04-01 13:45 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Add filter_by_user_branch to Koha/Suggestions.pm (5.61 KB, patch)
2020-04-02 11:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Display both local and all pending suggestions counts if the numbers differ (8.06 KB, patch)
2020-04-02 11:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Remove CountSuggestion (4.73 KB, patch)
2020-04-02 11:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Move branchfilter to top of page (3.66 KB, patch)
2020-04-02 11:29 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Add filter_by_user_branch to Koha/Suggestions.pm (5.66 KB, patch)
2020-04-02 15:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25033: Display both local and all pending suggestions counts if the numbers differ (8.11 KB, patch)
2020-04-02 15:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25033: Remove CountSuggestion (4.78 KB, patch)
2020-04-02 15:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25033: Move branchfilter to top of page (3.71 KB, patch)
2020-04-02 15:28 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 25033: (follow-up) Fix links and return self as fallback (3.95 KB, patch)
2020-04-14 11:24 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Display both local and all pending suggestions counts if the numbers differ (8.73 KB, patch)
2020-05-01 13:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Remove CountSuggestion (4.84 KB, patch)
2020-05-01 13:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Move branchfilter to top of page (3.66 KB, patch)
2020-05-01 13:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Display both local and all pending suggestions counts if the numbers differ (8.78 KB, patch)
2020-05-02 01:06 UTC, David Nind
Details | Diff | Splinter Review
Bug 25033: Remove CountSuggestion (4.89 KB, patch)
2020-05-02 01:06 UTC, David Nind
Details | Diff | Splinter Review
Bug 25033: Move branchfilter to top of page (3.71 KB, patch)
2020-05-02 01:06 UTC, David Nind
Details | Diff | Splinter Review
Bug 25033: Improve JS code a bit (2.61 KB, patch)
2020-05-05 10:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25033: (follow-up) Deal with 'Any' branch and don't show dropdown if organized by library (2.75 KB, patch)
2020-05-15 12:26 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: (follow-up) Don't delete branchcode key from suggestion_ref (1.08 KB, patch)
2020-08-05 11:52 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25033: Display both local and all pending suggestions counts if the numbers differ (8.87 KB, patch)
2020-08-23 10:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25033: Remove CountSuggestion (4.95 KB, patch)
2020-08-23 10:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25033: Move branchfilter to top of page (3.77 KB, patch)
2020-08-23 10:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25033: Improve JS code a bit (2.67 KB, patch)
2020-08-23 10:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25033: (follow-up) Deal with 'Any' branch and don't show dropdown if organized by library (2.82 KB, patch)
2020-08-23 10:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25033: (follow-up) Don't delete branchcode key from suggestion_ref (1.14 KB, patch)
2020-08-23 10:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25033: Fix number of tests (704 bytes, patch)
2020-08-23 10:46 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25033: (follow-up) Don't limit suggestions to branches if displaying by branch (1.17 KB, patch)
2020-08-24 12:31 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-04-01 12:37:06 UTC
On mainpage and acqui-home we show pending suggestions, however, the number is not filtered by the users branch. When you click the link to manage, the resulting page is filtered by the user's branch by default.

We should make the view a little clearer by showing both local and all pending suggestions
Comment 1 Nick Clemens 2020-04-01 13:45:34 UTC
Created attachment 102225 [details] [review]
Bug 25033: Add filter_by_user_branch to Koha/Suggestions.pm
Comment 2 Nick Clemens 2020-04-01 13:45:37 UTC
Created attachment 102226 [details] [review]
Bug 25033: Display both local and all pending suggestions counts if the numbers differ

To test:
 0 - Be in staff client as a superlibrarian
 1 - Place some suggestions
  1 for any branch
  1 for signed in branch
  1 for another branch
 2 - Go to Koha main page, 3 suggestions pending
 3 - Click 'Suggestions pending approval' - you see one suggestion
 4 - Click on 'Acquisitions' in breadcrumbs , 3 suggestions pending
 5 - Click manage suggestions - you see one suggestion
 6 - Apply patch
 7 - On mainpage and acqui-home you now see "1 (3)" suggestions
 8 - Turn on IndependentBranches
 9 - Create a user with acquisition and suggestions permissions but not superlibrarian in one of the branches used above
10 - Sign in as that user
11 - See 1 suggestion on mainpage and 1 on acqui home
Comment 3 Nick Clemens 2020-04-01 13:45:39 UTC
Created attachment 102227 [details] [review]
Bug 25033: Remove CountSuggestion
Comment 4 Nick Clemens 2020-04-01 13:45:42 UTC
Created attachment 102228 [details] [review]
Bug 25033: Move branchfilter to top of page

There is confusion becase it is not obvious when the suggestions page is being limited to a branch

Moving the branch filter to the top of the page makes it easier to see

To test:
1 - Apply patch
2 - From mainpage or acqui-home note the link to suggestions takes you to suggestions limited to current branch
3 - Change the dropdown
4 - Note the page refreshes and loads the correct suggestions
Comment 5 Nick Clemens 2020-04-01 18:58:17 UTC
Instead of:

1 (5)

Should be:

 Centerville 1 / All 5

The words above should be links to view current library suggestions or all library suggestions


Also, don't delete the branch choice when adding suggestions, d'oh
Comment 6 Nick Clemens 2020-04-02 11:29:23 UTC
Created attachment 102282 [details] [review]
Bug 25033: Add filter_by_user_branch to Koha/Suggestions.pm
Comment 7 Nick Clemens 2020-04-02 11:29:25 UTC
Created attachment 102283 [details] [review]
Bug 25033: Display both local and all pending suggestions counts if the numbers differ

To test:
 0 - Be in staff client as a superlibrarian
 1 - Place some suggestions
  1 for any branch
  1 for signed in branch
  1 for another branch
 2 - Go to Koha main page, 3 suggestions pending
 3 - Click 'Suggestions pending approval' - you see one suggestion
 4 - Click on 'Acquisitions' in breadcrumbs , 3 suggestions pending
 5 - Click manage suggestions - you see one suggestion
 6 - Apply patch
 7 - On mainpage and acqui-home you now see "Centerville: 1 / All libraries: 3" suggestions
 8 - Confirm that the links take you to suggestions view of your branch or all libraries respectively
 9 - Turn on IndependentBranches
10 - Create a user with acquisition and suggestions permissions but not superlibrarian in one of the branches used above
11 - Sign in as that user
11 - See "Centerville: 1" suggestion on mainpage and on acqui home
Comment 8 Nick Clemens 2020-04-02 11:29:28 UTC
Created attachment 102284 [details] [review]
Bug 25033: Remove CountSuggestion
Comment 9 Nick Clemens 2020-04-02 11:29:31 UTC
Created attachment 102285 [details] [review]
Bug 25033: Move branchfilter to top of page

There is confusion becase it is not obvious when the suggestions page is being limited to a branch

Moving the branch filter to the top of the page makes it easier to see

To test:
1 - Apply patch
2 - From mainpage or acqui-home note the link to suggestions takes you to suggestions limited to current branch
3 - Change the dropdown
4 - Note the page refreshes and loads the correct suggestions
Comment 10 Owen Leonard 2020-04-02 15:28:13 UTC
Created attachment 102299 [details] [review]
Bug 25033: Add filter_by_user_branch to Koha/Suggestions.pm

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Owen Leonard 2020-04-02 15:28:18 UTC
Created attachment 102300 [details] [review]
Bug 25033: Display both local and all pending suggestions counts if the numbers differ

To test:
 0 - Be in staff client as a superlibrarian
 1 - Place some suggestions
  1 for any branch
  1 for signed in branch
  1 for another branch
 2 - Go to Koha main page, 3 suggestions pending
 3 - Click 'Suggestions pending approval' - you see one suggestion
 4 - Click on 'Acquisitions' in breadcrumbs , 3 suggestions pending
 5 - Click manage suggestions - you see one suggestion
 6 - Apply patch
 7 - On mainpage and acqui-home you now see "Centerville: 1 / All libraries: 3" suggestions
 8 - Confirm that the links take you to suggestions view of your branch or all libraries respectively
 9 - Turn on IndependentBranches
10 - Create a user with acquisition and suggestions permissions but not superlibrarian in one of the branches used above
11 - Sign in as that user
11 - See "Centerville: 1" suggestion on mainpage and on acqui home

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 12 Owen Leonard 2020-04-02 15:28:22 UTC
Created attachment 102301 [details] [review]
Bug 25033: Remove CountSuggestion

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 Owen Leonard 2020-04-02 15:28:27 UTC
Created attachment 102302 [details] [review]
Bug 25033: Move branchfilter to top of page

There is confusion becase it is not obvious when the suggestions page is being limited to a branch

Moving the branch filter to the top of the page makes it easier to see

To test:
1 - Apply patch
2 - From mainpage or acqui-home note the link to suggestions takes you to suggestions limited to current branch
3 - Change the dropdown
4 - Note the page refreshes and loads the correct suggestions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 14 Jonathan Druart 2020-04-13 12:49:07 UTC
1. Can you rename the "$option" parameter to something more meaningful?
$option let think it's a hashref.

2. Reading filter_by_user_branch I'd say there is a case where we return undef, and it's not expected (chaining should be allowed).
I'd say the 'else' should be removed, to return $self in any cases.

3. Actually, maybe you should pass the logged in user instead of the branchcode, that will allow to filter for someone else than the logged in user, and that will make you remove the test on $userenv. What do you think?
Comment 15 Nick Clemens 2020-04-14 11:24:14 UTC
Created attachment 102922 [details] [review]
Bug 25033: (follow-up) Fix links and return self as fallback
Comment 16 Nick Clemens 2020-04-14 11:32:00 UTC
(In reply to Jonathan Druart from comment #14)
> 1. Can you rename the "$option" parameter to something more meaningful?
> $option let think it's a hashref.

Done
 
> 2. Reading filter_by_user_branch I'd say there is a case where we return
> undef, and it's not expected (chaining should be allowed).
> I'd say the 'else' should be removed, to return $self in any cases.

else removed.

> 3. Actually, maybe you should pass the logged in user instead of the
> branchcode, that will allow to filter for someone else than the logged in
> user, and that will make you remove the test on $userenv. What do you think?

I think we need the userenv as we want contextual filtering - i.e. it matters more which branch you are signed in
Comment 17 Jonathan Druart 2020-04-14 12:32:08 UTC
Resume for private discussion:
Maybe the method was not needed (but now we have it, and covered by tests).
Another solution would have been to do something like Koha::Patron->libraries_where_can_see_patrons, ie. get a whitelist of libraries for a given patron. (Note that this method currently use userenv but should not, and could avoid it).

Waiting for another QA feedback.
Comment 18 Jonathan Druart 2020-04-14 12:32:38 UTC
(In reply to Jonathan Druart from comment #17)
> Resume for private discussion:
*from*
Comment 19 Jonathan Druart 2020-04-15 10:18:09 UTC
Nick, I tried to implement a simple version of Koha::Objects::Limit::Library, that could be used here.

But I am having the impression that what we finally need here is simply using C4::Context::only_my_library from the controller script.

I do not think we want such subroutines appear in several of our Koha modules.
Comment 20 Jonathan Druart 2020-04-24 10:30:23 UTC
Failing QA, please remove that method.
Comment 21 Nick Clemens 2020-05-01 13:53:45 UTC
Created attachment 104091 [details] [review]
Bug 25033: Display both local and all pending suggestions counts if the numbers differ

To test:
 0 - Be in staff client as a superlibrarian
 1 - Place some suggestions
  1 for any branch
  1 for signed in branch
  1 for another branch
 2 - Go to Koha main page, 3 suggestions pending
 3 - Click 'Suggestions pending approval' - you see one suggestion
 4 - Click on 'Acquisitions' in breadcrumbs , 3 suggestions pending
 5 - Click manage suggestions - you see one suggestion
 6 - Apply patch
 7 - On mainpage and acqui-home you now see "Centerville: 1 / All libraries: 3" suggestions
 8 - Confirm that the links take you to suggestions view of your branch or all libraries respectively
 9 - Turn on IndependentBranches
10 - Create a user with acquisition and suggestions permissions but not superlibrarian in one of the branches used above
11 - Sign in as that user
11 - See "Centerville: 1" suggestion on mainpage and on acqui home
Comment 22 Nick Clemens 2020-05-01 13:53:49 UTC
Created attachment 104092 [details] [review]
Bug 25033: Remove CountSuggestion

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 23 Nick Clemens 2020-05-01 13:53:52 UTC
Created attachment 104093 [details] [review]
Bug 25033: Move branchfilter to top of page

There is confusion becase it is not obvious when the suggestions page is being limited to a branch

Moving the branch filter to the top of the page makes it easier to see

To test:
1 - Apply patch
2 - From mainpage or acqui-home note the link to suggestions takes you to suggestions limited to current branch
3 - Change the dropdown
4 - Note the page refreshes and loads the correct suggestions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 24 David Nind 2020-05-02 01:06:40 UTC
Created attachment 104160 [details] [review]
Bug 25033: Display both local and all pending suggestions counts if the numbers differ

To test:
 0 - Be in staff client as a superlibrarian
 1 - Place some suggestions
  1 for any branch
  1 for signed in branch
  1 for another branch
 2 - Go to Koha main page, 3 suggestions pending
 3 - Click 'Suggestions pending approval' - you see one suggestion
 4 - Click on 'Acquisitions' in breadcrumbs , 3 suggestions pending
 5 - Click manage suggestions - you see one suggestion
 6 - Apply patch
 7 - On mainpage and acqui-home you now see "Centerville: 1 / All libraries: 3" suggestions
 8 - Confirm that the links take you to suggestions view of your branch or all libraries respectively
 9 - Turn on IndependentBranches
10 - Create a user with acquisition and suggestions permissions but not superlibrarian in one of the branches used above
11 - Sign in as that user
11 - See "Centerville: 1" suggestion on mainpage and on acqui home

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2020-05-02 01:06:43 UTC
Created attachment 104161 [details] [review]
Bug 25033: Remove CountSuggestion

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 David Nind 2020-05-02 01:06:47 UTC
Created attachment 104162 [details] [review]
Bug 25033: Move branchfilter to top of page

There is confusion becase it is not obvious when the suggestions page is being limited to a branch

Moving the branch filter to the top of the page makes it easier to see

To test:
1 - Apply patch
2 - From mainpage or acqui-home note the link to suggestions takes you to suggestions limited to current branch
3 - Change the dropdown
4 - Note the page refreshes and loads the correct suggestions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>
Comment 27 Jonathan Druart 2020-05-05 10:33:06 UTC
1. We should remove the new dropdown list if the "Organize by" filter is set to Library

2.
- Select "Any"
- Select "Organize by library"

Can't call method "branchname" on an undefined value at /kohadevbox/koha/suggestion/suggestion.pl line 64
Comment 28 Jonathan Druart 2020-05-05 10:33:57 UTC
Created attachment 104357 [details] [review]
Bug 25033: Improve JS code a bit
Comment 29 Nick Clemens 2020-05-15 12:26:32 UTC
Created attachment 104940 [details] [review]
Bug 25033: (follow-up) Deal with 'Any' branch and don't show dropdown if organized by library
Comment 30 Jonathan Druart 2020-05-15 13:06:34 UTC
Found a bug:
Create a suggestion 'xxx' for CPL
Create a suggestion 'yyy' for FPL

You see: https://snipboard.io/tIeo86.jpg

Dropdown list has "FPL" and CPL is displayed (??)
Comment 31 Nick Clemens 2020-08-05 11:52:20 UTC
Created attachment 107831 [details] [review]
Bug 25033: (follow-up) Don't delete branchcode key from suggestion_ref
Comment 32 Katrin Fischer 2020-08-23 10:44:13 UTC
I really like this. This is a great improvement for a reoccurring support question I'd love to get rid of!

The only thing that gets me thinking a bit is that there is a bit of code repeat between the start page and the acquisition home page right now. But I'd not want this to be held up much longer.
Comment 33 Katrin Fischer 2020-08-23 10:45:42 UTC
Created attachment 108948 [details] [review]
Bug 25033: Display both local and all pending suggestions counts if the numbers differ

To test:
 0 - Be in staff client as a superlibrarian
 1 - Place some suggestions
  1 for any branch
  1 for signed in branch
  1 for another branch
 2 - Go to Koha main page, 3 suggestions pending
 3 - Click 'Suggestions pending approval' - you see one suggestion
 4 - Click on 'Acquisitions' in breadcrumbs , 3 suggestions pending
 5 - Click manage suggestions - you see one suggestion
 6 - Apply patch
 7 - On mainpage and acqui-home you now see "Centerville: 1 / All libraries: 3" suggestions
 8 - Confirm that the links take you to suggestions view of your branch or all libraries respectively
 9 - Turn on IndependentBranches
10 - Create a user with acquisition and suggestions permissions but not superlibrarian in one of the branches used above
11 - Sign in as that user
11 - See "Centerville: 1" suggestion on mainpage and on acqui home

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 34 Katrin Fischer 2020-08-23 10:45:46 UTC
Created attachment 108949 [details] [review]
Bug 25033: Remove CountSuggestion

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2020-08-23 10:45:50 UTC
Created attachment 108950 [details] [review]
Bug 25033: Move branchfilter to top of page

There is confusion becase it is not obvious when the suggestions page is being limited to a branch

Moving the branch filter to the top of the page makes it easier to see

To test:
1 - Apply patch
2 - From mainpage or acqui-home note the link to suggestions takes you to suggestions limited to current branch
3 - Change the dropdown
4 - Note the page refreshes and loads the correct suggestions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 36 Katrin Fischer 2020-08-23 10:45:53 UTC
Created attachment 108951 [details] [review]
Bug 25033: Improve JS code a bit

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 37 Katrin Fischer 2020-08-23 10:45:57 UTC
Created attachment 108952 [details] [review]
Bug 25033: (follow-up) Deal with 'Any' branch and don't show dropdown if organized by library

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 38 Katrin Fischer 2020-08-23 10:46:01 UTC
Created attachment 108953 [details] [review]
Bug 25033: (follow-up) Don't delete branchcode key from suggestion_ref

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 39 Katrin Fischer 2020-08-23 10:46:06 UTC
Created attachment 108954 [details] [review]
Bug 25033: Fix number of tests

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 40 Jonathan Druart 2020-08-24 10:10:41 UTC
I am sorry Nick, but I found something...

1. Go to /cgi-bin/koha/suggestion/suggestion.pl
2. Select all libraries:
https://snipboard.io/esC3ot.jpg
3. Select IPT
=> Nothing displayed # OK
4. Organize by library
=> Nothing displayed # KO?
/cgi-bin/koha/suggestion/suggestion.pl?branchcode=IPT&displayby=branchcode&budgetid=__ANY__

The problem is that there is no more "library" filter.
Maybe we should remove "branchcode" if organizing by library?
Comment 41 Nick Clemens 2020-08-24 12:31:23 UTC
Created attachment 108984 [details] [review]
Bug 25033: (follow-up) Don't limit suggestions to branches if displaying by branch

In the case of organizing by branches we should not filter by branch
Comment 42 Jonathan Druart 2020-08-25 13:16:14 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 43 Lucas Gass 2020-09-04 19:49:14 UTC
enhancement will not be backported to 20.05.x