Bug 22728

A more user-friendly solution might be to add a "mandatory" option to patron attributes.

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.

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     });

So I think it's a side-effect, not a feature :)

(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.

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.

(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.

(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.

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.

(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 :)

(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.

(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.

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.

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.

This is blocked by bug 22844 comment 23.

(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

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.

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.