Bug 22600

Summary: We should add an 'interface' field to accountlines
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: blocker    
Priority: P2 CC: jonathan.druart, josef.moravec, katrin.fischer, kyle, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18205
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22577
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17168
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20256
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00
Bug Depends on: 22512    
Bug Blocks: 22521, 22603, 22755, 23193, 24377, 25845, 26086    
Attachments: Bug 22600: Add 'interface' to accountlines
Bug 22600: Add 'interface' to accountlines
Bug 22600: Add 'api' to interface types and set appropriately
Bug 22600: Add 'interface' to accountlines
Bug 22600: Add 'api' to interface types and set appropriately
Bug 22600: Add 'cron' to interface types and set appropriately
Bug 22600: Set 'commandline' interface appropriately
Bug 22600: Add 'interface' to accountlines
Bug 22600: Add 'api' to interface types and set appropriately
Bug 22600: Add 'cron' to interface types and set appropriately
Bug 22600: Set 'commandline' interface appropriately
Bug 22600: Update Tests adding 'interface' as needed
Bug 22600: Add 'interface' to accountlines
Bug 22600: Add 'api' to interface types and set appropriately
Bug 22600: Add 'cron' to interface types and set appropriately
Bug 22600: Set 'commandline' interface appropriately
Bug 22600: Update Tests adding 'interface' as needed
Bug 22600: Add 'interface' to accountlines
Bug 22600: Add 'api' to interface types and set appropriately
Bug 22600: Add 'cron' to interface types and set appropriately
Bug 22600: Set 'commandline' interface appropriately
Bug 22600: Update Tests adding 'interface' as needed
Bug 22600: (QA follow-up) Raise an exception on missing interface param

Description Martin Renvoize 2019-03-28 08:24:09 UTC
Prior to bug 22008 we were defaulting to a manager_id of '0' wherever we found a userenv was not set.  This roughly equated to signifying that an action had taken place via a script of cronjob... however I'm not sure I saw this behaviour documented anywhere.

With bug 22008 adding constraints to manager_id it highlighted this behaviour and raised the question as to how we should overcome it.

I believe the clearest approach to identifying 'where' an action has taken place would be to add an interface field to the accountlines table and set it for any action.
Comment 1 Martin Renvoize 2019-03-28 10:08:13 UTC
Created attachment 87132 [details] [review]
Bug 22600: Add 'interface' to accountlines

This patch adds the 'interface' field to the accountlines table and
updates all Koha::Object routines and calls to use it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 2 Josef Moravec 2019-03-28 10:48:53 UTC
Comment on attachment 87132 [details] [review]
Bug 22600: Add 'interface' to accountlines

Review of attachment 87132 [details] [review]:
-----------------------------------------------------------------

::: C4/Overdues.pm
@@ +599,5 @@
>                      description => $desc,
>                      note        => undef,
>                      user_id     => undef,
> +                    interface   => C4::Context->interface,
> +                    library_id  => undef, #FIXME: Should we grab the checkout or circ-control branch here perhaps?

On the first view, I think if the Context is set then we should grab it from context, if not, stay with undef

::: C4/SIP/ILS/Transaction/FeePayment.pm
@@ +28,4 @@
>  our $debug = 0;
>  
>  my %fields = ();
> +C4::Context->interface('opac');

Should be 'SIP' I think

