Bug 31470

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>

I pushed some other relationship related patches today, so I tried to double check especially carefully here.

I notice that we still have this line:

koha-tmpl/intranet-tmpl/prog/js/members.js:        var guarantor_relationship = $('#relationship').val();

But you rename the id to special_relationship and I couldn't spot an id with relationship any more in my search. Can you please double check?

(In reply to Katrin Fischer from comment #19)
> I pushed some other relationship related patches today, so I tried to double
> check especially carefully here.
> 
> I notice that we still have this line:
> 
> koha-tmpl/intranet-tmpl/prog/js/members.js:        var
> guarantor_relationship = $('#relationship').val();
> 
> But you rename the id to special_relationship and I couldn't spot an id with
> relationship any more in my search. Can you please double check?

There is an id "relationship" in memberentrygen.tt. It was added in these patches. Id "special_character" is used auth-finder-search.inc.

(In reply to Emmi Takkinen from comment #20)
> (In reply to Katrin Fischer from comment #19)
> > I pushed some other relationship related patches today, so I tried to double
> > check especially carefully here.
> > 
> > I notice that we still have this line:
> > 
> > koha-tmpl/intranet-tmpl/prog/js/members.js:        var
> > guarantor_relationship = $('#relationship').val();
> > 
> > But you rename the id to special_relationship and I couldn't spot an id with
> > relationship any more in my search. Can you please double check?
> 
> There is an id "relationship" in memberentrygen.tt. It was added in these
> patches. Id "special_character" is used auth-finder-search.inc.

Thanks Emmi, I have missed that, but better safe than sorry :)

Pushed for 25.05!

Well done everyone, thank you!

Nice work everyone!

Pushed to 24.11.x for 24.11.01

Could you please add a release note?

I tried to get my head around this - what the problem is and how it is fixed (not technically, but what the effect is), but I think I need more coffee!