Bug 33973 - Sorting broken on ERM tables
Summary: Sorting broken on ERM tables
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 33066
Blocks: 34107 34201
  Show dependency treegraph
 
Reported: 2023-06-09 14:36 UTC by Jonathan Druart
Modified: 2023-12-28 20:47 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.02,22.11.08


Attachments
Bug 33973: Restore sorting on Vue tables (1.54 KB, patch)
2023-06-09 14:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30002: (QA follow-up) Add .perltidyrc to Makefile.PL mapping (989 bytes, patch)
2023-06-10 19:14 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 33973: Restore sorting on Vue tables (1.60 KB, patch)
2023-06-19 10:15 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33973: Restore sorting on Vue tables (1.66 KB, patch)
2023-06-22 22:02 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-06-09 14:36:40 UTC
Regression from the KohaTable Vue component added by bug 33066.
Comment 1 Jonathan Druart 2023-06-09 14:40:41 UTC
Created attachment 152254 [details] [review]
Bug 33973: Restore sorting on Vue tables

Regression from the KohaTable Vue component added by bug 33066.

_dt_default_ajax expects options.columns to contain the columns.

Test plan:
Add some agreements, go the the agreement list view and confirm that you
can now sort the agreements using the header columns.

Note that the CSS is not correct yet. My understanding is that the
DataTable Vue component keep the 'sorting' class on the th which display
the grey up/down icon over the blue icon which indicate the current sort
column.
Comment 2 Katrin Fischer 2023-06-10 19:14:06 UTC
Created attachment 152270 [details] [review]
Bug 30002: (QA follow-up) Add .perltidyrc to Makefile.PL mapping

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 3 Katrin Fischer 2023-06-14 09:20:55 UTC
Sign-off not quite right as I only re-uploaded the second patch (I'll try to fix later, won't mind if someone else does)
Comment 4 Jonathan Druart 2023-06-14 13:29:52 UTC
(In reply to Katrin Fischer from comment #3)
> Sign-off not quite right as I only re-uploaded the second patch (I'll try to
> fix later, won't mind if someone else does)

There was only one patch.
Comment 5 Pedro Amorim 2023-06-19 10:15:26 UTC
Created attachment 152471 [details] [review]
Bug 33973: Restore sorting on Vue tables

Regression from the KohaTable Vue component added by bug 33066.

_dt_default_ajax expects options.columns to contain the columns.

Test plan:
Add some agreements, go the the agreement list view and confirm that you
can now sort the agreements using the header columns.

Note that the CSS is not correct yet. My understanding is that the
DataTable Vue component keep the 'sorting' class on the th which display
the grey up/down icon over the blue icon which indicate the current sort
column.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 6 Pedro Amorim 2023-06-19 10:20:42 UTC
This fixes the reported issue.

However, Jonathan, I think we should consider moving cypress tests into db_dependent and test with live data coming from the API instead of mocking, otherwise creating a test for this (and other things) is pointless.

In this example, "_order_by: -me.publication_title" was not being sent on the request headers. We can't write a test for this as we're mocking the response instead of getting what the API actually returns.
I think we discussed moving in this direction in the past, but I can't recall what we decided upon or what were the reasons for not doing it!
Comment 7 Jonathan Druart 2023-06-19 10:31:39 UTC
(In reply to Pedro Amorim from comment #6)
> This fixes the reported issue.
> 
> However, Jonathan, I think we should consider moving cypress tests into
> db_dependent and test with live data coming from the API instead of mocking,
> otherwise creating a test for this (and other things) is pointless.
> 
> In this example, "_order_by: -me.publication_title" was not being sent on
> the request headers. We can't write a test for this as we're mocking the
> response instead of getting what the API actually returns.
> I think we discussed moving in this direction in the past, but I can't
> recall what we decided upon or what were the reasons for not doing it!

We need a way to access the config from cypress tests (for the DB connection).
Tests must create the data they need not rely on existing data.
Comment 8 Pedro Amorim 2023-06-19 10:57:34 UTC
I was thinking create the test data by POSTing to the REST API beforehand, and DELETE it after the tests have finished running? Is this reasonable?
Comment 9 Jonathan Druart 2023-06-19 12:47:13 UTC
(In reply to Pedro Amorim from comment #8)
> I was thinking create the test data by POSTing to the REST API beforehand,
> and DELETE it after the tests have finished running? Is this reasonable?

Yes but it is not always easy to create all the depend data you need. Eg for a patron you need a library and a patron category. Is is possible but we certainly need a lib to help us with dependent objects. Having a direct access to the DB would bring us the usage of TestBuilder.
Comment 10 Katrin Fischer 2023-06-22 21:57:33 UTC
Sorry for the earlier patch mess, but now I can QA :)
Comment 11 Katrin Fischer 2023-06-22 22:01:52 UTC
There is still something wrong when searching by Name. Add agreements in this sequence:
t
b
z

They get sequential IDs (1 2 3 or similar).
They get sequential IDs, when you sort, they by Name, they won't sort by the description, but by their ID: zbt or tbz

Filed separately as: 
Bug 34107 - Sorting agreements by Name actually sorts by ID
Comment 12 Katrin Fischer 2023-06-22 22:02:14 UTC
Created attachment 152605 [details] [review]
Bug 33973: Restore sorting on Vue tables

Regression from the KohaTable Vue component added by bug 33066.

_dt_default_ajax expects options.columns to contain the columns.

Test plan:
Add some agreements, go the the agreement list view and confirm that you
can now sort the agreements using the header columns.

Note that the CSS is not correct yet. My understanding is that the
DataTable Vue component keep the 'sorting' class on the th which display
the grey up/down icon over the blue icon which indicate the current sort
column.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Tomás Cohen Arazi 2023-06-23 12:46:13 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 14 Jonathan Druart 2023-07-05 11:47:00 UTC
> Note that the CSS is not correct yet. My understanding is that the
> DataTable Vue component keep the 'sorting' class on the th which display
> the grey up/down icon over the blue icon which indicate the current sort
> column.

I've opened bug 34201.
Comment 15 Martin Renvoize 2023-07-17 14:03:07 UTC
Thanks for all the hard work!

Pushed to 23.05.x for the next release
Comment 16 Pedro Amorim 2023-07-18 10:50:24 UTC
Nice work everyone!

Pushed to 22.11.x for next release