Created attachment 130608 [details] [review] Bug 30063: Number of checkouts
Created attachment 130609 [details] [review] Bug 30063: Overdues count
Created attachment 130610 [details] [review] Bug 30063: Make the main patron search use the REST API To test: * Length menu (PatronsPerPage) * Query description * Highlight of the current library * sticky header - Does not work (?) If the table does not show when you submit the filter form, make sure you regenerated the compiled CSS.
Created attachment 130611 [details] [review] Bug 30063: Remove svc/members/search
Created attachment 130612 [details] [review] Bug 30063: Remove occurrences of C4::Utils::DataTables::Members
Created attachment 130613 [details] [review] Bug 30063: Fix address format
Created attachment 130614 [details] [review] Bug 30063: Add kind of POD for the include file
Created attachment 130615 [details] [review] Bug 30063: Display search description for main search only
Please use the remote branch for testing, it contains all the tree.
Merge conflict on koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt Because of Bug 29136 pushed
In patrons search page /cgi-bin/koha/members/members-home.pl it removes : Browse by last name: A B C D ... And when searching I get no results table o_O I see the API call in network tought.
(In reply to Fridolin Somers from comment #10) > Merge conflict on koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt > Because of Bug 29136 pushed Remote branch rebased. (In reply to Fridolin Somers from comment #11) > In patrons search page /cgi-bin/koha/members/members-home.pl it removes : > Browse by last name: A B C D ... I am seeing it. > And when searching I get no results table o_O > I see the API call in network tought. See the commit message: "If the table does not show when you submit the filter form, make sure you regenerated the compiled CSS." You need to `yarn build css` and refresh your browser's cache.
Ahhh thanks, it works. Looks great :D Some feedbacks : 1) When arriving in patrons home page without searching : /cgi-bin/koha/members/members-home.pl We see the header with 'Select all' 'Clear all' ... This fells odd. We may hide it like in current master. 2) Why not keeping 'Browse by last name: A B ...' on top of header 'Select all' 'Clear all' ... ? 3) Perform a search with some text in left form. Click on a letter of 'Browse by last name'. Note that search text is not removed in left form like in current master. We may also clear form in page header, but keeping it is the current behavior.
(In reply to Fridolin Somers from comment #13) > Ahhh thanks, it works. > Looks great :D > > Some feedbacks : > > 1) > When arriving in patrons home page without searching : > /cgi-bin/koha/members/members-home.pl > We see the header with 'Select all' 'Clear all' ... > This fells odd. > We may hide it like in current master. I didn't manage to hide it. When it's hidden the "sticky" behaviour is broken (see patch "Fix sticky") > 2) > Why not keeping 'Browse by last name: A B ...' on top of header 'Select all' > 'Clear all' ... ? Because I had to rebuild a bit how the "blocks" are ordered. The header with actions (select/clear all, add to list, merge) is only available from the main patron search, when the "browse by" is available for all patron searches. It was easier to then in the "browse by" code into the include file. We can certainly change the order if there is a real need, but it will make the code a bit more complex then. > 3) > Perform a search with some text in left form. > Click on a letter of 'Browse by last name'. > Note that search text is not removed in left form like in current master. Don't you think this behaviour is better? > We may also clear form in page header, but keeping it is the current > behavior. When? I wouldn't modify another form, that would feel weird.
1) 2) OK, this is minor. 3) OK we will see if other feed-backs see it. Keeping input terms allows to rerun search, it can be a feature not a bug ;) I sign off.
(In reply to Fridolin Somers from comment #15) > 1) 2) > OK, this is minor. > > 3) > OK we will see if other feed-backs see it. > Keeping input terms allows to rerun search, it can be a feature not a bug ;) > > I sign off. Thanks Frido! Does it mean you signoff on the whole tree? This bug depends on several others that are not signed off yet :)
Yep looks strong enough to sign.
Ahhh the tree contains several bugs not yet signed, I revert
*** Bug 29125 has been marked as a duplicate of this bug. ***
*** Bug 26865 has been marked as a duplicate of this bug. ***
Created attachment 132387 [details] [review] Bug 30063: Fix table settings
patch doesn't apply on master, although all dependencies do apply properly :) <pre> 132387 - Bug 30063: Fix table settings Application de Bug 30063: Fix table settings Utilisation de l'information de l'index pour reconstruire un arbre de base... M koha-tmpl/intranet-tmpl/prog/en/modules/members/member.tt Retour à un patch de la base et fusion à 3 points... Fusion automatique de koha-tmpl/intranet-tmpl/prog/en/modules/members/member.tt CONFLIT (contenu) : Conflit de fusion dans koha-tmpl/intranet-tmpl/prog/en/modules/members/member.tt error: Échec d'intégration des modifications. l'application de la rustine a échoué à 0001 Bug 30063: Fix table settings astuce: Utilisez 'git am --show-current-patch=diff' pour visualiser le patch en échec When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-30063-Fix-table-settings-7voPZp.patch </pre>
This is a first feedback about the member-home.pl page only, tested using the branch available on Joubu's GitLab. 1) About Frido's point 3, I completely agree with the new behaviour that keeps the search performs in the form and then filters the results. As an example, at BULAC, we have plenty of homonyms specifically for Asiatic short names that can be used as firstname or family name and it can be really difficult sometimes to find someone. Having a new way to refine the search is really useful. 2) Sorting by 'Name' isn't logical for a user like me : the table display surname then firstname but sorting uses firstname then surname 3) Some special signs need to be escaped to be search, like % or _ Probably not related to this patch, but searching for % searches for not NULL datas. On 'Notes' filter column e.g., it bring some unexpected results displaying patrons without any circulation notes just because the SQL field is not NULL. 4) I had some strange behaviours using the column filters or general search : many times, the search only applied when I kind of 'refresh' the table by sorting it (see https://snipboard.io/6EGyV8.jpg) To reproduce : enter your search very quickly in the search box or filter box ; see that many results are not supposed to be here. 5) Hiding a column in table setting introduces a bug : the filter doesn't search the data in the selected column but the data of the column just on the left : see screenshots - https://snipboard.io/JAdjup.jpg : column 'Card' hidden, filter 'Date' search for the name - https://snipboard.io/1gb9Kq.jpg : column 'Card' hidden, filter 'Name' search for the card - https://snipboard.io/RKB4ip.jpg : column 'Card' hidden, filter 'Checkouts' search for the expiration date - https://snipboard.io/DBX5Ve.jpg : column 'Name' hidden, filter 'Date' search for the name You can also notice on these screenshots that there is some wrong with filter for it's not supposed to exists for the 'Checkout' and 'Actions' columns, and there is no more filter for 'Notes'. 6) It's not possible to got back to the form search if the results bring a unique result : the form reloaded automatically and bring us back to the patron account. This is not the behaviour in master and I feel uncomfortable with that change. 7) On side search form, 'Search type' = 'Starts with' doesn't work for none of the 'Search field' I tried (cardnumber, address, firstname, surname). 8) The patch doesn't work for users with only 'edit_borrowers' or 'view_borrower_infos_from_any_libraries' permissions : the full 'borrowers' permission seems to be needed to display results All the other points (Length menu, query description, highlight of the current library) are ok expect for length menu displaying two times '20 results' but I'll fill another bug as it's already existing in master and 20.11.11). I'll feedback about the other tables in the dedicated bugs.
Created attachment 132552 [details] [review] Bug 30063: Sort by "Name" firstname,surname As it's how it's displayed
(In reply to Séverine Queune from comment #23) > This is a first feedback about the member-home.pl page only, tested using > the branch available on Joubu's GitLab. > > 1) About Frido's point 3, I completely agree with the new behaviour that > keeps the search performs in the form and then filters the results. As an > example, at BULAC, we have plenty of homonyms specifically for Asiatic short > names that can be used as firstname or family name and it can be really > difficult sometimes to find someone. Having a new way to refine the search > is really useful. Thanks for the feedback! > 2) Sorting by 'Name' isn't logical for a user like me : the table display > surname then firstname but sorting uses firstname then surname Fixed! Bug 30063: Sort by "Name" firstname,surname > 3) Some special signs need to be escaped to be search, like % or _ > Probably not related to this patch, but searching for % searches for not > NULL datas. On 'Notes' filter column e.g., it bring some unexpected results > displaying patrons without any circulation notes just because the SQL field > is not NULL. Bug 30393. > 4) I had some strange behaviours using the column filters or general search > : many times, the search only applied when I kind of 'refresh' the table by > sorting it (see https://snipboard.io/6EGyV8.jpg) To reproduce : enter your > search very quickly in the search box or filter box ; see that many results > are not supposed to be here. Bug 30394. > 5) Hiding a column in table setting introduces a bug : the filter doesn't > search the data in the selected column but the data of the column just on > the left : see screenshots > - https://snipboard.io/JAdjup.jpg : column 'Card' hidden, filter 'Date' > search for the name > - https://snipboard.io/1gb9Kq.jpg : column 'Card' hidden, filter 'Name' > search for the card > - https://snipboard.io/RKB4ip.jpg : column 'Card' hidden, filter > 'Checkouts' search for the expiration date > - https://snipboard.io/DBX5Ve.jpg : column 'Name' hidden, filter 'Date' > search for the name > You can also notice on these screenshots that there is some wrong with > filter for it's not supposed to exists for the 'Checkout' and 'Actions' > columns, and there is no more filter for 'Notes'. Opened bug 30406. > 6) It's not possible to got back to the form search if the results bring a > unique result : the form reloaded automatically and bring us back to the > patron account. This is not the behaviour in master and I feel uncomfortable > with that change. Not sure it's fixed, I am getting an error on Firefox but it's working on Chromium. Bug 30063: Restore back behaviour > 7) On side search form, 'Search type' = 'Starts with' doesn't work for none > of the 'Search field' I tried (cardnumber, address, firstname, surname). Fixed! Bug 30063: Fix start with > 8) The patch doesn't work for users with only 'edit_borrowers' or > 'view_borrower_infos_from_any_libraries' permissions : the full 'borrowers' > permission seems to be needed to display results Fixed! Bug 30063: Fix permission for GET /patrons > All the other points (Length menu, query description, highlight of the > current library) are ok expect for length menu displaying two times '20 > results' but I'll fill another bug as it's already existing in master and > 20.11.11). > > I'll feedback about the other tables in the dedicated bugs. Thanks a lot!
Thanks a lot Jonathan ! I fail the patch once again as the sorting by surname then firstname doesn't work. I've got the same problem on point 6 : it works with Chromium but not Firefox. Weird... All the other points related to this specific bugs are fixed :)
(In reply to Jonathan Druart from comment #25) > (In reply to Séverine Queune from comment #23) > > 2) Sorting by 'Name' isn't logical for a user like me : the table display > > surname then firstname but sorting uses firstname then surname > > Fixed! > Bug 30063: Sort by "Name" firstname,surname Replaced by Bug 30063: Sort by "Name" surname:firstname
(In reply to Séverine Queune from comment #26) > Thanks a lot Jonathan ! > > I fail the patch once again as the sorting by surname then firstname doesn't > work. > > I've got the same problem on point 6 : it works with Chromium but not > Firefox. Weird... > > All the other points related to this specific bugs are fixed :) Should be fixed now!
Tested using Joubu's branch, everything seems fixed !
Created attachment 132731 [details] [review] Bug 30063: Number of checkouts Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132732 [details] [review] Bug 30063: Overdues count Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132733 [details] [review] Bug 30063: Make the main patron search use the REST API To test: * Length menu (PatronsPerPage) * Query description * Highlight of the current library * sticky header - Does not work (?) If the table does not show when you submit the filter form, make sure you regenerated the compiled CSS. Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132734 [details] [review] Bug 30063: Remove svc/members/search Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132735 [details] [review] Bug 30063: Remove occurrences of C4::Utils::DataTables::Members Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132736 [details] [review] Bug 30063: Fix address format Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132737 [details] [review] Bug 30063: Add kind of POD for the include file Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132738 [details] [review] Bug 30063: Display search description for main search only Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132739 [details] [review] Bug 30063: Fix selenium tests Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132740 [details] [review] Bug 30063: Fix sticky Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132741 [details] [review] Bug 30063: Add selenium tests Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132742 [details] [review] Bug 30063: Be more flexible for filters On later follow-ups (ERM) we need to filter columns that contain AVs, and so be more flexible. Here we are expecting a _id and _str keys we are gonna use to build the select's options Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132743 [details] [review] Bug 30063: Fix table settings Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132744 [details] [review] Bug 30063: Sort by "Name" firstname,surname As it's how it's displayed Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132745 [details] [review] Bug 30063: Restore back behaviour Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132746 [details] [review] Bug 30063: Fix start with It was doing "end with"... Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132747 [details] [review] Bug 30063: Fix permission for GET /patrons edit_borrowers should be enough, we don't need the whole borrowers module flag Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132748 [details] [review] Bug 30063: Sort by "Name" surname:firstname Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
Created attachment 132749 [details] [review] Bug 30063: Fix QA issues Signed-off-by: Séverine Queune <severine.queune@bulac.fr>
I've spoken to Severine and she tells me she's already highlighted one issue but we both agree it can be handled as a distinct bug.. I'll report it and add screenshots of before and after to highlight the issue Otherwise, this is great again.
Created attachment 132759 [details] [review] Bug 30063: Number of checkouts Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132760 [details] [review] Bug 30063: Overdues count Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132761 [details] [review] Bug 30063: Make the main patron search use the REST API To test: * Length menu (PatronsPerPage) * Query description * Highlight of the current library * sticky header - Does not work (?) If the table does not show when you submit the filter form, make sure you regenerated the compiled CSS. Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132762 [details] [review] Bug 30063: Remove svc/members/search Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132763 [details] [review] Bug 30063: Remove occurrences of C4::Utils::DataTables::Members Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132764 [details] [review] Bug 30063: Fix address format Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132765 [details] [review] Bug 30063: Add kind of POD for the include file Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132766 [details] [review] Bug 30063: Display search description for main search only Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132767 [details] [review] Bug 30063: Fix selenium tests Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132768 [details] [review] Bug 30063: Fix sticky Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132769 [details] [review] Bug 30063: Add selenium tests Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132770 [details] [review] Bug 30063: Be more flexible for filters On later follow-ups (ERM) we need to filter columns that contain AVs, and so be more flexible. Here we are expecting a _id and _str keys we are gonna use to build the select's options Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132771 [details] [review] Bug 30063: Fix table settings Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132772 [details] [review] Bug 30063: Sort by "Name" firstname,surname As it's how it's displayed Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132773 [details] [review] Bug 30063: Restore back behaviour Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132774 [details] [review] Bug 30063: Fix start with It was doing "end with"... Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132775 [details] [review] Bug 30063: Fix permission for GET /patrons edit_borrowers should be enough, we don't need the whole borrowers module flag Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132776 [details] [review] Bug 30063: Sort by "Name" surname:firstname Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 132777 [details] [review] Bug 30063: Fix QA issues Signed-off-by: Séverine Queune <severine.queune@bulac.fr> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
OK, I can't find any hard regressions.. minor UX changes and tweeks yes, but not real breakage. This gives us some great foundations to continue improving upon.. I'd like to see things like the 'overdues' alias for 'get_overdues' actually progress to just replace the 'get_overdues' method instead.. and I'd love to see that same method turned into a relationship at the dbic level to benefit from dbic query handling. But.. generally I'm more than happy to PQA on this... I'll add bugs to record my other thougths.
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Thanks to everybody involved in this huge patch series!
Created attachment 132958 [details] [review] Bug 30063: (follow-up) Fix missing TT filters
> Bug 30063: (follow-up) Fix missing TT filters Was also missing : $("#[% sticky_header | html %]").show(); I've added my commit : c636b39a89 Bug 30063: Fix missing TT filters Then yours : 5fdfed1ee5 Bug 30063: (follow-up) Fix missing TT filters
patrons_search.t selenium test is failing and I'm not entirely sure why :(
(In reply to Martin Renvoize from comment #74) > patrons_search.t selenium test is failing and I'm not entirely sure why :( Fixed on bug 30626.