+1

Created attachment 170932 [details] [review]
Bug 37360: Add protected as a borrower mod option

This patch adds protected status as a batch borrower modification
option, if the user performing the modification is a superlibrarian.

TO TEST:

a)  login as a superlibrarian
b)  attempt to batch modify these k.t.d user barcodes:
    1)  23529000035676
    2)  23529001000463
    3)  23529000050113
c)  notice that there is no protected option
d)  APPLY PATCH
e)  repeat step b
f)  notice that protected is now an option
g)  set all borrowers to protected (yes)
    1)  notice that the status has changed, in each individual user
	record
h)  set all borrowers to unprotected (no)
    1)  notice that the status has changed, in each individual user
	record
i)  set the proctedted status back to yes, but also set some other
    random options (e.g. city to Cork)
    1)  notice that the city and protected status are both updated
j)  use the disabled checkbox next to protected
    1)  notice that this always sets the protected status to zero
k)  SIGN OFF

Thanks in advance for the sign-off and QA :-)

Jake.

Created attachment 170991 [details] [review]
Bug 37360: Add protected as a borrower mod option

This patch adds protected status as a batch borrower modification
option, if the user performing the modification is a superlibrarian.

TO TEST:

a)  login as a superlibrarian
b)  attempt to batch modify these k.t.d user barcodes:
    1)  23529000035676
    2)  23529001000463
    3)  23529000050113
c)  notice that there is no protected option
d)  APPLY PATCH
e)  repeat step b
f)  notice that protected is now an option
g)  set all borrowers to protected (yes)
    1)  notice that the status has changed, in each individual user
	record
h)  set all borrowers to unprotected (no)
    1)  notice that the status has changed, in each individual user
	record
i)  set the proctedted status back to yes, but also set some other
    random options (e.g. city to Cork)
    1)  notice that the city and protected status are both updated
j)  use the disabled checkbox next to protected
    1)  notice that this always sets the protected status to zero
k)  SIGN OFF

Signed-off-by: Catrina <catrina@bywatersolutions.com>

Test plan works as described. Selecting the box to clear the value on "Protected" sets the users to "No" (not protected) - some clarifying helper text might be useful there.

Thanks for the sign-off, Catrina. I could add a tooltip, but it might also be worth considering properly documenting this?

Jake.

Just curious, why push, push, unshift for the options for the dropdown? 
Could you just define them all at once or push the empty option first?

I think the bigger issue is that the values will not be translatable, failing for this


You also change the check for empty fields from a truth check to only assigning if not empty string, then checking if defined, is there a reason for that?

Thanks for the feedback. Because this hasn't been done before, I sort of adapted what's there. I guess the solution would be to put the Yes/No text within the template? I can put together a follow-up to address this.

Jake.

Created attachment 172385 [details] [review]
Bug 37360: (follow-up) added bool type to modborrowers

This follow-up patch adds a bool type to modborrowers, so that the
options can be translated within the template files.

All done – thanks for the FQA on this one. I think this is a much cleaner solution.

Jake.

Looks like this one has been signed off already. Should it be marked as Signed off status?

Created attachment 172977 [details] [review]
Bug 37360: Add protected as a borrower mod option

This patch adds protected status as a batch borrower modification
option, if the user performing the modification is a superlibrarian.

TO TEST:

a)  login as a superlibrarian
b)  attempt to batch modify these k.t.d user barcodes:
    1)  23529000035676
    2)  23529001000463
    3)  23529000050113
c)  notice that there is no protected option
d)  APPLY PATCH
e)  repeat step b
f)  notice that protected is now an option
g)  set all borrowers to protected (yes)
    1)  notice that the status has changed, in each individual user
	record
h)  set all borrowers to unprotected (no)
    1)  notice that the status has changed, in each individual user
	record
i)  set the proctedted status back to yes, but also set some other
    random options (e.g. city to Cork)
    1)  notice that the city and protected status are both updated
j)  use the disabled checkbox next to protected
    1)  notice that this always sets the protected status to zero
k)  SIGN OFF

Signed-off-by: Catrina <catrina@bywatersolutions.com>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Created attachment 172978 [details] [review]
Bug 37360: (follow-up) added bool type to modborrowers

This follow-up patch adds a bool type to modborrowers, so that the
options can be translated within the template files.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Created attachment 173273 [details] [review]
Bug 37360: Add protected as a borrower mod option

This patch adds protected status as a batch borrower modification
option, if the user performing the modification is a superlibrarian.

TO TEST:

a)  login as a superlibrarian
b)  attempt to batch modify these k.t.d user barcodes:
    1)  23529000035676
    2)  23529001000463
    3)  23529000050113
c)  notice that there is no protected option
d)  APPLY PATCH
e)  repeat step b
f)  notice that protected is now an option
g)  set all borrowers to protected (yes)
    1)  notice that the status has changed, in each individual user
	record
h)  set all borrowers to unprotected (no)
    1)  notice that the status has changed, in each individual user
	record
i)  set the proctedted status back to yes, but also set some other
    random options (e.g. city to Cork)
    1)  notice that the city and protected status are both updated
j)  use the disabled checkbox next to protected
    1)  notice that this always sets the protected status to zero
k)  SIGN OFF

Created attachment 173274 [details] [review]
Bug 37360: (follow-up) added bool type to modborrowers

This follow-up patch adds a bool type to modborrowers, so that the
options can be translated within the template files.

