Bug 22728 - Manual for BorrowerMandatoryField should include note on extended attributes
Summary: Manual for BorrowerMandatoryField should include note on extended attributes
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Documentation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on: 22844
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-17 17:33 UTC by Andrew Fuerste-Henry
Modified: 2021-06-14 21:28 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2019-04-17 17:33:59 UTC
In explaining BorrowerMandatoryField, we should include a note on how to denote extended patron attributes. They can be made mandatory by entering "patron_attr_1" (or other correct number), but I don't see that documented anywhere.
Comment 1 Owen Leonard 2019-05-13 18:24:10 UTC
A more user-friendly solution might be to add a "mandatory" option to patron attributes.
Comment 2 Jonathan Druart 2019-05-13 23:18:47 UTC
When has it been added, do you have a bug number?
It does not seem to work for me, I have 2 patron attributes, added patron_attr_1 to BorrowerMandatoryField and nothing marked as required on the patron edit form.
Also I do not see the code that could deal with that.
Comment 3 Jonathan Druart 2019-05-13 23:22:47 UTC
Ha, maybe this (js/members.js):

281     var mandatory_fields = $("input[name='BorrowerMandatoryField']").val().split ('|');
282     $(mandatory_fields).each(function(){ 
283         $("[name='"+this+"']").attr('required', 'required');
284     });
Comment 4 Jonathan Druart 2019-05-13 23:25:03 UTC
So I think it's a side-effect, not a feature :)
Comment 5 Jonathan Druart 2019-05-13 23:29:11 UTC
(In reply to Jonathan Druart from comment #2)
> When has it been added, do you have a bug number?
> It does not seem to work for me, I have 2 patron attributes, added
> patron_attr_1 to BorrowerMandatoryField and nothing marked as required on
> the patron edit form.

It actually works, but the input is not marked as required (class "required") like other required inputs. The form submission is indeed prevented when one of them is not filled.
Comment 6 Katrin Fischer 2019-05-14 06:02:03 UTC
As you said, it appears more like a lucky side effect, but hard to tell how many libraries may be using it. Could we replace it with a proper implementation? I think a 'mandatory' flag in the patron attribute definition would make more sense.
Comment 7 Owen Leonard 2019-05-14 11:35:29 UTC
(In reply to Jonathan Druart from comment #5)
> It actually works, but the input is not marked as required (class
> "required") like other required inputs.

I saw this too and considered "fixing" it, but I'm not sure it's worth the time if we can come up with a better implementation.
Comment 8 Jonathan Druart 2019-05-14 13:41:58 UTC
(In reply to Katrin Fischer from comment #6)
> As you said, it appears more like a lucky side effect, but hard to tell how
> many libraries may be using it. Could we replace it with a proper
> implementation? I think a 'mandatory' flag in the patron attribute
> definition would make more sense.

Why would it make more sense? I think that we should deal with them in the pref, as it is where we deal with mandatory fields.
Comment 9 Andrew Fuerste-Henry 2019-05-14 13:58:25 UTC
Complicating things a bit, bug 22844 changes the interface for borrower mandatory fields and may close the loophole making mandatory attributes possible. Katrin linked here from that bug, figured I may as well link in the other direction, too.
Comment 10 Jonathan Druart 2019-05-14 14:12:01 UTC
(In reply to Andrew from comment #9)
> Complicating things a bit, bug 22844 changes the interface for borrower
> mandatory fields and may close the loophole making mandatory attributes
> possible. Katrin linked here from that bug, figured I may as well link in
> the other direction, too.

Yes, and that's why I told that as well :)
Comment 11 Katrin Fischer 2019-05-14 14:23:09 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to Katrin Fischer from comment #6)
> > As you said, it appears more like a lucky side effect, but hard to tell how
> > many libraries may be using it. Could we replace it with a proper
> > implementation? I think a 'mandatory' flag in the patron attribute
> > definition would make more sense.
> 
> Why would it make more sense? I think that we should deal with them in the
> pref, as it is where we deal with mandatory fields.

Because we already define if they show up, if they are repeatable etc. there. And it seemed easier. If we can find a way to autoamtically update Owen's modal with the descrpitions instead of the current naming scheme... that would be nice too.
Comment 12 Owen Leonard 2019-05-14 14:44:58 UTC
(In reply to Katrin Fischer from comment #11)

> Because we already define if they show up, if they are repeatable etc.
> there.

I agree. Including a mandatory option would fit perfectly within the parameters of what kind of features a patron attribute can have.
Comment 13 Jonathan Druart 2019-05-14 15:21:31 UTC
Well, I guess we could talk about that for hours :)

It seems easier to me to define the mandatory fields in only one place, the pref (I have provided a POC on bug 22844 in that direction).

If the two of you think it is better, your are certainly right.
Comment 14 Andrew Fuerste-Henry 2019-05-16 20:28:11 UTC
Given enough time, I could probably get around to arguing either side of this. But I think I'm leaning more toward putting it in the attribute setup. There's enough specific setup stuff for each attribute there already that I think that's where folks are more likely to expect this.
Comment 15 Jonathan Druart 2019-05-17 14:50:26 UTC
This is blocked by bug 22844 comment 23.
Comment 16 Christopher Brannon 2020-07-06 18:46:08 UTC
(In reply to Andrew Fuerste-Henry from comment #14)
> Given enough time, I could probably get around to arguing either side of
> this. But I think I'm leaning more toward putting it in the attribute setup.
> There's enough specific setup stuff for each attribute there already that I
> think that's where folks are more likely to expect this.

+1
Comment 17 Christopher Brannon 2020-07-07 15:34:26 UTC
I'm not seeing this point here, so I will just add to the mix.  It DOES seem like it is currently a side-effect.  The one problem with implementing this is that it is based off field names, and those names vary, depending on whether some of the patron attributes are used for all libraries.  This can cause problems with those libraries that don't use a certain attribute, as it offsets the attribute names.

I haven't tested, but this could also be complicated by those attributes that allow more than one instance of the field.

Seems like a mandatory setting that can stick to the specific field would be better than trying to make it mandatory through the field name.

I would suggest that the mandatory option be unavailable to those fields that can be duplicated, or only make the first instance mandatory?  Just some thoughts.
Comment 18 Katrin Fischer 2020-07-18 17:25:15 UTC
I am closing this WONTFIX now as we got bug 22844 in PQA now - this will add the mandatory option to the patron attribute setup, so a note will no longer be needed.