In members.js line 112 variable guarantor_relationship is selected with id "relationship". Correct selector should however be class "relationship" since id "relationship" is used in auth-finder-search.inc, not in memberentry.pl.
Created attachment 139826 [details] [review] Use class "relationship" instead of id "relationship" in members.js In members.js line 112 variable guarantor_relationship is selected with id "relationship". Correct selector should however be class "relationship" since id "relationship" is used in auth-finder-search.inc, not in memberentry.pl. Sponsored-by: Koha-Suomi Oy
Hi Emmi, could you please add a test plan?
Created attachment 139939 [details] [review] Bug 31470: Use class "relationship" instead of id "relationship" in members.js In members.js line 112 variable guarantor_relationship is selected with id "relationship". Correct selector should however be class "relationship" since id "relationship" is used in auth-finder-search.inc, not in memberentry.pl. One way to see what kind of effect incorrect selector has (and how I originally stumbled into this), is to add into "IntranetUserJS" a line which changes non-patron guarantors relationship default selected value as other than empty. We use script to change "relationship" instead of "new_guarantor_relationship" because selector input for patron guarantors relationship actually uses default selected value from non-patron guarantors relationship. To test: 1. Make sure you have some value(s) in syspref "borrowerRelationship" e.g. "guarantor". 2. Add following line to "IntranetUseJS": $('.relationship option:eq(1)').attr('selected', 'selected'); 3. Add guarantee patron and note that non-patron guarantors relationship is now "guarantor". 4. Add patron guarantor and note that relationship value is empty. 5. Apply this patch and refresh page. 6. Repeat step 4. => Note that now relationship value is "guarantor". Sponsored-by: Koha-Suomi O
This highlights another problem. If relationship is going to a class in memberentry.pl then something must be done about its label: <label for="relationship">Relationship: </label> The value of the for attribute of the label element must be the ID, not a class.
(In reply to Lucas Gass from comment #4) > This highlights another problem. If relationship is going to a class in > memberentry.pl then something must be done about its label: > > <label for="relationship">Relationship: </label> > > The value of the for attribute of the label element must be the ID, not a > class. Hmm, you're right. So either we change ID from auth-finder-search.inc as something like "special_relationship" to avoid conflicts and change relationship in memberentry from class to ID. Or we add new ID to memberentry (e.g. non_patron_relationship). Latter is probably preferable solution. And we could use that ID in members.js instead of class relationship (which at hindsight could cause problems in future). And looking at this more closely, we have this same problem with other label as well: <label for="guarantor_relationship">Relationship: </label> <select class="new_guarantor_relationship" name="new_guarantor_relationship">
Emmi, In this bug should we change the ID in auth-finder-search.inc. Then we can file a new bug to deal with the label problems in memberentry.pl?
Created attachment 144570 [details] [review] Bug 31470: Add id "relationship" to memberentrygen.tt, change id "relationship" in auth-finder-search.inc In members.js line 112 variable guarantor_relationship is selected with id "relationship". However id "relationship" is used in auth-finder-search.inc, not in memberentrygen.tt. One way to see what kind of effect incorrect selector has (and how I originally stumbled into this), is to add into "IntranetUserJS" a line which changes non-patron guarantors relationship default selected value as other than empty. Script has to change "relationship" instead of "new_guarantor_relationship" because selector input for patron guarantors relationship actually uses default selected value from non-patron guarantors relationship. This patch changes two things. Firstly it adds id "relationship" to memberentrygen.tt so that member.js can fetch correct value to variable guarantor_relatioship. Secondly it changes id "relationship" in auth-finder-search.tt as "special_relationship" to avoid confusion. To test memberentrygen.tt: 1. Make sure you have some value(s) in syspref "borrowerRelationship" e.g. "guarantor". 2. Add following line to "IntranetUseJS": $('.relationship option:eq(1)').attr('selected', 'selected'); 3. Add guarantee patron and note that non-patron guarantors relationship is now "guarantor". 4. Add patron guarantor and note that relationship value is empty. 5. Apply this patch and refresh page. 6. Repeat step 4. => Note that now relationship value is "guarantor". To test auth-finder-search.inc: 1. Add authority e.g. Chronological Term 2. Modify field 551$a, click "Tag editor" icon from right 3. Searc pop-up is opened. => Make sure that field "Special relationship" is highlighted when you hover mouse over label. => Make sure that after performing a search, search value in "Special relationship" field persists. Sponsored-by: Koha-Suomi Oy
Made changes based on Lucas's comments.
(In reply to Emmi Takkinen from comment #7) > Created attachment 144570 [details] [review] [review] > Bug 31470: Add id "relationship" to memberentrygen.tt, change id > "relationship" in auth-finder-search.inc > > In members.js line 112 variable guarantor_relationship is selected > with id "relationship". However id "relationship" is used in > auth-finder-search.inc, not in memberentrygen.tt. > > One way to see what kind of effect incorrect selector has (and how I > originally stumbled into this), is to add into "IntranetUserJS" a line > which changes non-patron guarantors relationship default selected value as > other than empty. Script has to change "relationship" instead of > "new_guarantor_relationship" because selector input for patron guarantors > relationship actually uses default selected value from non-patron guarantors > relationship. > > This patch changes two things. Firstly it adds id "relationship" to > memberentrygen.tt so that member.js can fetch correct value > to variable guarantor_relatioship. Secondly it changes id "relationship" > in auth-finder-search.tt as "special_relationship" to avoid confusion. > > To test memberentrygen.tt: > 1. Make sure you have some value(s) in syspref "borrowerRelationship" e.g. > "guarantor". > 2. Add following line to "IntranetUseJS": $('.relationship > option:eq(1)').attr('selected', 'selected'); > 3. Add guarantee patron and note that non-patron guarantors relationship is > now "guarantor". > 4. Add patron guarantor and note that relationship value is empty. > 5. Apply this patch and refresh page. > 6. Repeat step 4. > => Note that now relationship value is "guarantor". > > To test auth-finder-search.inc: > 1. Add authority e.g. Chronological Term > 2. Modify field 551$a, click "Tag editor" icon from right > 3. Searc pop-up is opened. > => Make sure that field "Special relationship" is highlighted > when you hover mouse over label. > => Make sure that after performing a search, search value in > "Special relationship" field persists. > > Sponsored-by: Koha-Suomi Oy Signed-off-by: Laura Escamilla <laura.escamilla@bywatersolutions.com>
102 var guarantor_surname = $('#guarantor_surname').val(); 103 fieldset.find('.new_guarantor_surname').first().val( guarantor_surname ); 104 fieldset.find('.new_guarantor_surname_text').first().text( guarantor_surname ); 105 $('#guarantor_surname').val(""); 106 107 var guarantor_firstname = $('#guarantor_firstname').val(); 108 fieldset.find('.new_guarantor_firstname').first().val( guarantor_firstname ); 109 fieldset.find('.new_guarantor_firstname_text').first().text( guarantor_firstname ); 110 $('#guarantor_firstname').val(""); 111 112 var guarantor_relationship = $('#relationship').val(); 113 fieldset.find('.new_guarantor_relationship').first().val( guarantor_relationship ); 114 $('#relationship').find('option:eq(0)').prop('selected', true); This code is not clear to me, what are we trying to do? Is it really want we want? In my test I see an inconsistency: Edit a patron, fill "Non-patron guarantor" input and select Add guarantor, select an user => surname and firstname inputs are kept, but select is emptied
(In reply to Jonathan Druart from comment #10) > 102 var guarantor_surname = $('#guarantor_surname').val(); > 103 fieldset.find('.new_guarantor_surname').first().val( > guarantor_surname ); > 104 fieldset.find('.new_guarantor_surname_text').first().text( > guarantor_surname ); > 105 $('#guarantor_surname').val(""); > 106 > 107 var guarantor_firstname = $('#guarantor_firstname').val(); > 108 fieldset.find('.new_guarantor_firstname').first().val( > guarantor_firstname ); > 109 fieldset.find('.new_guarantor_firstname_text').first().text( > guarantor_firstname ); > 110 $('#guarantor_firstname').val(""); > 111 > 112 var guarantor_relationship = $('#relationship').val(); > 113 fieldset.find('.new_guarantor_relationship').first().val( > guarantor_relationship ); > 114 $('#relationship').find('option:eq(0)').prop('selected', true); > > > > This code is not clear to me, what are we trying to do? > Is it really want we want? > Those guarantor_surname and guarantor_firstname blocks set guarantors name to the form. Not sure what that guarantor_relationship is supposed to do, probably same thing if guarantor already exists (tested this it doesn't, no idea what it does). > In my test I see an inconsistency: > Edit a patron, fill "Non-patron guarantor" input and select > Add guarantor, select an user > => surname and firstname inputs are kept, but select is emptied Weird. And to add more weirdness to this, if I comment out that guarantor_relationship mentioned above, relationship remains in non-patron guarantor field set but disappears from patron guarantor :D
It's line 114 in members.js that sets non-patron relationship empty when patron guarantor is added. $('#relationship').find('option:eq(0)').prop('selected', true); It was added in bug 14570, maybe it is a fix for "There's no way to set the relationship". But since there hasn't actually been id "relationship" it hasn't done anything.
Created attachment 167432 [details] [review] Bug 31470: Add id "relationship" to memberentrygen.tt, change id "relationship" in auth-finder-search.inc In members.js line 112 variable guarantor_relationship is selected with id "relationship". However id "relationship" is used in auth-finder-search.inc, not in memberentrygen.tt. One way to see what kind of effect incorrect selector has (and how I originally stumbled into this), is to add into "IntranetUserJS" a line which changes non-patron guarantors relationship default selected value as other than empty. Script has to change "relationship" instead of "new_guarantor_relationship" because selector input for patron guarantors relationship actually uses default selected value from non-patron guarantors relationship. This patch changes two things. Firstly it adds id "relationship" to memberentrygen.tt so that member.js can fetch correct value to variable guarantor_relatioship. Secondly it changes id "relationship" in auth-finder-search.tt as "special_relationship" to avoid confusion. To test memberentrygen.tt: 1. Make sure you have some value(s) in syspref "borrowerRelationship" e.g. "guarantor". 2. Add following line to "IntranetUseJS": $('.relationship option:eq(1)').attr('selected', 'selected'); 3. Add guarantee patron and note that non-patron guarantors relationship is now "guarantor". 4. Add patron guarantor and note that relationship value is empty. 5. Apply this patch and refresh page. 6. Repeat step 4. => Note that now relationship value is "guarantor". To test auth-finder-search.inc: 1. Add authority e.g. Chronological Term 2. Modify field 551$a, click "Tag editor" icon from right 3. Searc pop-up is opened. => Make sure that field "Special relationship" is highlighted when you hover mouse over label. => Make sure that after performing a search, search value in "Special relationship" field persists. Sponsored-by: Koha-Suomi Oy
Created attachment 167433 [details] [review] Bug 31470: Do not change relationship value when adding a patron guarantor Adding new id "relationship" led to unexpected behaviour in relationship handling. If one edits patron with non-patron guarantor and added patron guarantor, non-patron guarantors relatioship field was set as empty value which is first option on select field. This patch removes code which sets relationship select field to use first value of option automatically when patron guarantor is added. Also thi patch add missing id to mandatory relatioship field. To test: 1. Add or edit patron. 2. Add non-patron guarantor for patron and select some value as their relationship. 3. Then add a patron guarantor. => Note that non-patron guarantors relationship is empty. 4. Apply this patch. 5. Repeat steps 1 to 3. => Relationship shouldn't change. Sponsored-by: Koha-Suomi Oy
Created attachment 167842 [details] [review] Bug 31470: Add id "relationship" to memberentrygen.tt, change id "relationship" in auth-finder-search.inc In members.js line 112 variable guarantor_relationship is selected with id "relationship". However id "relationship" is used in auth-finder-search.inc, not in memberentrygen.tt. One way to see what kind of effect incorrect selector has (and how I originally stumbled into this), is to add into "IntranetUserJS" a line which changes non-patron guarantors relationship default selected value as other than empty. Script has to change "relationship" instead of "new_guarantor_relationship" because selector input for patron guarantors relationship actually uses default selected value from non-patron guarantors relationship. This patch changes two things. Firstly it adds id "relationship" to memberentrygen.tt so that member.js can fetch correct value to variable guarantor_relatioship. Secondly it changes id "relationship" in auth-finder-search.tt as "special_relationship" to avoid confusion. To test memberentrygen.tt: 1. Make sure you have some value(s) in syspref "borrowerRelationship" e.g. "guarantor". 2. Add following line to "IntranetUseJS": $('.relationship option:eq(1)').attr('selected', 'selected'); 3. Add guarantee patron and note that non-patron guarantors relationship is now "guarantor". 4. Add patron guarantor and note that relationship value is empty. 5. Apply this patch and refresh page. 6. Repeat step 4. => Note that now relationship value is "guarantor". To test auth-finder-search.inc: 1. Add authority e.g. Chronological Term 2. Modify field 551$a, click "Tag editor" icon from right 3. Searc pop-up is opened. => Make sure that field "Special relationship" is highlighted when you hover mouse over label. => Make sure that after performing a search, search value in "Special relationship" field persists. Sponsored-by: Koha-Suomi Oy Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 167843 [details] [review] Bug 31470: Do not change relationship value when adding a patron guarantor Adding new id "relationship" led to unexpected behaviour in relationship handling. If one edits patron with non-patron guarantor and added patron guarantor, non-patron guarantors relatioship field was set as empty value which is first option on select field. This patch removes code which sets relationship select field to use first value of option automatically when patron guarantor is added. Also thi patch add missing id to mandatory relatioship field. To test: 1. Add or edit patron. 2. Add non-patron guarantor for patron and select some value as their relationship. 3. Then add a patron guarantor. => Note that non-patron guarantors relationship is empty. 4. Apply this patch. 5. Repeat steps 1 to 3. => Relationship shouldn't change. Sponsored-by: Koha-Suomi Oy Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 171946 [details] [review] Bug 31470: Add id "relationship" to memberentrygen.tt, change id "relationship" in auth-finder-search.inc In members.js line 112 variable guarantor_relationship is selected with id "relationship". However id "relationship" is used in auth-finder-search.inc, not in memberentrygen.tt. One way to see what kind of effect incorrect selector has (and how I originally stumbled into this), is to add into "IntranetUserJS" a line which changes non-patron guarantors relationship default selected value as other than empty. Script has to change "relationship" instead of "new_guarantor_relationship" because selector input for patron guarantors relationship actually uses default selected value from non-patron guarantors relationship. This patch changes two things. Firstly it adds id "relationship" to memberentrygen.tt so that member.js can fetch correct value to variable guarantor_relatioship. Secondly it changes id "relationship" in auth-finder-search.tt as "special_relationship" to avoid confusion. To test memberentrygen.tt: 1. Make sure you have some value(s) in syspref "borrowerRelationship" e.g. "guarantor". 2. Add following line to "IntranetUseJS": $('.relationship option:eq(1)').attr('selected', 'selected'); 3. Add guarantee patron and note that non-patron guarantors relationship is now "guarantor". 4. Add patron guarantor and note that relationship value is empty. 5. Apply this patch and refresh page. 6. Repeat step 4. => Note that now relationship value is "guarantor". To test auth-finder-search.inc: 1. Add authority e.g. Chronological Term 2. Modify field 551$a, click "Tag editor" icon from right 3. Searc pop-up is opened. => Make sure that field "Special relationship" is highlighted when you hover mouse over label. => Make sure that after performing a search, search value in "Special relationship" field persists. Sponsored-by: Koha-Suomi Oy Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>
Created attachment 171947 [details] [review] Bug 31470: Do not change relationship value when adding a patron guarantor Adding new id "relationship" led to unexpected behaviour in relationship handling. If one edits patron with non-patron guarantor and added patron guarantor, non-patron guarantors relatioship field was set as empty value which is first option on select field. This patch removes code which sets relationship select field to use first value of option automatically when patron guarantor is added. Also thi patch add missing id to mandatory relatioship field. To test: 1. Add or edit patron. 2. Add non-patron guarantor for patron and select some value as their relationship. 3. Then add a patron guarantor. => Note that non-patron guarantors relationship is empty. 4. Apply this patch. 5. Repeat steps 1 to 3. => Relationship shouldn't change. Sponsored-by: Koha-Suomi Oy Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>