The patron search from the top bar has broken recently. Search for Henry in ktd from the top bar, autocomplete works, but hitting return returns zero results (and doesn't redirect to the Henry user either any more)
Confirmed.
Created attachment 163096 [details] [review] Bug 36302: Remove defer_loading logic Test plan, ktd: 1) Search "henry" on the top search bar, notice 1 result is auto suggested 2) Ignore the auto-complete, press 'Enter' instead 3) Notice no results are returned. 4) Apply patch. Repeat.
This is a PoC highlighting and fixing the issue, but I'm unsure what that defer_loading code was doing int he first but was introduced by bug 30063 patch "Restore back behaviour". @Joubu are you able to provide feedback here? Basically I believe my patch is fixing the issue but may be reintroducing past issues.
I believe this defer_loading check was added here to have the "back" browser behavior work in the event that a search only returned 1 result and the cgi->redirect to that result kicks in (as opposed to showing the search results with just 1 entry). Is this cgi->redirect (in members/member.pl) still needed now that we have auto-complete suggestions?
Created attachment 163099 [details] [review] Bug 36302: Remove defer_loading logic Test plan, ktd: 1) Search "henry" on the top search bar, notice 1 result is auto suggested 2) Ignore the auto-complete, press 'Enter' instead 3) Notice no results are returned. 4) Apply patch. Repeat. Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
(In reply to Pedro Amorim from comment #4) > I believe this defer_loading check was added here to have the "back" browser > behavior work in the event that a search only returned 1 result and the > cgi->redirect to that result kicks in (as opposed to showing the search > results with just 1 entry). > Is this cgi->redirect (in members/member.pl) still needed now that we have > auto-complete suggestions? I believe we did have the auto-suggest before this was added, it's a bit of a different use case. Having the back button work would be preferrable, but maybe we can fix the immediate issue first and further improve again in another bug.
Not sure if I'm missing something, but with this patch applied the back button still works how I would expect it to. If I search from the main page for Henry, it takes me directly to that patron, and the back button takes me back to the main page. If I search for Bob it takes me to the results page with two entries. Then if I search for Henry it takes me to the patron record and back takes me back to the results page.
defer_loading is coming from commit f6394501f2ae6468cef8279ba8e296427cb69188 Bug 30063: Restore back behaviour See 30063 comment 23 (point 6). To recreate: 1. search 'henry' 2. the table loads, only one result 3. redirected to the patron detail view 4. go back 5. the table loads, only one result 6. redirected to the patron detail view 5 should not display the page. It's the behaviour on master, which is broken by this patch. The regression has been introduced by commit f3bd0e7ebddac73552dd175b39d458855d7d663e Bug 34478: Manual fix - change to get remove csrf token - member-search-box where we changed from POST to GET and so we do no longer set the variable: members/member.pl:72 my $defer_loading = $input->request_method() eq "GET" && !$circsearch ? 1 : 0;
defer_loading is coming from commit f6394501f2ae6468cef8279ba8e296427cb69188 Bug 30063: Restore back behaviour See 30063 comment 23 (point 6). To recreate: 1. search 'henry' 2. the table loads, only one result 3. redirected to the patron detail view 4. go back 5. the table loads, only one result 6. redirected to the patron detail view 5 should not display the table result. It's the behaviour on master, which is broken by this patch. The regression has been introduced by commit f3bd0e7ebddac73552dd175b39d458855d7d663e Bug 34478: Manual fix - change to get remove csrf token - member-search-box where we changed from POST to GET and so we do no longer set the variable: members/member.pl:72 my $defer_loading = $input->request_method() eq "GET" && !$circsearch ? 1 : 0;
Another regression/behaviour change is that hitting http://localhost:8081/cgi-bin/koha/members/members-home.pl will load the table and display the X first patrons.
Having discussed this with Jonathan, the regression he mentions on https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36302#c9 is only reproduce able with Firefox, not Chrome.
Created attachment 163105 [details] [review] Bug 36302: Use replace instead of href This will change URL but not fiddle with browser history. Works the same (and as expected) in both Chrome and Firefox
Created attachment 163106 [details] [review] Bug 36302: Remove unused code Please can someone tell me why we need this code? Inititally I thought this is where the redirect was coming from, but it's coming from JS, not here, so why do we need this?
(In reply to Pedro Amorim from comment #13) > Created attachment 163106 [details] [review] [review] > Bug 36302: Remove unused code > > Please can someone tell me why we need this code? > Inititally I thought this is where the redirect was coming from, but it's > coming from JS, not here, so why do we need this? We need it. If you search for 42 you want the patron with cardnumber to be displayed, not the table with all the occurrences of "42".
(In reply to Pedro Amorim from comment #12) > Created attachment 163105 [details] [review] [review] > Bug 36302: Use replace instead of href > > This will change URL but not fiddle with browser history. > Works the same (and as expected) in both Chrome and Firefox This is not fixing comment 10.
(In reply to Pedro Amorim from comment #13) > Created attachment 163106 [details] [review] [review] [review] > Bug 36302: Remove unused code > > Please can someone tell me why we need this code? > Inititally I thought this is where the redirect was coming from, but it's > coming from JS, not here, so why do we need this? We need it. If you search for 42 you want the patron with cardnumber=42 to be displayed, not the table with all the occurrences of "42".
Created attachment 163108 [details] [review] Bug 36302: Get rid of defer_loading Update JS submit of patron_search_form to check for URLParams instead of defer_loading. This is to keep behavior of not doing a search when accessing http://localhost:8081/cgi-bin/koha/members/members-home.pl directly
Created attachment 163109 [details] [review] Bug 36302: use replace instead of href This will change URL but not fiddle with browser history. Works the same (and as expected) in both Chrome and Firefox Test plan, ktd: 1) Search henry on the top search bar, notice 1 result is auto suggested 2) Ignore the auto-complete, press 'Enter' instead 3) Notice no results are returned. 4) Apply patch. Repeat. 5) Notice you get redirected to the single result of "henry" 6) Press the browser 'back' button and notice it works as expected (both Chrome+Firefox) 7) Visit patrons home directly: http://localhost:8081/cgi-bin/koha/members/members-home.pl 8) Notice no search is performed
(In reply to Brendan Lawlor from comment #7) > Not sure if I'm missing something, but with this patch applied the back > button still works how I would expect it to. > > If I search from the main page for Henry, it takes me directly to that > patron, and the back button takes me back to the main page. > > If I search for Bob it takes me to the results page with two entries. Then > if I search for Henry it takes me to the patron record and back takes me > back to the results page. Thank you Brendan for your initial sign-off. I've since followed up while discussing with Jonathan's in case you want to take a look again,
Still not happy with it, under Firefox: * Go to mainpage * Click "Patrons" in the header => You are on http://localhost:8081/cgi-bin/koha/members/members-home.pl * "Search for patrons": Search: "henry" (form on the left) => Redirected to the patron detail page * back => You are now on the mainpage! We expect the patron search page.
Created attachment 163113 [details] [review] Bug 36302: Revert back to POST Pass the csrf token and 'cud-noop' After having discussed with Joubu and multiple failed attempts at fixing the issue AND restoring previous behavior, this is the fix we agreed on. This will change URL but not fiddle with browser history. Works the same (and as expected) in both Chrome and Firefox Test plan, ktd: 1) Search henry on the top search bar, notice 1 result is auto suggested 2) Ignore the auto-complete, press 'Enter' instead 3) Notice no results are returned. 4) Apply patch. Repeat. 5) Notice you get redirected to the single result of henry 6) Press the browser 'back' button and notice it works as expected (both Chrome+Firefox) 7) Visit patrons home directly: http://localhost:8081/cgi-bin/koha/members/members-home.pl 8) Notice no search is performed
Created attachment 163121 [details] [review] Bug 36302: Revert back to POST Pass the csrf token and 'cud-noop' After having discussed with Joubu and multiple failed attempts at fixing the issue AND restoring previous behavior, this is the fix we agreed on. This will change URL but not fiddle with browser history. Works the same (and as expected) in both Chrome and Firefox Test plan, ktd: 1) Search henry on the top search bar, notice 1 result is auto suggested 2) Ignore the auto-complete, press 'Enter' instead 3) Notice no results are returned. 4) Apply patch. Repeat. 5) Notice you get redirected to the single result of henry 6) Press the browser 'back' button and notice it works as expected (both Chrome+Firefox) 7) Visit patrons home directly: http://localhost:8081/cgi-bin/koha/members/members-home.pl 8) Notice no search is performed Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 163122 [details] [review] Bug 36302: Revert back to POST Pass the csrf token and 'cud-noop' After having discussed with Joubu and multiple failed attempts at fixing the issue AND restoring previous behavior, this is the fix we agreed on. This will change URL but not fiddle with browser history. Works the same (and as expected) in both Chrome and Firefox Test plan, ktd: 1) Search henry on the top search bar, notice 1 result is auto suggested 2) Ignore the auto-complete, press 'Enter' instead 3) Notice no results are returned. 4) Apply patch. Repeat. 5) Notice you get redirected to the single result of henry 6) Press the browser 'back' button and notice it works as expected (both Chrome+Firefox) 7) Visit patrons home directly: http://localhost:8081/cgi-bin/koha/members/members-home.pl 8) Notice no search is performed Signed-off-by: David Nind <david@davidnind.com>
I am sorry but... I am still getting the redirect to the patron detail page after hitting the back button. BUT I have tried v23.11.00 and it behaving like that as well. Sure I tried yesterday a pre-csrf and it works "working" (no redirect). David, what did you notice during your testing session?
(In reply to Jonathan Druart from comment #24) > I am sorry but... I am still getting the redirect to the patron detail page > after hitting the back button. > > BUT I have tried v23.11.00 and it behaving like that as well. > > Sure I tried yesterday a pre-csrf and it works "working" (no redirect). > > David, what did you notice during your testing session? I tested this a bit today, there are slightly different results when searching from mainpage vs members-home - the former using patron-search-box the latter member-search-box. Some of this is covered by discussion, just highlighting it all together From mainpage the url is: http://localhost:8081/cgi-bin/koha/members/member.pl?quicksearch=1&circsearch=1&searchmember=henry From members-home the url is: http://localhost:8081/cgi-bin/koha/members/member.pl?quicksearch=1&searchmember=henry&searchfieldstype=standard&searchtype=starts_with&branchcode_filter=&categorycode_filter= circsearch makes the difference in that without that, the table is blank, this seems to be from defer_loading which prevents the form submission Chrome and Firefox behave differently because it seems Chrome/Edge is designed to prevent a redirect when clicking back - Firefox does not do this When the patch changes from GET to POST this effects the defer_loading variable so really seems to be effectively the same as the initial patch. My only thought has been to store the initial page or pass it through as a variable, and offer a "Back to {page}" link on the patron screen
Ha good catch! There is another form to adjust!
(In reply to Jonathan Druart from comment #26) > Ha good catch! There is another form to adjust! Absolutely not!
Nick, do you know what is the behaviour in stable branches?
(In reply to Jonathan Druart from comment #28) > Nick, do you know what is the behaviour in stable branches? On a 23.05 site In firefox I get redirected to patron if I hit back In chrome I get the member search with no results when I hit back after searching for a patron from the members module If I perform a checkout search from anywhere else, back takes me to the original page, i.e. Serials home
From the test plan: > 6) Press the browser 'back' button and notice it works as expected (both Chrome+Firefox) What is the expected behaviour ? I get "this page has expired" error
I don't agree with this behavior, i.e. redirecting to the single search result if only one is returned: 1) Maybe this is the norm? But I have personally not seen other search systems with this behavior. 2) We're not consistent: Some tables in Koha (most?) return a single result if only one is found, not redirect happens. 3) It's artificial, unexpected and a pain to maintain, as we've proven here. Having said that, we've gone to great lengths in making sure this historic behavior is kept, and it appears we've reached a consensus that, in Firefox, when clicking the "back" button from the patron detail page, being redirected to it again is what was happening before the CSRF work, meaning this patch fixes the search issue introduced by CSRF and keeps the original behaviour. (In reply to Julian Maurice from comment #30) > From the test plan: > > 6) Press the browser 'back' button and notice it works as expected (both Chrome+Firefox) > What is the expected behaviour ? I get "this page has expired" error When I first tested my patch I was sure it had the same behavior on both browsers, but having tested it again after Joubu's comment, it appears it does not. The current expected behavior: Chrome: You get redirected to whatever page you started the header patron search from. Firefox: You get redirected to patrons-home and immediately redirected to the single patron detail page afterwards, regardless of where you started the header patron search form.
Created attachment 163207 [details] [review] Bug 36302: [ALTERNATIVE] Get rid of defer_loading Same test plan as the other patch, behavior is expected to be the same on both But this alternative removes technical debt, whereas the other patch adds.
The noop approach is/was hacky and it doesn't seem to fix the issue entirely anyway. I'm submitting my first approach again as an alternative patch and let others decide which route is better. There may of course be a better approach at fixing this that I've not been able to come up with, open to input as always.
(In reply to Pedro Amorim from comment #31) > I don't agree with this behavior, i.e. redirecting to the single search > result if only one is returned: > 1) Maybe this is the norm? But I have personally not seen other search > systems with this behavior. I think this is/was about saving clicks for the circulation desk mostly. If you scan a card, they want to go directly to the patron account. I am not sure if it's also applied for other searches/results in general. But I believe it's always been the case for cardnumber searches (since 3.2 at least, when I began using Koha). I believe if you picked an entry, using the auto-complete, you'd also want/expect this behavior. (Not sure those comments fully apply here, just trying to give a user point of view) > 2) We're not consistent: Some tables in Koha (most?) return a single result > if only one is found, not redirect happens. > 3) It's artificial, unexpected and a pain to maintain, as we've proven here. > > Having said that, we've gone to great lengths in making sure this historic > behavior is kept, and it appears we've reached a consensus that, in Firefox, > when clicking the "back" button from the patron detail page, being > redirected to it again is what was happening before the CSRF work, meaning > this patch fixes the search issue introduced by CSRF and keeps the original > behaviour.
(In reply to Katrin Fischer from comment #34) > (In reply to Pedro Amorim from comment #31) > > I don't agree with this behavior, i.e. redirecting to the single search > > result if only one is returned: > > 1) Maybe this is the norm? But I have personally not seen other search > > systems with this behavior. > > I think this is/was about saving clicks for the circulation desk mostly. If > you scan a card, they want to go directly to the patron account. Cardnumber searches are still handled directly with a redirect in the script The case here is a standard search that returns a single patron
(In reply to Nick Clemens from comment #35) > (In reply to Katrin Fischer from comment #34) > > (In reply to Pedro Amorim from comment #31) > > > I don't agree with this behavior, i.e. redirecting to the single search > > > result if only one is returned: > > > 1) Maybe this is the norm? But I have personally not seen other search > > > systems with this behavior. > > > > I think this is/was about saving clicks for the circulation desk mostly. If > > you scan a card, they want to go directly to the patron account. > > Cardnumber searches are still handled directly with a redirect in the script > > The case here is a standard search that returns a single patron Hi Nick, thanks for clarifying! For another search I'd day it's not that important (but no guarantee everyone agrees with me) :)
Created attachment 163226 [details] [review] Bug 36302: [ALTERNATIVE] Get rid of defer_loading Same test plan as the other patch, behavior is expected to be the same on both But this alternative removes technical debt, whereas the other patch adds. Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Katrin Fischer from comment #34) > I think this is/was about saving clicks for the circulation desk mostly. If > you scan a card, they want to go directly to the patron account. I am not > sure if it's also applied for other searches/results in general. But I > believe it's always been the case for cardnumber searches (since 3.2 at > least, when I began using Koha). > > I believe if you picked an entry, using the auto-complete, you'd also > want/expect this behavior. > > (Not sure those comments fully apply here, just trying to give a user point > of view) The following use case: "If you scan a card, they want to go directly to the patron account." This is already handled in member.pl. i.e. this is already ensured, regardless of the patches in this bug or the issue described by it. This functionality is not broken and is still in place. This: "I believe if you picked an entry, using the auto-complete, you'd also want/expect this behavior." I agree, and this is also already the case as well. Again, regardless of the patches in this bug or the issue described by it. When you click on any of the suggestions given by the auto-complete, you're forwarded directly to the patron you clicked on. The behavior that I don't agree with: "Search -> Browser goes to search results page -> JavaScript checks if only 1 result found -> Browser redirects user to this single result detail page" Doesn't happen in either of the use-cases above. A search is a search, if you submit a search then you should be presented with a search results page, regardless of how many results are found. Having a special case for: if a cardnumber is submitted, which is a unique identifier, Koha forwards you directly to the patron detail page and it does NOT first visit the search results page -> This is fine If you click on an auto-complete suggestion, Koha forwards you to the patron detail page and it does NOT first visit the search results page -> This is fine (and expected).
I've tested the ALTERNATIVE patch alone and I get a weird JS error in the console: Uncaught SyntaxError: unexpected token: '!' http://localhost:8081/cgi-bin/koha/members/members-home.pl
(In reply to Jonathan Druart from comment #39) > I've tested the ALTERNATIVE patch alone and I get a weird JS error in the > console: > > Uncaught SyntaxError: unexpected token: '!' > > http://localhost:8081/cgi-bin/koha/members/members-home.pl I've retried the ALTERNATIVE patch alone again on current master on both Chrome and Firefox, I'm unable to reproduce this.
(In reply to Pedro Amorim from comment #40) > (In reply to Jonathan Druart from comment #39) > > I've tested the ALTERNATIVE patch alone and I get a weird JS error in the > > console: > > > > Uncaught SyntaxError: unexpected token: '!' > > > > http://localhost:8081/cgi-bin/koha/members/members-home.pl > > I've retried the ALTERNATIVE patch alone again on current master on both > Chrome and Firefox, I'm unable to reproduce this. Hum, retried and the error disappeared after a reset_all + restart of Firefox. Weird because I had the error with the patch applied and not on master. Forget that for now, I will keep an eye on it in case it pops up again.
Created attachment 163487 [details] [review] Bug 36302: Get rid of defer_loading in patron-search Same test plan as the other patch, behavior is expected to be the same on both But this alternative removes technical debt, whereas the other patch adds. Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Not convinced this is the best we can do but at least we are fixing the bad bug. We will see for the side-effects later.
Pushed for 24.05! Well done everyone, thank you!
Is this needed for backport ? I applyed on 23.11.x and the behavior is : 1) Enter "henry" on the top search bar 2) Press 'Enter' => There is "henry" in left form but the form is not submitted anymore!
It's a fix for a bug caused by the csrf work.. so I don't believe it's required for your branch fridolin.
OK thanks No backport
Regression: patron list is loaded when placing a hold: http://localhost:8081/cgi-bin/koha/reserve/request.pl?biblionumber=9
Created attachment 163917 [details] [review] Bug 36302: Reinstate defer_loading defer_loading is now a patron_search_js BLOCK param - default on
Created attachment 163918 [details] [review] Bug 36302: members-home.pl defers loading, member.pl does not
Can we have QA here please? :)
I am wondering if loading the table is expected when coming from this link: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt 233 <a class="btn btn-link" href="/cgi-bin/koha/members/member.pl" class="toggler save_entryform"> We will fix later if needed.
Created attachment 163974 [details] [review] bug 36302: Restore passing deferLoading to DT
You have not put back defer_loading to DT's options, was it expected? If we don't the GET is done (ie. we will fetch the first 20 patrons).
+ let defer_loading = Number( '[% defer_loading | html %]' || 1 ); This is kind of working but it reads really bad IMO. Number ('0' || 1) => 0 Number (0 || 1) => 1 Javascript...
(In reply to Jonathan Druart from comment #54) > You have not put back defer_loading to DT's options, was it expected? If we > don't the GET is done (ie. we will fetch the first 20 patrons). Thanks Jonathan, I left it out because I couldn't tell what it was doing (saw no difference with or without) and according to docs is deprecated: https://datatables.net/reference/option/deferLoading It was intentional, but I don't feel strongly about it.
(In reply to Pedro Amorim from comment #56) > (In reply to Jonathan Druart from comment #54) > > You have not put back defer_loading to DT's options, was it expected? If we > > don't the GET is done (ie. we will fetch the first 20 patrons). > > Thanks Jonathan, I left it out because I couldn't tell what it was doing > (saw no difference with or without) and according to docs is deprecated: > https://datatables.net/reference/option/deferLoading > > It was intentional, but I don't feel strongly about it. Either we add it back or we need to workaround again... You decide :D You should avoid the unnecessary GET. Especially because you will see the full table before the actual fitlering (it's blinking: table shows with full results then it's updated with the results of the user's request).
Created attachment 164002 [details] [review] bug 36302: Restore passing deferLoading to DT Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
I couldn't see the blinking but there was definitely a GET happening in the background. I think this should be good to go as is?
Created attachment 164003 [details] [review] Bug 36302: Reinstate defer_loading defer_loading is now a patron_search_js BLOCK param - default on Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164004 [details] [review] Bug 36302: members-home.pl defers loading, member.pl does not Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 164005 [details] [review] bug 36302: Restore passing deferLoading to DT Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Follow-up "Restore passing deferLoading to DT" pushed to master.
(In reply to Katrin Fischer from comment #64) > Pushed for 24.05! > > Well done everyone, thank you! I don't think all the patches were pushed. Only bug 36302: Restore passing deferLoading to DT.
Note to myself: push second and third patch too!
Missing patches pushed to master!
I'm currently facing problem with patron search in current master. There is an error in browsers console > Uncaught ReferenceError: defer_loading is not defined It seems last patch here is actually missing the variable defer_loading.
(In reply to Emmi Takkinen from comment #68) > I'm currently facing problem with patron search in current master. There is > an error in browsers console > > Uncaught ReferenceError: defer_loading is not defined > > It seems last patch here is actually missing the variable defer_loading. I do not see that. How to recreate the error?
(In reply to Emmi Takkinen from comment #68) > I'm currently facing problem with patron search in current master. There is > an error in browsers console > > Uncaught ReferenceError: defer_loading is not defined > > It seems last patch here is actually missing the variable defer_loading. defer_loading was reinstated by patch "Reinstate defer_loading". Current master now contains all 4 patches, but for a while it only contained the first and last patch here (out of 4 total). Can you `git pull master` and try again? I've tested and cannot reproduce this.
(In reply to Jonathan Druart from comment #69) > (In reply to Emmi Takkinen from comment #68) > > I'm currently facing problem with patron search in current master. There is > > an error in browsers console > > > Uncaught ReferenceError: defer_loading is not defined > > > > It seems last patch here is actually missing the variable defer_loading. > > I do not see that. How to recreate the error? I updated my master just now again and error is gone and patron search is working. The problem seems to be that my master (updated it this morning) didn't have all patches from this patch (?) so defer_loading was missing. So all good here, sorry!
Hi Frido, why add the keyword, when this is already pushed?
(In reply to Katrin Fischer from comment #72) > Hi Frido, why add the keyword, when this is already pushed? Sorry it was to remember no backport to 23.11.x ;)
Looks resolved