Acquisitions scripts use three different variable names for vendor id on different pages: booksellerid, supplierid, and id. In order to make the code more consistent and to allow us better global navigation in acquisitions I propose that we standardize on booksellerid. This one is most commonly used.
Created attachment 6106 [details] [review] Proposed fix
Hi Owen, thank you for your work on this! I started testing and found some small issues: Save changes to contract 1) Add contract > redirect to vendor broken 2) Edit contract > redirect to vendor broken 3) Delete contract > rediret to vendor broken URL still has supplierid: http://localhost:8080/cgi-bin/koha/acqui/supplier.pl?supplierid=6
Saving changes to vendor 1) Edit existing vendor 2) Save vendor URL: http://localhost:8080/cgi-bin/koha/acqui/booksellers.pl?id=3 Unrelated to bug: Edit vendor link from toolbar on basketgroups page Page: http://localhost:8080/cgi-bin/koha/acqui/basketgroup.pl?booksellerid=3 URL: http://localhost:8080/cgi-bin/koha/acqui/booksellers.pl?booksellerid=3
Deleting an order from receive page redirects to wrong page Delete Order & Delete order and catalog record Page: http://localhost:8080/cgi-bin/koha/acqui/parcel.pl?invoice=test&booksellerid=3&gst=0&datereceived=2011-10-29 URL: http://localhost:8080/cgi-bin/koha/acqui/basket.pl?basketno=
Created attachment 6114 [details] [review] [SIGNED-OFF] Fix for Bug 7113 - Standardize vendor id name in templates and scripts Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Signed off this patch, but before it can go into master a follow up is needed to fix the found issues marked as 'BROKEN' in the test plan. This patch touches a lot of links in acquisitions. Tested: Vendor result list (booksellers) - link to vendor summary page - link to receive shipment - new basket Vendor summary (supplier) - links to contract summary - BROKEN: Edit vendor: redirect after saving - edit vendor > cancel - BROKEN: redirect after saving/editing/adding contract - contracts list from toolbar - new basket Basketgroups (basketgroup) - breadcrumbs: link to vendor - link back to vendor summary - edit basketgroup > save - delete basketgroup - link to vendor from toolbar - (BROKEN) edit vendor from toolbar - does not open edit page - uncertain prices Uncertain prices (uncertainprice) - breadcrumbs: link to vendor - link to vendor summary - link to edit vendor - here it works - edit po line - changing price and submitting Basket header (basketheader) - breadcrumbs: link back to vendor - link for saving New basket (basket) - breadcrumbs: link back to vendor - delete basket (redirects to search list, ok) - export as csv - close basket - cancel order - modify order Order from existing record (neworderempty) - breadcrumbs: link back to vendor - order Order from suggestion (newordersuggestion) - breadcrumbs: link back to vendor - order - cancel Order from external source (z3950_search) - breadcrumbs: link to vendor - order - after ordering: neworderenry > edit record Order new (neworderentry) - breadcrumbs: link to vendor - save - cancel Receive shiptment (parcels) - breadcrumbs: link to vendor - cancel Order receive (parcel) - Finish receiving - Cancel receiving - link to order line - receive - BROKEN: delete order redirects to wrong page - BROKEN: delete order and catalog record redirects to wrong page Late orders (lateorders) - filter on specific vendor
(In reply to comment #5) > - BROKEN: delete order redirects to wrong page > - BROKEN: delete order and catalog record redirects to wrong page These two at least are bugs in master.
Created attachment 6124 [details] [review] Follow-up fix Fixes: - Edit vendor: redirect after saving - redirect after saving/editing/adding contract - edit vendor from toolbar - does not open edit page Since the first patch has a sign-off I kept these changes as a separate one which must be applied on top of the first.
(In reply to comment #5) > - BROKEN: delete order redirects to wrong page > - BROKEN: delete order and catalog record redirects to wrong page This is now Bug 7120
Created attachment 6189 [details] [review] Follow-up fixes for Bug 7113, Standardize vendor id name in templates and scripts Fixed: - Edit vendor: redirect after saving - redirect after saving/editing/adding contract - edit vendor from toolbar - does not open edit page Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> 1) redirect after editing vendor is ok 2) saving and deleting contract is perfect, one little improvement: The cancel link takes you back to the list of contracts, not to the vendor detail page. I think it would be a little better if save and cancel redirected to the same page. 3) editing vendor from basket group toolbar works now
Thx Owen!
aqcontract.tt still passes supplierid param, even though it's being populated with the booksellerid value. Serials doesn't seem to be making this change, particularly in claims.tt. Is it outside the scope of this report? What about variables in C4 modules? Have to mark this as Failed QA due to the first paragraph... please fix, and clarify the scope for this report
Created attachment 6369 [details] [review] Follow-up fix Fix for a missed instance of the wrong variable name In terms of scope: I undertook the changes in this bug in order to lay the groundwork for an overhaul of navigation in the Acquisitions section. Standardizing the variable names was necessary in order to be able to have navigation includes use a consistent variable name. Changes to C4 and serials pages are outside the scope of my initial effort because of the practical reason for making the changes. Getting this initial batch of changes approved will let me move forward in other work. Getting the other sections brought in line could happen separately.
Created attachment 6437 [details] [review] [SIGNED-OFF] Fix for Bug 7113 - Standardize vendor id name in templates and scripts Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Update: Fixed conflict in basket.tt Signed off this patch, but before it can go into master a follow up is needed to fix the found issues marked as 'BROKEN' in the test plan. This patch touches a lot of links in acquisitions. Tested: Vendor result list (booksellers) - link to vendor summary page - link to receive shipment - new basket Vendor summary (supplier) - links to contract summary - BROKEN: Edit vendor: redirect after saving - edit vendor > cancel - BROKEN: redirect after saving/editing/adding contract - contracts list from toolbar - new basket Basketgroups (basketgroup) - breadcrumbs: link to vendor - link back to vendor summary - edit basketgroup > save - delete basketgroup - link to vendor from toolbar - (BROKEN) edit vendor from toolbar - does not open edit page - uncertain prices Uncertain prices (uncertainprice) - breadcrumbs: link to vendor - link to vendor summary - link to edit vendor - here it works - edit po line - changing price and submitting Basket header (basketheader) - breadcrumbs: link back to vendor - link for saving New basket (basket) - breadcrumbs: link back to vendor - delete basket (redirects to search list, ok) - export as csv - close basket - cancel order - modify order Order from existing record (neworderempty) - breadcrumbs: link back to vendor - order Order from suggestion (newordersuggestion) - breadcrumbs: link back to vendor - order - cancel Order from external source (z3950_search) - breadcrumbs: link to vendor - order - after ordering: neworderenry > edit record Order new (neworderentry) - breadcrumbs: link to vendor - save - cancel Receive shiptment (parcels) - breadcrumbs: link to vendor - cancel Order receive (parcel) - Finish receiving - Cancel receiving - link to order line - receive - BROKEN: delete order redirects to wrong page - BROKEN: delete order and catalog record redirects to wrong page Late orders (lateorders) - filter on specific vendor Follow-up fixes for Bug 7113, Standardize vendor id name in templates and scripts Fixed: - Edit vendor: redirect after saving - redirect after saving/editing/adding contract - edit vendor from toolbar - does not open edit page Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> 1) redirect after editing vendor is ok 2) saving and deleting contract is perfect, one little improvement: The cancel link takes you back to the list of contracts, not to the vendor detail page. I think it would be a little better if save and cancel redirected to the same page. 3) editing vendor from basket group toolbar works now [SIGNED-OFF] Bug 7113 Fix for a missed instance of the wrong variable name Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Link back to vendor in breadcrumbs of contract page works now too.
Tested the 3rd patch on top of the 2 others and sqashed them into one.
Created attachment 6767 [details] [review] Fix for Bug 7113 - Standardize vendor id name in templates and scripts Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Update: Fixed conflict in basket.tt Update 2011-12-14: Fixed conflict in basketgroup.tt Signed off this patch, but before it can go into master a follow up is needed to fix the found issues marked as 'BROKEN' in the test plan. This patch touches a lot of links in acquisitions. Tested: Vendor result list (booksellers) - link to vendor summary page - link to receive shipment - new basket Vendor summary (supplier) - links to contract summary - BROKEN: Edit vendor: redirect after saving - edit vendor > cancel - BROKEN: redirect after saving/editing/adding contract - contracts list from toolbar - new basket Basketgroups (basketgroup) - breadcrumbs: link to vendor - link back to vendor summary - edit basketgroup > save - delete basketgroup - link to vendor from toolbar - (BROKEN) edit vendor from toolbar - does not open edit page - uncertain prices Uncertain prices (uncertainprice) - breadcrumbs: link to vendor - link to vendor summary - link to edit vendor - here it works - edit po line - changing price and submitting Basket header (basketheader) - breadcrumbs: link back to vendor - link for saving New basket (basket) - breadcrumbs: link back to vendor - delete basket (redirects to search list, ok) - export as csv - close basket - cancel order - modify order Order from existing record (neworderempty) - breadcrumbs: link back to vendor - order Order from suggestion (newordersuggestion) - breadcrumbs: link back to vendor - order - cancel Order from external source (z3950_search) - breadcrumbs: link to vendor - order - after ordering: neworderenry > edit record Order new (neworderentry) - breadcrumbs: link to vendor - save - cancel Receive shiptment (parcels) - breadcrumbs: link to vendor - cancel Order receive (parcel) - Finish receiving - Cancel receiving - link to order line - receive - BROKEN: delete order redirects to wrong page - BROKEN: delete order and catalog record redirects to wrong page Late orders (lateorders) - filter on specific vendor Follow-up fixes for Bug 7113, Standardize vendor id name in templates and scripts Fixed: - Edit vendor: redirect after saving - redirect after saving/editing/adding contract - edit vendor from toolbar - does not open edit page Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> 1) redirect after editing vendor is ok 2) saving and deleting contract is perfect, one little improvement: The cancel link takes you back to the list of contracts, not to the vendor detail page. I think it would be a little better if save and cancel redirected to the same page. 3) editing vendor from basket group toolbar works now [SIGNED-OFF] Bug 7113 Fix for a missed instance of the wrong variable name Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Link back to vendor in breadcrumbs of contract page works now too.
This patch doesn't apply, conflict in z3950_search.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/acqui/z3950_search.tt ps: I think occurrences in C4/Acquisition.pm can be replaced easily. For example: if (defined $supplierid) { $from .= ' AND aqbasket.booksellerid = ? '; push @query_params, $supplierid; }
Created attachment 7261 [details] [review] Revised for current master
Hi Owen, the patch does not apply any longer - I took a look at the problems but was not completely sure how to fix. Could you resubmit? I will try to test again as soon as poosible.
Created attachment 7337 [details] [review] Bug 7113 [REVISED] Standardize vendor id name in templates and scripts New revision updates for current master and cleans up new instances introduced by recent commits.
Created attachment 7345 [details] [review] [SIGNED-OFF] Bug 7113: Standardize vendor id name in templates and scripts New revision updates for current master and cleans up new instances introduced by recent commits. Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> 2 problems found, fixing those in follo up patches: - late orders don't allow more than 1 order to be selected - basketgroups: 'Edit vendor' does the same as 'Manage orders'
Created attachment 7346 [details] [review] Bug 7113: Fix 'Edit vendor' link on basketgroup page 'Edit vendor' didn't open the vendor page for editing.
Created attachment 7347 [details] [review] Bug 7133: Late orders don't allow to select more than one order To test: 1) Make sure you have at least 2 late orders each for 2 different vendors 2) Filter list to show only late orders of one vendor 3) Use Check all/Uncheck all 4) Select more than 1 order manually 5) Remove filter on vendor, show all late orders 6) When checking an order from one vendor, checkboxes for other vendors get blocked 7) Check you can select all late orders for one vendor
Created attachment 7348 [details] [review] Bug 7133: Late orders don't allow to select more than one order To test: Fix selection of orders from vendors 1) Make sure you have at least 2 late orders each for 2 different vendors 2) Filter list to show only late orders of one vendor 3) Use Check all/Uncheck all 4) Select more than 1 order manually 5) Remove filter on vendor, show all late orders 6) When checking an order from one vendor, checkboxes for other vendors get blocked 7) Check you can select all late orders for one vendor Display fix: show vendor id in first column instead of () 1) Veryfy first column of table looks something like: (2) 19/01/2012 (8 days) and has no empty ()
Created attachment 7355 [details] [review] [SIGNED-OFF] Bug 7113: Fix 'Edit vendor' link on basketgroup page
Created attachment 7356 [details] [review] [SIGNED-OFF] Bug 7133: Late orders don't allow to select more than one order All tests listed in the patch description pass for me.
QA comment: * I agree that this could be considered as a bug, but it changes a lot of things in the API, so setting it for 3.8 * code is OK. * I made a lot of tests, and could not find any problem. The late orders don't show anything, but on master it doesn't either, it must be my test database. passed QA, will push
Works in master. Not to be pushed to 3.6