Bug 7671 - add a real primary key "accountlinesid" in accountlines
Summary: add a real primary key "accountlinesid" in accountlines
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: stephane.delaune
QA Contact: Ian Walls
URL:
Whiteboard:
Keywords:
Depends on: 9038
Blocks: 7985 5905
  Show dependency treegraph
 
Reported: 2012-03-08 09:05 UTC by stephane.delaune
Modified: 2013-05-23 06:24 UTC (History)
9 users (show)

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


Attachments
proposed patch (25.75 KB, patch)
2012-03-19 14:08 UTC, stephane.delaune
Details | Diff | Splinter Review
Bug 7671 - add a real primary key "accountlinesid" in accountlines (26.24 KB, patch)
2012-06-09 13:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 7671 - add a real primary key "accountlinesid" in accountlines (26.20 KB, patch)
2012-06-13 16:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
[REBASED] Bug 7671 - add a real primary key "accountlinesid" in accountlines (26.17 KB, patch)
2012-06-14 16:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7671 : add a real primary key accountlinesid in accountlines (26.92 KB, patch)
2012-06-18 04:51 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 7671 : add a real primary key accountlinesid in accountlines (26.95 KB, patch)
2012-08-06 08:23 UTC, Chris Cormack
Details | Diff | Splinter Review
proposed patch (27.27 KB, patch)
2012-08-31 15:21 UTC, stephane.delaune
Details | Diff | Splinter Review
followup (1.72 KB, patch)
2012-09-07 15:05 UTC, stephane.delaune
Details | Diff | Splinter Review
Bug 7671 : add a real primary key accountlines_id in accountlines (27.37 KB, patch)
2012-09-07 15:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7671 : followup : delete return of makepayment because unused, fix updatedatabase (1.81 KB, patch)
2012-09-07 15:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7671 : add a real primary key accountlines_id in accountlines (27.40 KB, patch)
2012-09-14 12:49 UTC, Mason James
Details | Diff | Splinter Review
Bug 7671 : add a real primary key accountlines_id in accountlines (27.40 KB, patch)
2012-09-14 12:50 UTC, Mason James
Details | Diff | Splinter Review
Bug 7671 : add a real primary key accountlines_id in accountlines (27.40 KB, patch)
2012-09-14 12:58 UTC, Mason James
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description stephane.delaune 2012-03-08 09:05:07 UTC
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
Comment 1 stephane.delaune 2012-03-19 14:08:06 UTC Comment hidden (obsolete)
Comment 2 Leila 2012-03-21 10:11:54 UTC
Hello
Can't be tested with the sandbox : need to update the database.
Sorry
Comment 3 stephane.delaune 2012-03-27 07:15:44 UTC
during hackfest, functions involved in this patch have been modified. I remade the patch
Comment 4 Marc Véron 2012-04-15 19:06:32 UTC
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
Comment 5 Chris Cormack 2012-04-16 03:28:33 UTC
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
Comment 6 Marc Véron 2012-04-18 08:02:17 UTC
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
Comment 7 Martin Renvoize 2012-06-09 13:09:59 UTC Comment hidden (obsolete)
Comment 8 Marc Véron 2012-06-13 13:37:54 UTC
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
Comment 9 Martin Renvoize 2012-06-13 16:13:10 UTC Comment hidden (obsolete)
Comment 10 Marc Véron 2012-06-13 19:07:56 UTC
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
Comment 11 Martin Renvoize 2012-06-14 16:01:02 UTC Comment hidden (obsolete)
Comment 12 Marc Véron 2012-06-14 18:13:13 UTC
- 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
Comment 13 Martin Renvoize 2012-06-15 13:42:32 UTC
(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.
Comment 14 Marc Véron 2012-06-18 04:51:37 UTC Comment hidden (obsolete)
Comment 15 Mason James 2012-07-17 03:51:54 UTC
(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
Comment 16 Kyle M Hall 2012-08-03 17:18:50 UTC
I cannot apply this patch. I receive this error:

fatal: cannot convert from UTF-8utf-8 to UTF-8
Comment 17 Marc Véron 2012-08-04 06:20:36 UTC
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
Comment 18 Chris Cormack 2012-08-06 08:22:40 UTC
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
Comment 19 Chris Cormack 2012-08-06 08:23:39 UTC Comment hidden (obsolete)
Comment 20 Mason James 2012-08-10 05:38:54 UTC
(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
Comment 21 Paul Poulain 2012-08-29 14:02:03 UTC
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)
Comment 22 stephane.delaune 2012-08-31 15:21:44 UTC Comment hidden (obsolete)
Comment 23 Paul Poulain 2012-09-05 12:06:45 UTC
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;");
Comment 24 stephane.delaune 2012-09-07 15:05:26 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2012-09-07 15:33:19 UTC Comment hidden (obsolete)
Comment 26 Kyle M Hall 2012-09-07 15:33:31 UTC Comment hidden (obsolete)
Comment 27 Mason James 2012-09-14 12:49:50 UTC Comment hidden (obsolete)
Comment 28 Mason James 2012-09-14 12:50:09 UTC Comment hidden (obsolete)
Comment 29 Mason James 2012-09-14 12:51:17 UTC
Comment on attachment 12220 [details] [review]
Bug 7671 : add a real primary key accountlines_id in accountlines

oops
Comment 30 Mason James 2012-09-14 12:58:18 UTC
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>
Comment 31 Mason James 2012-09-14 13:02:50 UTC
> 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
Comment 32 Paul Poulain 2012-09-15 05:37:29 UTC
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.