Ah whoops, removed the SO lines... one tick

Created attachment 173275 [details] [review]
Bug 37360: Add protected as a borrower mod option

This patch adds protected status as a batch borrower modification
option, if the user performing the modification is a superlibrarian.

TO TEST:

a)  login as a superlibrarian
b)  attempt to batch modify these k.t.d user barcodes:
    1)  23529000035676
    2)  23529001000463
    3)  23529000050113
c)  notice that there is no protected option
d)  APPLY PATCH
e)  repeat step b
f)  notice that protected is now an option
g)  set all borrowers to protected (yes)
    1)  notice that the status has changed, in each individual user
	record
h)  set all borrowers to unprotected (no)
    1)  notice that the status has changed, in each individual user
	record
i)  set the proctedted status back to yes, but also set some other
    random options (e.g. city to Cork)
    1)  notice that the city and protected status are both updated
j)  use the disabled checkbox next to protected
    1)  notice that this always sets the protected status to zero
k)  SIGN OFF

Signed-off-by: Catrina <catrina@bywatersolutions.com>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Created attachment 173276 [details] [review]
Bug 37360: (follow-up) added bool type to modborrowers

This follow-up patch adds a bool type to modborrowers, so that the
options can be translated within the template files.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Those patches are now rebased on main :-)

Jake.

Any chance of a QA for this one in time for 24.11?

(In reply to Jake Deery from comment #20)
> Any chance of a QA for this one in time for 24.11?

You wont pass the bug freeze anymore now. Sorry.
Note that this feels like an enh instead of a bug ?

This does not look good:
+        my $value = $input->param($field) if $input->param($field) ne '';
+        $infos->{$field} = $value if defined $value;
Never declare a variable with an if statement like that. The next line you are referring to it.

Our qa tools see it too:
 FAIL   tools/modborrowers.pl
   FAIL   critic
                # Variables::ProhibitConditionalDeclarations: Got 1 violation(s).
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 0, now: 4)

Please tidy too.

Created attachment 174919 [details] [review]
Bug 37360: (follow-up) added bool type to modborrowers

This follow-up patch adds a bool type to modborrowers, so that the
options can be translated within the template files.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Hi Marcel,

Yes, I would agree, this is an enhancement. It will need to wait now until 25.05. I have tidied the offending file and rebased for good measure :-)

Jake.

Just having a question still about this change:

-        my $value = $input->param($field);
-        $infos->{$field} = $value if $value;
-        $infos->{$field} = ""     if grep { $_ eq $field } @disabled;
+        my $value;
+        $value           = $input->param($field) if $input->param($field) ne '';
+        $infos->{$field} = $value                if defined $value;
+        $infos->{$field} = ""                    if grep { $_ eq $field } @disabled;

On first glance, nothing special happens here. Code looks more complicated than before.
And the $value assignemnt may trigger a warning now if the input->param is undef.

Please explain why you are doing this.

Hi Marcel,

I believe it was due to triggering Perl::Critic::Policy::Variables::ProhibitConditionalDeclarations the old way. Happy to put it back, if we think this is safe to ignore? Or is there another way? :-) 

Jake.

(In reply to Jake Deery from comment #25)
> Hi Marcel,
> 
> I believe it was due to triggering
> Perl::Critic::Policy::Variables::ProhibitConditionalDeclarations the old
> way. Happy to put it back, if we think this is safe to ignore? Or is there
> another way? :-) 
> 
> Jake.

-        my $value = $input->param($field);
-        $infos->{$field} = $value if $value;
-        $infos->{$field} = ""     if grep { $_ eq $field } @disabled;

I remember the conditional declaration, but the original code above does not contain that. Could we just put these lines back? Or was there any issue with empty string versus undef that you wanted to fix here?

Created attachment 175253 [details] [review]
Bug 37360: (follow-up) added bool type to modborrowers

This follow-up patch adds a bool type to modborrowers, so that the
options can be translated within the template files.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Hi Marcel,

I've reverted the changes to those lines, but left the perltidy in as a cleansing operation. Hope this helps! :-)

Jake.

Created attachment 175254 [details] [review]
Bug 37360: Add protected as a borrower mod option

This patch adds protected status as a batch borrower modification
option, if the user performing the modification is a superlibrarian.

TO TEST:

a)  login as a superlibrarian
b)  attempt to batch modify these k.t.d user barcodes:
    1)  23529000035676
    2)  23529001000463
    3)  23529000050113
c)  notice that there is no protected option
d)  APPLY PATCH
e)  repeat step b
f)  notice that protected is now an option
g)  set all borrowers to protected (yes)
    1)  notice that the status has changed, in each individual user
	record
h)  set all borrowers to unprotected (no)
    1)  notice that the status has changed, in each individual user
	record
i)  set the proctedted status back to yes, but also set some other
    random options (e.g. city to Cork)
    1)  notice that the city and protected status are both updated
j)  use the disabled checkbox next to protected
    1)  notice that this always sets the protected status to zero
k)  SIGN OFF

Signed-off-by: Catrina <catrina@bywatersolutions.com>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 175255 [details] [review]
Bug 37360: (follow-up) added bool type to modborrowers

This follow-up patch adds a bool type to modborrowers, so that the
options can be translated within the template files.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Small thing... CanUpdateProtectPatron... Why not ProtectedPatron?

Pushed for 25.05!

Well done everyone, thank you!