Summary: | Change booksellers search results display | ||
---|---|---|---|
Product: | Koha | Reporter: | Julian Maurice <julian.maurice> |
Component: | Acquisitions | Assignee: | Julian Maurice <julian.maurice> |
Status: | CLOSED FIXED | QA Contact: | Ian Walls <koha.sekjal> |
Severity: | enhancement | ||
Priority: | P3 | CC: | claire.hernandez, jean-manuel.broust, jonathan.druart, koha, paul.poulain |
Version: | 3.8 | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | Sponsored | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Circulation function: | |||
Bug Depends on: | 6836 | ||
Bug Blocks: | 5336, 7163 | ||
Attachments: |
booksellers.pl
Patch Bug 7169: Change booksellers search results display [SIGNED-OFF] Bug 7169: Change booksellers search results display screenshot showing changes Bug 7169: Follow-up: Change booksellers search results display Bug 7169: Change booksellers search results display Follow-up: Change booksellers search results display |
Created attachment 7134 [details] [review] Patch Bug 7169: Change booksellers search results display - Replace vendor table by a div (avoid table in a table) - Add two columns in basket tables: biblio count and expected (not received) items) - Replace tablesorter by datatables - Add a list of all returned bookseller names in top of the page (easier to find a bookseller in a large list of results) The script devel/misc/testbugzillapatches.pl has detected that this bug does not apply anymore, so i'm marking this bug as "does not apply". The script is supposed to handle conflicts related to updatedatabase, but no other cases of merge conflict. So the conflict may be trivial to fix. If it happens that the patch attached does apply cleanly, please send me an email, there's something wrong in the script. It's possible, as it's a recent script. If the patch applies does not apply as I think, please rebase and resubmit, thanks Created attachment 7858 [details] [review] Bug 7169: Change booksellers search results display Patch rebased on master Hi Julian, I tested the changes and I think the display is cleaner this way. I especially like the buttons for adding baskets and receiving shipments. The single entries make take up a bit more space though. I think it would be nice to get some more opinions on the display. Some notes: I am wondering a bit about the list of vendors on top, perhaps this could go into a filter box on the left side instead? Perhaps we could also make the columns on the result page match the descriptions on the order receive page (/acqui/parcels.pl?booksellerid=3). Item Count Biblio count Items expected For functionality I only found one small thing: I noticed there is a warning in the logs when searching booksellers. Perhaps you could fix this in a follow-up? [Sun Feb 26 10:24:22 2012] [error] [client 127.0.0.1] [Sun Feb 26 10:24:22 2012] booksellers.pl: Argument "user" isn't numeric in numeric eq (==) at /home/katrin/kohaclone/acqui/booksellers.pl line 121., referer: http://localhost:8080/cgi-bin/koha/acqui/booksellers.pl There is also an error when checking for XHTML validity: Line 304, Column 151: cannot generate system identifier for general entity "op" Created attachment 7885 [details] [review] [SIGNED-OFF] Bug 7169: Change booksellers search results display - Replace vendor table by a div (avoid table in a table) - Add two columns in basket tables: biblio count and expected (not received) items) - Replace tablesorter by datatables - Add a list of all returned bookseller names in top of the page (easier to find a bookseller in a large list of results) Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Created attachment 7886 [details]
screenshot showing changes
QA comment: * remove some SQL code in .sql, that's good ! * switch from tableSorter to datatable, that's good * cleanly add a sub in C4/Acquisition * perlcritic is OK Marking failed QA because small/trivial follow-up suggested by katrin would be cool: (In reply to comment #5) > make take up a bit more space though. I think it would be nice to get some more > opinions on the display. Usually the library enter at least a part of the supplier, so it's OK to have a wider list. The gain for having non-nested <table> is worth it ! > Some notes: > I am wondering a bit about the list of vendors on top, perhaps this could go > into a filter box on the left side instead? Good idea. And another option would be to have a select instead of <a href>. The librarian want to find just 1 vendor usually. What is interesting with the idea is that all booksellers are on the top of the page. With dozens of suppliers, the list will be long. With a select, it will solve this problem. (select with auto-submit, no need to select THEN click OK) julian a small follow-up possible ? > Perhaps we could also make the columns on the result page match the > descriptions on the order receive page (/acqui/parcels.pl?booksellerid=3). > Item Count Biblio count Items expected good catch, will also easy translatability ! Julian, follow-up required > For functionality I only found one small thing: I noticed there is a warning in > the logs when searching booksellers. Perhaps you could fix this in a follow-up? I couldn't reproduce it. Where you testing with "mysql" user or any specific thing ? > There is also an error when checking for XHTML validity: > Line 304, Column 151: cannot generate system identifier for general entity "op" Katrin, how did you get this ? Hi Paul and Julian, List of vendors: I really like the idea of a select box over the long list of links! XHTML: I used http://validator.w3.org/ direct input option. Warning in logs: I was logged in as a normal user. I think it was a warning for an unitialized variable. I can try to reproduce later. I often use an 'empty' search to get all vendors in the system. Perhaps this makes a difference in testing. Ah, I see I copied the warning into the bug. Not sure what went wrong there, I will retest. Created attachment 7956 [details] [review] Bug 7169: Follow-up: Change booksellers search results display - List of booksellers changed to a drop-down list - Baskets columns renamed and reordered to make them consistent with acqui/parcels.pl - warning in log removed - changed a '&' to '&' to pass W3 validator Applying: Bug 7169: Change booksellers search results display Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging acqui/booksellers.pl CONFLICT (content): Merge conflict in acqui/booksellers.pl Auto-merging koha-tmpl/intranet-tmpl/prog/en/css/staff-global.css Failed to merge in the changes. Patch failed at 0001 Bug 7169: Change booksellers search results display Created attachment 8294 [details] [review] Bug 7169: Change booksellers search results display Rebased on master since it does not apply anymore, but already signed off by Katrin, but the "Signed-off-by:" is not in this commit message. How can we avoid to ask for a new sign-off in this case? Created attachment 8295 [details] [review] Follow-up: Change booksellers search results display Rebased on master Appearence OK. Count of items and biblio in the commande and items still expected OK. Select box OK. QA comment: * removes a sub from a .pl, which is good (for plack compliance) * better display * perlcritic OK * valid_template.t OK (there is a warning on (xt/author/valid-templates.t .. 414/? Argument "" isn't numeric in numeric lt (<) at /home/paul/koha.dev/koha-community/koha-tmpl/intranet-tmpl/prog/en/modules/serials/subscription-detail.tt line 226. that is not related to this patch passed QA Ok in master, marked as resolved. |
Created attachment 6231 [details] booksellers.pl Change appearance of booksellers and add more basket informations. It will look like in the screenshot attached. BibLibre infos: Repository: git://git.biblibre.com/koha_biblibre.git Branch: stetiennes/ft/MT5504