Bug 5436 - Extended patron attributes display improvements
Summary: Extended patron attributes display improvements
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: 3.4
Hardware: All All
: PATCH-Sent (DO NOT USE) enhancement (vote)
Target Milestone: ---
Assignee: Guillaume Hatt
QA Contact: Bugs List
URL: http://wiki.koha-community.org/wiki/E...
Whiteboard:
Keywords:
Depends on: 6669
Blocks: 6930
  Show dependency treegraph
 
Reported: 2010-11-24 16:09 UTC by Paul Poulain
Modified: 2013-12-05 20:04 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch from BibLibre (24.79 KB, patch)
2011-03-21 10:03 UTC, Julian Maurice
Details | Diff | Splinter Review
patch appliable for master (24.87 KB, patch)
2011-04-04 16:31 UTC, dev_patches
Details | Diff | Splinter Review
Screenshot of the "renewed" message (16.75 KB, image/png)
2011-07-08 09:51 UTC, Magnus Enger
Details
followup patch. This assigns the problem of expirydate displayed (1.68 KB, patch)
2011-07-08 17:06 UTC, dev_patches
Details | Diff | Splinter Review
Combined, signed-off patch (32.54 KB, patch)
2011-07-15 23:29 UTC, Magnus Enger
Details | Diff | Splinter Review
proposed patch (passed QA) (32.40 KB, patch)
2011-08-03 15:16 UTC, Paul Poulain
Details | Diff | Splinter Review
proposed patch (follow-up comment 20) (11.16 KB, patch)
2011-08-04 12:08 UTC, Paul Poulain
Details | Diff | Splinter Review
Proposed fix (22.63 KB, patch)
2011-08-05 18:59 UTC, Owen Leonard
Details | Diff | Splinter Review
proposed patch (signed OFF) (22.65 KB, patch)
2011-08-09 14:05 UTC, Paul Poulain
Details | Diff | Splinter Review
Signed off patch (rebased) (23.62 KB, patch)
2011-09-22 02:34 UTC, Ian Walls
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Poulain 2010-11-24 16:09:09 UTC
Extended patron attributes are a great tool: it provides nice flexibility in the information one wants to manage in the patron's record. But as of version 3.2 we don't have any way to manage how this information will be displayed once entered.
For some extended attributes, it might be useful to have them display in circ/circulation.pl, in the Check Out tab, which is arguably the most important tab in this page, rather than at the end of the Details page.
This RFC proposes:
when looking up a patron from members-home.pl and having one and only one result, one should go straight to the member.pl, not have a table with 1 result only;
on circ/returns.pl page when I click on a patron's name (link), I should not go to koha/members/moremember.pl?borrowernumber=XXX but to koha/circ/circulation.pl?borrowernumber=XXX
to add a field to admin/patron-att-types.pl?op=edit
Comment 1 Julian Maurice 2011-03-21 10:03:35 UTC Comment hidden (obsolete)
Comment 2 Nicole C. Engard 2011-03-22 20:48:42 UTC
I was testing this and I clicked on the patron 'check out' tab and I saw a message that my patron has been renewed ... I didn't click renew - just 'check out' so I think there is a tiny bug in this patch.  

I'm back to testing.
Comment 3 Nicole C. Engard 2011-03-22 20:50:28 UTC
That's the only thing I found wrong with this patch - but I just want to check to see what you mean by attributes - there are patron attributes in Koha - but it sounds to me like you're referring to their address and not the custom attributes.
Comment 4 dev_patches 2011-04-04 16:31:36 UTC Comment hidden (obsolete)
Comment 5 Guillaume Hatt 2011-04-05 12:19:07 UTC
I was testing last version of the patch (attachment 3647 [details] [review]) and I saw a message that my patron has been renewed too. 
There's no modification of the enrollment period, it's still shown on top of the page of circulation circ/circulation.pl?borrowernumber=xxx
Guillaume
Comment 6 Chris Cormack 2011-06-01 02:32:03 UTC
This is blocking bug 5872
Comment 7 Chris Cormack 2011-06-07 04:04:02 UTC
Reformatted for template toolkit and pushed to
new/awaiting_qa/bug_5436

