|
Description
David Roberts
2015-04-22 09:21:32 UTC
Created attachment 38314 [details] [review] Proposed patch To test: Install this patch. Ensure a user has a salutation (Mr, Dr, Miss etc) set, and checkout an item to this user. In the area above the barcode box, the patron's title should display, e.g: Checking out to Mr John Doe (0001). Wanted to test, does not apply: CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/includes/patron-title.inc Failed to merge in the changes. Created attachment 38495 [details] [review] Add patron salutation to checkout screen Sorry, my mistake - I was tracking the wrong version! A new patch has now been submitted so hopefully it will work this time. Created attachment 38498 [details] [review] [Signed-off] Add patron salutation to checkout screen To test: Install this patch. Ensure a user has a salutation (Mr, Dr, Miss etc) set, and checkout an item to this user. In the area above the barcode box, the patron's title should display, e.g: Checking out to Mr John Doe (0001). --- Followed test plan. Works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 38734 [details] [review] [PASSED QA] Bug 14039 - Add patron salutation to checkout screen To test: Install this patch. Ensure a user has a salutation (Mr, Dr, Miss etc) set, and checkout an item to this user. In the area above the barcode box, the patron's title should display, e.g: Checking out to Mr John Doe (0001). Followed test plan. Works as expected. Signed-off-by: Marc Veron <veron@veron.ch> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 1) I think this patch introduces a change in much more places than expected:
$ git grep patron-title.inc | wc -l
1491
I suggest that such a change should be optional, and could be done like this:
- [%- borrower.firstname %] [% IF borrower.othernames %] ([% borrower.othernames %]) [% END %] [% borrower.surname %]
+ <span id="patron-title">[%- borrower.title %] </span>[% borrower.firstname %] [% IF borrower.othernames %] ([% borrower.othernames %]) [% END %] [% borrower.surname %]
i.e. putting the title inside a span with an id, and change CSS to hide it by default. So you keep the enhancement you want, but people can opt-in instead of making it mandatory (e.g. #patron-title { display: none; } )
2) Please make it check for borrower.title to avoid warnings? [%- IF borrower.title -%] etc
Created attachment 64069 [details] [review] Add patron salutation to checkout screen Belatedly revisiting this. This patch was intended to only apply to the checkout screen as this is where the customer wanted it to display, but if it appears elsewhere then that isn't the end of the world.
I've added a span to the title so that it can be hidden in CSS - I believe it is better to deliver this with it enabled so that the user knows the functionality is there, and can be easily hidden. It is much harder to inform the user that it is possible to enable a function they don't know exists.
I've added the test to prevent errors it there is no borrower.title present in the user record.
Amended test plan:
1) Install this patch.
2) Ensure a user has a salutation (Mr, Dr, Miss etc) set, and checkout an item to this user.
In the area above the barcode box, the patron's title should display, e.g:
Checking out to Mr John Doe (0001).
3) Add #patron-title { display: none; } to IntranetUserCSS
Check that the patron title no longer displays
4) With a user without a salutation, check that no errors display on checkout.
Sorry, I don't know why the original patch (38734) wasn't marked as obsolete Created attachment 64077 [details] [review] [SIGNED-OFF] Bug 14039 - Add patron salutation to checkout screen To test: Install this patch. Ensure a user has a salutation (Mr, Dr, Miss etc) set, and checkout an item to this user. In the area above the barcode box, the patron's title should display, e.g: Checking out to Mr John Doe (0001). Signed-off-by: Owen Leonard <oleonard@myacpl.org> (In reply to Tomás Cohen Arazi from comment #7) > I suggest that such a change should be optional This patch does not make the change hidden by default, but I have signed off because I think it's okay the way it is. I believe that if libraries are collecting this information they most likely would like to see it. Sorry, edited the wrong bug. Changing status back to signed off. Comment on attachment 64077 [details] [review] [SIGNED-OFF] Bug 14039 - Add patron salutation to checkout screen Review of attachment 64077 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/includes/patron-title.inc @@ +5,4 @@ > [%- IF invert_name %] > [%- borrower.surname | html %], [% borrower.firstname | html %] [% IF borrower.othernames %] ([% borrower.othernames | html %]) [% END %] > [%- ELSE %] > + [% IF borrower.title %] <span id="patron-title">[%- borrower.title | html %] </span>[% END %][%- borrower.firstname | html %] [% IF borrower.othernames %] ([% borrower.othernames | html %]) [% END %] [% borrower.surname | html %] I do not think it's a good idea to have an id here, there are certainly places where this include file is used several times. The only reason I added an id was because it failed QA without one. Now it's failed QA with one! Can't win. Hi David, I think you can fix it easily - just make the id a class. If you have multiple elements with the same id on a page, this will cause problems, for example you will not be able to target them with jQuery in my experience. So Jonathan is right that we might want to be careful here. class attributes can appear many times and will work nicely here to be on the safe side. Created attachment 65482 [details] [review] Add patron salutation to checkout screen Thanks Katrin,
I've made this change. The test plan is now:
1) Install this patch.
2) Ensure a user has a salutation (Mr, Dr, Miss etc) set, and checkout an item to this user.
In the area above the barcode box, the patron's title should display, e.g:
Checking out to Mr John Doe (0001).
3) Add .patron-title { display: none; } to IntranetUserCSS
Check that the patron title no longer displays
4) With a user without a salutation, check that no errors display on checkout.
Created attachment 65496 [details] [review] [SIGNED-OFF] Add patron salutation to checkout screen https://bugs.koha-community.org/show_bug.cgi?id=14039 Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 65777 [details] [review] Bug 14039: Add the title to all places There are other scripts where the borrower variable is not defined and the fields are passed one by one. To have a consistent behaviour we should add the title at the different places. Note that this script also add the use of the include file for statistics.tt and remove the pass of parameters to the template, already done later: 99 $template->param(%$borrower); Created attachment 67766 [details] [review] Add patron salutation to checkout screen https://bugs.koha-community.org/show_bug.cgi?id=14039 Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 67767 [details] [review] Bug 14039: Add the title to all places There are other scripts where the borrower variable is not defined and the fields are passed one by one. To have a consistent behaviour we should add the title at the different places. Note that this script also add the use of the include file for statistics.tt and remove the pass of parameters to the template, already done later: 99 $template->param(%$borrower); Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Pushed to master for 17.11, thanks to everybody involved! Created attachment 67870 [details] [review] Bug 14039: Fix patron search The JSON was malformed Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Last patch pushed to master. |