Bug 12388 - IssueSlip output should be sorted by date_due, not timestamp
Summary: IssueSlip output should be sorted by date_due, not timestamp
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Johanna Räisä
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-09 12:01 UTC by paxed
Modified: 2023-01-24 13:43 UTC (History)
11 users (show)

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


Attachments
Patch (957 bytes, patch)
2015-08-21 13:09 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 12388: IssueSlip output should be sorted by date_due, not timestamp (1.22 KB, patch)
2017-04-19 11:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 12388: IssueSlip output should be sorted by date_due, not timestamp (2.05 KB, patch)
2019-07-15 09:12 UTC, Johanna Räisä
Details | Diff | Splinter Review
[PATCH] Bug 12388: IssueSlip output should be sorted by date_due, not (2.11 KB, patch)
2019-07-25 09:49 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12388: Adjust tests (1.19 KB, patch)
2020-04-15 09:42 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 12388: IssueSlip output should be sorted by date_due, not timestamp (2.17 KB, patch)
2020-04-30 13:21 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 12388: Adjust tests (1.25 KB, patch)
2020-04-30 13:21 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 12388: Adjust tests some more (2.77 KB, patch)
2020-12-04 09:16 UTC, Emmi Takkinen
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description paxed 2014-06-09 12:01:43 UTC
Sorting the quick slip output by date_due is more useful for patrons - the timestamp when the issues was created is not as important.
Comment 1 Joonas Kylmälä 2015-08-21 13:09:32 UTC
Created attachment 41784 [details] [review]
Patch

