Bug 29530 - When NumSavedReports is set, show value in pull down of entries
Summary: When NumSavedReports is set, show value in pull down of entries
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial
Assignee: Owen Leonard
QA Contact: Testopia
URL:
Keywords:
Depends on: 29488
Blocks: 29648
  Show dependency tree / graph
 
Reported: 2021-11-19 14:35 UTC by Katrin Fischer
Modified: 2022-12-12 21:23 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This updates the way the NumSavedReports preference value is used on the saved reports page. For the "Show" dropwdown list: - it now displays the number set in NumSavedReports (previously it showed 20) - when expanded it now shows the number set in NumSavedReports sequentially (for example, if NumSavedReports is 78, the menu options should be "10, 20, 50, 78, 100, All"), and - it now displays 'All' if NumSavedReports is blank. It also updates the description for the NumSavedReports preference to clarify that all reports are shown when no value is entered.
Version(s) released in:
22.05.00,21.11.02,21.05.09
Circulation function:

Attachments
Bug 29530: Fix handling of NumSavedReports preference in reports table (5.56 KB, patch)
2021-11-23 18:58 UTC, Owen Leonard 		Details | Diff | Splinter Review
Bug 29530: Fix handling of NumSavedReports preference in reports table (5.61 KB, patch)
2021-11-25 19:46 UTC, David Nind 		Details | Diff | Splinter Review
Bug 29530: Fix handling of NumSavedReports preference in reports table (5.69 KB, patch)
2021-12-02 14:54 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Bug 29530: Fix handling of NumSavedReports preference in reports table (5.68 KB, patch)
2021-12-17 08:15 UTC, Jonathan Druart 		Details | Diff | Splinter Review
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2021-11-19 14:35:51 UTC 
When NumSavedReports is set to a value, the list of reports defaults to the set number of reports showing on a page, but the pull down of settings for per page entries will be empty. So after changing the value, you cannot reset it. We should make the entry show up there as well (if it doesn't match one of the existing already).
Comment 1 Owen Leonard 2021-11-23 18:58:32 UTC 
Created attachment 127965 [details] [review]
Bug 29530: Fix handling of NumSavedReports preference in reports table

This patch updates the way the NumSavedReports preference value is used
on the saved reports page so that the setting is correctly incorporated
into the DataTable.

The patch also expands the description of the NumSavedReports preference
in order to clarify the expected behavior when no value is saved.

To test, apply the patch and go to Administration -> System
preferences and note the value of NumSavedReports.

 - Go to Reports -> Use saved.
 - Confirm that the first page of saved reports shows the number
   specified in NumSavedReports.
 - In the "Show" dropdown menu, confirm that the number from
   NumSavedReports is preselected by default.
   - Expand the dropdown menu to confirm that the NumSavedReports number
     is positioned sequentially with the default values. For example, if
     NumSavedReports = "78," the menu options should be
     "10, 20, 50, 78, 100, All".
 - Test with various values of NumSavedReports. A blank value should
   result in the "All" option being selected. A non-numeric or
   non-positive value should result in the default set of options being
   used ("10, 20, 50, 100, All").
Comment 2 David Nind 2021-11-25 19:46:11 UTC 
Created attachment 128016 [details] [review]
Bug 29530: Fix handling of NumSavedReports preference in reports table

This patch updates the way the NumSavedReports preference value is used
on the saved reports page so that the setting is correctly incorporated
into the DataTable.

The patch also expands the description of the NumSavedReports preference
in order to clarify the expected behavior when no value is saved.

To test, apply the patch and go to Administration -> System
preferences and note the value of NumSavedReports.

 - Go to Reports -> Use saved.
 - Confirm that the first page of saved reports shows the number
   specified in NumSavedReports.
 - In the "Show" dropdown menu, confirm that the number from
   NumSavedReports is preselected by default.
   - Expand the dropdown menu to confirm that the NumSavedReports number
     is positioned sequentially with the default values. For example, if
     NumSavedReports = "78," the menu options should be
     "10, 20, 50, 78, 100, All".
 - Test with various values of NumSavedReports. A blank value should
   result in the "All" option being selected. A non-numeric or
   non-positive value should result in the default set of options being
   used ("10, 20, 50, 100, All").

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Jonathan Druart 2021-12-02 14:54:55 UTC 
Created attachment 128172 [details] [review]
Bug 29530: Fix handling of NumSavedReports preference in reports table

This patch updates the way the NumSavedReports preference value is used
on the saved reports page so that the setting is correctly incorporated
into the DataTable.

The patch also expands the description of the NumSavedReports preference
in order to clarify the expected behavior when no value is saved.

To test, apply the patch and go to Administration -> System
preferences and note the value of NumSavedReports.

 - Go to Reports -> Use saved.
 - Confirm that the first page of saved reports shows the number
   specified in NumSavedReports.
 - In the "Show" dropdown menu, confirm that the number from
   NumSavedReports is preselected by default.
   - Expand the dropdown menu to confirm that the NumSavedReports number
     is positioned sequentially with the default values. For example, if
     NumSavedReports = "78," the menu options should be
     "10, 20, 50, 78, 100, All".
 - Test with various values of NumSavedReports. A blank value should
   result in the "All" option being selected. A non-numeric or
   non-positive value should result in the default set of options being
   used ("10, 20, 50, 100, All").

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 4 Jonathan Druart 2021-12-02 14:56:55 UTC 
It's working but we need to make this code generic.

First the NumSavedReports syspref should be removed in favour of the "default display length" table setting we now have.
Then it would be actually great to make this setting work for all the tables, everywhere KohaTable is used. Owen, is that something you would like to work on? I could help of course.
Comment 5 Owen Leonard 2021-12-06 15:16:04 UTC 
(In reply to Jonathan Druart from comment #4)

> First the NumSavedReports syspref should be removed in favour of the
> "default display length" table setting we now have.

Bug 17465 didn't have any discussion on it about why we needed a separate system preference, but since the numSearchResults predates it I think we must assume that the intention of Bug 17465 was to create an exception to the numSearchResults setting.

On the other hand, perhaps the need for NumSavedReports is not as great now that we have better searching via DataTables?
Comment 6 David Nind 2021-12-06 18:41:45 UTC 
If we have a generic way of doing this for all tables with DataTables, then we should probably do that - whatever way, it should work.

I'm all for making things as consistent as possible in the UI, and one less system preference is probably a good thing!

Maybe a separate bug, since you have got this back working...
Comment 7 Katrin Fischer 2021-12-06 19:45:22 UTC 
(In reply to David Nind from comment #6)
> If we have a generic way of doing this for all tables with DataTables, then
> we should probably do that - whatever way, it should work.
> 
> I'm all for making things as consistent as possible in the UI, and one less
> system preference is probably a good thing!
> 
> Maybe a separate bug, since you have got this back working...

That's my thought too - let's make the move to table configuration but treat this as a bug fix until we have the better solution.
Comment 8 David Nind 2021-12-06 19:55:05 UTC 
(In reply to Katrin Fischer from comment #7)

> That's my thought too - let's make the move to table configuration but treat
> this as a bug fix until we have the better solution.

+1
Comment 9 Jonathan Druart 2021-12-17 08:15:48 UTC 
Created attachment 128644 [details] [review]
Bug 29530: Fix handling of NumSavedReports preference in reports table

This patch updates the way the NumSavedReports preference value is used
on the saved reports page so that the setting is correctly incorporated
into the DataTable.

The patch also expands the description of the NumSavedReports preference
in order to clarify the expected behavior when no value is saved.

To test, apply the patch and go to Administration -> System
preferences and note the value of NumSavedReports.

 - Go to Reports -> Use saved.
 - Confirm that the first page of saved reports shows the number
   specified in NumSavedReports.
 - In the "Show" dropdown menu, confirm that the number from
   NumSavedReports is preselected by default.
   - Expand the dropdown menu to confirm that the NumSavedReports number
     is positioned sequentially with the default values. For example, if
     NumSavedReports = "78," the menu options should be
     "10, 20, 50, 78, 100, All".
 - Test with various values of NumSavedReports. A blank value should
   result in the "All" option being selected. A non-numeric or
   non-positive value should result in the default set of options being
   used ("10, 20, 50, 100, All").

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2021-12-17 08:16:07 UTC 
Rebased on top of bug 29488.
Comment 11 Fridolin Somers 2022-01-14 02:38:35 UTC 
Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑
Comment 12 Kyle M Hall (khall) 2022-01-14 14:26:50 UTC 
Pushed to 21.11.x for 21.11.02
Comment 13 Andrew Fuerste-Henry 2022-01-25 17:35:33 UTC 
Pushed to 21.05.x for 21.05.09
Comment 14 Victor Grousset/tuxayo 2022-02-11 20:31:34 UTC 
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.