all select in accountlines are " WHERE borrowernumber = ? AND accountno=?". Best practice is to have an id. I'll add it and modify calls to accountlines to use it
Created attachment 8280 [details] [review] proposed patch
Hello Can't be tested with the sandbox : need to update the database. Sorry
during hackfest, functions involved in this patch have been modified. I remade the patch
Stephane, I added (manualy) the primary key to my database an tested fines payment, no errores encountered. The attachment states: Signed-off-by: Stéphane Delaune <stephane.delaune@biblibre.com> Could it be that you forgot to switch te status from Needs Signoff to Signed Off? Or should the patch be signed off again? Marc
Hi Marc It does need another signoff (Stephane wrote it). But if you have tested it, you should be able to sign off on it :) Chris
Hi Chris and Stephane, Sorry, I was not sure about signing off, and then I missed the Email notification of Chris' comment. :( In the meantime I already deleted the branch bug_7671bug. Now I created a new branch bug_7671 and tried to apply the patch again, resultin in a "Does not apply" with: ---------- Applying: Bug 7671 : add a real primary key accountlinesid in accountlines /usr/share/kohaclone/.git/rebase-apply/patch:321: trailing whitespace. $usth->execute($data->{'accountlinesid'}); # We might be adjusting an a ccount for some OTHER borrowernumber now. Not the one we passed in. error: patch failed: installer/data/mysql/updatedatabase.pl:4765 error: installer/data/mysql/updatedatabase.pl: patch does not apply error: patch failed: members/pay.pl:86 error: members/pay.pl: patch does not apply Patch failed at 0001 Bug 7671 : add a real primary key accountlinesid in account lines When you have resolved this problem run "git am --resolved". If you would prefer to skip this patch, instead run "git am --skip". To restore the original branch and stop patching run "git am --abort". Patch left in /tmp/proposed-patch-UvbvKO.patch ---------- I tried to resolve it, first I did: git am --abort git am -iu3 /tmp/proposed-patch-UvbvKO.patch Then I followed the steps in http://wiki.koha-community.org/wiki/Resolve_Merge_Issues to resolve ...but I think I messed something up, because I get Errors doing a payment: -------- Global symbol "$writeoff_sth" requires explicit package name at /usr/share/kohaclone/members/pay.pl line 117. Global symbol "$add_writeoff_sth" requires explicit package name at /usr/share/kohaclone/members/pay.pl line 120. Global symbol "$writeoff_sth" requires explicit package name at /usr/share/kohaclone/members/pay.pl line 276. Global symbol "$writeoff_sth" requires explicit package name at /usr/share/kohaclone/members/pay.pl line 284. Global symbol "$add_writeoff_sth" requires explicit package name at /usr/share/kohaclone/members/pay.pl line 288. -------- Marc
Created attachment 10022 [details] [review] Bug 7671 - add a real primary key "accountlinesid" in accountlines Rebased against master...
Tried to apply the patch and got: "fatal: cannot convert from UTF-8utf-8 to UTF-8" In the header of the patch the Content-Type appears twice: (...) Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="utf-8" (...) This was not the case in the previous patch, it was only: (...) Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (...) So my guess is that the double Content-Type header is the reason for the error. Marc
Created attachment 10260 [details] [review] Bug 7671 - add a real primary key "accountlinesid" in accountlines Hmm, That's strange, not seen that before. Just format-patched it again and the header seems correct this time. How odd.. New patch attached. Martin
Sorry, Martin, but now I have error: patch failed: installer/data/mysql/updatedatabase.pl:5308 error: installer/data/mysql/updatedatabase.pl: patch does not apply Marc
Created attachment 10283 [details] [review] [REBASED] Bug 7671 - add a real primary key "accountlinesid" in accountlines Rebased again... touch wood master doesn't move on again before we're signed off. Looking forward to Bug 7167 making it into master so this becomes less of an issue.
- Patch applied. - Update database done. - Went to Home › Patrons › Account for (...) - Clicked Tab "Pay fines" Result: Global symbol "$writeoff_sth" requires explicit package name at /usr/share/kohaclone/members/pay.pl line 117. Global symbol "$add_writeoff_sth" requires explicit package name at /usr/share/kohaclone/members/pay.pl line 120. Global symbol "$writeoff_sth" requires explicit package name at /usr/share/kohaclone/members/pay.pl line 276. Global symbol "$writeoff_sth" requires explicit package name at /usr/share/kohaclone/members/pay.pl line 284. Global symbol "$add_writeoff_sth" requires explicit package name at /usr/share/kohaclone/members/pay.pl line 288. (Same errors as at the nd of my comment #6) - Switched back to Master to make sure that this is relatd to the patch - Went to Home › Patrons › Account for (...) - Clicked Tab "Pay fines" Result: No erros on branch Master
(In reply to comment #3) > during hackfest, functions involved in this patch have been modified. I > remade the patch Hi Stephanie, Quick question, i'm not seeing a remade patch in the above attachments.. your comment of a remade patch is a few days after the original patch was attached.. did you attach a new one.. Looking at Marc's comments i've got a feeling i rebased the old patch, not taking into account the function changes.. Might be easier for you to do that bit as it's originals your code? Although i'm happy to take a look if i get a chance.
Created attachment 10343 [details] [review] [SIGNED-OFF] Bug 7671 : add a real primary key accountlinesid in accountlines Signed-off-by: Stéphane Delaune <stephane.delaune@biblibre.com> Signed-off-by: Marc Veron <veron@veron.ch> NOTE: After applying the patch I got following errors in members/pay.pl: Global symbol "$writeoff_sth" requires explicit package name Global symbol "$add_writeoff_sth" requires explicit package name Added to lines at the begin of members/pay.pl: our $writeoff_sth; our $add_writeoff_sth; Now the patch worked as expected. However I am not quite sure if signing off is OK in this situation. Marc
(In reply to comment #14) > Created attachment 10343 [details] [review] > [SIGNED-OFF] Bug 7671 : add a real primary key accountlinesid in accountlines > > Signed-off-by: Stéphane Delaune <stephane.delaune@biblibre.com> > Signed-off-by: Marc Veron <veron@veron.ch> > > NOTE: After applying the patch I got following errors in members/pay.pl: > Global symbol "$writeoff_sth" requires explicit package name > Global symbol "$add_writeoff_sth" requires explicit package name > > Added to lines at the begin of members/pay.pl: > our $writeoff_sth; > our $add_writeoff_sth; > > Now the patch worked as expected. > > However I am not quite sure if signing off is OK in this situation. QA comment... Stephane, can you (or someone else) sign-off on Marc's syntax fixes to your patch
I cannot apply this patch. I receive this error: fatal: cannot convert from UTF-8utf-8 to UTF-8
I had the same error, see comment #8 Martin Renvoize then sent a new patch. I foound following solution from Markus Enger, see http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=1633#c4 ----------------- * Patch did not apply, giving this message: "fatal: cannot convert from UTF-8utf-8 to UTF-8". Removing 'Content-Type: text/plain; charset="utf-8"' from the patch by hand solved this. ----------------- Marc
Before +----------------+-----------+------------+------------+----------+-------------+---------+-------------+-------------------+---------------+---------------------+-----------+--------------+------+------------+ | borrowernumber | accountno | itemnumber | date | amount | description | dispute | accounttype | amountoutstanding | lastincrement | timestamp | notify_id | notify_level | note | manager_id | +----------------+-----------+------------+------------+----------+-------------+---------+-------------+-------------------+---------------+---------------------+-----------+--------------+------+------------+ | 1 | 1 | NULL | 2012-08-06 | 2.000000 | NULL | NULL | Pay | NULL | NULL | 2012-08-06 20:15:58 | 0 | 0 | NULL | NULL | +----------------+-----------+------------+------------+----------+-------------+---------+-------------+-------------------+---------------+---------------------+-----------+--------------+------+------------+ After patch +----------------+----------------+-----------+------------+------------+----------+-------------+---------+-------------+-------------------+---------------+---------------------+-----------+--------------+------+------------+ | accountlinesid | borrowernumber | accountno | itemnumber | date | amount | description | dispute | accounttype | amountoutstanding | lastincrement | timestamp | notify_id | notify_level | note | manager_id | +----------------+----------------+-----------+------------+------------+----------+-------------+---------+-------------+-------------------+---------------+---------------------+-----------+--------------+------+------------+ | 1 | 1 | 1 | NULL | 2012-08-06 | 2.000000 | NULL | NULL | Pay | NULL | NULL | 2012-08-06 20:15:58 | 0 | 0 | NULL | NULL | +----------------+----------------+-----------+------------+------------+----------+-------------+---------+-------------+-------------------+---------------+---------------------+-----------+--------------+------+------------+ I then tested creating manual invoices, credits, reversing payments, paying, and writing off. The only thing I didnt test was overdue charges being added correctly. But as 2 others have already signed off on this. Marking signed off
Created attachment 11374 [details] [review] Bug 7671 : add a real primary key accountlinesid in accountlines Signed-off-by: Stéphane Delaune <stephane.delaune@biblibre.com> Signed-off-by: Marc Veron <veron@veron.ch> NOTE: After applying the patch I got following errors in members/pay.pl: Global symbol "$writeoff_sth" requires explicit package name Global symbol "$add_writeoff_sth" requires explicit package name Added to lines at the begin of members/pay.pl: our $writeoff_sth; our $add_writeoff_sth; Now the patch worked as expected. However I am not quite sure if signing off is OK in this situation. Marc Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
(In reply to comment #19) > Created attachment 11374 [details] [review] > Bug 7671 : add a real primary key accountlinesid in accountlines > > Signed-off-by: Stéphane Delaune <stephane.delaune@biblibre.com> > Signed-off-by: Marc Veron <veron@veron.ch> > Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> patch looks good and applies to master 322a9c7fbbcf456084d1937f48345b3cdec058ce passing QA, nice patch - well done! mason@xen1:~/git/head$ koha-qa.pl * f6f6a46 Bug 7671 : add a real primary key accountlinesid in accountlines C4/Accounts.pm C4/Circulation.pm installer/data/mysql/kohastructure.sql installer/data/mysql/updatedatabase.pl koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt members/boraccount.pl members/pay.pl members/paycollect.pl members/printinvoice.pl - perlcritic-progressive tests... OK - perl -c syntax tests... OK - xt/tt_valid.t tests... OK - xt/author/valid-template.t tests... OK
mmm... i'm a little bit annoyed: we discussed on koha-devel about column naming conventions. The final decision was to use something_id (see http://wiki.koha-community.org/wiki/Coding_Guidelines#Database_Design) The column proposed here is accounlinesid (without _), which does not respect this guideline. However, the patch has been submitted before the rule has been defined. So, I think it's unfair to ask for an update. OTOH, it should not be too hard to add the _ before pushing the patch. Thus: Stephane, would you be OK to improve your patch -once again- ? If you say non, then i'll push it like this. But I would prefer that you say "yes" (RM speaking here, not the boss)
Created attachment 11922 [details] [review] proposed patch done (add _ on id's name)
QA comment: This part of the code is wrong: + my $sthr = $dbh->prepare("SELECT max(accountlines_id) AS lastinsertid FROM accountlines"); + $sthr->execute(); + my $datalastinsertid = $sthr->fetchrow_hashref; + $sthr->finish; + return $datalastinsertid->{'lastinsertid'}; => in case 2 lines are added by 2 librarians almost at the same time, you may get a wrong lastinsertid + this add an unneeded SQL query. Please use $rv = $dbh->last_insert_id($catalog, $schema, $table, $field); instead Also note that your updatedatabase misses a } (it does not compile): $dbh->do("UPDATE systempreferences SET value = 'call_number' WHERE variable = 'OPACdefaultSortField' AND value = 'callnumber'"); print "Upgrade to $DBversion done (Bug 8657 - Default sort by call number does not work. Correcting system preference value.)\n"; SetVersion ($DBversion); + <<<<< SHOULD BE A } HERE + +$DBversion = "XXX"; +if ( C4::Context->preference("Version") < TransformToNum($DBversion) ) { + $dbh->do("ALTER TABLE `accountlines` ADD `accountlines_id` INT NOT NULL AUTO_INCREMENT PRIMARY KEY FIRST;");
Created attachment 12051 [details] [review] followup followup : delete return of makepayment because unused, fix updatedatabase
Created attachment 12055 [details] [review] Bug 7671 : add a real primary key accountlines_id in accountlines Signed-off-by: Stéphane Delaune <stephane.delaune@biblibre.com> Signed-off-by: Marc Veron <veron@veron.ch> NOTE: After applying the patch I got following errors in members/pay.pl: Global symbol "$writeoff_sth" requires explicit package name Global symbol "$add_writeoff_sth" requires explicit package name Added to lines at the begin of members/pay.pl: our $writeoff_sth; our $add_writeoff_sth; Now the patch worked as expected. However I am not quite sure if signing off is OK in this situation. Marc Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12056 [details] [review] Bug 7671 : followup : delete return of makepayment because unused, fix updatedatabase Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12219 [details] [review] Bug 7671 : add a real primary key accountlines_id in accountlines Signed-off-by: Stéphane Delaune <stephane.delaune@biblibre.com> Signed-off-by: Marc Veron <veron@veron.ch> NOTE: After applying the patch I got following errors in members/pay.pl: Global symbol "$writeoff_sth" requires explicit package name Global symbol "$add_writeoff_sth" requires explicit package name Added to lines at the begin of members/pay.pl: our $writeoff_sth; our $add_writeoff_sth; Now the patch worked as expected. However I am not quite sure if signing off is OK in this situation. Marc Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 12220 [details] [review] Bug 7671 : add a real primary key accountlines_id in accountlines Signed-off-by: Stéphane Delaune <stephane.delaune@biblibre.com> Signed-off-by: Marc Veron <veron@veron.ch> NOTE: After applying the patch I got following errors in members/pay.pl: Global symbol "$writeoff_sth" requires explicit package name Global symbol "$add_writeoff_sth" requires explicit package name Added to lines at the begin of members/pay.pl: our $writeoff_sth; our $add_writeoff_sth; Now the patch worked as expected. However I am not quite sure if signing off is OK in this situation. Marc Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment on attachment 12220 [details] [review] Bug 7671 : add a real primary key accountlines_id in accountlines oops
Created attachment 12221 [details] [review] Bug 7671 : add a real primary key accountlines_id in accountlines Signed-off-by: Stéphane Delaune <stephane.delaune@biblibre.com> Signed-off-by: Marc Veron <veron@veron.ch> NOTE: After applying the patch I got following errors in members/pay.pl: Global symbol "$writeoff_sth" requires explicit package name Global symbol "$add_writeoff_sth" requires explicit package name Added to lines at the begin of members/pay.pl: our $writeoff_sth; our $add_writeoff_sth; Now the patch worked as expected. However I am not quite sure if signing off is OK in this situation. Marc Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
> Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> > Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> woah, sorry about that, git-bz when a little crazy on me... :/ i rebased these patches as i got a UTF-utf header error patches looks good, passing QA… $ koha-qa.pl -c 2 testing 2 commit(s) (applied to commit 0acebb8) * f38273f Bug 7671 : followup : delete return of makepayment because unused C4/Accounts.pm installer/data/mysql/updatedatabase.pl * c9c2f88 Bug 7671 : add a real primary key accountlines_id in accountlines C4/Accounts.pm C4/Circulation.pm installer/data/mysql/kohastructure.sql installer/data/mysql/updatedatabase.pl koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt members/boraccount.pl members/pay.pl members/paycollect.pl members/printinvoice.pl * C4/Accounts.pm OK * installer/data/mysql/updatedatabase.pl OK * C4/Circulation.pm OK * members/boraccount.pl OK * members/pay.pl OK * members/paycollect.pl OK * members/printinvoice.pl OK * koha-tmpl/intranet-tmpl/prog/en/modules/members/boraccount.tt OK * koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt OK * koha-tmpl/intranet-tmpl/prog/en/modules/members/paycollect.tt OK
Patch pushed to master I've marked this patch as an ENH. I think it's not wise to push it to 3.8, because there can be side effect that could be hard to spot. So proposing to delay this to 3.10 only.