Bug 22600 - We should add an 'interface' field to accountlines
Summary: We should add an 'interface' field to accountlines
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P2 blocker (vote)
Assignee: Martin Renvoize
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 22512
Blocks: 22521 22603 22755 23193 24377 25845 26086
  Show dependency treegraph
 
Reported: 2019-03-28 08:24 UTC by Martin Renvoize
Modified: 2021-06-14 21:29 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00


Attachments
Bug 22600: Add 'interface' to accountlines (20.48 KB, patch)
2019-03-28 10:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22600: Add 'interface' to accountlines (20.70 KB, patch)
2019-03-28 11:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22600: Add 'api' to interface types and set appropriately (4.80 KB, patch)
2019-03-28 11:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22600: Add 'interface' to accountlines (21.62 KB, patch)
2019-03-28 12:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22600: Add 'api' to interface types and set appropriately (4.80 KB, patch)
2019-03-28 12:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22600: Add 'cron' to interface types and set appropriately (24.38 KB, patch)
2019-03-28 12:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22600: Set 'commandline' interface appropriately (44.91 KB, patch)
2019-03-28 14:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22600: Add 'interface' to accountlines (21.58 KB, patch)
2019-03-29 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22600: Add 'api' to interface types and set appropriately (4.80 KB, patch)
2019-03-29 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22600: Add 'cron' to interface types and set appropriately (24.38 KB, patch)
2019-03-29 12:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22600: Set 'commandline' interface appropriately (44.91 KB, patch)
2019-03-29 12:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22600: Update Tests adding 'interface' as needed (48.81 KB, patch)
2019-03-29 12:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22600: Add 'interface' to accountlines (21.71 KB, patch)
2019-04-08 13:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22600: Add 'api' to interface types and set appropriately (4.86 KB, patch)
2019-04-08 13:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22600: Add 'cron' to interface types and set appropriately (24.68 KB, patch)
2019-04-08 13:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22600: Set 'commandline' interface appropriately (45.45 KB, patch)
2019-04-08 13:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22600: Update Tests adding 'interface' as needed (48.91 KB, patch)
2019-04-08 13:29 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22600: Add 'interface' to accountlines (21.69 KB, patch)
2019-04-09 15:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22600: Add 'api' to interface types and set appropriately (4.91 KB, patch)
2019-04-09 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22600: Add 'cron' to interface types and set appropriately (24.49 KB, patch)
2019-04-09 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22600: Set 'commandline' interface appropriately (45.02 KB, patch)
2019-04-09 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22600: Update Tests adding 'interface' as needed (48.92 KB, patch)
2019-04-09 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22600: (QA follow-up) Raise an exception on missing interface param (14.52 KB, patch)
2019-04-09 15:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.