Some libraries would like to auto-populate item callnumbers for items on a record based on the system preference "itemcallnumber". Only items with empty callnumbers would affected. This would add a button "Populate empty item callnumbers" on catalogue/detail.pl, as well as a "Populate callnumber" button for each line in the holdings table to set the callnumber for that specific item.
Created attachment 155623 [details] [review] Bug 34784: Add REST API endpoints for updating single item or all applicable items on a bib Test Plan: 1) Apply this patch 2) prove t/db_dependent/api/v1/biblios.t
Created attachment 155624 [details] [review] Bug 34784: Implement feature in staff interface Some libraries would like to auto-populate item callnumbers for items on a record based on the system preference "itemcallnumber". Only items with empty callnumbers would affected. This would add a button "Populate empty item callnumbers" on catalogue/detail.pl, as well as a "Populate callnumber" button for each line in the holdings table to set the callnumber for that specific item. Test Plan: 1) Set the systempreference to something ( "245a" will make the callnumber the record title for testing purposes ) 2) Apply this patch set 3) Restart all the things! 4) Browse to a record details page, note no new buttons 5) Enable the new syspref EnablePopulateCallnumbers 5) Reload the record details, note the new "Populate callnumbers" button in the cataloging toolbar 7) Add one or more items with no callnumber to the record 8) Test the "Populate callnumbers" button 9) Add another item with no callnumber to the record 10) Test the "Populate callnumber" button for that single item
Created attachment 155626 [details] [review] Bug 34784: Add REST API endpoints for updating single item or all applicable items on a bib Test Plan: 1) Apply this patch 2) prove t/db_dependent/api/v1/biblios.t
Created attachment 155627 [details] [review] Bug 34784: Implement feature in staff interface Some libraries would like to auto-populate item callnumbers for items on a record based on the system preference "itemcallnumber". Only items with empty callnumbers would affected. This would add a button "Populate empty item callnumbers" on catalogue/detail.pl, as well as a "Populate callnumber" button for each line in the holdings table to set the callnumber for that specific item. Test Plan: 1) Set the systempreference to something ( "245a" will make the callnumber the record title for testing purposes ) 2) Apply this patch set 3) Restart all the things! 4) Browse to a record details page, note no new buttons 5) Enable the new syspref EnablePopulateCallnumbers 5) Reload the record details, note the new "Populate callnumbers" button in the cataloging toolbar 7) Add one or more items with no callnumber to the record 8) Test the "Populate callnumbers" button 9) Add another item with no callnumber to the record 10) Test the "Populate callnumber" button for that single item
Created attachment 156140 [details] [review] Bug 34784: Add REST API endpoints for updating single item or all applicable items on a bib Test Plan: 1) Apply this patch 2) prove t/db_dependent/api/v1/biblios.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 156141 [details] [review] Bug 34784: Implement feature in staff interface Some libraries would like to auto-populate item callnumbers for items on a record based on the system preference "itemcallnumber". Only items with empty callnumbers would affected. This would add a button "Populate empty item callnumbers" on catalogue/detail.pl, as well as a "Populate callnumber" button for each line in the holdings table to set the callnumber for that specific item. Test Plan: 1) Set the systempreference to something ( "245a" will make the callnumber the record title for testing purposes ) 2) Apply this patch set 3) Restart all the things! 4) Browse to a record details page, note no new buttons 5) Enable the new syspref EnablePopulateCallnumbers 5) Reload the record details, note the new "Populate callnumbers" button in the cataloging toolbar 7) Add one or more items with no callnumber to the record 8) Test the "Populate callnumbers" button 9) Add another item with no callnumber to the record 10) Test the "Populate callnumber" button for that single item Signed-off-by: David Nind <david@davidnind.com>
I'd like for someone who knows their way around the REST API a little better to have a second look here. Most of these are cosmetic and should be quick to follow-up: 1) Needs a follow-up to fix documentation. +=head3 populate_empty_callnumbers + +Controller function that handles deleting a biblio object 2) Spelling In the item form and the holding table we are using 'call number', but the patch uses 'callnumber'. I believe we should follow the GUI. See also: https://wiki.koha-community.org/wiki/Terminology 3) I feel we should include the call number that would be used in the confirmation question. if ( confirm(__('Are you sure you want to populate callnumbers for all items without a callnumber on this record?')) ) 4) Please no line breaks in translatable strings! This is a known issue with Pootle, not sure about Weblate, but translators often might not know what it is and accidentally remove it. msg += __('\nReload the page?'); let msg = __('Item populated with the callnumber "%s"\nReload the page?').format(callnumber); 4) Button in the holdings table The button for every single item in the holdings table takes up quite a bit of space. I wonder if we could find a way to make this work a little better. Maybe a split button with Edit as the main action? 5) Button in the detail page's toolbar (not blocker) Why not move this into the edit menu? 4) Maintenance script? (not blocker) I wonder about the use case to require such a one by one change. If you have itemcallnumber set, this would only apply to items that have been created before, that have been imported or otherwise added from outside of Koha. Are there plans for a maintenance script to do this in batch?
Created attachment 158554 [details] [review] Bug 34784: Add REST API endpoints for updating single item or all applicable items on a bib Test Plan: 1) Apply this patch 2) prove t/db_dependent/api/v1/biblios.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158555 [details] [review] Bug 34784: Implement feature in staff interface Some libraries would like to auto-populate item callnumbers for items on a record based on the system preference "itemcallnumber". Only items with empty callnumbers would affected. This would add a button "Populate empty item callnumbers" on catalogue/detail.pl, as well as a "Populate callnumber" button for each line in the holdings table to set the callnumber for that specific item. Test Plan: 1) Set the systempreference to something ( "245a" will make the callnumber the record title for testing purposes ) 2) Apply this patch set 3) Restart all the things! 4) Browse to a record details page, note no new buttons 5) Enable the new syspref EnablePopulateCallnumbers 5) Reload the record details, note the new "Populate callnumbers" button in the cataloging toolbar 7) Add one or more items with no callnumber to the record 8) Test the "Populate callnumbers" button 9) Add another item with no callnumber to the record 10) Test the "Populate callnumber" button for that single item Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158556 [details] [review] Bug 34784: (QA follow-up)
> Most of these are cosmetic and should be quick to follow-up: > > 1) Needs a follow-up to fix documentation. > > +=head3 populate_empty_callnumbers > + > +Controller function that handles deleting a biblio object Fixed! > 2) Spelling > > In the item form and the holding table we are using 'call number', but the > patch uses 'callnumber'. I believe we should follow the GUI. > > See also: https://wiki.koha-community.org/wiki/Terminology Fixed! > > 3) I feel we should include the call number that would be used in the > confirmation question. > > if ( confirm(__('Are you sure you want to populate callnumbers for all items > without a callnumber on this record?')) ) Wow, that was more work than I expected but I was able to make that happen! > > 4) Please no line breaks in translatable strings! > > This is a known issue with Pootle, not sure about Weblate, but translators > often might not know what it is and accidentally remove it. > > msg += __('\nReload the page?'); > let msg = __('Item populated with the callnumber "%s"\nReload the > page?').format(callnumber); I think I've fixed this based on your description. We should add a coding guideline for this. > > 4) Button in the holdings table > > The button for every single item in the holdings table takes up quite a bit > of space. I wonder if we could find a way to make this work a little better. > Maybe a split button with Edit as the main action? Done! > > 5) Button in the detail page's toolbar (not blocker) > > Why not move this into the edit menu? Done! > > > 4) Maintenance script? (not blocker) > > I wonder about the use case to require such a one by one change. If you have > itemcallnumber set, this would only apply to items that have been created > before, that have been imported or otherwise added from outside of Koha. > > Are there plans for a maintenance script to do this in batch? There are no plans for a maintenance script but I'd be happy to make one on a followup bug report if you can provide the parameters you'd like to see!
Created attachment 158569 [details] [review] Bug 34784: Add REST API endpoints for updating single item or all applicable items on a bib Test Plan: 1) Apply this patch 2) prove t/db_dependent/api/v1/biblios.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158570 [details] [review] Bug 34784: Implement feature in staff interface Some libraries would like to auto-populate item callnumbers for items on a record based on the system preference "itemcallnumber". Only items with empty callnumbers would affected. This would add a button "Populate empty item callnumbers" on catalogue/detail.pl, as well as a "Populate callnumber" button for each line in the holdings table to set the callnumber for that specific item. Test Plan: 1) Set the systempreference to something ( "245a" will make the callnumber the record title for testing purposes ) 2) Apply this patch set 3) Restart all the things! 4) Browse to a record details page, note no new buttons 5) Enable the new syspref EnablePopulateCallnumbers 5) Reload the record details, note the new "Populate callnumbers" button in the cataloging toolbar 7) Add one or more items with no callnumber to the record 8) Test the "Populate callnumbers" button 9) Add another item with no callnumber to the record 10) Test the "Populate callnumber" button for that single item Signed-off-by: David Nind <david@davidnind.com>
Created attachment 158571 [details] [review] Bug 34784: (QA follow-up)
Created attachment 158572 [details] [review] Bug 34784: (QA follow-up) Terminology: callnumber, biblio, syspref
(In reply to Kyle M Hall from comment #11) > > Most of these are cosmetic and should be quick to follow-up: > > > > 1) Needs a follow-up to fix documentation. > > > > +=head3 populate_empty_callnumbers > > + > > +Controller function that handles deleting a biblio object > > Fixed! Yes, but introducing more wrong terminology (see 2). > > 2) Spelling > > > > In the item form and the holding table we are using 'call number', but the > > patch uses 'callnumber'. I believe we should follow the GUI. > > > > See also: https://wiki.koha-community.org/wiki/Terminology > > Fixed! Not quite, see my follow-up. > > 3) I feel we should include the call number that would be used in the > > confirmation question. > > > > if ( confirm(__('Are you sure you want to populate callnumbers for all items > > without a callnumber on this record?')) ) > > Wow, that was more work than I expected but I was able to make that happen! Sorry, but I believe it's worth it! And it works nicely too. > > 4) Please no line breaks in translatable strings! > > > > This is a known issue with Pootle, not sure about Weblate, but translators > > often might not know what it is and accidentally remove it. > > > > msg += __('\nReload the page?'); > > let msg = __('Item populated with the callnumber "%s"\nReload the > > page?').format(callnumber); You did: + msg += " " + __('Reload the page?'); You could just have done this to keep your line break: + msg += "\n" + __('Reload the page?'); We can do line breaks, they just should not be with the translated string :) > I think I've fixed this based on your description. We should add a coding > guideline for this. I thought we had one, but it might be lost in time. I fixed a ton of those strings back in the day. This one goes in the right direction, even if it uses HTML formatting: JS3: Avoid strings which mix text and markup > > 4) Button in the holdings table > > > > The button for every single item in the holdings table takes up quite a bit > > of space. I wonder if we could find a way to make this work a little better. > > Maybe a split button with Edit as the main action? > Done! Yes, but it no longer works :( POST http://localhost:8081/api/v1/biblios/undefined/items/undefined/populate_empty_callnumbers The undefined don't look good. > > > > 5) Button in the detail page's toolbar (not blocker) > > > > Why not move this into the edit menu? > > Done! Yes, but it no longer works :( POST http://localhost:8081/api/v1/biblios/undefined/items/populate_empty_callnumbers > > > > 4) Maintenance script? (not blocker) > > > > I wonder about the use case to require such a one by one change. If you have > > itemcallnumber set, this would only apply to items that have been created > > before, that have been imported or otherwise added from outside of Koha. > > > > Are there plans for a maintenance script to do this in batch? > > There are no plans for a maintenance script but I'd be happy to make one on > a followup bug report if you can provide the parameters you'd like to see! I don't need one, I was just wondering if there was more planned around this.
Created attachment 158593 [details] [review] Bug 34784: (QA follow-up) Fix broken links, re-add newlines
> Yes, but introducing more wrong terminology (see 2). > Not quite, see my follow-up. Thanks for the follow up! > Sorry, but I believe it's worth it! And it works nicely too. Excellent :) > You could just have done this to keep your line break: > > + msg += "\n" + __('Reload the page?'); Thanks for the example, now I know exactly what you mean! > Yes, but it no longer works :( > POST > http://localhost:8081/api/v1/biblios/undefined/items/undefined/ > populate_empty_callnumbers > > The undefined don't look good. > Yes, but it no longer works :( > POST > http://localhost:8081/api/v1/biblios/undefined/items/ > populate_empty_callnumbers Fixed! > I don't need one, I was just wondering if there was more planned around this. This is an atomic enhancement, at least for the time being!
Created attachment 158627 [details] [review] Bug 34784: Add REST API endpoints for updating single item or all applicable items on a bib Test Plan: 1) Apply this patch 2) prove t/db_dependent/api/v1/biblios.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158628 [details] [review] Bug 34784: Implement feature in staff interface Some libraries would like to auto-populate item callnumbers for items on a record based on the system preference "itemcallnumber". Only items with empty callnumbers would affected. This would add a button "Populate empty item callnumbers" on catalogue/detail.pl, as well as a "Populate callnumber" button for each line in the holdings table to set the callnumber for that specific item. Test Plan: 1) Set the systempreference to something ( "245a" will make the callnumber the record title for testing purposes ) 2) Apply this patch set 3) Restart all the things! 4) Browse to a record details page, note no new buttons 5) Enable the new syspref EnablePopulateCallnumbers 5) Reload the record details, note the new "Populate callnumbers" button in the cataloging toolbar 7) Add one or more items with no callnumber to the record 8) Test the "Populate callnumbers" button 9) Add another item with no callnumber to the record 10) Test the "Populate callnumber" button for that single item Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158629 [details] [review] Bug 34784: (QA follow-up) Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158630 [details] [review] Bug 34784: (QA follow-up) Terminology: callnumber, biblio, syspref Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 158631 [details] [review] Bug 34784: (QA follow-up) Fix broken links, re-add newlines Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 160349 [details] [review] Bug 34784: (QA follow-up) Add additionalProperties to pass xt
Hi Kyle, I am sorry, I believe there are still issues here highlighted by the xt/api.t test. Referring to Tomas and Martin to have a look at the API part. As I also was the one to QA this, I think another set of eyes would be no mistake.
NOTE: Number of tests in t/db_dependent/api/v1/biblios.t also needs to be increased to 15 (had this fixed locally)
The endpoints introduced here feel very specific and RPC rather than REST.. I'm going to review with Tomas and see if we can come up with something a little more in keeping with the rest of the REST api's
(In reply to Martin Renvoize from comment #27) > The endpoints introduced here feel very specific and RPC rather than REST.. > I'm going to review with Tomas and see if we can come up with something a > little more in keeping with the rest of the REST api's I agree and have said something similar on private conversations with Kyle. What I proposed Kyle in that conversation, was that we already have something similar, and flexible enough so we can use the same pattern everywhere... POST /jobs. If we made this particular case process immediately, e.g. POST /jobs/bach_call_number_update could return a regular job object, with Location => /jobs/:job_id, and the usual payload found in the job object would just carry the needed information to avoid a second API call... He didn't like it that it is not really a background job. I mention this conversation in case it triggers some better idea to someone. I have thought about this a bit more, and I feel like we could do something like: POST /biblios/:biblio_id/batch_call_number_update which will return an object, representing the batch operation results (e.g. the items that got modified, those that failed if partial success is accepted, etc). It resembles too much what a 'job' is, hence my hesitation. We should really find some middle ground and move ahead with this cool feature. I don't entirely like the pattern, but it would be more RESTful. My two cents.
Thanks, Tomas. Kyle, can you take a look and let us know what you think? I certainly agree about the cool feature.
I'm going to resume this with a POC endpoint I've talked with Martin about introducing. I'd add that after the recent template changes this needs to be redone UI-wise.
Created attachment 173076 [details] [review] Bug 34784: API spec Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 173077 [details] [review] Bug 34784: Add RPC route for updating item callnumbers for a biblio This patch takes the idea implemented on the original patch, moves it to the /rpc namespace to make it clear this is not a RESTful endpoint. Code gets improved a bit, using some patterns we are using nowadays. The return value gets a bit more information and. A big change is it now uses `Koha::Items->batch_update`. It originally used `Koha::Items->update` with the `no_trigger` option. It felt like `batch_update` is a bit more smart in terms of triggering indexing at the end of the update, and only for the host biblio. It looks like that was the spirit of the original implementation, but missed to actually trigger indexing at the end of the call. To test: 1. Apply this patches 2.Run: $ ktd --shell k$ yarn api:bundle k$ restart_all 3. Run the tests: k$ prove t/db_dependent/api/v1/rpc/biblios.t => SUCCESS: Tests pass! 4. Try the new routes with Postman or your favourite tool (if they don't match) => SUCCESS: The desired behavior is what you get. 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Kyle, my promised two cents. I reworked the endpoint. Hope you agree and can adjust your UI implementation on top of it.
Created attachment 173078 [details] [review] Bug 34784: Add RPC route for updating item callnumbers for a biblio This patch takes the idea implemented on the original patch, moves it to the /rpc namespace to make it clear this is not a RESTful endpoint. Code gets improved a bit, using some patterns we are using nowadays. The return value gets a bit more information and. A big change is it now uses `Koha::Items->batch_update`. It originally used `Koha::Items->update` with the `no_trigger` option. It felt like `batch_update` is a bit more smart in terms of triggering indexing at the end of the update, and only for the host biblio. It looks like that was the spirit of the original implementation, but missed to actually trigger indexing at the end of the call. To test: 1. Apply this patches 2.Run: $ ktd --shell k$ yarn api:bundle k$ restart_all 3. Run the tests: k$ prove t/db_dependent/api/v1/rpc/biblios.t => SUCCESS: Tests pass! 4. Try the new routes with Postman or your favourite tool (if they don't match) => SUCCESS: The desired behavior is what you get. 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
The endpoint code and discussion is now moved to bug 38224. This patches should get the API portion stripped and resubmitted.
(In reply to Katrin Fischer from comment #7) > 4) Maintenance script? (not blocker) > > I wonder about the use case to require such a one by one change. If you have > itemcallnumber set, this would only apply to items that have been created > before, that have been imported or otherwise added from outside of Koha. > > Are there plans for a maintenance script to do this in batch? As I was rethinking the code, I filed bug 38224 in which I move the code into Koha::Biblio. I think it is worth filing a bug, depending on it, with a proposal for a maintenance script. Maybe someone can/wants to do it. So better provide a good spec.