Please take a look
Comment 8 dev_patches 2011-06-08 19:37:45 UTC
can't reproduce the buggy behaviour on my instance.
I would like to ask the person who tested the patch to test against your branch.
So that they can attest whether or not it is still happening.
Comment 9 Magnus Enger 2011-07-08 09:43:59 UTC
Sorry, I'm seeing the same as Nicole and Guillaume. 

I did a "git checkout origin/new/awaiting_qa/bug_5436", then did a specific search for a patron, which took me straight to the details tab for that patron (great!). But when I click on the "Check out" tab I see the message "Patron's account has been renewed until 06/30/2019". 

When I click on "Details" and then on "Check out" again the same message is still displayed. 

Doing "git checkout master" and clicking on the "Check out" tab removes the message. 

Confirming failed QA. 

BTW: This would have been so much easier to test if each separate feature/fix had it's own separate bug/patch. And this thing with the renewed-message would not be blocking all the other bits.
Comment 10 Magnus Enger 2011-07-08 09:51:09 UTC Comment hidden (obsolete)
Comment 11 Magnus Enger 2011-07-08 11:02:16 UTC
I did a clean English install with all sample data, and the "renewed" message still shows when I do "git checkout origin/new/awaiting_qa/bug_5436". 

I also did a clean French install with all sample data, created a library and a staff user and again the "renewed" message shows when I do "git checkout origin/new/awaiting_qa/bug_5436".
Comment 12 dev_patches 2011-07-08 17:04:59 UTC
finally reproduced the error.
Sent a patch fixing that.
Comment 13 dev_patches 2011-07-08 17:06:48 UTC Comment hidden (obsolete)
Comment 14 Magnus Enger 2011-07-15 19:16:59 UTC
Yep, attachment 4608 [details] [review] fixes the "renewed" message, when it is applied on top of new/awaiting_qa/bug_5436. 

Changing status to "Needs signoff".
Comment 15 Magnus Enger 2011-07-15 23:28:24 UTC
* When a patron search returns just 1 patron, go straight to that patron - OK

* Borrower link on circ/return.pl goes to circulation.pl instead of moremembers.pl - OK

* "View/Hide address" mostly works as expected, and saves some screen real estate, making more of the tabs visible on smaller screens. But there is a problem with the "Notices" tab: All the other tabs display this: 

  Joann Duffy (23529000218553)
    Student (ST)
    Midway
    View/Hide address

But the "Notices" displays this: 

  Joann Duffy (23529000218553)
    (ST)
    View/Hide address

The same problem exists even without the patches for the present bug - I'll check if there is a separate bug for it, or report one if there isn't. 

All in all - OK

* Making extended attributes display in the "borrower toolbar"

This patch adds a new field to all extended attributes: "Display in check-out". I created two new extended attributes, one with this field checked and one where it was not checked. The one that had it checked is displayed in the "borrower toolbar" and the other one is not displayed, as expected. 

The extended attributes that should display in the "borrower toolbar" is only displayed on the "Check Out" and "Details" tabs in circulation, I had expected it to show on all the tabs, but from reading the comments on the bug is seems this is what was intended, so all in all - OK. 

Please note: The patch has a database change in installer/data/mysql/atomicupdate/0009-patron-attr-display-checkout.pl
Comment 16 Magnus Enger 2011-07-15 23:29:59 UTC Comment hidden (obsolete)
Comment 17 Magnus Enger 2011-07-16 11:14:06 UTC
I have reported the problem with the "Notices" tab as Bug 6593.
Comment 18 Paul Poulain 2011-08-03 15:15:49 UTC
QA comment

* works as announced. beware the updatedatabase is in an atomicupdate file, must be added in updatedatabase before applying to master

