Bug 35695 - Remove useless item group code from cataloging_additem.js
Summary: Remove useless item group code from cataloging_additem.js
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Lucas Gass (lukeg)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 33639
Blocks:
  Show dependency treegraph
 
Reported: 2024-01-03 22:55 UTC by Lucas Gass (lukeg)
Modified: 2024-12-20 21:52 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.03,23.05.09
Circulation function:


Attachments
Bug 35695: Remove useless JS from cataloging_additem.js (1.33 KB, patch)
2024-01-03 23:00 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 35695: Remove useless JS from cataloging_additem.js (1.38 KB, patch)
2024-01-05 21:03 UTC, David Nind
Details | Diff | Splinter Review
Bug 35695: Remove useless JS from cataloging_additem.js (1.44 KB, patch)
2024-01-25 15:34 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass (lukeg) 2024-01-03 22:55:53 UTC
As I worked on Bug 33639 I noticed some code in cataloging_additem.js that doesn't work/isn't needed:

137     $('#item-group-add-or-create-form-select').on('change', function() {
138         if ( ! $('input.items-enumchron').val() ) {
139             let item_group_selector = '#item-group-' + $(this).val();
140             let enumchron = $(item_group_selector).val();
141             $('input.items-enumchron').val( enumchron );
142         }
143     });


The selectors are bad and it would be easy to make them work. But I don't see any reason why item groups should be updating the items.enumchron field.
Comment 1 Lucas Gass (lukeg) 2024-01-03 23:00:40 UTC
Created attachment 160508 [details] [review]
Bug 35695: Remove useless JS from cataloging_additem.js

To test:
1. APPLY PATCH
2. Turn on EnableItemGroups
3. Go to a record with item groups, or create some.
4. Add an item, making sure items can be added to a group. ( Bug 33639 must be applied )

If I am wrong and this code should stay, tell me. I'll fix the jQuery instead of removing it. :)
Comment 2 David Nind 2024-01-05 21:03:51 UTC
Created attachment 160594 [details] [review]
Bug 35695: Remove useless JS from cataloging_additem.js

To test:
1. APPLY PATCH
2. Turn on EnableItemGroups
3. Go to a record with item groups, or create some.
4. Add an item, making sure items can be added to a group. ( Bug 33639 must be applied )

If I am wrong and this code should stay, tell me. I'll fix the jQuery instead of removing it. :)

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Nind 2024-01-05 21:10:37 UTC
Testing notes (using KTD):

1. I added and edited groups, and added items to groups (both from the normal page view, and when adding new items).

2. No differences or errors noted before and after the patch was applied.
Comment 4 Jonathan Druart 2024-01-11 15:02:59 UTC
Coming from:

commit f83c7f86553641063725ff0b10d1b3d29a3506a9
Author: Nick Clemens <nick@bywatersolutions.com>
Date:   Tue Mar 8 12:03:38 2022 +0000

    Bug 24857: Add ability to set item group when adding a new item
    
    During cataloging a user may wish to add an item to a group when
    creating a new item
    
    This patch also copies the group description to the enumchron field
    
    To test:
    1 - Browse to details page for a record
    2 - Create or ensure the record has item group(s)
    3 - Click New->New item
    4 - Note the bottom of the page has a form to attach to existing group, or create new
    5 - Note when a group is selected the enumchron field is populated
    6 - Confirm item is saved to group when saved


Did you ask Nick?
Comment 5 Lucas Gass (lukeg) 2024-01-11 15:26:13 UTC
> Did you ask Nick?

I did, we feel like this is leftover cruft from a previous iteration to item groups. Currently, the JS I removed doesn't work/do anything anyways. We think it should be removed.
Comment 6 Martin Renvoize (ashimema) 2024-01-25 15:34:17 UTC
Created attachment 161430 [details] [review]
Bug 35695: Remove useless JS from cataloging_additem.js

To test:
1. APPLY PATCH
2. Turn on EnableItemGroups
3. Go to a record with item groups, or create some.
4. Add an item, making sure items can be added to a group. ( Bug 33639 must be applied )

If I am wrong and this code should stay, tell me. I'll fix the jQuery instead of removing it. :)

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize (ashimema) 2024-01-25 15:34:33 UTC
I agree with the decision.. Passing QA
Comment 8 Katrin Fischer 2024-01-26 13:42:11 UTC
I tested one of the iterations and I remember seeing the update, but not agreeing with it. I don't think they should be connected in that way. So I am happy to have this removed.
Comment 9 Katrin Fischer 2024-01-26 14:17:59 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 10 Fridolin Somers 2024-01-31 13:29:21 UTC
Pushed to 23.11.x for 23.11.03
Comment 11 Lucas Gass (lukeg) 2024-02-09 20:21:06 UTC
Backported to 23.05.x for upcoming 23.05.09