Bug 7113 - Standardize vendor id name in templates and scripts
Summary: Standardize vendor id name in templates and scripts
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Owen Leonard
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks: 7606
  Show dependency treegraph
 
Reported: 2011-10-28 18:43 UTC by Owen Leonard
Modified: 2012-10-25 23:10 UTC (History)
4 users (show)

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


Attachments
Proposed fix (61.06 KB, patch)
2011-10-28 20:13 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Fix for Bug 7113 - Standardize vendor id name in templates and scripts (62.95 KB, patch)
2011-10-29 14:48 UTC, Katrin Fischer
Details | Diff | Splinter Review
Follow-up fix (3.36 KB, patch)
2011-10-31 14:30 UTC, Owen Leonard
Details | Diff | Splinter Review
Follow-up fixes for Bug 7113, Standardize vendor id name in templates and scripts (3.71 KB, patch)
2011-11-04 09:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
Follow-up fix (1.15 KB, patch)
2011-11-21 14:22 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Fix for Bug 7113 - Standardize vendor id name in templates and scripts (66.78 KB, patch)
2011-11-27 17:13 UTC, Katrin Fischer
Details | Diff | Splinter Review
Fix for Bug 7113 - Standardize vendor id name in templates and scripts (67.11 KB, patch)
2011-12-14 06:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Revised for current master (62.31 KB, patch)
2012-01-20 17:31 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 7113 [REVISED] Standardize vendor id name in templates and scripts (72.46 KB, patch)
2012-01-26 14:37 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7113: Standardize vendor id name in templates and scripts (72.65 KB, patch)
2012-01-27 07:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7113: Fix 'Edit vendor' link on basketgroup page (1.24 KB, patch)
2012-01-27 07:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7133: Late orders don't allow to select more than one order (1.63 KB, patch)
2012-01-27 07:34 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 7133: Late orders don't allow to select more than one order (2.17 KB, patch)
2012-01-27 07:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7113: Fix 'Edit vendor' link on basketgroup page (1.34 KB, patch)
2012-01-27 15:28 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7133: Late orders don't allow to select more than one order (2.28 KB, patch)
2012-01-27 16:05 UTC, Owen Leonard
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2011-10-28 18:43:05 UTC
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.
Comment 1 Owen Leonard 2011-10-28 20:13:12 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2011-10-29 14:11:28 UTC
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
Comment 3 Katrin Fischer 2011-10-29 14:37:24 UTC
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
Comment 4 Katrin Fischer 2011-10-29 14:41:40 UTC
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=
Comment 5 Katrin Fischer 2011-10-29 14:48:17 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2011-10-30 21:40:40 UTC
(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.
Comment 7 Owen Leonard 2011-10-31 14:30:08 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2011-10-31 14:41:18 UTC
(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
Comment 9 Katrin Fischer 2011-11-04 09:39:30 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2011-11-04 09:40:15 UTC
Thx Owen!
Comment 11 Ian Walls 2011-11-06 09:41:01 UTC
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
Comment 12 Owen Leonard 2011-11-21 14:22:28 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2011-11-27 17:13:27 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2011-11-27 17:14:20 UTC
Tested the 3rd patch on top of the 2 others and sqashed them into one.
Comment 15 Katrin Fischer 2011-12-14 06:05:22 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2012-01-19 11:07:30 UTC
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;
    }
Comment 17 Owen Leonard 2012-01-20 17:31:38 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2012-01-22 20:50:26 UTC
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.
Comment 19 Owen Leonard 2012-01-26 14:37:54 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2012-01-27 07:34:36 UTC
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'
Comment 21 Katrin Fischer 2012-01-27 07:34:51 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2012-01-27 07:34:58 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2012-01-27 07:51:44 UTC Comment hidden (obsolete)
Comment 24 Owen Leonard 2012-01-27 15:28:26 UTC
Created attachment 7355 [details] [review]
[SIGNED-OFF] Bug 7113: Fix 'Edit vendor' link on basketgroup page
Comment 25 Owen Leonard 2012-01-27 16:05:54 UTC
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.
Comment 26 Paul Poulain 2012-02-17 18:21:30 UTC
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
Comment 27 Katrin Fischer 2012-03-17 15:59:05 UTC
Works in master. Not to be pushed to 3.6