Bug 32933 - Use val() instead of attr("value") when getting field values with jQuery
Summary: Use val() instead of attr("value") when getting field values with jQuery
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Owen Leonard
QA Contact: Katrin Fischer
URL:
Keywords:
: 32936 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-02-09 16:48 UTC by Owen Leonard
Modified: 2023-12-28 20:43 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.04,22.05.11,21.11.19


Attachments
Bug 32933: Use val() instead of attr("value") when getting field values with jQuery (12.02 KB, patch)
2023-02-09 17:09 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 32933: Use val() instead of attr("value") when getting field values with jQuery (12.10 KB, patch)
2023-02-13 20:56 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 32933: Use val() instead of attr("value") when getting field values with jQuery (12.16 KB, patch)
2023-02-20 21:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 32933: [22.05.x] Use val() instead of attr("value") when getting field values with jQuery (12.03 KB, patch)
2023-03-02 12:30 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 2023-02-09 16:48:56 UTC
This issue came up because I noticed that the tag review page's "test" feature wasn't working. Clicking the test button didn't trigger any message about the tag's approval status.

$(input).attr("value") is getting the initial contents of the "value" attribute when the DOM initially loads. In the case of the tags page, this will always be an empty string.

Using $(input).val() gets the current value of the field.

I think other instances of this code aren't buggy in the same way, but I think it's worth updating all of them for consistency.
Comment 1 Owen Leonard 2023-02-09 17:09:39 UTC
Created attachment 146444 [details] [review]
Bug 32933: Use val() instead of attr("value") when getting field values with jQuery

This patch replaces instances in Koha's templates where "attr('value')" is
used to get the value of a form field, using "val()" instead. This
ensures that we get the current value of the field, not just the
value of the field when the page first loaded.

To test, apply the patch and test the following pages:

- Tools -> Tags. Use the "Check lists" form in the right-hand sidebar
  to test the approval status of some approved and rejected tags. It
  should correctly respond with the tag's status.
- Cataloging -> Label creator.
  - New -> Printer profile. Each offset and creep field should correctly
    reflect the selected value in the "Units" dropdown. Changing the
    dropdown should make the units change accordingly.
  - New -> Label template. Perform the same tests as above.
- Tools -> Patron card creator.
  - New -> Layout. If necessary, check the "Field 1" checkbox. The
    units should be displayed correctly and change as you change the
    "Units" dropdown.
  - New -> Printer profile,
  - New -> Card template: Perform the same tests as above.
- Lists -> List contents.
  - Check that the "Add to" dropdown menu works for adding selected
    titles to a cart or another list.
- Back record modification. Submit a batch of records for modification
  and go to Administration -> Manage jobs.
  - View the details of the job you just submitted.
  - Confirm that the "Add modified records to the following list"
    dropdown works correctly.
- Cataloging -> Find a bibliographic record -> Edit items.
  - Confirm that clicking a cell in the table of existing items
    triggers the "Edit item/Delete item" menu, and that these links
    work correctly.
Comment 2 Alex Buckley 2023-02-13 20:33:57 UTC
*** Bug 32936 has been marked as a duplicate of this bug. ***
Comment 3 Alex Buckley 2023-02-13 20:56:39 UTC
Created attachment 146601 [details] [review]
Bug 32933: Use val() instead of attr("value") when getting field values with jQuery

This patch replaces instances in Koha's templates where "attr('value')" is
used to get the value of a form field, using "val()" instead. This
ensures that we get the current value of the field, not just the
value of the field when the page first loaded.

To test, apply the patch and test the following pages:

- Tools -> Tags. Use the "Check lists" form in the right-hand sidebar
  to test the approval status of some approved and rejected tags. It
  should correctly respond with the tag's status.
- Cataloging -> Label creator.
  - New -> Printer profile. Each offset and creep field should correctly
    reflect the selected value in the "Units" dropdown. Changing the
    dropdown should make the units change accordingly.
  - New -> Label template. Perform the same tests as above.