* there are a few reindenting or minor & unnecessary syntax changes, like: 
-    my ($warning_year, $warning_month, $warning_day) = split /-/, $borrower->{'dateexpiry'};
-    my (  $enrol_year,   $enrol_month,   $enrol_day) = split /-/, $borrower->{'dateenrolled'};
+    my ($warning_year, $warning_month, $warning_day) = split (/-/, $borrower->{'dateexpiry'});
+    my (  $enrol_year,   $enrol_month,   $enrol_day) = split (/-/, $borrower->{'dateenrolled'});
but nothing worth a failed QA

* there are some properties added to staff-global.css, not sure they are all relevant (cartmenulink sounds strange. collapseaddress is used & OK). I suggest asking owen

OWEN = I mark it "passed QA", if you notice something strange on staff-global, please object
Comment 19 Paul Poulain 2011-08-03 15:16:36 UTC Comment hidden (obsolete)
Comment 20 Owen Leonard 2011-08-03 16:01:36 UTC
These changes to staff-global.css should not be included:

+#cartmenulink img {
+       background-image : none;
+       display : inline;
+       padding : 3px 6px 0 0;
+}
+
+* html #cartmenulink {
+       border : 1px solid #336600;
+}
+
+* html #listsmenulink {
+       border : 1px solid #006699;
+}
+

-----------------------

In admin/patron-attr-types.tt:

+            <span>Check to show this attribute in member check-out.</span>

The standard term is "patron," not member.

-----------------------

Updates to returns.tt revert two recent improvements. The change to this template should include *only* the link to circulation.pl

-----------------------

Why this change in moremember.tt?

-    <li><span class="label">Gender:</span>
-    [% IF ( sex == 'F' ) %]Female[% ELSIF ( sex == 'M' ) %]Male[% ELSE %][% sex %][% END %]
-    </li>[% END %]
+    <li><span class="label">Sex:</span>[% sex %]</li>[% END %]

or this?

-    <li><span class="label">Username: </span>[% userid %]</li>
-    <li><span class="label">Password: </span>
+    <li><span class="label">OPAC login: </span>[% userid %]</li>
+    <li><span class="label">OPAC password: </span>

-----------------------

All of the changes in moremember.tt to the "Relatives Checkouts" section appear to be reversions of previous corrections. Should any of this have been touched by this patch?
Comment 21 Paul Poulain 2011-08-04 12:08:29 UTC Comment hidden (obsolete)
Comment 22 Owen Leonard 2011-08-04 17:12:37 UTC
I'm going to split out some of the minor changes in this patch which aren't related to patron attributes, starting with Bug 6669, If a patron search returns one result, redirect to patron details page.
Comment 23 Paul Poulain 2011-08-05 09:15:28 UTC
Owen, and what about the main goal of this patch? Do you want me to take care of it or do you plan to do it as well?
Comment 24 Owen Leonard 2011-08-05 16:29:28 UTC
Paul I hope that when I'm done I'll have the original Extended patron attributes functionality isolated and ready to attach to this patch for evaluation.
Comment 25 Owen Leonard 2011-08-05 18:59:16 UTC Comment hidden (obsolete)
Comment 26 Paul Poulain 2011-08-09 14:05:58 UTC Comment hidden (obsolete)
Comment 27 Ian Walls 2011-09-22 02:34:33 UTC
Created attachment 5526 [details] [review]
Signed off patch (rebased)

Patch rebased to compatability with bug 3498 (partial payments) and database documentation (added documentation to new borrower_attributes.display_checkout field)
Comment 28 Ian Walls 2011-09-22 02:36:08 UTC
After fixing the merge conflicts with latest master, I'm happy to pass this patch through QA.  Great new feature!
Comment 29 Chris Cormack 2011-09-23 03:38:08 UTC
Pushed, please test
Comment 30 Chris Cormack 2011-10-19 14:14:45 UTC
This is causing 6390, because the atomic update is never run, adding the code to update database and pushing
Comment 31 Jared Camins-Esakov 2012-12-31 00:17:14 UTC
There have been no further reports of problems so I am marking this bug resolved.