Bug 33490 - Agreements - Filter by expired results in error
Summary: Agreements - Filter by expired results in error
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 32030 33483
Blocks:
  Show dependency treegraph
 
Reported: 2023-04-11 16:31 UTC by Pedro Amorim
Modified: 2023-06-15 19:43 UTC (History)
5 users (show)

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


Attachments
/cgi-bin/koha/erm/agreements?by_expired=true (179.70 KB, image/png)
2023-04-12 10:09 UTC, Pedro Amorim
Details
Bug 33490: Prevent 400 because agreements?by_expired=true is missing date (1.79 KB, patch)
2023-04-12 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Prevent 400 on agreements?by_expired=true (1.77 KB, patch)
2023-04-12 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Prevent 400 on agreements?by_expired=true (1.83 KB, patch)
2023-04-12 10:39 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33490: Unit tests (3.17 KB, patch)
2023-04-13 13:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33490: Prevent 400 on agreements?by_expired=true (1.89 KB, patch)
2023-04-13 13:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33490: Make url consistent (2.32 KB, patch)
2023-04-13 17:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33490: Make url consistent (3.09 KB, patch)
2023-04-14 07:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33490: Improve max date handling (3.93 KB, patch)
2023-04-14 09:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Use computed values (4.89 KB, patch)
2023-04-14 10:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Unit tests (3.18 KB, patch)
2023-04-14 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Prevent 400 on agreements?by_expired=true (1.90 KB, patch)
2023-04-14 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Make url consistent (3.06 KB, patch)
2023-04-14 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Improve max date handling (3.93 KB, patch)
2023-04-14 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Use computed values (4.89 KB, patch)
2023-04-14 12:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Rollback computed (4.15 KB, patch)
2023-04-21 12:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33490: Fix ambiguous query error (835 bytes, patch)
2023-04-21 12:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33490: Add test (1.24 KB, patch)
2023-04-21 12:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33490: Add test (1.23 KB, patch)
2023-04-21 14:47 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 33490: Unit tests (3.26 KB, patch)
2023-04-25 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Prevent 400 on agreements?by_expired=true (1.97 KB, patch)
2023-04-25 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Make url consistent (3.13 KB, patch)
2023-04-25 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Improve max date handling (4.01 KB, patch)
2023-04-25 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Use computed values (4.96 KB, patch)
2023-04-25 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Rollback computed (4.22 KB, patch)
2023-04-25 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Fix ambiguous query error (911 bytes, patch)
2023-04-25 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33490: Add test (1.30 KB, patch)
2023-04-25 12:47 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Pedro Amorim 2023-04-11 16:31:55 UTC
[ERROR] GET /api/v1/erm/agreements: unhandled exception (DBIx::Class::Exception)<<DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Column 'agreement_id' in where clause is ambiguous at /kohadevbox/koha/Koha/Objects.pm line 394
Comment 1 Jonathan Druart 2023-04-11 19:22:44 UTC
I don't recreate this one, can you provide more details please?
Comment 2 Pedro Amorim 2023-04-12 10:09:20 UTC
Created attachment 149519 [details]
/cgi-bin/koha/erm/agreements?by_expired=true
Comment 3 Jonathan Druart 2023-04-12 10:24:15 UTC
Created attachment 149521 [details] [review]
Bug 33490: Prevent 400 because agreements?by_expired=true is missing date

We need to update the filter THEN update the current route
Comment 4 Jonathan Druart 2023-04-12 10:30:08 UTC
Created attachment 149522 [details] [review]
Bug 33490: Prevent 400 on agreements?by_expired=true

We need to update the filter THEN update the current route
Comment 5 Pedro Amorim 2023-04-12 10:39:36 UTC
Created attachment 149523 [details] [review]
Bug 33490: Prevent 400 on agreements?by_expired=true

We need to update the filter THEN update the current route

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 6 Pedro Amorim 2023-04-12 10:41:27 UTC
Turns out the error I was experiencing was caused by an outdated schema on my end. However, in the process, Joubu discovered this error 400 and provided the fix.
The problem can be reproduced by visiting:
/cgi-bin/koha/erm/agreements?by_expired=true