Here is a patch from Pasi which I just found for this bug.
Comment 2 Olli-Antti Kivilahti 2017-03-21 12:36:23 UTC
Old but gold.
Comment 3 Lari Taskula 2017-04-19 11:29:11 UTC
Created attachment 62361 [details] [review]
Bug 12388: IssueSlip output should be sorted by date_due, not timestamp
Comment 4 Marc Véron 2017-04-19 13:07:57 UTC
(In reply to Lari Taskula from comment #3)
> Created attachment 62361 [details] [review] [review]
> Bug 12388: IssueSlip output should be sorted by date_due, not timestamp

Works as expected, however comment in source code line 1184 is now misleading:
	
    # Sort on timestamp then on issuedate (useful for tests and could be if modified in a batch
Comment 5 Katrin Fischer 2017-05-02 18:45:12 UTC
The QA scripts are not happy about this patch:

 FAIL	C4/Members.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   FAIL	  valid
		Useless use of numeric comparison (<=>) in void context 
		Useless use of private variable in void context
Comment 6 Johanna Räisä 2019-07-15 09:12:16 UTC
Created attachment 91522 [details] [review]
Bug 12388: IssueSlip output should be sorted by date_due, not timestamp

Fixed the patch to apply master. I think the QA fail also is fixed since the structure is changed a lot.
Comment 7 Emmi Takkinen 2019-07-25 09:49:13 UTC
Created attachment 91766 [details] [review]
[PATCH] Bug 12388: IssueSlip output should be sorted by date_due, not

 timestamp

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Comment 8 Jonathan Druart 2019-12-03 11:09:12 UTC
Please adjust the tests.

t/db_dependent/Members/IssueSlip.t
Comment 9 Emmi Takkinen 2020-04-15 09:42:31 UTC
Created attachment 102977 [details] [review]
Bug 12388: Adjust tests

To test, prove t/db_dependent/Members/IssueSlip.t

Sponsored-by: Koha-Suomi Oy
Comment 10 Bernardo Gonzalez Kriegel 2020-04-30 13:21:39 UTC
Created attachment 104033 [details] [review]
Bug 12388: IssueSlip output should be sorted by date_due, not timestamp

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Emmi Takkinen <emmi.takkinen@outlook.com>
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 11 Bernardo Gonzalez Kriegel 2020-04-30 13:21:44 UTC
Created attachment 104034 [details] [review]
Bug 12388: Adjust tests

To test, prove t/db_dependent/Members/IssueSlip.t

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Comment 12 Joonas Kylmälä 2020-06-03 08:14:29 UTC
Hi,

the tests didn't remove/replace the now unneeded code under the comments

> # Set timestamps to the same value to avoid a different order

Also the test cases now only test sorting with issue date, not by date due. Testing of date due sorting should be added to cover this change fully.
Comment 13 Emmi Takkinen 2020-11-17 14:11:34 UTC
(In reply to Joonas Kylmälä from comment #12)
> Hi,
> 
> the tests didn't remove/replace the now unneeded code under the comments
> 
> > # Set timestamps to the same value to avoid a different order
Hmm, sure, I guess there's no point to adjust timestamp anymore since we sort result from pending_checkouts by date_due. So those code lines can be removed.

> Also the test cases now only test sorting with issue date, not by date due.
> Testing of date due sorting should be added to cover this change fully.
Sorry, I don't fully understand what you mean by this. I don't see any test where we specially test sorting with issue date. However every test expects output to have issues in correct order and that includes having correct date due value.
Comment 14 Joonas Kylmälä 2020-11-18 08:04:01 UTC
(In reply to Emmi Takkinen from comment #13)
> > Also the test cases now only test sorting with issue date, not by date due.
> > Testing of date due sorting should be added to cover this change fully.
> Sorry, I don't fully understand what you mean by this. I don't see any test
> where we specially test sorting with issue date. However every test expects
> output to have issues in correct order and that includes having correct date
> due value.

I think earlier I must have meant that in the patch "Bug 12388: Adjust tests" the tests' the changes are made so that the expected order is by issue date). If you read the code for the first test case changed here it sets for both of the checkouts the same due date "$date_due = $today_daily;" so it won't be able to test whether the sorting by date_due works. But yes, what I meant that maybe we should have tests for the correct sorting, now I understand that the original patch "Adjust tests" might have not had that goal because it changed a test that was only for checking whether we get separate "Checked out" and "Overdues" sections in the slip. So I think the patch "Bug 12388: Adjust tests" is okay, but/and if you think it is important there should be test for testing sorting order as well for the slip.
Comment 15 Emmi Takkinen 2020-12-02 10:19:58 UTC

(In reply to Joonas Kylmälä from comment #14)
> I think earlier I must have meant that in the patch "Bug 12388: Adjust
> tests" the tests' the changes are made so that the expected order is by
> issue date). If you read the code for the first test case changed here it
> sets for both of the checkouts the same due date "$date_due = $today_daily;"
> so it won't be able to test whether the sorting by date_due works. But yes,
> what I meant that maybe we should have tests for the correct sorting, now I
> understand that the original patch "Adjust tests" might have not had that
> goal because it changed a test that was only for checking whether we get
> separate "Checked out" and "Overdues" sections in the slip. So I think the
> patch "Bug 12388: Adjust tests" is okay, but/and if you think it is
> important there should be test for testing sorting order as well for the
> slip.
Yeah, "Bug 12388: Adjust tests" was simply meant to prevent tests from failing after adding sort with date_due. Just swapping titles and barcodes was enough to achieve this but maybe it wouldn't hurt to have different date due for another of those items (or just add another item with different date due).
Comment 16 Emmi Takkinen 2020-12-04 09:16:21 UTC
Created attachment 114180 [details] [review]
Bug 12388: Adjust tests some more

We should now test if issues are really sorted with
date_due. This patch adjusts test for daily loans so
that test issues have different date_due values.
Also removes some now unneeded code from IssueSlip.t.

To test prove t/db_dependent/Members/IssueSlip.t

Sponsored-by: Koha-Suomi Oy
Comment 17 Joonas Kylmälä 2022-09-03 08:01:41 UTC
Running prove t/db_dependent/Members/IssueSlip.t gives the following warning:

t/db_dependent/Members/IssueSlip.t .. DBIx::Class::ResultSet::_construct_results(): Unable to properly collapse has_many results in iterator mode due to order criteria - performed an eager cursor slurp underneath. Consider using ->all() instead at /kohadevbox/koha/Koha/Objects.pm line 312

Can you also please clone $tomorrow and $yesterday from $today, and subtract/add one day? This prevents the race condition that could happen in the unit tests when the time is 23:59.
Comment 18 Emmi Takkinen 2022-09-14 08:08:48 UTC
(In reply to Joonas Kylmälä from comment #17)
> Running prove t/db_dependent/Members/IssueSlip.t gives the following warning:
> 
> t/db_dependent/Members/IssueSlip.t ..
> DBIx::Class::ResultSet::_construct_results(): Unable to properly collapse
> has_many results in iterator mode due to order criteria - performed an eager
> cursor slurp underneath. Consider using ->all() instead at
> /kohadevbox/koha/Koha/Objects.pm line 312
> 
If I understand correctly (and correct me if I'm wrong), this happens because sub pending_checkouts uses prefetch to get data (at least) from biblio table. Bug 28561 handled same kind of problem, comment from there:

> DBIx says about prefetching and collapse the following:
> If an "order_by" is already declared, and orders the resultset in a way that 
> makes collapsing as described above impossible (e.g. ORDER BY 
> has_many_rel.column or ORDER BY RANDOM()), DBIC will automatically switch to 
> "eager" mode and slurp the entire resultset before constructing the first 
> object returned by "next".

In bug 28561 this was handled by replacing prefetch with join. However using this same approach here would mean we lose data from biblio table since join returns data only from, in this case, issues table.

At least one solution for this would be to move those order_by statements (date_due, issuedate) from sub IssuesSlip to sub pending_reserves. However I'm not sure if this could have some side effects in other parts of Koha.