Description
Paul Poulain
2010-11-24 16:09:09 UTC
Created attachment 3378 [details] [review] Patch from BibLibre 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. 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. Created attachment 3647 [details] [review] patch appliable for master the patch proposed could not apply any longer. This patch is rebased on master. 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 Reformatted for template toolkit and pushed to new/awaiting_qa/bug_5436 Please take a look 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. 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. Created attachment 4595 [details]
Screenshot of the "renewed" message
This is what the "renwed" message looks like.
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". finally reproduced the error. Sent a patch fixing that. Created attachment 4608 [details] [review] followup patch. This assigns the problem of expirydate displayed 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". * 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 Created attachment 4638 [details] [review] Combined, signed-off patch This patch combines the changes pushed to new/awaiting_qa/bug_5436 with this follow-up patch: http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=4608 I have reported the problem with the "Notices" tab as Bug 6593. 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 Created attachment 4829 [details] [review] proposed patch (passed QA) 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? Created attachment 4838 [details] [review] proposed patch (follow-up comment 20) (In reply to comment #20) > These changes to staff-global.css should not be included: I've removed them in the patch i'll sent soon > In admin/patron-attr-types.tt: > The standard term is "patron," not member. right, updated too > Updates to returns.tt revert two recent improvements. The change to this > template should include *only* the link to circulation.pl OK, i've updated the patch as well > Why this change in moremember.tt? I don't know, i've discarded all changes in moremember.tt that seems not related to this bug. Tested again, works as expected. Owen, could you check (and sign-off my follow-up. Once it's done, I think we will be able to set "passed QA", i'll ask ian/chris about that) 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. 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? 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. Created attachment 4872 [details] [review] Proposed fix This patch takes changes in the previous patch and isolates just the parts relating to display of patron attributes. Because function for pulling patron attributes was merged with the function for displaying address information in SetMemberInfosInTemplate() (also found in Bug 5749), I moved the call to GetBorrowerAttributes into the individual scripts. That keeps the functionality relating specifically to patron attributes separate from the proposed changes to displaying addresses. Because I think it's important to keep the display consistent, I added display of patron attributes to all pages which include the patron information sidebar. Created attachment 4906 [details] [review] proposed patch (signed OFF) The patch from Owen works fine. Once it has been applied, 5749 can be tested as well. 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) After fixing the merge conflicts with latest master, I'm happy to pass this patch through QA. Great new feature! Pushed, please test This is causing 6390, because the atomic update is never run, adding the code to update database and pushing There have been no further reports of problems so I am marking this bug resolved. |