Apply the patch and visit the URL again.
Comment 7 Martin Renvoize 2023-04-12 12:56:41 UTC
This works great.. but I think we need a cyrpess test to prevent a future regression here.. I'm going to look at using this one to learn how to write such a test :)
Comment 8 Martin Renvoize 2023-04-13 13:40:26 UTC
Created attachment 149603 [details] [review]
Bug 33490: Unit tests

This patch adds cypress tests for the filter by expired functionality in
the ERM Agreements page.
Comment 9 Martin Renvoize 2023-04-13 13:40:29 UTC
Created attachment 149604 [details] [review]
Bug 33490: Prevent 400 on agreements?by_expired=true

We need to update the filter THEN update the current route

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2023-04-13 13:47:58 UTC
OK, that was a nice distraction for a few hours and I feel like I've wrapped my brain around Cypress testing much better now.

I've added tests that I believe catch this particular issue, though I'd love a review if anyone fancies it.

* I test for the existence of the filter options when there are any agreements in the system.
* I then test for the default date of today being filled after filtering when the 'Filter by expired' checkbox is checked.
* This tests:
  a) that the xhr includes the correct max_expiration_date
  b) that the window url is updated to reflect the filtering
  c) the input box value is set and displayed
* We then reload the page using the url as defined in step b) above and test that the xhr and input box value get correctly filled again
* We then add a user selected date in the input box and check
  a) that the xhr includes the new correct max_expiration_date
  b) the window url is updated to reflect the filtering (including the new date)
  c) the input box value is set and displaying the new date too
* Finally, we reload the page again in b) above and confirm all the changes stuck.

I added a note that we should also really be testing the other filter too.. but I felt that was out of scope for this particular patch (and I was running low on tuits)
Comment 11 Martin Renvoize 2023-04-13 13:48:38 UTC
PS.. I'm happy to PQA on the bug as a whole if someone wants to just feedback/verify my cypress test addition here is sane.
Comment 12 Jonathan Druart 2023-04-13 14:30:22 UTC
I've noticed when reading the tests that something is not totally correct.

+        cy.url().should('include', "/cgi-bin/koha/erm/agreements?by_expired=true"); // Browser url also updated

It should contain max_expiration_date here.

The problem is the following:
Go to /cgi-bin/koha/erm/agreements
Tick "Filter by expired"
Click "Filter"

New route is "/cgi-bin/koha/erm/agreements?by_expired=true"
Input date is filled with today

Click "Filter" again
New route is "/cgi-bin/koha/erm/agreements?by_expired=true&max_expiration_date=2023-04-13"

The date should be in the URL directly.

Anybody who'd take a look at it?
Comment 13 Martin Renvoize 2023-04-13 17:13:47 UTC
Created attachment 149615 [details] [review]
Bug 33490: Make url consistent

This ensures we populate the max_expiration_date param on the browser
url for consistency with selected dates.
Comment 14 Martin Renvoize 2023-04-13 17:14:53 UTC
That last patch does the trick, but I don't like it.. we're repeating code with it.. but for the life of me I couldn't work out where to move it to get it to work for both cases.

Remaining Failed QA seeking more eyes.
Comment 15 Martin Renvoize 2023-04-14 07:25:37 UTC
Created attachment 149643 [details] [review]
Bug 33490: Make url consistent

This ensures we populate the max_expiration_date param on the browser
url for consistency with selected dates.
Comment 16 Martin Renvoize 2023-04-14 07:28:50 UTC
OK.. I'm happy with that last patch now.

However, it does mean that the old url of `/agreements?by_expired=true` leads to a datatables error as we no longer autofill table_url with a default max_expiration_date when it's not passed in the url.

The url you'll see in the address bar will include max_expiration_date however, so as long as someone isn't manually typing their url and leaving out the date we're OK.. Does this seem reasonable to you Jonathan.. or should we have belt and braces and autofill for both cases as my last patch did?
Comment 17 Jonathan Druart 2023-04-14 09:11:54 UTC
Two things do not work with this last patch:
1. As said by Martin already /agreements?by_expired=true will explode (400)
2. The date is not filled in with today's date when you click "filter", empty the date, click "filter" again.

