Description
Pedro Amorim
2023-04-11 16:31:55 UTC
I don't recreate this one, can you provide more details please? Created attachment 149519 [details]
/cgi-bin/koha/erm/agreements?by_expired=true
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 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 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> 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. 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 :) 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. 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> 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) 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. 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? 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. 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. 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. 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? 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... 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. 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. Note that this last patch adds an new behaviour, the input is disabled when the checkbox is not ticked. Created attachment 149661 [details] [review] Bug 33490: Use computed values 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! 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. 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. 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> 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. 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. Created attachment 149676 [details] [review] Bug 33490: Use computed values Rebased on top of bug 33483. But that's not enough IMO, there are some tiny bugs in the behaviours, see comment 22. Created attachment 150038 [details] [review] Bug 33490: Rollback computed Use a watch instead 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 Created attachment 150040 [details] [review] Bug 33490: Add test Getting 400 when hitting /erm/agreements?by_expired=true But I don't think it's blocker now. 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? Created attachment 150048 [details] [review] Bug 33490: Add test (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. 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> 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> 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> 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> Created attachment 150198 [details] [review] Bug 33490: Use computed values Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 150199 [details] [review] Bug 33490: Rollback computed Use a watch instead Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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> Created attachment 150201 [details] [review] Bug 33490: Add test Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> This is good to go now! Thanks! Pushed to master for 23.05. Nice work everyone, thanks! Nice work everyone! Pushed to stable for 22.11.x Missing dependencies for 22.05.x, no backport |