Description
Nick Clemens (kidclamp)
2020-04-01 12:37:06 UTC
Created attachment 102225 [details] [review] Bug 25033: Add filter_by_user_branch to Koha/Suggestions.pm 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 Created attachment 102227 [details] [review] Bug 25033: Remove CountSuggestion 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 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 Created attachment 102282 [details] [review] Bug 25033: Add filter_by_user_branch to Koha/Suggestions.pm 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 Created attachment 102284 [details] [review] Bug 25033: Remove CountSuggestion 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 Created attachment 102299 [details] [review] Bug 25033: Add filter_by_user_branch to Koha/Suggestions.pm Signed-off-by: Owen Leonard <oleonard@myacpl.org> 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> Created attachment 102301 [details] [review] Bug 25033: Remove CountSuggestion Signed-off-by: Owen Leonard <oleonard@myacpl.org> 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> 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? Created attachment 102922 [details] [review] Bug 25033: (follow-up) Fix links and return self as fallback (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 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. (In reply to Jonathan Druart from comment #17) > Resume for private discussion: *from* 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. Failing QA, please remove that method. 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 Created attachment 104092 [details] [review] Bug 25033: Remove CountSuggestion Signed-off-by: Owen Leonard <oleonard@myacpl.org> 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> 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> 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> 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> 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 Created attachment 104357 [details] [review] Bug 25033: Improve JS code a bit Created attachment 104940 [details] [review] Bug 25033: (follow-up) Deal with 'Any' branch and don't show dropdown if organized by library 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 (??) Created attachment 107831 [details] [review] Bug 25033: (follow-up) Don't delete branchcode key from suggestion_ref 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. 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> 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> 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> Created attachment 108951 [details] [review] Bug 25033: Improve JS code a bit Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> 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> 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> Created attachment 108954 [details] [review] Bug 25033: Fix number of tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> 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? 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 Pushed to master for 20.11, thanks to everybody involved! enhancement will not be backported to 20.05.x |