New patch coming...
Comment 18 Jonathan Druart 2023-04-14 09:19:21 UTC
Created attachment 149652 [details] [review]
Bug 33490: Improve max date handling

It fixes:
1. /agreements?by_expired=true do no longer explode
2. Fill date with today's date when we setup the component and when we
hit "filter"

However there are a couple of things totally wrong:
1. We should not need is_fp_disabled, we could be able to use
  :disabled="!filters.by_expired"
in the template, but it does not work
2. We should not need to deal with filters.max_expiration_date the "old
JS way", but should use a Vue computed value instead. I've failed at
that

I think those 2 problems have the same root, the way we deal with
filters in data() is wrong, and we may be loosing the ref at some point.
Comment 19 Jonathan Druart 2023-04-14 09:21:47 UTC
This last patch (and the current code and difficulty we have in fixing this) highlight a problem with the way we handle the filters object. I am really not sure how to deal with it properly, but I think we should spend time on it now, or we are going to carry that and it makes the code too complicated.
Comment 20 Jonathan Druart 2023-04-14 09:23:41 UTC
Note that this last patch adds an new behaviour, the input is disabled when the checkbox is not ticked.
Comment 21 Jonathan Druart 2023-04-14 10:07:28 UTC
Created attachment 149661 [details] [review]
Bug 33490: Use computed values
Comment 22 Jonathan Druart 2023-04-14 10:09:22 UTC
I am almost there. I think the only problem is that the input is not filled in with today's date when we click "filter".

The last patch gets rid of the unnecessary is_fp_disabled and updateMaxExpirationDate() I was referring to in my previous comment.

I won't have more time for this today, please have a look!
Comment 23 Martin Renvoize 2023-04-14 10:42:31 UTC
Sorry Jonathan, I can't get those patches to apply.. sha1 issues :(.

That looks reasonable to me though.

This can wait for Monday and fresher brains though, so no hurry.
Comment 24 Jonathan Druart 2023-04-14 12:32:33 UTC
Created attachment 149672 [details] [review]
Bug 33490: Unit tests

This patch adds cypress tests for the filter by expired functionality in
the ERM Agreements page.
Comment 25 Jonathan Druart 2023-04-14 12:32:36 UTC
Created attachment 149673 [details] [review]
Bug 33490: Prevent 400 on agreements?by_expired=true

We need to update the filter THEN update the current route

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Jonathan Druart 2023-04-14 12:32:38 UTC
Created attachment 149674 [details] [review]
Bug 33490: Make url consistent

This ensures we populate the max_expiration_date param on the browser
url for consistency with selected dates.
Comment 27 Jonathan Druart 2023-04-14 12:32:41 UTC
Created attachment 149675 [details] [review]
Bug 33490: Improve max date handling

It fixes:
1. /agreements?by_expired=true do no longer explode
2. Fill date with today's date when we setup the component and when we
hit "filter"

However there are a couple of things totally wrong:
1. We should not need is_fp_disabled, we could be able to use
  :disabled="!filters.by_expired"
in the template, but it does not work
2. We should not need to deal with filters.max_expiration_date the "old
JS way", but should use a Vue computed value instead. I've failed at
that

I think those 2 problems have the same root, the way we deal with
filters in data() is wrong, and we may be loosing the ref at some point.
Comment 28 Jonathan Druart 2023-04-14 12:32:44 UTC
Created attachment 149676 [details] [review]
Bug 33490: Use computed values
Comment 29 Jonathan Druart 2023-04-14 12:34:32 UTC
Rebased on top of bug 33483.

But that's not enough IMO, there are some tiny bugs in the behaviours, see comment 22.
Comment 30 Pedro Amorim 2023-04-21 12:56:06 UTC
Created attachment 150038 [details] [review]
Bug 33490: Rollback computed

Use a watch instead
Comment 31 Pedro Amorim 2023-04-21 12:56:09 UTC
Created attachment 150039 [details] [review]
Bug 33490: Fix ambiguous query error