- Tools -> Patron card creator.
  - New -> Layout. If necessary, check the "Field 1" checkbox. The
    units should be displayed correctly and change as you change the
    "Units" dropdown.
  - New -> Printer profile,
  - New -> Card template: Perform the same tests as above.
- Lists -> List contents.
  - Check that the "Add to" dropdown menu works for adding selected
    titles to a cart or another list.
- Back record modification. Submit a batch of records for modification
  and go to Administration -> Manage jobs.
  - View the details of the job you just submitted.
  - Confirm that the "Add modified records to the following list"
    dropdown works correctly.
- Cataloging -> Find a bibliographic record -> Edit items.
  - Confirm that clicking a cell in the table of existing items
    triggers the "Edit item/Delete item" menu, and that these links
    work correctly.

Works as described.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 4 Alex Buckley 2023-02-13 21:00:36 UTC
Hi Owen, 

I have a question from testing this patch, when I do:

1. Navigate to: Tools -> Patron card creator > New -> Layout:
2. Check "Field 1" checkbox
3. Change the value in the "Units" dropdown
4. I observed that the unit beside the 'Font size' input box does not get updated - it stays as 'pt'.

My question is should the "Font size" unit also update given #font_size is listed here: https://github.com/Koha-Community/Koha/blob/178e40ae9fd7246fa638a64c34fde06f94548ff0/koha-tmpl/intranet-tmpl/prog/en/modules/patroncards/edit-layout.tt#L503
Comment 5 Owen Leonard 2023-02-14 15:22:36 UTC
(In reply to Alex Buckley from comment #4)
> My question is should the "Font size" unit also update given #font_size is
> listed here

I assumed based on the behavior in master that changing the "global" units selection should not affect the font size inputs. The "#font_size" ID doesn't exist on its own in the template, which makes me wonder if the entry is a relic of a previous version.
Comment 6 Alex Buckley 2023-02-14 22:19:31 UTC
(In reply to Owen Leonard from comment #5)
> (In reply to Alex Buckley from comment #4)
> > My question is should the "Font size" unit also update given #font_size is
> > listed here
> 
> I assumed based on the behavior in master that changing the "global" units
> selection should not affect the font size inputs. The "#font_size" ID
> doesn't exist on its own in the template, which makes me wonder if the entry
> is a relic of a previous version.

Yes, that sounds fair enough. Thank you Owen.
Comment 7 Katrin Fischer 2023-02-20 21:49:47 UTC
 
> I think other instances of this code aren't buggy in the same way, but I
> think it's worth updating all of them for consistency.

Should these (or some of these) be fixed as well?


koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-overlay-rules.tt:            .attr('value', op)
koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-overlay-rules.tt:                .attr('value', id)
koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-overlay-rules.tt:            doSubmit('doedit', $("#doedit").attr('value'));
koha-tmpl/intranet-tmpl/prog/en/modules/admin/oai_set_mappings.tt:              tr.find('input[type="text"]').attr("value","");
koha-tmpl/intranet-tmpl/prog/en/modules/admin/systempreferences.tt:                    $('#preftype').attr("value",$(this).attr("id"));
koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt:        $("#confirm_not_duplicate").attr("value","1");
koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt:                        opt.attr("value", filter.search_filter_id);
koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/search-history.tt:                        .attr('value', $(this).attr('value'));
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt:        $("#redirect").attr("value",dest);
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt:        $("#confirm_not_duplicate").attr("value","1");
koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt:        $("#redirect").attr("value",redirect);
koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt:                let amount_outstanding = parseFloat( $('#amountoutstanding').attr('value') );
koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt:                let amount_writeoff = parseFloat( $('#amountwrittenoff').attr('value') );
koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt:                                  .attr('value', action)
koha-tmpl/intranet-tmpl/prog/en/modules/tools/stage-marc-import.tt:                    opt.attr("value", profile.profile_id);
koha-tmpl/intranet-tmpl/prog/js/members.js:    $("input#patron_attr_" + newId, clone).attr('value','');
koha-tmpl/intranet-tmpl/prog/js/members.js:    $("select#patron_attr_" + newId, clone).attr('value','');
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:                    .find('textarea').attr('value', '').end()
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:                    .find('select').attr('value', '').end();
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:                $('input[type!="hidden"],select,textarea', clone).attr('id', 'patron-attr-' + newId).attr('value', '');
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:                $("label", clone).attr('for', 'patron-attr-' + newId).attr('value', '');
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt:                $("#hold_form_biblios").attr("value", numList);

I used: git grep "\.attr(.v"
Comment 8 Katrin Fischer 2023-02-20 21:51:43 UTC
Created attachment 146978 [details] [review]
Bug 32933: Use val() instead of attr("value") when getting field values with jQuery

This patch replaces instances in Koha's templates where "attr('value')" is
used to get the value of a form field, using "val()" instead. This
ensures that we get the current value of the field, not just the
value of the field when the page first loaded.

To test, apply the patch and test the following pages:

- Tools -> Tags. Use the "Check lists" form in the right-hand sidebar
  to test the approval status of some approved and rejected tags. It
  should correctly respond with the tag's status.
- Cataloging -> Label creator.
  - New -> Printer profile. Each offset and creep field should correctly
    reflect the selected value in the "Units" dropdown. Changing the
    dropdown should make the units change accordingly.
  - New -> Label template. Perform the same tests as above.
- Tools -> Patron card creator.
  - New -> Layout. If necessary, check the "Field 1" checkbox. The
    units should be displayed correctly and change as you change the
    "Units" dropdown.
  - New -> Printer profile,
  - New -> Card template: Perform the same tests as above.
- Lists -> List contents.
  - Check that the "Add to" dropdown menu works for adding selected
    titles to a cart or another list.
- Back record modification. Submit a batch of records for modification
  and go to Administration -> Manage jobs.
  - View the details of the job you just submitted.
  - Confirm that the "Add modified records to the following list"
    dropdown works correctly.
- Cataloging -> Find a bibliographic record -> Edit items.
  - Confirm that clicking a cell in the table of existing items
    triggers the "Edit item/Delete item" menu, and that these links
    work correctly.

Works as described.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Katrin Fischer 2023-02-20 21:52:33 UTC
Decided to not hold this one up, as it has nice test plans and all that. Owen, could you please verify if we should have a 'part 2' type of bug to fix the other occurences?
>  
> > I think other instances of this code aren't buggy in the same way, but I
> > think it's worth updating all of them for consistency.
> 
> Should these (or some of these) be fixed as well?
> 
> 
> koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-overlay-rules.tt:        
> .attr('value', op)
> koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-overlay-rules.tt:        
> .attr('value', id)
> koha-tmpl/intranet-tmpl/prog/en/modules/admin/marc-overlay-rules.tt:        
> doSubmit('doedit', $("#doedit").attr('value'));
> koha-tmpl/intranet-tmpl/prog/en/modules/admin/oai_set_mappings.tt:          
> tr.find('input[type="text"]').attr("value","");
> koha-tmpl/intranet-tmpl/prog/en/modules/admin/systempreferences.tt:         
> $('#preftype').attr("value",$(this).attr("id"));
> koha-tmpl/intranet-tmpl/prog/en/modules/authorities/authorities.tt:       
> $("#confirm_not_duplicate").attr("value","1");
> koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt:               
> opt.attr("value", filter.search_filter_id);
> koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/search-history.tt:        
> .attr('value', $(this).attr('value'));
> koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt:       
> $("#redirect").attr("value",dest);
> koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt:       
> $("#confirm_not_duplicate").attr("value","1");
> koha-tmpl/intranet-tmpl/prog/en/modules/cataloguing/addbiblio.tt:       
> $("#redirect").attr("value",redirect);
> koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt:              
> let amount_outstanding = parseFloat( $('#amountoutstanding').attr('value') );
> koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt:              
> let amount_writeoff = parseFloat( $('#amountwrittenoff').attr('value') );
> koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt:           
> .attr('value', action)
> koha-tmpl/intranet-tmpl/prog/en/modules/tools/stage-marc-import.tt:         
> opt.attr("value", profile.profile_id);
> koha-tmpl/intranet-tmpl/prog/js/members.js:    $("input#patron_attr_" +
> newId, clone).attr('value','');
> koha-tmpl/intranet-tmpl/prog/js/members.js:    $("select#patron_attr_" +
> newId, clone).attr('value','');
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:               
> .find('textarea').attr('value', '').end()
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:               
> .find('select').attr('value', '').end();
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:               
> $('input[type!="hidden"],select,textarea', clone).attr('id', 'patron-attr-'
> + newId).attr('value', '');
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-memberentry.tt:               
> $("label", clone).attr('for', 'patron-attr-' + newId).attr('value', '');
> koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-results.tt:               
> $("#hold_form_biblios").attr("value", numList);
> 
> I used: git grep "\.attr(.v"
Comment 10 Tomás Cohen Arazi 2023-02-21 12:18:37 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 11 Jacob O'Mara 2023-02-24 11:33:15 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 12 Jacob O'Mara 2023-02-24 11:36:27 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 13 Lucas Gass 2023-03-01 14:56:12 UTC
Lots of merge conflicts here with 22.05.x, no backport.
Comment 14 Owen Leonard 2023-03-02 12:30:12 UTC
Created attachment 147605 [details] [review]
Bug 32933: [22.05.x] Use val() instead of attr("value") when getting field values with jQuery

This patch replaces instances in Koha's templates where "attr('value')" is
used to get the value of a form field, using "val()" instead. This
ensures that we get the current value of the field, not just the
value of the field when the page first loaded.

To test, apply the patch and test the following pages:

- Tools -> Tags. Use the "Check lists" form in the right-hand sidebar
  to test the approval status of some approved and rejected tags. It
  should correctly respond with the tag's status.
- Cataloging -> Label creator.
  - New -> Printer profile. Each offset and creep field should correctly
    reflect the selected value in the "Units" dropdown. Changing the
    dropdown should make the units change accordingly.
  - New -> Label template. Perform the same tests as above.
- Tools -> Patron card creator.
  - New -> Layout. If necessary, check the "Field 1" checkbox. The
    units should be displayed correctly and change as you change the
    "Units" dropdown.
  - New -> Printer profile,
  - New -> Card template: Perform the same tests as above.
- Lists -> List contents.
  - Check that the "Add to" dropdown menu works for adding selected
    titles to a cart or another list.
- Back record modification. Submit a batch of records for modification
  and go to Administration -> Manage jobs.
  - View the details of the job you just submitted.
  - Confirm that the "Add modified records to the following list"
    dropdown works correctly.
- Cataloging -> Find a bibliographic record -> Edit items.
  - Confirm that clicking a cell in the table of existing items
    triggers the "Edit item/Delete item" menu, and that these links
    work correctly.

Works as described.

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Owen Leonard 2023-03-02 12:34:33 UTC
I rebased this for 22.05.x because the issue that originally prompted the patch (using the "Check lists" form in tags) exists in that version too.
Comment 16 Lucas Gass 2023-03-02 14:06:13 UTC
Thanks Owen! Backported to 22.05.x for upcoming 22.05.11
Comment 17 Arthur Suzuki 2023-03-29 12:09:19 UTC
applied to 21.11.x for 21.11.19
Comment 18 wainuiwitikapark 2023-04-27 04:32:03 UTC
Not backported to 21.05.x