|
Description
Pedro Amorim
2024-02-27 17:15:17 UTC
Created attachment 162515 [details] [review] Bug 36182: Add booksellerid column to holdings table 1) Apply patch, reset_all 2) Configure holdings_table, visit: <staff_url>/cgi-bin/koha/admin/columns_settings.pl?module=catalogue&page=detail&table=holdings_table 3) Confirm holdings_booksellerid is there and is hidden by default 4) Visit a record: <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 5) Edit one of the items and add a value to field 'e' Source of acquisition 6) Go back to the record and click on the 'Columns' above the table 7) Click 'Source of acquisition'. Confirm it shows as expected Created attachment 162519 [details] [review] [23.11] Bug 36182: Add booksellerid column to holdings table 1) Apply patch, reset_all 2) Configure holdings_table, visit: <staff_url>/cgi-bin/koha/admin/columns_settings.pl?module=catalogue&page=detail&table=holdings_table 3) Confirm holdings_booksellerid is there and is hidden by default 4) Visit a record: <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 5) Edit one of the items and add a value to field 'e' Source of acquisition 6) Go back to the record and click on the 'Columns' above the table 7) Click 'Source of acquisition'. Confirm it shows as expected Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> [comment not intended as a blocker]
Hi Pedro,
I wonder if it would make sense to resolve the ID to the vendor's name. Could be a separate bug.
To explain:
Libraries not using the acq module like to write a vendor name in the field in the cataloguing editor as there is no plugin or similar to search for a vendor to create a proper link (there is a bug for that somewhere...)
For libraries using the acq module, Koha adds the booksellerid of the vendor automatically in the acquisition process.
Some libraries will have a mix of both (retro-cataloguing, migrated data, donations, etc.)
If we have the ID, we won't resolve to the vendor name with this patch and there is no easy way for the library to figure out the actual vendor. We neither allow searching by booksellerid, nor do we display the ID on the vendor's detail page. You can only see it in the URL.
I see that the items tab in the staff interface (moredetails.pl) uses this code:
if ($item_info->{'basketno'}){
my $basket = GetBasket($item_info->{'basketno'});
my $bookseller = Koha::Acquisition::Booksellers->find( $basket->{booksellerid} );
$item_info->{'vendor'} = $bookseller->name;
}
So if an order for the item exists, it gets resolved to the name, otherwise it remains as is.
Also something that could probably be improved on. But re-using the logic would give us a consistent display/behavior between items tab and holdings table.
Hi Katrin, thanks for the insight, definitely something to add as a follow-up. I'll take a look at adding it soon. (In reply to Katrin Fischer from comment #3) > For libraries using the acq module, Koha adds the booksellerid of the vendor > automatically in the acquisition process. I'm trying to go through with this use case but failing. Are you able to provide simple instructions on how to do it? I'm currently creating an order from a suggestion, it creates a biblio record but no item(s), and not with the vendor id in booksellerid. Thanks! I can't do a full run now, but when you create the basket, you need to select either "create on order" or "create on receive" I believe. I can try to test soon, but no guarantees for this week I am afraid :( (In reply to Jonathan Druart from comment #7) > On top of bug 33568 please. Ping Pedro Created attachment 165265 [details] [review] Bug 36182: Add booksellerid columns to holdings table 1) Apply patch, reset_all 2) Configure holdings_table, visit: <staff_url>/cgi-bin/koha/admin/columns_settings.pl?module=catalogue&page=detail&table=holdings_table 3) Confirm holdings_booksellerid is there and is hidden by default 4) Visit a record: <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 5) Edit one of the items and add a value to field 'e' Source of acquisition 5.5) You may need to click the 'show filters' link in order to have the 'Columns' table button show up 6) Go back to the record and click on the 'Columns' above the table 7) Click 'Source of acquisition'. Confirm it shows as expected Hi guys I've left the original patch as 23.11 for backport, re-tested, works as intended. Although this is a small enhancement and not a bugfix, would be great to have this backported if possible as it'd be one less bug that we'd have to backport manually to our customers. Submitted new patch based on master on top of bug 33568. (In reply to Katrin Fischer from comment #3) > [comment not intended as a blocker] > > Hi Pedro, > > I wonder if it would make sense to resolve the ID to the vendor's name. > Could be a separate bug. > > To explain: > Libraries not using the acq module like to write a vendor name in the field > in the cataloguing editor as there is no plugin or similar to search for a > vendor to create a proper link (there is a bug for that somewhere...) > For libraries using the acq module, Koha adds the booksellerid of the vendor > automatically in the acquisition process. > Some libraries will have a mix of both (retro-cataloguing, migrated data, > donations, etc.) > > If we have the ID, we won't resolve to the vendor name with this patch and > there is no easy way for the library to figure out the actual vendor. We > neither allow searching by booksellerid, nor do we display the ID on the > vendor's detail page. You can only see it in the URL. > > I see that the items tab in the staff interface (moredetails.pl) uses this > code: > > if ($item_info->{'basketno'}){ > my $basket = GetBasket($item_info->{'basketno'}); > my $bookseller = Koha::Acquisition::Booksellers->find( > $basket->{booksellerid} ); > $item_info->{'vendor'} = $bookseller->name; > } > > So if an order for the item exists, it gets resolved to the name, otherwise > it remains as is. > > Also something that could probably be improved on. But re-using the logic > would give us a consistent display/behavior between items tab and holdings > table. Would it be okay if this is handled in a different bug as further enhancement with a detailed test plan? I'm still having a hard time reproducing this as I don't think I'm experienced enough in acquisitions to get this use-case going. Thanks. Needs rebased now that we have Bug 36182. (In reply to Lucas Gass from comment #11) > Needs rebased now that we have Bug 36182. Patch applies for me, double-checking: have you applied interactively and commented the [23.11] one? (In reply to Pedro Amorim from comment #12) > (In reply to Lucas Gass from comment #11) > > Needs rebased now that we have Bug 36182. > > Patch applies for me, double-checking: have you applied interactively and > commented the [23.11] one? My mistake, I only noticed the 23.11 patch. Created attachment 165485 [details] [review] Bug 36182: Add booksellerid columns to holdings table 1) Apply patch, reset_all 2) Configure holdings_table, visit: <staff_url>/cgi-bin/koha/admin/columns_settings.pl?module=catalogue&page=detail&table=holdings_table 3) Confirm holdings_booksellerid is there and is hidden by default 4) Visit a record: <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 5) Edit one of the items and add a value to field 'e' Source of acquisition 5.5) You may need to click the 'show filters' link in order to have the 'Columns' table button show up 6) Go back to the record and click on the 'Columns' above the table 7) Click 'Source of acquisition'. Confirm it shows as expected Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> *** Bug 36747 has been marked as a duplicate of this bug. *** This will still only display the vendor ID for all libraries using the acquisition module with item creation. I think as such it's not really useful right now and the change requested would not add much extra work. See: bug 36750 (In reply to Keith Dembek from comment #17) > +1 for me, in our system I'm probably the only current staff member who > would know a vendor #, so having the name would be very helpful for other > staff! (In reply to Katrin Fischer from comment #16) > This will still only display the vendor ID for all libraries using the > acquisition module with item creation. I think as such it's not really > useful right now and the change requested would not add much extra work. > > See: bug 36750 > (In reply to Keith Dembek from comment #17) > > +1 for me, in our system I'm probably the only current staff member who > > would know a vendor #, so having the name would be very helpful for other > > staff! Is this referring to the below? I wonder if it would make sense to resolve the ID to the vendor's name. Could be a separate bug. Could I please have a test plan for this? I understand the requirement but don't know how to "link" the stuff that's needed here to come up with this. I checked in our production systems with acq and the booksellerid is always set to the vendor id. So this should work: 1) Create a new basket, make sure you set item creation to "on order" 2) Create an order line with one or more items 3) Close the basket 4) Receive shipment for that vendor 5) Receive your order line with the item 6) Check the item - some fields should have been set by the acq module, like price and booksellerid. I feel the feature as suggested is not useful and might be confusing to a lot of libraries even. I made a suggestion on how to resolve to vendor name in comment#3, it's the same code used on the items tab (moredetails.pl), so it would give us consistency to use the same logic. I was hoping to spot the logic in a proper module, but... *sigh* https://git.bsz-bw.de/koha/koha/-/blob/master/acqui/finishreceive.pl#L181 my $new_order_object = Koha::Acquisition::Orders->find( $new_ordernumber ); # FIXME we should not need to refetch it my $items = $new_order_object->items; while ( my $item = $items->next ) { $item->update({ booksellerid => $booksellerid, dateaccessioned => $datereceived, datelastseen => $datereceived, price => $unitprice, replacementprice => $replacementprice, replacementpricedate => $datereceived, }); } Created attachment 166276 [details] [review] Bug 36182: Show vendor name if acquisition_source is a vendor_id This will make it so if acquisition_source is of an existing vendor_id, it'll show the vendor name instead of the id. If it's not of an existing vendor_id, show the raw value instead. I'm not 100% happy with this patch as it adds an edge case to Item::strings_map but I believe the only alternative is to make 952$e Source of acquisition an AV field in the default framework. Not only that, but this would have to be an AV field of type 'vendors' that'd have to allow to pick from existing vendors (similar to current 'branches'). The above is a ton more work than initial suggestions would imply. Happy to be proven wrong (in fact, hoping I am). This is compatible with main Koha only, not backportable to 23.11 Created attachment 166277 [details] Receive orderline with item (In reply to Katrin Fischer from comment #18) > I checked in our production systems with acq and the booksellerid is always > set to the vendor id. So this should work: > > 1) Create a new basket, make sure you set item creation to "on order" > 2) Create an order line with one or more items > 3) Close the basket > 4) Receive shipment for that vendor > 5) Receive your order line with the item > 6) Check the item - some fields should have been set by the acq module, like > price and booksellerid. Couldn't go through step 5, the items table shows empty and the "Confirm" button disabled, but went ahead with the development anyway changing data manually to an existing vendor_id. Comment on attachment 166277 [details]
Receive orderline with item
That doesn't look right. I hope we didn't break something :( You should see your items on the left side if you have created them "on order" within the table.
To unlock the button you'd then need to checkbox an item in the table OR to add something in "quantity received".
This still applies. Note that when applying on main, skip the [23.11] patch. Likewise, when applying on 23.11.x, apply only the [23.11] patch. I don't want to make a mess here, could you please rebase? Happy to have another look then! (In reply to Katrin Fischer from comment #23) > I don't want to make a mess here, could you please rebase? > Happy to have another look then! This still applies. Note that when applying on main, skip the [23.11] patch. Likewise, when applying on 23.11.x, apply only the [23.11] patch. That's probably what I missed. Will have another go once we got Jenkins happy again. (In reply to Katrin Fischer from comment #25) > That's probably what I missed. Will have another go once we got Jenkins > happy again. Sorry, I had missed that one. Having a look now. The patch no longer applies, I'll keep the rel_24_11_candidate as this is "mostly display" bug. Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 36182: Add booksellerid column to holdings table Using index info to reconstruct a base tree... M admin/columns_settings.yml M koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt Falling back to patching base and 3-way merge... Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/detail.tt Auto-merging admin/columns_settings.yml CONFLICT (content): Merge conflict in admin/columns_settings.yml error: Failed to merge in the changes. Patch failed at 0001 Bug 36182: Add booksellerid column to holdings table hint: Use 'git am --show-current-patch=diff' to see the failed 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/2311-Bug-36182-Add-booksellerid-column-to-holdings-ujf64c3n.patch Created attachment 174399 [details] [review] Bug 36182: Add booksellerid columns to holdings table 1) Apply patch, reset_all 2) Configure holdings_table, visit: <staff_url>/cgi-bin/koha/admin/columns_settings.pl?module=catalogue&page=detail&table=holdings_table 3) Confirm holdings_booksellerid is there and is hidden by default 4) Visit a record: <staff_url>/cgi-bin/koha/catalogue/detail.pl?biblionumber=76 5) Edit one of the items and add a value to field 'e' Source of acquisition 5.5) You may need to click the 'show filters' link in order to have the 'Columns' table button show up 6) Go back to the record and click on the 'Columns' above the table 7) Click 'Source of acquisition'. Confirm it shows as expected Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 174400 [details] [review] Bug 36182: Show vendor name if acquisition_source is a vendor_id This will make it so if acquisition_source is of an existing vendor_id, it'll show the vendor name instead of the id. If it's not of an existing vendor_id, show the raw value instead. I'm not 100% happy with this patch as it adds an edge case to Item::strings_map but I believe the only alternative is to make 952$e Source of acquisition an AV field in the default framework. Not only that, but this would have to be an AV field of type 'vendors' that'd have to allow to pick from existing vendors (similar to current 'branches'). The above is a ton more work than initial suggestions would imply. Happy to be proven wrong (in fact, hoping I am). This is compatible with main Koha only, not backportable to 23.11 Updated from "Doesnt apply" to PQA but it was never PQA, it was SO. Changing it back. (In reply to Pedro Amorim from comment #30) > Updated from "Doesnt apply" to PQA but it was never PQA, it was SO. Changing > it back. I had looked at this one last when taking things out of the QA queue as mine was empty... similar situation now. I will come back for this later :) Tested and works, will push to main. Pushed for 24.11! Well done everyone, thank you! Is this really working? It's hidden if there is no data, but it's displayed if there is at least one item with a source of acq. Looks broken for the other columns as well but I am wondering why this has not been caught here. (In reply to Jonathan Druart from comment #34) > Is this really working? It's hidden if there is no data, but it's displayed > if there is at least one item with a source of acq. > > Looks broken for the other columns as well but I am wondering why this has > not been caught here. Hm I thought that it was hidden by default was expected. I made it visible for testing and then tested with values linked to acq (booksellerid) and plain text entries. I'll need to take another look at what I missed there. |