Doing actual testing with some agreements with expired periods, this was returning a 500 error
Comment 32 Pedro Amorim 2023-04-21 12:56:12 UTC
Created attachment 150040 [details] [review]
Bug 33490: Add test
Comment 33 Jonathan Druart 2023-04-21 13:03:37 UTC
Getting 400 when hitting /erm/agreements?by_expired=true
But I don't think it's blocker now.
Comment 34 Jonathan Druart 2023-04-21 13:10:54 UTC
Tests are using 'today_us' that was not used. That means that the tests will fail if dateformat is not the value from the sample data.

Not ideal but better to have a test.


+        cy.url().should('include', "/cgi-bin/koha/erm/agreements?by_expired=true"); // Browser url also updated

Should not we adjust this test?
Comment 35 Pedro Amorim 2023-04-21 14:47:16 UTC
Created attachment 150048 [details] [review]
Bug 33490: Add test
Comment 36 Pedro Amorim 2023-04-21 14:47:36 UTC
(In reply to Jonathan Druart from comment #34)
> Tests are using 'today_us' that was not used. That means that the tests will
> fail if dateformat is not the value from the sample data.
> 
> Not ideal but better to have a test.
> 
> 
> +        cy.url().should('include',
> "/cgi-bin/koha/erm/agreements?by_expired=true"); // Browser url also updated
> 
> Should not we adjust this test?

The flatpickr input in the UI is using today_us e.g. 04/20/2023
The flatpickr element attribute that stores the date is today_iso e.g. 2023-04-20
The URL dateformat is today_iso e.g. 2023-04-20

There was a slight difference in the selector ("+input") of my test. Adjusted now.
Comment 37 Jonathan Druart 2023-04-25 12:47:17 UTC
Created attachment 150194 [details] [review]
Bug 33490: Unit tests

This patch adds cypress tests for the filter by expired functionality in
the ERM Agreements page.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 38 Jonathan Druart 2023-04-25 12:47:20 UTC
Created attachment 150195 [details] [review]
Bug 33490: Prevent 400 on agreements?by_expired=true

We need to update the filter THEN update the current route

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 39 Jonathan Druart 2023-04-25 12:47:23 UTC
Created attachment 150196 [details] [review]
Bug 33490: Make url consistent

This ensures we populate the max_expiration_date param on the browser
url for consistency with selected dates.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 40 Jonathan Druart 2023-04-25 12:47:26 UTC
Created attachment 150197 [details] [review]
Bug 33490: Improve max date handling

It fixes:
1. /agreements?by_expired=true do no longer explode
2. Fill date with today's date when we setup the component and when we
hit "filter"

However there are a couple of things totally wrong:
1. We should not need is_fp_disabled, we could be able to use
  :disabled="!filters.by_expired"
in the template, but it does not work
2. We should not need to deal with filters.max_expiration_date the "old
JS way", but should use a Vue computed value instead. I've failed at
that

I think those 2 problems have the same root, the way we deal with
filters in data() is wrong, and we may be loosing the ref at some point.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 41 Jonathan Druart 2023-04-25 12:47:29 UTC
Created attachment 150198 [details] [review]
Bug 33490: Use computed values

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 42 Jonathan Druart 2023-04-25 12:47:32 UTC
Created attachment 150199 [details] [review]
Bug 33490: Rollback computed

Use a watch instead

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 43 Jonathan Druart 2023-04-25 12:47:35 UTC
Created attachment 150200 [details] [review]
Bug 33490: Fix ambiguous query error

Doing actual testing with some agreements with expired periods, this was returning a 500 error

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 44 Jonathan Druart 2023-04-25 12:47:38 UTC
Created attachment 150201 [details] [review]
Bug 33490: Add test

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 45 Jonathan Druart 2023-04-25 12:48:08 UTC
This is good to go now! Thanks!
Comment 46 Tomás Cohen Arazi 2023-05-05 12:17:34 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 47 Matt Blenkinsop 2023-05-15 14:12:53 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 48 Lucas Gass 2023-06-15 19:43:22 UTC
Missing dependencies for 22.05.x, no backport