Bug 7169 - Change booksellers search results display
Summary: Change booksellers search results display
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 3.8
Hardware: All All
: P3 enhancement (vote)
Assignee: Julian Maurice
QA Contact: Ian Walls
URL:
Keywords:
Depends on: 6836
Blocks: 5336 7163
  Show dependency treegraph
 
Reported: 2011-11-06 03:43 UTC by Julian Maurice
Modified: 2012-10-25 23:10 UTC (History)
6 users (show)

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


Attachments
booksellers.pl (45.30 KB, image/png)
2011-11-06 03:43 UTC, Julian Maurice
Details
Patch (15.79 KB, patch)
2012-01-13 11:18 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7169: Change booksellers search results display (15.81 KB, patch)
2012-02-24 16:00 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7169: Change booksellers search results display (15.88 KB, patch)
2012-02-26 10:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
screenshot showing changes (51.54 KB, image/png)
2012-02-26 10:10 UTC, Katrin Fischer
Details
Bug 7169: Follow-up: Change booksellers search results display (4.75 KB, patch)
2012-03-01 08:51 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 7169: Change booksellers search results display (15.57 KB, patch)
2012-03-19 15:08 UTC, Julian Maurice
Details | Diff | Splinter Review
Follow-up: Change booksellers search results display (4.98 KB, patch)
2012-03-19 15:08 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2011-11-06 03:43:03 UTC
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
Comment 1 Julian Maurice 2012-01-13 11:18:42 UTC Comment hidden (obsolete)
Comment 2 Julian Maurice 2012-01-13 11:19:31 UTC
This patch depends on bug 6836 (datatables)
Comment 3 Paul Poulain 2012-02-17 14:39:06 UTC
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
Comment 4 Julian Maurice 2012-02-24 16:00:17 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2012-02-26 09:46:37 UTC
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"
Comment 6 Katrin Fischer 2012-02-26 10:01:46 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2012-02-26 10:10:19 UTC
Created attachment 7886 [details]
screenshot showing changes
Comment 8 Paul Poulain 2012-02-27 15:58:59 UTC
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 ?
Comment 9 Katrin Fischer 2012-02-27 16:18:12 UTC
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.
Comment 10 Katrin Fischer 2012-02-27 16:20:20 UTC
Ah, I see I copied the warning into the bug. Not sure what went wrong there, I will retest.
Comment 11 Julian Maurice 2012-03-01 08:51:25 UTC Comment hidden (obsolete)
Comment 12 jmbroust 2012-03-19 11:17:46 UTC
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
Comment 13 Julian Maurice 2012-03-19 15:08:01 UTC
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?
Comment 14 Julian Maurice 2012-03-19 15:08:51 UTC
Created attachment 8295 [details] [review]
Follow-up: Change booksellers search results display

Rebased on master
Comment 15 Koha Team University Lyon 3 2012-03-19 15:53:41 UTC
Appearence OK. Count of items and biblio in the commande and items still expected OK.
Select box OK.
Comment 16 Paul Poulain 2012-03-21 17:00:50 UTC
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
Comment 17 Julian Maurice 2012-03-22 07:59:39 UTC
Ok in master, marked as resolved.