::: installer/data/mysql/atomicupdate/bug_22600.perl
@@ +2,5 @@
> +
> +$DBversion = 'XXX';    # will be replaced by the RM
> +if ( CheckVersion($DBversion) ) {
> +
> +    if ( !column_exists( 'accountlines', 'status' ) ) {

should be 'interface' instead of 'status'

@@ +7,5 @@
> +        $dbh->do(
> +            qq{
> +            ALTER TABLE `accountlines`
> +            ADD
> +              `interface` varchar(16) DEFAULT 'script'

'script' default is weird here

@@ +35,5 @@
> +          manager_id != borrowernumber;
> +    });
> +
> +    SetVersion($DBversion);
> +    print "Upgrade to $DBversion done (Bug 22518 - Fix accounttype 'O' to 'FU')\n";

Not right description I think ;)
Comment 3 Martin Renvoize 2019-03-28 11:36:30 UTC
Created attachment 87133 [details] [review]
Bug 22600: Add 'interface' to accountlines

This patch adds the 'interface' field to the accountlines table and
updates all Koha::Object routines and calls to use it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2019-03-28 11:36:32 UTC
Created attachment 87134 [details] [review]
Bug 22600: Add 'api' to interface types and set appropriately

This patch adds 'api' as a valid interface and sets it appropriately for
both the new REST api and previous /svc/ api's.  Handling to keep the
interface of a logged in session is included such that if the OPAC or
Intranet use the API's internally via a Cookie we will maintain the
interface throughout the session.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2019-03-28 12:35:06 UTC
Created attachment 87137 [details] [review]
Bug 22600: Add 'interface' to accountlines

This patch adds the 'interface' field to the accountlines table and
updates all Koha::Object routines and calls to use it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2019-03-28 12:35:09 UTC
Created attachment 87138 [details] [review]
Bug 22600: Add 'api' to interface types and set appropriately

This patch adds 'api' as a valid interface and sets it appropriately for
both the new REST api and previous /svc/ api's.  Handling to keep the
interface of a logged in session is included such that if the OPAC or
Intranet use the API's internally via a Cookie we will maintain the
interface throughout the session.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2019-03-28 12:35:12 UTC
Created attachment 87139 [details] [review]
Bug 22600: Add 'cron' to interface types and set appropriately

This patch adds 'cron' as a valid interface and sets it appropriately for
existing cron scripts.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Josef Moravec 2019-03-28 13:22:42 UTC
Just started testing, but found:

prove t/db_dependent/Koha/Account.t

Field 'interface' doesn't have a default value
Comment 9 Martin Renvoize 2019-03-28 14:34:09 UTC
Created attachment 87140 [details] [review]
Bug 22600: Set 'commandline' interface appropriately

This patch change Koha::Cron to be a more generic Koha::Script class and
update all commanline driven scripts to use it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2019-03-29 12:40:50 UTC
Created attachment 87189 [details] [review]
Bug 22600: Add 'interface' to accountlines

This patch adds the 'interface' field to the accountlines table and
updates all Koha::Object routines and calls to use it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2019-03-29 12:40:54 UTC
Created attachment 87190 [details] [review]
Bug 22600: Add 'api' to interface types and set appropriately

This patch adds 'api' as a valid interface and sets it appropriately for
both the new REST api and previous /svc/ api's.  Handling to keep the
interface of a logged in session is included such that if the OPAC or
Intranet use the API's internally via a Cookie we will maintain the
interface throughout the session.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2019-03-29 12:40:57 UTC
Created attachment 87191 [details] [review]
Bug 22600: Add 'cron' to interface types and set appropriately

This patch adds 'cron' as a valid interface and sets it appropriately for
existing cron scripts.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-03-29 12:41:01 UTC
Created attachment 87192 [details] [review]
Bug 22600: Set 'commandline' interface appropriately

This patch change Koha::Cron to be a more generic Koha::Script class and
update all commanline driven scripts to use it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2019-03-29 12:41:05 UTC
Created attachment 87193 [details] [review]
Bug 22600: Update Tests adding 'interface' as needed
Comment 15 Martin Renvoize 2019-03-29 12:42:36 UTC
Hopefully that most recent patchset should address the test failure and your other comments.. back to NSO
Comment 16 Josef Moravec 2019-04-08 13:29:11 UTC
Created attachment 87515 [details] [review]
Bug 22600: Add 'interface' to accountlines

This patch adds the 'interface' field to the accountlines table and
updates all Koha::Object routines and calls to use it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 17 Josef Moravec 2019-04-08 13:29:14 UTC
Created attachment 87516 [details] [review]
Bug 22600: Add 'api' to interface types and set appropriately

This patch adds 'api' as a valid interface and sets it appropriately for
both the new REST api and previous /svc/ api's.  Handling to keep the
interface of a logged in session is included such that if the OPAC or
Intranet use the API's internally via a Cookie we will maintain the
interface throughout the session.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 18 Josef Moravec 2019-04-08 13:29:18 UTC
Created attachment 87517 [details] [review]
Bug 22600: Add 'cron' to interface types and set appropriately

This patch adds 'cron' as a valid interface and sets it appropriately for
existing cron scripts.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 19 Josef Moravec 2019-04-08 13:29:21 UTC
Created attachment 87518 [details] [review]
Bug 22600: Set 'commandline' interface appropriately

This patch change Koha::Cron to be a more generic Koha::Script class and
update all commanline driven scripts to use it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 20 Josef Moravec 2019-04-08 13:29:25 UTC
Created attachment 87519 [details] [review]
Bug 22600: Update Tests adding 'interface' as needed

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 21 Tomás Cohen Arazi 2019-04-09 15:01:59 UTC
Created attachment 87611 [details] [review]
Bug 22600: Add 'interface' to accountlines

This patch adds the 'interface' field to the accountlines table and
updates all Koha::Object routines and calls to use it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2019-04-09 15:02:04 UTC
Created attachment 87612 [details] [review]
Bug 22600: Add 'api' to interface types and set appropriately

This patch adds 'api' as a valid interface and sets it appropriately for
both the new REST api and previous /svc/ api's.  Handling to keep the
interface of a logged in session is included such that if the OPAC or
Intranet use the API's internally via a Cookie we will maintain the
interface throughout the session.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2019-04-09 15:02:10 UTC
Created attachment 87613 [details] [review]
Bug 22600: Add 'cron' to interface types and set appropriately

This patch adds 'cron' as a valid interface and sets it appropriately for
existing cron scripts.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 24 Tomás Cohen Arazi 2019-04-09 15:02:15 UTC
Created attachment 87614 [details] [review]
Bug 22600: Set 'commandline' interface appropriately

This patch change Koha::Cron to be a more generic Koha::Script class and
update all commanline driven scripts to use it.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2019-04-09 15:02:22 UTC
Created attachment 87615 [details] [review]
Bug 22600: Update Tests adding 'interface' as needed

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2019-04-09 15:02:26 UTC
Created attachment 87616 [details] [review]
Bug 22600: (QA follow-up) Raise an exception on missing interface param

This patch makes add_credit and add_debit raise a
Koha::Exceptions::MissingParameter exception if the 'interface'
parameter is ommited.

The database will fail to generate the line anyways in strict mode, and
we better handle it gracefuly.

Bonus: fixed the TODOs in the tests.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Martin Renvoize 2019-04-09 15:30:13 UTC
Thanks for the follow-up, looks good to me :)
Comment 28 Nick Clemens 2019-04-10 20:14:40 UTC
Awesome work all!

Pushed to master for 19.05
Comment 29 Martin Renvoize 2019-04-15 10:56:29 UTC
Enhancement will not be backported to 18.11.x series.