Created attachment 25115 [details] [review] Bug 11708: Add aqbasketgroups.closeddate
Created attachment 25116 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate
Created attachment 25117 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables
Patch tested with a sandbox, by juliette levast <juliette.levast@iepg.fr>
Created attachment 26003 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr>
Created attachment 26004 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr>
Created attachment 26006 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr>
Looking at this now, mostly small things: 1) Change wording from "Basket grouping" to "Basket groups" to be more consistent with other pages in Koha and also match the navigation link. 2) When using the link in the navigation, you end up with the heading: Basket grouping for (because no vendor was selected) 3) I am missing the vendor in the basket group table, I think the new view for all baskets would make more sense with it. 4) Use "Amount spent" instead of "Amount charged" - also to match other pages 5) Add correct bug number in database update It's only small things, I think only 3) is a bit bigger - everything else looks ok so far.
Ah, something else. 6) Show "basket groups" link only if the user has "group_manage" permission.
I did not look at your code, but from the description, some features (adding closeddate) reminds me a patch I wrote some time ago, and which needs sign off: Bug 11438 If your patch is pushed first, I suppose mine will need a rebase. I mark it in "see also" Mathieu
I was not sure which term would be better, so I asked: (13:01:32) cait: i was wondering what's better: amount spent oder spent amount (13:02:17) ashimema: british english... 'amount spent' sounds better to me
As I wrote, I made a patch adding the same feature (bug 11438). It does not apply, and I prefer not to rewrite it, but to wait for Jonathan's patches to be pushed. I put here the differences between my patch and Jonathan ones. Maybe it can be useful for QA, maybe not... Functionaly : My patch creates a field for closedate, but also an other for creationdate of the bg. I think it can be useful, but can be added in a 2d time after bug 11708 is pushed In the templates, the 2 dates are displayed in - in lists of opened/closed basketgroups - in an individual basketgroup. Internaly: I named the new fields "bg_closedate" and "bg_creationdate", to be sure there won't be any conflict with fields present in other tables with ambiguous names like "closedate" or "creationdate" I removed "closed" field, because I don't think it is needed anymore And finally, updatedatabase.pl creates the 2 columns and removes closed column, but also populates the 2 new columns: - bg_creationdate with the most recent closing date of the baskets included in each basketgroup - bg_closingdate with NULL is the bg is open, or with the most recent closing date of the baskets included in each basketgroup Mathieu
Oups, I made Jonathan the author of this patch, while it is Julian. Sorry for that.
Julian, Please add this change: MT12443: explicitly remove pagination The use of columnfilter seems to keep the pagination even if the sDom is 't'. We want to remove the pagination here in order to show all data in the table. To reproduce: Add 10+ closed basketgroup and verify all are displayed. Without this patch, only the 10 first are displayed. diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt index 2d5715a..b3ec49d 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt @@ -140,6 +140,7 @@ function submitForm(form) { { "aTargets": [ -1 ], "bSortable": false, "bSearchable": false }, ], sDom: 't', + "bPaginate": false, "bAutoWidth": false } )) .columnFilter(column_filter_options);
Created attachment 28868 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Rebased on master
Created attachment 28869 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Rebased on master
Created attachment 28870 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers Rebased on master
Created attachment 28871 [details] [review] Bug 11708: explicitly remove pagination The use of columnfilter seems to keep the pagination even if the sDom is 't'. We want to remove the pagination here in order to show all data in the table. To reproduce: Add 10+ closed basketgroup and verify all are displayed. Without this patch, only the 10 first are displayed.
Created attachment 28872 [details] [review] Bug 11708: All basketgroups on one page - several fixes 1/ Change wording from "Basket grouping" to "Basket groups" 2/ Display vendor name in front of "Baskets group for" 3/ Change wording from "Charged amount" to "Amount spent" 4/ Change wording from "Ordered amount" to "Amount ordered" 5/ Add bug number in updatedatabase.pl 6/ Show "Basket groups" link in menu only if user have group_manage permission.
(In reply to Katrin Fischer from comment #8) > 3) I am missing the vendor in the basket group table, I think the new view > for all baskets would make more sense with it. I didn't added the vendor in tables because it is already displayed before each pair of tables ("Basket groups for <vendor>")
(In reply to Julian Maurice from comment #20) > (In reply to Katrin Fischer from comment #8) > > 3) I am missing the vendor in the basket group table, I think the new view > > for all baskets would make more sense with it. > > I didn't added the vendor in tables because it is already displayed before > each pair of tables ("Basket groups for <vendor>") Hi Julian, not sure, maybe I thought it would be one big table. Looking at the notes I think I had done a search without selecting any vendor: >2) When using the link in the navigation, you end up with the heading: > Basket grouping for (because no vendor was selected)
I've tried to apply the patch against master 3.17.00.011 Applying: Bug 11708: Add aqbasketgroups.closeddate Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging installer/data/mysql/updatedatabase.pl CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Auto-merging C4/Acquisition.pm Failed to merge in the changes. Patch failed at 0001 Bug 11708: Add aqbasketgroups.closeddate When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". So I pass the patch to "Patch doesn't apply" status.
Created attachment 31305 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Rebased on master
Created attachment 31306 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Rebased on master
Created attachment 31307 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers Rebased on master
Created attachment 31308 [details] [review] Bug 11708: explicitly remove pagination Rebased on master
Created attachment 31309 [details] [review] Bug 11708: All basketgroups on one page - several fixes Rebased on master
I tried to test this patch with the sandbox system, it does not work, here is the result & paste what is written: 'Some problems occurred applying patches from bug 11708: <h1>Something went wrong !</h1>Applying: Bug 11708: Add aqbasketgroups.closeddate Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Warning: commit message does not conform to UTF-8. You may want to amend it after fixing the message, or set the config variable i18n.commitencoding to the encoding your project uses. Applying: Bug 11708: Unit tests for aqbasketgroups.closeddate Warning: commit message does not conform to UTF-8. You may want to amend it after fixing the message, or set the config variable i18n.commitencoding to the encoding your project uses. Applying: Bug 11708: basketgroup.pl can now display basketgroups for all suppliers Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Renaming intranet-tmpl/prog/en/css/datatables.css => koha-tmpl/opac-tmpl/prog/en/css/datatables.css Auto-merging koha-tmpl/opac-tmpl/prog/en/css/datatables.css CONFLICT (rename/modify): Merge conflict in koha-tmpl/opac-tmpl/prog/en/css/datatables.css Failed to merge in the changes. Patch failed at 0001 Bug 11708: basketgroup.pl can now display basketgroups for all suppliers 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. Bug 11708 - Display all basketgroups on one page, and new column aqbasketgroups.closeddate 31305 - Bug 11708: Add aqbasketgroups.closeddate 31306 - Bug 11708: Unit tests for aqbasketgroups.closeddate 31307 - Bug 11708: basketgroup.pl can now display basketgroups for all suppliers 31308 - Bug 11708: explicitly remove pagination 31309 - Bug 11708: All basketgroups on one page - several fixes Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-11708-basketgrouppl-can-now-display-basketgrou-rdAn9M.patch .'
Created attachment 31530 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Rebased on master
Created attachment 31531 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Rebased on master
Created attachment 31532 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers Rebased on master
Created attachment 31533 [details] [review] Bug 11708: explicitly remove pagination Rebased on master
Created attachment 31534 [details] [review] Bug 11708: All basketgroups on one page - several fixes Rebased on master
Created attachment 31535 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Rebased on master
Created attachment 31536 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Rebased on master
Created attachment 31537 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers Rebased on master
Created attachment 31538 [details] [review] Bug 11708: explicitly remove pagination Rebased on master
Created attachment 31539 [details] [review] Bug 11708: All basketgroups on one page - several fixes Rebased on master
Created attachment 31540 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Rebased on master
Created attachment 31541 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Rebased on master
Created attachment 31542 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers Rebased on master
Created attachment 31543 [details] [review] Bug 11708: explicitly remove pagination Rebased on master
Created attachment 31544 [details] [review] Bug 11708: All basketgroups on one page - several fixes Rebased on master
Ok, took me a while to figure out why these patches didn't apply well on sandbox. I was using "diff.noprefix=true" git option, thinking it was just a cosmetic option, but it also generate patches that don't apply well with "git am". The last patches should apply correctly everywhere now ;)
Against master 3.17.00.023 I've applied the patch with git am (OK). basketgroup = bg I received an order, for a closed basket linked to closed bg. On the bg page, the "Amount ordered" of the bg remained the same, instead of decreasing of the total of the received order. The meaning of "Amount ordered" being "left on order", I think this is an error. [Otherwise, I beg your pardon and pray to turn back the status to "Needs signoff".] So I pass the patch to "Failed QA" status.
Created attachment 31702 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr>
Created attachment 31703 [details] [review] Bug 11708: explicitly remove pagination The use of columnfilter seems to keep the pagination even if the sDom is 't'. We want to remove the pagination here in order to show all data in the table. To reproduce: Add 10+ closed basketgroup and verify all are displayed. Without this patch, only the 10 first are displayed.
Created attachment 31704 [details] [review] Bug 11708: All basketgroups on one page - several fixes 1/ Change wording from "Basket grouping" to "Basket groups" 2/ Display vendor name in front of "Baskets group for" 3/ Change wording from "Charged amount" to "Amount spent" 4/ Change wording from "Ordered amount" to "Amount ordered" 5/ Add bug number in updatedatabase.pl 6/ Show "Basket groups" link in menu only if user have group_manage permission.
Created attachment 31705 [details] [review] Bug 11708: Remove the amount columns This should be managed in a specific bug.
(In reply to Paola Rossi from comment #45) Paola, thanks for testing. Please forget these 2 columns, I removed them in the last patch. The calculation is wrong and should be managed after the vat rewrite.
I've applied the patch against 3.17.00.025 The patches can be applied with the following note. NB. The 5-th patch: All basketgroups on one page - several fixes changes updatadatabase.pl So, upgrading the database has to be done after having applied the 5-th patch. Otherwise, if database is upgraded soon after the 1-st patch, the 6-th patch can't be applied. That is the error: Applying: Bug 11708: Remove the amount columns Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt Failed to merge in the changes. ------------------------------------------------------------------------------ So I pass the patch to "Signed Off" status. ------------------------------------------------------------------------------ Having no test plan, I've made some tests on basket groups and written the following notes. The first notes esplicitly relate to the new all bgs' page. IMO the 7-th and 8-th notes come straightforward using the basket groups. I invite to consider the 9-th note to define a better use (I can't suggest) for that option. In the case described in 10-th the name of the basket group is empty in the db, so perhaps the bug can be described better. I beg your pardon if some of the following notes/errors are out of this patch. In any case I hope the following notes can help in QA. basketgroup = bg 1) On the page of all bgs, after having selected "Delete" action on a bg, the next page is not the current one; the next page is about only one vendor [he is the one whose bg I've asked to delete] and it is the bgs' page of the vendor selected on the closed bgs' tag (ERROR). I think that the next page should be on the open bgs, at least. The closed bgs are completely different from deleted bgs. Moveover no closed bg can be deleted. So the current next page after deleting bgs can be misleading, and is unfriendly. 2) Displaying all bgs on one page, the "Delete" action is shown for all the open bgs. But only bgs without baskets inside are really deleted. So, the "Delete" action should not be shown [, or the delete should be done,] for open bgs with baskets inside. 3) On the page of all bgs, after having selected "Delete" action on a bg with baskets inside, now no message was shown if the bg is not deleted (see above, 2)). 4) : starting from a single vendor's bg, the behaviours described in 1), 2) and 3) are exactly the same. I beg your pardon if this point 4) is certainly out of this patch, but the problem is the same. 5) : the 2) and 3) problems occurred also deleting from the tag "xDelete basket group" [shown at the top of a just reopened bg, "Edit"ing an open bg, ...]. 6) On the all bgs page, I had a closed bg whose Billing place and Delivery place fields were none ("--"). I selected the "View" action of the bg: on the contrary the two fields were shown filled [with the current user's library]. This error occurred also: : 6b) on open bgs page, using "Edit" action in the all bgs page 6c) from the basket page, selecting the link to the linked closed bg 6d) editing a closed bg, using the "Reopen this bg" tag at the top of the page. Perhaps the case of a closed bg with Billing place and Delivery place fields set to none might be prevented by koha at all. 7) On the all baskets page, you can consider if the bg closeddate' column might be added. On the all baskets page, the "Bg" column is at the left of the column of the "Closed" date (which is the basket's close date). No basket can be attached to a bg before it (the basket) has been closed, so exchanging the two columns would be consistent with the sequence of events. If added, the bg's closeddate column would be the last column on the right before the actions' one. 8) On ACQ, I select a vendor. Under the right permission, in the "New" tag the Basketgroup option might be added to the current ones: Basket, Contract and Vendor. 9) baskets can be homonymous; bgs can be homonymous too. Closing an open basket, the option "Attach this basket to a new basket group with the same name" can easely create homonymous bgs, adding new closed ones to already existent open and closed ones. 10) For a vendor I created a new bg without filling the Basket group name. Then I selected a closed basket: but, using "Change bg" option, I saw an empty row added to the drop down list of the attachable bgs. ------------------------------------------------------------------------------
Created attachment 31748 [details] [review] Bug 11708: Add aqbasketgroups.closeddate I've applied the patch against 3.17.00.025 The patches can be applied with the following note. NB. The 5-th patch: All basketgroups on one page - several fixes changes updatadatabase.pl So, upgrading the database has to be done after having applied the 5-th patch. Otherwise, if database is upgraded soon after the 1-st patch, the 6-th patch can't be applied. That is the error: Applying: Bug 11708: Remove the amount columns Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt Failed to merge in the changes. ------------------------------------------------------------------------------ So I pass the patch to "Signed Off" status. ------------------------------------------------------------------------------ Having no test plan, I've made some tests on basket groups and written the following notes. The first notes esplicitly relate to the new all bgs' page. IMO the 7-th and 8-th notes come straightforward using the basket groups. I invite to consider the 9-th note to define a better use (I can't suggest) for that option. In the case described in 10-th the name of the basket group is empty in the db, so perhaps the bug can be described better. I beg your pardon if some of the following notes/errors are out of this patch. In any case I hope the following notes can help in QA. basketgroup = bg 1) On the page of all bgs, after having selected "Delete" action on a bg, the next page is not the current one; the next page is about only one vendor [he is the one whose bg I've asked to delete] and it is the bgs' page of the vendor selected on the closed bgs' tag (ERROR). I think that the next page should be on the open bgs, at least. The closed bgs are completely different from deleted bgs. Moveover no closed bg can be deleted. So the current next page after deleting bgs can be misleading, and is unfriendly. 2) Displaying all bgs on one page, the "Delete" action is shown for all the open bgs. But only bgs without baskets inside are really deleted. So, the "Delete" action should not be shown [, or the delete should be done,] for open bgs with baskets inside. 3) On the page of all bgs, after having selected "Delete" action on a bg with baskets inside, now no message was shown if the bg is not deleted (see above, 2)). 4) : starting from a single vendor's bg, the behaviours described in 1), 2) and 3) are exactly the same. I beg your pardon if this point 4) is certainly out of this patch, but the problem is the same. 5) : the 2) and 3) problems occurred also deleting from the tag "xDelete basket group" [shown at the top of a just reopened bg, "Edit"ing an open bg, ...]. 6) On the all bgs page, I had a closed bg whose Billing place and Delivery place fields were none ("--"). I selected the "View" action of the bg: on the contrary the two fields were shown filled [with the current user's library]. This error occurred also: : 6b) on open bgs page, using "Edit" action in the all bgs page 6c) from the basket page, selecting the link to the linked closed bg 6d) editing a closed bg, using the "Reopen this bg" tag at the top of the page. Perhaps the case of a closed bg with Billing place and Delivery place fields set to none might be prevented by koha at all. 7) On the all baskets page, you can consider if the bg closeddate' column might be added. On the all baskets page, the "Bg" column is at the left of the column of the "Closed" date (which is the basket's close date). No basket can be attached to a bg before it (the basket) has been closed, so exchanging the two columns would be consistent with the sequence of events. If added, the bg's closeddate column would be the last column on the right before the actions' one. 8) On ACQ, I select a vendor. Under the right permission, in the "New" tag the Basketgroup option might be added to the current ones: Basket, Contract and Vendor. 9) baskets can be homonymous; bgs can be homonymous too. Closing an open basket, the option "Attach this basket to a new basket group with the same name" can easely create homonymous bgs, adding new closed ones to already existent open and closed ones. 10) For a vendor I created a new bg without filling the Basket group name. Then I selected a closed basket: but, using "Change bg" option, I saw an empty row added to the drop down list of the attachable bgs. ------------------------------------------------------------------------------
Created attachment 31749 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate
Created attachment 31750 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers
Created attachment 31751 [details] [review] Bug 11708: explicitly remove pagination
Created attachment 31752 [details] [review] Bug 11708: All basketgroups on one page - several fixes
Created attachment 31753 [details] [review] Bug 11708: Remove the amount columns
Hi Julian, could you please rebase? I have a conflict in one of the tt files that I am not sure about how to fix.
Created attachment 32909 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Rebased on master
Created attachment 32910 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Rebased on master
Created attachment 32911 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers Rebased on master
Created attachment 32912 [details] [review] Bug 11708: explicitly remove pagination Rebased on master
Created attachment 32913 [details] [review] Bug 11708: All basketgroups on one page - several fixes Rebased on master
Created attachment 32914 [details] [review] Bug 11708: Remove the amount columns Rebased on master
I am sorry Julian, I am having trouble with this... AGAIN. Patch 3 had a conflict, that I thought I fixed pretty well, but now patch 5 gives me trouble: Applying: Bug 11708: All basketgroups on one page - several fixes fatal: sha1 information is lacking or useless (installer/data/mysql/updatedatabase.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 11708: All basketgroups on one page - several fixes The copy of the patch that failed is found in: /home/katrin/kohaclone/.git/rebase-apply/patch 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". Patch left in /tmp/Bug-11708-All-basketgroups-on-one-page---several-f-dTc5Ih.patch I am sorry :(
Created attachment 33308 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Rebased on master
Created attachment 33309 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Rebased on master
Created attachment 33310 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers Rebased on master
Created attachment 33311 [details] [review] Bug 11708: explicitly remove pagination Rebased on master
Created attachment 33312 [details] [review] Bug 11708: All basketgroups on one page - several fixes Rebased on master
Created attachment 33313 [details] [review] Bug 11708: Remove the amount columns Rebased on master
There were a couple of conflicts about [% yuipath %] being removed in favor of [% interface %]. Oddly, I didn't had the same problem as you Katrin about the 5th patch
Working on this next.
Turns out I missed some of the conflicts when trying to resolve them - it's related to the push of the new Booksellers module from Jonathan. :( Julian, could you please rebase?
Created attachment 34936 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr>
Created attachment 34937 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr>
Created attachment 34938 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr>
Created attachment 34939 [details] [review] Bug 11708: explicitly remove pagination The use of columnfilter seems to keep the pagination even if the sDom is 't'. We want to remove the pagination here in order to show all data in the table. To reproduce: Add 10+ closed basketgroup and verify all are displayed. Without this patch, only the 10 first are displayed.
Created attachment 34940 [details] [review] Bug 11708: All basketgroups on one page - several fixes 1/ Change wording from "Basket grouping" to "Basket groups" 2/ Display vendor name in front of "Baskets group for" 3/ Change wording from "Charged amount" to "Amount spent" 4/ Change wording from "Ordered amount" to "Amount ordered" 5/ Add bug number in updatedatabase.pl 6/ Show "Basket groups" link in menu only if user have group_manage permission.
Created attachment 34941 [details] [review] Bug 11708: Remove the amount columns This should be managed in a specific bug. Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Conflicts with bug 12896 fixed.
Thx!
When trying to access the basket group page from the aquisition start page: Software error: Undefined subroutine &main::GetBookSeller called at /home/katrin/kohaclone/acqui/basketgroup.pl line 411.
Created attachment 34977 [details] [review] Bug 11708: Fix bad resolution conflict with bug 12896
Having a look at this one now..
QA Comment: Passes qa tools. Verified Basketgroups.t. This test does not contain any testnames in the calls to ok, is, etc. It is encouraged to do so. Could you please add? Question: Do we still need the column closed, if you add a closeddate in aqbasketgroups? Do we create redundancy? And what about confusion like: Hey closed=0 but there is a closeddate? Please convince me that we should have both columns. [PATCH 6/7] Bug 11708: Remove the amount columns Commit message says only: This should be managed in a specific bug. Please explain. Before continuing this QA session, I would appreciate feedback on the points listed. Thanks!
Patch doesn't apply currently. Please also take a loot at Marcel's questions in comment#86.
Created attachment 35748 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Rebased on master
Created attachment 35749 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Rebased on master
Created attachment 35750 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers Rebased on master
Created attachment 35751 [details] [review] Bug 11708: explicitly remove pagination Rebased on master
Created attachment 35752 [details] [review] Bug 11708: All basketgroups on one page - several fixes Rebased on master
Created attachment 35753 [details] [review] Bug 11708: Remove the amount columns Rebased on master
Created attachment 35754 [details] [review] Bug 11708: Fix bad resolution conflict with bug 12896 Rebased on master
Created attachment 35755 [details] [review] Bug 11708: Add descriptions in unit tests
Created attachment 35756 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate
(In reply to M. de Rooy from comment #86) > QA Comment: > > Passes qa tools. > > Verified Basketgroups.t. > This test does not contain any testnames in the calls to ok, is, etc. It is > encouraged to do so. Could you please add? Added by attachment 35755 [details] [review] > > Question: Do we still need the column closed, if you add a closeddate in > aqbasketgroups? > Do we create redundancy? And what about confusion like: Hey closed=0 but > there is a closeddate? > Please convince me that we should have both columns. We probably don't need both. Removed in attachment 35756 [details] [review] > > [PATCH 6/7] Bug 11708: Remove the amount columns > Commit message says only: This should be managed in a specific bug. > Please explain. The amount columns show wrong amounts (comment 45) so they are removed until a specific bug addresses this issue. If you think it's better to keep them, we can remove this patch I think.
Created attachment 35971 [details] [review] Bug 11708: Add aqbasketgroups.closeddate I've applied the 9 patches against master 3.19.00.011 head 7741 . The first patch: Add aqbasketgroups.closeddate needed a trivial rebase. Just as required, this patch : 1) on ACQ start page, basketgroup.pl displayed basketgroups for all suppliers 2) added these columns in basketgroups' table: -- No. of ordered titles -- No. of received titles -- closed date in closed basketgroups tables. 3) Added filters to basketgroup tables columns. 4) Shew "Basket groups" link only if the user has "group_manage" permission. 5) Removed basketgroups pagination (20+ tested). 6) Instead of "Basket grouping" used "Basket groups" . 7) Displayed vendor name in front of "Basket groups for" . 8) Added bug number in updatedatabase.pl. 9) Verified Basketgroups.t. 10) From the ACQ start page, the Baskets groups link could be run. The upgrading of DB set the current date as the closed date to the closed existent basket groups. So I pass this patch to "Signed Off" status. IMO in the 2 tags "Open" and "Closed" the numbers of the related basket groups could be added: "Open (X)" "Closed (X)" increasing the plainness of the resulting list.
Created attachment 35972 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate
Created attachment 35973 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers
Created attachment 35974 [details] [review] Bug 11708: explicitly remove pagination
Created attachment 35975 [details] [review] Bug 11708: All basketgroups on one page - several fixes
Created attachment 35976 [details] [review] Bug 11708: Remove the amount columns
Created attachment 35977 [details] [review] Bug 11708: Fix bad resolution conflict with bug 12896
Created attachment 35978 [details] [review] Bug 11708: Add descriptions in unit tests
Created attachment 35979 [details] [review] Bug 11708: Remove column aqbasketgroups.closed
koha-pl is OK too. >>koha-qa.pl -c 9 -v 2 testing 9 commit(s) (applied to 6b12857 'Bug 7741 [Follow-up] Clear search ter') Processing files before patches |========================>| 6 / 6 (100.00%) Processing files after patches |========================>| 6 / 6 (100.00%) OK C4/Acquisition.pm OK critic OK forbidden patterns OK pod OK valid OK installer/data/mysql/updatedatabase.pl OK critic OK forbidden patterns OK pod OK valid OK t/db_dependent/Acquisition/Basketgroups.t OK critic OK forbidden patterns OK pod OK valid OK acqui/basketgroup.pl OK critic OK forbidden patterns OK pod OK valid OK koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt OK forbidden patterns OK tt_valid OK valid_template OK koha-tmpl/intranet-tmpl/prog/en/includes/acquisitions-menu.inc OK forbidden patterns OK tt_valid OK valid_template
Some first findings from the code review: 1) Database update removes closed column, but it's still in kohastructure.sql. 2) I wonder if Basket groups should go first in the side navigation, as in the workflow the basket group happens before the orders are late or invoices can be done. 3) Why no paging on the basket groups datatable? 4) The database update statement is missing the version XXX - so it cannot be run from commandline. Mostly small things, please fix.
Created attachment 40260 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: Use atomic update
Created attachment 40261 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 40262 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 40263 [details] [review] Bug 11708: explicitly remove pagination The use of columnfilter seems to keep the pagination even if the sDom is 't'. We want to remove the pagination here in order to show all data in the table. To reproduce: Add 10+ closed basketgroup and verify all are displayed. Without this patch, only the 10 first are displayed. Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 40264 [details] [review] Bug 11708: All basketgroups on one page - several fixes 1/ Change wording from "Basket grouping" to "Basket groups" 2/ Display vendor name in front of "Baskets group for" 3/ Change wording from "Charged amount" to "Amount spent" 4/ Change wording from "Ordered amount" to "Amount ordered" 5/ Add bug number in updatedatabase.pl 6/ Show "Basket groups" link in menu only if user have group_manage permission. Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 40265 [details] [review] Bug 11708: Remove the amount columns This should be managed in a specific bug. Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 40266 [details] [review] Bug 11708: Fix bad resolution conflict with bug 12896 Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 40267 [details] [review] Bug 11708: Add descriptions in unit tests Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 40268 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: use atmoic update
Patches rebased and updated with use of atomic updates
Created attachment 40272 [details] [review] [Follow-up] Bug 11708 - Fix QA failures: - Update kohastructure.sql for removing of aqbasketgroups.closed column, - Place "Basket groups" menu entry on top of the side navigation.
(In reply to Katrin Fischer from comment #108) > Some first findings from the code review: Rebased/updated patch and last folow-up fix some of them: > 1) Database update removes closed column, but it's still in > kohastructure.sql. Fixed in followup > > 2) I wonder if Basket groups should go first in the side navigation, as in > the workflow the basket group happens before the orders are late or invoices > can be done. Done in the followup > > 3) Why no paging on the basket groups datatable? Jonathan tells it "conflict" with using columnfilter. I didn't manage to reproduce this. So i wonder if anyone already has this problem. > > 4) The database update statement is missing the version XXX - so it cannot > be run from commandline. I updated the patches to replace statement in updatedatabase with atomic updates. > > Mostly small things, please fix.
Could you rebase please?
Created attachment 48992 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: Use atomic update
Created attachment 48993 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 48994 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 48995 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: use atomic update
Created attachment 48996 [details] [review] Bug 11708: Place "Basket groups" menu entry on top of the side navigation
Patches squashed and rebased on master
Created attachment 52417 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: Use atomic update
Created attachment 52418 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 52419 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 52420 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: use atomic update
Created attachment 52421 [details] [review] Bug 11708: Place "Basket groups" menu entry on top of the side navigation
Patches rebased on master
1/ There are still occurrences of 'aqbasketgroup.closed' in tests. 2/ Use is/isnt instead of ok 3/ Use Koha::DateUtils instead of sprintf in tests
I'd like to know if you plan to rebase bug 13371 one day. Because it seems that what you try to achieve here is the same problematic as for the baskets search and it would make sense to have the same kind of view for both.
We plan to re-submit bug 13371 someday, yes. Thanks for the link.
These patches still apply cleanly and the QA issues seem fairly minor. Can someone give it some attention?
Created attachment 55277 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. In the process, this patch adds 4 new "Koha::Object" modules: - Koha::Bookseller(s) - Koha::Basket(s) - Koha::Basketgroup(s) - Koha::Order(s) It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed
Created attachment 55278 [details] [review] Bug 11708: UT: Replace 'ok' by 'is' where appropriate
Created attachment 55279 [details] [review] Bug 11708: UT: Use DateTime instead of localtime+sprintf
(In reply to Jonathan Druart from comment #134) > 1/ There are still occurrences of 'aqbasketgroup.closed' in tests. These are not references to the deleted SQL column, it's a hash key that is set by GetBasketgroup and GetBasketgroups (depending on the value of closeddate), and it's also understood by NewBasketgroup and ModBasketgroup (to set closeddate correctly). Not essential, but not harmful either IMO
Julian, We have a problem here, you added Koha::Bookseller and Koha::Order, but we already have Koha::Acquisition::Bookseller and Koha::Acquisition::Order. They are not based on Koha::Object because they have been pushed before its introduction, but bug 13726 already does this job.
Ok, I can rewrite these patches on top of bug 13726. The downside of this is that this bug will be indirectly dependent on the VAT-rewrite stuff (bug 13323 and bug 13321) for no real reason. Do you have an idea to avoid that ?
Created attachment 58770 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: Use atomic update
Created attachment 58771 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 58772 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 58773 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: use atomic update
Created attachment 58774 [details] [review] Bug 11708: Place "Basket groups" menu entry on top of the side navigation
Created attachment 58775 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. In the process, this patch adds 4 new "Koha::Object" modules: - Koha::Bookseller(s) - Koha::Basket(s) - Koha::Basketgroup(s) - Koha::Order(s) It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed
Created attachment 58776 [details] [review] Bug 11708: UT: Replace 'ok' by 'is' where appropriate
Created attachment 58777 [details] [review] Bug 11708: UT: Use DateTime instead of localtime+sprintf
Created attachment 58778 [details] [review] Bug 11708: Move acq related Koha::Objects to Koha::Acquisition namespace and fix Koha::Acquisition::Order usage everywhere
Patch does not apply Applying: Bug 11708: Add aqbasketgroups.closeddate Applying: Bug 11708: Unit tests for aqbasketgroups.closeddate Applying: Bug 11708: basketgroup.pl can now display basketgroups for all suppliers Using index info to reconstruct a base tree... M acqui/basketgroup.pl M koha-tmpl/intranet-tmpl/prog/css/datatables.css M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt
Created attachment 59301 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: Use atomic update
Created attachment 59302 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 59303 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 59304 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: use atomic update
Created attachment 59305 [details] [review] Bug 11708: Place "Basket groups" menu entry on top of the side navigation
Created attachment 59306 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. In the process, this patch adds 4 new "Koha::Object" modules: - Koha::Bookseller(s) - Koha::Basket(s) - Koha::Basketgroup(s) - Koha::Order(s) It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed
Created attachment 59307 [details] [review] Bug 11708: UT: Replace 'ok' by 'is' where appropriate
Created attachment 59308 [details] [review] Bug 11708: UT: Use DateTime instead of localtime+sprintf
Created attachment 59309 [details] [review] Bug 11708: Move acq related Koha::Objects to Koha::Acquisition namespace and fix Koha::Acquisition::Order usage everywhere
Created attachment 61018 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: Use atomic update
Created attachment 61019 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 61020 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 61021 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: use atomic update
Created attachment 61022 [details] [review] Bug 11708: Place "Basket groups" menu entry on top of the side navigation
Created attachment 61023 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. In the process, this patch adds 4 new "Koha::Object" modules: - Koha::Bookseller(s) - Koha::Basket(s) - Koha::Basketgroup(s) - Koha::Order(s) It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed
Created attachment 61024 [details] [review] Bug 11708: UT: Replace 'ok' by 'is' where appropriate
Created attachment 61025 [details] [review] Bug 11708: UT: Use DateTime instead of localtime+sprintf
Created attachment 61026 [details] [review] Bug 11708: Move acq related Koha::Objects to Koha::Acquisition namespace and fix Koha::Acquisition::Order usage everywhere
Hi, while trying to record an acquisition, I got this error (URL:http://pro.user14-koha.sandbox.biblibre.eu/cgi-bin/koha/acqui/addorder.pl) : Software error: DBIx::Class::Row::store_column(): No such column 'field_value' on Koha::Schema::Result::Aqorder at /home/koha/src/Koha/Object.pm line 72 For help, please send mail to the webmaster ([no address given]), giving this error message and the time and date of the error. http://pro.user14-koha.sandbox.biblibre.eu/cgi-bin/koha/acqui/addorder.pl
Created attachment 71260 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: Use atomic update
Created attachment 71261 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 71262 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 71263 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: use atomic update
Created attachment 71264 [details] [review] Bug 11708: Place "Basket groups" menu entry on top of the side navigation
Created attachment 71265 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. In the process, this patch adds 4 new "Koha::Object" modules: - Koha::Bookseller(s) - Koha::Basket(s) - Koha::Basketgroup(s) - Koha::Order(s) It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed
Created attachment 71266 [details] [review] Bug 11708: UT: Replace 'ok' by 'is' where appropriate
Created attachment 71267 [details] [review] Bug 11708: UT: Use DateTime instead of localtime+sprintf
Created attachment 71268 [details] [review] Bug 11708: Move acq related Koha::Objects to Koha::Acquisition namespace and fix Koha::Acquisition::Order usage everywhere
Created attachment 72069 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: Use atomic update
Created attachment 72070 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 72071 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Created attachment 72072 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: use atomic update
Created attachment 72073 [details] [review] Bug 11708: Place "Basket groups" menu entry on top of the side navigation
Created attachment 72074 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. In the process, this patch adds 4 new "Koha::Object" modules: - Koha::Bookseller(s) - Koha::Basket(s) - Koha::Basketgroup(s) - Koha::Order(s) It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed
Created attachment 72075 [details] [review] Bug 11708: UT: Replace 'ok' by 'is' where appropriate
Created attachment 72076 [details] [review] Bug 11708: UT: Use DateTime instead of localtime+sprintf
Created attachment 72077 [details] [review] Bug 11708: Move acq related Koha::Objects to Koha::Acquisition namespace and fix Koha::Acquisition::Order usage everywhere
TypeError: $(...).kohaDataTable is not a function
Seen with Séverine, the error disappeared... Probably a browser cache problem
Created attachment 72691 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: Use atomic update Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 72692 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 72693 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 72694 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: use atomic update Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 72695 [details] [review] Bug 11708: Place "Basket groups" menu entry on top of the side navigation Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 72696 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. In the process, this patch adds 4 new "Koha::Object" modules: - Koha::Bookseller(s) - Koha::Basket(s) - Koha::Basketgroup(s) - Koha::Order(s) It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 72697 [details] [review] Bug 11708: UT: Replace 'ok' by 'is' where appropriate Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 72698 [details] [review] Bug 11708: UT: Use DateTime instead of localtime+sprintf Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 72699 [details] [review] Bug 11708: Move acq related Koha::Objects to Koha::Acquisition namespace and fix Koha::Acquisition::Order usage everywhere Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Useful enhencement ! I create a new ticket to improve it more with a floating toolbar. https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20373
Please rebase and check for changed needed!
Created attachment 74213 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: Use atomic update Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74214 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74215 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74216 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: use atomic update Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74217 [details] [review] Bug 11708: Place "Basket groups" menu entry on top of the side navigation Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74218 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. In the process, this patch adds 4 new "Koha::Object" modules: - Koha::Bookseller(s) - Koha::Basket(s) - Koha::Basketgroup(s) - Koha::Order(s) It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74219 [details] [review] Bug 11708: UT: Replace 'ok' by 'is' where appropriate Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74220 [details] [review] Bug 11708: UT: Use DateTime instead of localtime+sprintf Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74221 [details] [review] Bug 11708: Move acq related Koha::Objects to Koha::Acquisition namespace and fix Koha::Acquisition::Order usage everywhere Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74222 [details] [review] Bug 11708: Use Asset plugin in acqui/basketgroups.tt
Hi Julian, can you give it another go? I think your branch might have been missing the latest commits (bug 18791). Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 11708: Add aqbasketgroups.closeddate Applying: Bug 11708: Unit tests for aqbasketgroups.closeddate Applying: Bug 11708: basketgroup.pl can now display basketgroups for all suppliers Applying: Bug 11708: Remove column aqbasketgroups.closed Applying: Bug 11708: Place "Basket groups" menu entry on top of the side navigation Applying: Bug 11708: New page for basket groups Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/includes/datatables.inc M koha-tmpl/intranet-tmpl/prog/js/datatables.js Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/js/datatables.js CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/js/datatables.js Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/datatables.inc Failed to merge in the changes. Patch failed at 0001 Bug 11708: New page for basket groups The copy of the patch that failed is found in: /home/vagrant/kohaclone/.git/rebase-apply/patch 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". Patch left in /tmp/Bug-11708-New-page-for-basket-groups-pzWRYQ.patch
Created attachment 74302 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: Use atomic update Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74303 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74304 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74305 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: use atomic update Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74306 [details] [review] Bug 11708: Place "Basket groups" menu entry on top of the side navigation Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74307 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. In the process, this patch adds 4 new "Koha::Object" modules: - Koha::Bookseller(s) - Koha::Basket(s) - Koha::Basketgroup(s) - Koha::Order(s) It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74308 [details] [review] Bug 11708: UT: Replace 'ok' by 'is' where appropriate Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74309 [details] [review] Bug 11708: UT: Use DateTime instead of localtime+sprintf Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74310 [details] [review] Bug 11708: Move acq related Koha::Objects to Koha::Acquisition namespace and fix Koha::Acquisition::Order usage everywhere Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74311 [details] [review] Bug 11708: Use Asset plugin in acqui/basketgroups.tt
Created attachment 74349 [details] [review] Bug 11708: Add aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: Use atomic update Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74350 [details] [review] Bug 11708: Unit tests for aqbasketgroups.closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74351 [details] [review] Bug 11708: basketgroup.pl can now display basketgroups for all suppliers This happens when no booksellerid is given. This patch also adds 4 columns: - No. of ordered titles - No. of received titles - Ordered amount - Charged amount (amount of received orders) Add filters to basketgroup tables columns Show closed date in closed basketgroups tables Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74352 [details] [review] Bug 11708: Remove column aqbasketgroups.closed It is redundant with aqbasketgroups.closeddate Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Patch updated: use atomic update Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74353 [details] [review] Bug 11708: Place "Basket groups" menu entry on top of the side navigation Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74354 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. In the process, this patch adds 4 new "Koha::Object" modules: - Koha::Bookseller(s) - Koha::Basket(s) - Koha::Basketgroup(s) - Koha::Order(s) It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74355 [details] [review] Bug 11708: UT: Replace 'ok' by 'is' where appropriate Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74356 [details] [review] Bug 11708: UT: Use DateTime instead of localtime+sprintf Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74357 [details] [review] Bug 11708: Move acq related Koha::Objects to Koha::Acquisition namespace and fix Koha::Acquisition::Order usage everywhere Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 74358 [details] [review] Bug 11708: Use Asset plugin in acqui/basketgroups.tt
Sorry Julian, patches apply, but still some problems: 1) QA scripts Lots of those: FAIL Koha/Baskets.pm FAIL git manipulation The file has been added and deleted in the same patchset Maybe see if you can squash some with the same sign-offer? And: FAIL t/db_dependent/Acquisition/close_reopen_basket.t OK critic OK forbidden patterns OK git manipulation OK pod OK spelling FAIL valid Global symbol "$order1" requires explicit package name Global symbol "$order2" requires explicit package name t/db_dependent/Acquisition/close_reopen_basket.t had compilation errors. FAIL t/db_dependent/Acquisition/Basketgroups.t OK critic OK forbidden patterns OK git manipulation OK pod OK spelling FAIL valid "AddBookseller" is not exported by the C4::Bookseller module Can't continue after import errors 2) Code review a) Koha/Acquisition/Baskets.pm -# Copyright 2017 Aleisha Amohia <aleisha@catalyst.net.nz> -# Why remove the Copyright statement? And POD? b) Similar: Koha/Acquisition/Orders.pm + my $pdfformatfile = './' . ($pdfformat =~ s,::,/,gr) . '.pm'; + require $pdfformatfile; Seems to be an unrelated change? I remember that we hardcoded the valid template names once for added security. could only find one mention of it tho: bug 11459#c11 c) admin/currency.pl +use Koha::Acquisition::Currency; Unrelated change? d) koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroups.tt <th>Bookseller</th> Should be vendor! e) Generate EDIFACT Order Capitalization: Generate EDIFACT order f) Changes to koha-tmpl/intranet-tmpl/prog/js/datatables.js Do you remember why those changes were needed or was it not intentional? Trying to figure out what needs to be tested with this patch and it turns out a lot of code is touched. g) Merge atomicupdates? 3) Unit tests prove t/db_dependent/Acquisition/Basketgroups.t t/db_dependent/Acquisition/Basketgroups.t .. "AddBookseller" is not exported by the C4::Bookseller module Can't continue after import errors at t/db_dependent/Acquisition/Basketgroups.t line 9. BEGIN failed--compilation aborted at t/db_dependent/Acquisition/Basketgroups.t line 9. prove t/db_dependent/Acquisition/close_reopen_basket.t t/db_dependent/Acquisition/close_reopen_basket.t .. Global symbol "$order1" requires explicit package name at t/db_dependent/Acquisition/close_reopen_basket.t line 58. Global symbol "$order2" requires explicit package name at t/db_dependent/Acquisition/close_reopen_basket.t line 68. Execution of t/db_dependent/Acquisition/close_reopen_basket.t aborted due to compilation errors.
Created attachment 75003 [details] [review] Bug 11708: Change aqbasketgroups.closed to closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 75004 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. This patch also adds 3 columns: - No. of ordered titles - No. of received titles - Date closed It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Katrin, thanks for the review ! I did a bit of cleanup here : I squashed all patches into only two patches and removed all changes that were not related to (or needed by) this patchset. I think it will be easier to read. QA script is happy now and I think I fixed every problem you mentioned in comment 237, except for the following: > + my $pdfformatfile = './' . ($pdfformat =~ s,::,/,gr) . '.pm'; > + require $pdfformatfile; > Seems to be an unrelated change? I remember that we hardcoded the valid template names once for added security. could only find one mention of it tho: bug 11459#c11 It's just part of the "basketgroup.pl cleanup" process. perlcritic complains when we use the eval "string" construct. It shouldn't change anything. > f) Changes to koha-tmpl/intranet-tmpl/prog/js/datatables.js > Do you remember why those changes were needed or was it not intentional? Trying to figure out what needs to be tested with this patch and it turns out a lot of code is touched. I don't remember why exactly, but I needed to use the 1.10+ API of DataTables. These changes just map legacy API defaults to new API ones. This shouldn't change anything for code using the legacy API.
I tried to test bug 20810 but this dependency doesn't apply : Bug 11708 - Display all basketgroups on one page, and new column aqbasketgroups.closeddate 75003 - Bug 11708: Change aqbasketgroups.closed to closeddate 75004 - Bug 11708: New page for basket groups Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 11708: Change aqbasketgroups.closed to closeddate Applying: Bug 11708: New page for basket groups Using index info to reconstruct a base tree... M acqui/basketgroup.pl M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt Falling back to patching base and 3-way merge... error: The following untracked working tree files would be overwritten by merge: Koha/Acquisition/Basketgroup.pm Koha/Acquisition/Basketgroups.pm acqui/basketgroups.pl koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroups.tt Please move or remove them before you can merge. Aborting Also failed on Biblibre's sandbox #11
Created attachment 75908 [details] [review] Bug 11708: Change aqbasketgroups.closed to closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 75909 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. This patch also adds 3 columns: - No. of ordered titles - No. of received titles - Date closed It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
1. aqbasketgroups.closedate is set to NOW, maybe better to use basket.closedate (the latest)? 2. ->bookseller should be ->vendor (not blocker as we already have both in Koha::*...) 3. a. I would not "cache" ->baskets, we already deal with the context in Koha::Objects->search b. ->baskets_count - no need to have it, it should be replaced with baskets->count => Maybe you want to keep them for performance purpose, but is it really a performance improvement? 5. basketgroup.tt has a [% IF booksellerid %], it should not be useful now. We may want to display a friendly message if called without a [valid] id 6. basketgroups.tt should have JS at the bottom 7. Some links still point to basketgroup.pl?booksellerid=X without the basketgroupid, they should be replaced right? i.e. code related to 'sub displaybasketgroups' in basketgroup.pl should be removed (?) 8. Are you sure you need the changes done to C4::Acquisition? 9. acqui/basketgroup.pl + use List::MoreUtils qw/uniq/; => Not needed 10. "kohaDataTable": We already have KohaTable. You are adding a 3rd way to init a table.
Created attachment 78662 [details] [review] Bug 11708: Change aqbasketgroups.closed to closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 78663 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. This patch also adds 3 columns: - No. of ordered titles - No. of received titles - Date closed It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 78664 [details] [review] Bug 11708: Fix QA issues - Change how closedate is set in database update - Reword bookseller to vendor in Koha::Acquisition::Basketgroup - Do not "cache" baskets in Koha::Acquisition::Basketgroup - Remove useless IF in basketgroup.tt - Move JS to bottom in basketgroups.tt - Fix some links to basketgroup.pl - Remove useless use in acqui/basketgroup.pl - Remove useless sub displaybasketgroups in acqui/basketgroup.pl - Remove useless code in C4::Acquisition
(In reply to Jonathan Druart from comment #245) > 1. aqbasketgroups.closedate is set to NOW, maybe better to use > basket.closedate (the latest)? Fixed > 2. ->bookseller should be ->vendor (not blocker as we already have both in > Koha::*...) Fixed > 3. a. I would not "cache" ->baskets, we already deal with the context in > Koha::Objects->search I don't know what is the context you are talking about, but I removed the "caching" > b. ->baskets_count - no need to have it, it should be replaced with > baskets->count > => Maybe you want to keep them for performance purpose, but is it really a > performance improvement? It's used only in templates, because TT seems to force the list context even with a directive like this: [% basketgroup.baskets.count %] It tries to call count on the first item of @{ $basketgroup->baskets } I'd happily remove this sub if you have a better idea. > 5. basketgroup.tt has a [% IF booksellerid %], it should not be useful now. Fixed > We may want to display a friendly message if called without a [valid] id Is returning a 404 error friendly ? Because it's probably what should be done if the id is invalid. > 6. basketgroups.tt should have JS at the bottom Fixed > 7. Some links still point to basketgroup.pl?booksellerid=X without the > basketgroupid, they should be replaced right? > i.e. code related to 'sub displaybasketgroups' in basketgroup.pl should be > removed (?) Fixed > 8. Are you sure you need the changes done to C4::Acquisition? No. They're not needed anymore. I removed them. > 9. acqui/basketgroup.pl > + use List::MoreUtils qw/uniq/; > => Not needed Fixed > 10. "kohaDataTable": We already have KohaTable. You are adding a 3rd way to > init a table. We already have 3 ways to initialize DT: 1. The "legacy API" way: $("#table_id").dataTable($.extend(true, {}, dataTablesDefaults, { ... }); 2. The "legacy API + column_settings" way: KohaTable('table_id', ...); 3. The "1.10+ API" way $('#table_id').DataTable(...) kohaDataTable is just a wrapper around the 3rd that provides the same defaults than the 1st and 2nd methods. I don't see how this is bad.
Sorry, this no longer applies, can you please rebase? Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 11708: Change aqbasketgroups.closed to closeddate Applying: Bug 11708: New page for basket groups fatal: sha1 information is lacking or useless (C4/Acquisition.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 11708: New page for basket groups The copy of the patch that failed is found in: /home/vagrant/kohaclone/.git/rebase-apply/patch 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". Patch left in /tmp/Bug-11708-New-page-for-basket-groups-maTm2R.patch
Please also make sure to run the newest qa script on the code to check for missing filters!
Created attachment 80697 [details] [review] Bug 11708: Change aqbasketgroups.closed to closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 80698 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. This patch also adds 3 columns: - No. of ordered titles - No. of received titles - Date closed It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 80699 [details] [review] Bug 11708: Fix QA issues - Change how closedate is set in database update - Reword bookseller to vendor in Koha::Acquisition::Basketgroup - Do not "cache" baskets in Koha::Acquisition::Basketgroup - Remove useless IF in basketgroup.tt - Move JS to bottom in basketgroups.tt - Fix some links to basketgroup.pl - Remove useless use in acqui/basketgroup.pl - Remove useless sub displaybasketgroups in acqui/basketgroup.pl - Remove useless code in C4::Acquisition
Created attachment 80700 [details] [review] Bug 11708: Add missing TT filters and fix perlcritic errors
Patches rebased + another patch of QA fixes
Patches apply, QA script is clean. I am wondering about the changes to koha-tmpl/intranet-tmpl/prog/js/datatables.js as well. Adding Owen, maybe he has a moment to take a look at the datatables question here. Jonathan, could you have another look at Julian's answers in comment#249?
This patch inadvertently changes the filename of two existing files (which didn't exist when the patch was written): BasketGroup.pm and BasketGroups.pm
Created attachment 80979 [details] [review] Bug 11708: Change aqbasketgroups.closed to closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 80980 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. This patch also adds 3 columns: - No. of ordered titles - No. of received titles - Date closed It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 80981 [details] [review] Bug 11708: Fix QA issues - Change how closedate is set in database update - Reword bookseller to vendor in Koha::Acquisition::BasketGroup - Do not "cache" baskets in Koha::Acquisition::BasketGroup - Remove useless IF in basketgroup.tt - Move JS to bottom in basketgroups.tt - Fix some links to basketgroup.pl - Remove useless use in acqui/basketgroup.pl - Remove useless sub displaybasketgroups in acqui/basketgroup.pl - Remove useless code in C4::Acquisition
Created attachment 80982 [details] [review] Bug 11708: Add missing TT filters and fix perlcritic errors
I get an error when I close a basket group and choose 'Generate EDIFACT order': bookseller id is required to edit a basketgroup at /home/vagrant/kohaclone/C4/Acquisition.pm line 1100.
Created attachment 85786 [details] [review] Bug 11708: Change aqbasketgroups.closed to closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 85787 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. This patch also adds 3 columns: - No. of ordered titles - No. of received titles - Date closed It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed 5. prove t/db_dependent/Koha/Acquisition/Basket.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Patches rebased + squashed
Sorry Julian for the lack of reaction... CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/acquisitions-menu.inc Auto-merging acqui/basketgroup.pl CONFLICT (content): Merge conflict in acqui/basketgroup.pl Auto-merging acqui/basket.pl Auto-merging Koha/Acquisition/Basket.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 11708: New page for basket groups
Created attachment 91285 [details] [review] Bug 11708: Change aqbasketgroups.closed to closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 91286 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. This patch also adds 3 columns: - No. of ordered titles - No. of received titles - Date closed It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed 5. prove t/db_dependent/Koha/Acquisition/Basket.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Overall this looks good to me. There are a couple of issues raised by the QA tool which need to be addressed. The only problem I ran into was exporting as PDF. I get this error: Can't call method "header" on an undefined value at /home/vagrant/kohaclone/acqui/basketgroup.pl line 159
Created attachment 92378 [details] [review] Bug 11708: Change aqbasketgroups.closed to closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 92379 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. This patch also adds 3 columns: - No. of ordered titles - No. of received titles - Date closed It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed 5. prove t/db_dependent/Koha/Acquisition/Basket.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 92380 [details] [review] Bug 11708: Add missing POD in Koha::Acquisition::Basket
Created attachment 92381 [details] [review] Bug 11708: Use bootstrap grid instead of yui
Created attachment 92382 [details] [review] Bug 11708: Fix PDF export
The sandbox you've requested is not ready. Some problems occurred applying patches from bug 11708: <h1>Something went wrong !</h1>Applying: Bug 11708: Change aqbasketgroups.closed to closeddate Warning: commit message did not conform to UTF-8. You may want to amend it after fixing the message, or set the config variable i18n.commitencoding to the encoding your project uses. Applying: Bug 11708: New page for basket groups Using index info to reconstruct a base tree... M koha-tmpl/intranet-tmpl/prog/en/includes/datatables.inc M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt M koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt M koha-tmpl/intranet-tmpl/prog/js/datatables.js Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/js/datatables.js CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/js/datatables.js Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/parcel.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroup.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basket.tt Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/datatables.inc error: Failed to merge in the changes. Patch failed at 0001 Bug 11708: New page for basket groups
Created attachment 93460 [details] [review] Bug 11708: Change aqbasketgroups.closed to closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93461 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. This patch also adds 3 columns: - No. of ordered titles - No. of received titles - Date closed It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed 5. prove t/db_dependent/Koha/Acquisition/Basket.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93462 [details] [review] Bug 11708: Add missing POD in Koha::Acquisition::Basket
Created attachment 93463 [details] [review] Bug 11708: Use bootstrap grid instead of yui
Created attachment 93464 [details] [review] Bug 11708: Fix PDF export
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93482 [details] [review] Bug 11708: Change aqbasketgroups.closed to closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93483 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. This patch also adds 3 columns: - No. of ordered titles - No. of received titles - Date closed It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed 5. prove t/db_dependent/Koha/Acquisition/Basket.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93484 [details] [review] Bug 11708: Add missing POD in Koha::Acquisition::Basket Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93485 [details] [review] Bug 11708: Use bootstrap grid instead of yui Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 93487 [details] [review] Bug 11708: Fix PDF export Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
0) There is a QA script fail: FAIL koha-tmpl/intranet-tmpl/prog/en/modules/acqui/basketgroups.tt OK filters FAIL forbidden patterns forbidden pattern: Attribute type should not be used for script tags (bug 20053) (line 122) 1) Not sure about the naming of these: ordered_titles_count, received_titles_count Not a blocker, just noting (no blocker) 2) Effects on EDI I wonder if the change to closeddate affects EDI. Asking Kyle/Martin to have a look. 3) When there are no basket groups yet, the link in the sidebar leads to an empty page. It might be nicer to display a little message. 4) The datatable on the new basket group page is broken: TypeError: $(...).kohaDataTable is not a function 5) When you are on the pages of a vendor, there is a tab labelled 'Basket groups' and there is also a link now below with the same label. This could be confusing. Not sure what to do about that - noting it to see if someone has a good idea.
Created attachment 104928 [details] [review] Bug 11708: Change aqbasketgroups.closed to closeddate Signed-off-by: juliette levast <juliette.levast@iepg.fr> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 104929 [details] [review] Bug 11708: New page for basket groups This patch moves the code responsible for displaying a list of basket groups into its own Perl script (acqui/basketgroups.pl), making the code in basketgroup.pl and basketgroup.tt a little bit easier to read. basketgroups.pl displays all basket groups in a single table (as bug 13371 for vendors) where rows are grouped by bookseller. This patch also adds 3 columns: - No. of ordered titles - No. of received titles - Date closed It also adds a wrapper around the new DataTable() constructor to be able to use it with the same defaults than the previous dataTable() constructor Test plan: 0. Create a bunch of booksellers and basketgroups 1. Go back to acquisitions home page and click on "Basket groups" link on the left 2. Play with the table (sort, filter) and try every possible actions (Edit, Close and export as PDF, View, Reopen, Export as CSV) 3. Go to a specific vendor page and click on "Basket groups" tab 4. Check that only the vendor's basket groups are displayed 5. prove t/db_dependent/Koha/Acquisition/Basket.t Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 104930 [details] [review] Bug 11708: Add missing POD in Koha::Acquisition::Basket Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 104931 [details] [review] Bug 11708: Use bootstrap grid instead of yui Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 104932 [details] [review] Bug 11708: Fix PDF export Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 104933 [details] [review] Bug 11708: Reword 'Basket groups' into 'All basket groups' Hopefully this will be enough to clarify the difference between this link and the 'Basket groups' tab
Created attachment 104934 [details] [review] Bug 11708: Show a message when there are no basket groups
Created attachment 104935 [details] [review] Bug 11708: Remove useless type attribute from script tag
(In reply to Katrin Fischer from comment #288) > 0) There is a QA script fail: Fixed > 3) When there are no basket groups yet, the link in the sidebar leads to an > empty page. It might be nicer to display a little message. Fixed > 4) The datatable on the new basket group page is broken: > TypeError: $(...).kohaDataTable is not a function I cannot reproduce. kohaDataTable is declared in those patches. Maybe a browser cache issue ? > 5) When you are on the pages of a vendor, there is a tab labelled 'Basket > groups' and there is also a link now below with the same label. This could > be confusing. Not sure what to do about that - noting it to see if someone > has a good idea. I changed it to 'All basket groups'... The UI can certainly be improved in this area
I have a sense of déjà vu with this one... :) 1) QA tools: FAIL t/db_dependent/Acquisition/Basketgroups.t OK critic FAIL forbidden patterns forbidden pattern: Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840) (line 42) FAIL t/db_dependent/Acquisition/Basketgroups.t OK critic FAIL forbidden patterns forbidden pattern: Use of DateTime->now should certainly be replaced with dt_from_string (bug 24840) (line 42) FAIL acqui/basketgroup.pl OK critic FAIL forbidden patterns forbidden pattern: Script permissions is authnotrequired => 0, it could be correct for an OPAC script if it is was you really want error (bug 24663) (line 69) Can you please fix?