Bug 24016 - manager_id in Koha::Patron::Message->store should not depend on userenv alone
Summary: manager_id in Koha::Patron::Message->store should not depend on userenv alone
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Lari Taskula
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on:
Blocks: 23998
  Show dependency treegraph
 
Reported: 2019-11-12 12:15 UTC by Lari Taskula
Modified: 2021-06-14 21:28 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Using `userenv` within Koha::* object classes is deprecated in favour of passing parameters.
Version(s) released in:
20.05.00, 19.11.02, 19.05.08, 18.11.14


Attachments
Bug 24016: Add a failing test for overwrite-able manager_id in Koha::Patron::Message (4.57 KB, patch)
2019-11-12 12:26 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 24016: manager_id in Koha::Patron::Message->store should not depend on userenv alone (1.35 KB, patch)
2019-11-12 12:27 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 24016: Add a failing test for overwrite-able manager_id in Koha::Patron::Message (4.60 KB, patch)
2019-11-12 12:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 24016: manager_id in Koha::Patron::Message->store should not depend on userenv alone (1.38 KB, patch)
2019-11-12 12:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 24016: Add a failing test for overwrite-able manager_id in Koha::Patron::Message (4.65 KB, patch)
2019-11-15 19:24 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 24016: manager_id in Koha::Patron::Message->store should not depend on userenv alone (1.43 KB, patch)
2019-11-15 19:24 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 24016: Add a failing test for overwrite-able manager_id in Koha::Patron::Message (4.81 KB, patch)
2019-12-17 14:14 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 24016: manager_id in Koha::Patron::Message->store should not depend on userenv alone (1.58 KB, patch)
2019-12-17 14:15 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 24016: (QA follow-up) Format code to adhere to perltidy format (1.13 KB, patch)
2019-12-17 14:15 UTC, Joonas Kylmälä
Details | Diff | Splinter Review
Bug 24016: (RM follow-up) Remove userenv requirement entirely (1.47 KB, patch)
2020-01-02 15:16 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2019-11-12 12:15:30 UTC
Koha::Patron::Message->store has this

>     my $userenv = C4::Context->userenv;
>     $self->manager_id( $userenv ? $userenv->{number} : undef);

This breaks in REST API when userenv is not set.

Let API user pass manager_id without relying on C4::Context->userenv.
Comment 1 Lari Taskula 2019-11-12 12:26:57 UTC
Created attachment 95299 [details] [review]
Bug 24016: Add a failing test for overwrite-able manager_id in Koha::Patron::Message

To test:
1. prove t/db_dependent/Koha/Patron/Messages.t
2. Observe failure

t/db_dependent/Koha/Patron/Messages.t .. 2/12
   Failed test 'Manager id should be overwrite-able even if defined in userenv'
   at t/db_dependent/Koha/Patron/Messages.t line 90.
          got: '1100'
     expected: '1101'
 Looks like you failed 1 test of 12.
t/db_dependent/Koha/Patron/Messages.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/12 subtests
Comment 3 Lari Taskula 2019-11-12 12:27:59 UTC
Created attachment 95300 [details] [review]
Bug 24016: manager_id in Koha::Patron::Message->store should not depend on userenv alone

Koha::Patron::Message->store has this

    my $userenv = C4::Context->userenv;
    $self->manager_id( $userenv ? $userenv->{number} : undef);

This breaks in REST API when userenv is not set.

Let API user pass manager_id without relying on C4::Context->userenv.

To test:
1. prove t/db_dependent/Koha/Patron/Messages.t
2. Observe success
Comment 4 Lari Taskula 2019-11-12 12:29:26 UTC
Created attachment 95301 [details] [review]
Bug 24016: Add a failing test for overwrite-able manager_id in Koha::Patron::Message

To test:
1. prove t/db_dependent/Koha/Patron/Messages.t
2. Observe failure

t/db_dependent/Koha/Patron/Messages.t .. 2/12
   Failed test 'Manager id should be overwrite-able even if defined in userenv'
   at t/db_dependent/Koha/Patron/Messages.t line 90.
          got: '1100'
     expected: '1101'
 Looks like you failed 1 test of 12.
t/db_dependent/Koha/Patron/Messages.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/12 subtests

Sponsored-by: Koha-Suomi Oy
Comment 5 Lari Taskula 2019-11-12 12:29:37 UTC
Created attachment 95302 [details] [review]
Bug 24016: manager_id in Koha::Patron::Message->store should not depend on userenv alone

Koha::Patron::Message->store has this

    my $userenv = C4::Context->userenv;
    $self->manager_id( $userenv ? $userenv->{number} : undef);

This breaks in REST API when userenv is not set.

Let API user pass manager_id without relying on C4::Context->userenv.

To test:
1. prove t/db_dependent/Koha/Patron/Messages.t
2. Observe success

Sponsored-by: Koha-Suomi Oy
Comment 6 Joonas Kylmälä 2019-11-15 12:38:16 UTC
I think I'm missing some context from here. Why manager id should be overwrite-able even if defined in userenv?
Comment 7 Lari Taskula 2019-11-15 13:47:56 UTC
(In reply to Joonas Kylmälä from comment #6)
> I think I'm missing some context from here. Why manager id should be
> overwrite-able even if defined in userenv?

The point is that userenv should not be the only way to set manager_id (or any other values of Koha::Objects), because we can not expect userenv to be defined. It breaks in REST API when you are using basic auth method.

Also, it's not a very nice idea to forcefully restrict developers using models in such way. I think the use of userenv should only be a fall-back method of accessing user data in Koha::Objects.
Comment 8 Maryse Simard 2019-11-15 19:24:13 UTC
Created attachment 95461 [details] [review]
Bug 24016: Add a failing test for overwrite-able manager_id in Koha::Patron::Message

To test:
1. prove t/db_dependent/Koha/Patron/Messages.t
2. Observe failure

t/db_dependent/Koha/Patron/Messages.t .. 2/12
   Failed test 'Manager id should be overwrite-able even if defined in userenv'
   at t/db_dependent/Koha/Patron/Messages.t line 90.
          got: '1100'
     expected: '1101'
 Looks like you failed 1 test of 12.
t/db_dependent/Koha/Patron/Messages.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/12 subtests

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 9 Maryse Simard 2019-11-15 19:24:15 UTC
Created attachment 95462 [details] [review]
Bug 24016: manager_id in Koha::Patron::Message->store should not depend on userenv alone

Koha::Patron::Message->store has this

    my $userenv = C4::Context->userenv;
    $self->manager_id( $userenv ? $userenv->{number} : undef);

This breaks in REST API when userenv is not set.

Let API user pass manager_id without relying on C4::Context->userenv.

To test:
1. prove t/db_dependent/Koha/Patron/Messages.t
2. Observe success

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Comment 10 Joonas Kylmälä 2019-12-17 14:14:57 UTC
Created attachment 96379 [details] [review]
Bug 24016: Add a failing test for overwrite-able manager_id in Koha::Patron::Message

To test:
1. prove t/db_dependent/Koha/Patron/Messages.t
2. Observe failure

t/db_dependent/Koha/Patron/Messages.t .. 2/12
   Failed test 'Manager id should be overwrite-able even if defined in userenv'
   at t/db_dependent/Koha/Patron/Messages.t line 90.
          got: '1100'
     expected: '1101'
 Looks like you failed 1 test of 12.
t/db_dependent/Koha/Patron/Messages.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/12 subtests

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 11 Joonas Kylmälä 2019-12-17 14:15:00 UTC
Created attachment 96380 [details] [review]
Bug 24016: manager_id in Koha::Patron::Message->store should not depend on userenv alone

Koha::Patron::Message->store has this

    my $userenv = C4::Context->userenv;
    $self->manager_id( $userenv ? $userenv->{number} : undef);

This breaks in REST API when userenv is not set.

Let API user pass manager_id without relying on C4::Context->userenv.

To test:
1. prove t/db_dependent/Koha/Patron/Messages.t
2. Observe success

Sponsored-by: Koha-Suomi Oy
Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 12 Joonas Kylmälä 2019-12-17 14:15:03 UTC
Created attachment 96381 [details] [review]
Bug 24016: (QA follow-up) Format code to adhere to perltidy format

Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 13 Martin Renvoize 2020-01-02 15:16:51 UTC
Created attachment 96761 [details] [review]
Bug 24016: (RM follow-up) Remove userenv requirement entirely

Koha::* Object based classes would not require a userenv to be set to
function correctly at all.
Comment 14 Martin Renvoize 2020-01-02 16:19:14 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 15 Joy Nelson 2020-01-18 14:58:02 UTC
Pushed to 19.11.x branch for 19.11.02
Comment 16 Lucas Gass 2020-01-31 21:16:07 UTC
applying this patchset to 19.05.x does not fix failing tests:

root@9d08dec7451a:koha(rmain1905)$ kshell
kohadev-koha@9d08dec7451a:/kohadevbox/koha$ prove t/db_dependent/Koha/Patron/Messages.t
t/db_dependent/Koha/Patron/Messages.t .. DBD::mysql::st execute failed: Unknown column 'guarantorid' in 'field list' [for Statement "INSERT INTO `borrowers` ( `B_address`, `B_address2`, `B_city`, `B_country`, `B_email`, `B_phone`, `B_state`, `B_streetnumber`, `B_streettype`, `B_zipcode`, `address`, `address2`, `altcontactaddress1`, `altcontactaddress2`, `altcontactaddress3`, `altcontactcountry`, `altcontactfirstname`, `altcontactphone`, `altcontactstate`, `altcontactsurname`, `altcontactzipcode`, `anonymized`, `borrowernotes`, `branchcode`, `cardnumber`, `categorycode`, `checkprevcheckout`, `city`, `contactfirstname`, `contactname`, `contactnote`, `contacttitle`, `country`, `date_renewed`, `dateenrolled`, `dateexpiry`, `dateofbirth`, `debarred`, `debarredcomment`, `email`, `emailpro`, `fax`, `firstname`, `flags`, `gonenoaddress`, `guarantorid`, `initials`, `lang`, `lastseen`, `login_attempts`, `lost`, `mobile`, `opacnote`, `othernames`, `overdrive_auth_token`, `password`, `phone`, `phonepro`, `privacy`, `privacy_guarantor_checkouts`, `relationship`, `sex`, `sms_provider_id`, `smsalertnumber`, `sort1`, `sort2`, `state`, `streetnumber`, `streettype`, `surname`, `title`, `updated_on`, `userid`, `zipcode`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0='dsfXiD0bhN_Iri6D8_7Rve0MH_Le6eYtfogBFV35tjHc_OYo_5ZGbMlCSokxv1sESnPqhVykkN5qZJyXRky8ySiYte9RvGybEQ', 1='bdRSyw0kq', 2='fPBf1Ji', 3='u7RsyL', 4='OfcT6ffQf', 5='rcD6M_iG3m', 6='fnUS5CyoXp', 7='EWseWBt', 8='ju_b4e_TdcgwUJPFR_IsUSZvBeb1sLK2uLNJUibWvx4F6GcAK', 9='T2Q22IQFrTo_og9QMbBml', 10='gVpx0mTmSG', 11='wyGJtbEs', 12='qkd0eaSUnb42MW8oAvWDOU_16KWfnVP3PIfX8sdNO0Cm8RtosH95MT5m2yhsUVRx5lV16MNPi8Oi48Ru4QxbiHBUif8xOBK3pRg4PgRwKPjjqRzp1SyNW5NmcTMuil0acZIlDOE1rslDAVwyMHYktiS5Eqg1VAnE4gYrQsFruC', 13='msGSC825iBX4M7OuRhQKuXV__tjKAXUQriwPvwGn_aYNAViOC5uijMZux6t1bhmO2525PGllcOVeTr3XxI5IJ8uZVg4R9d4P0HfArsfbypi77fIvn0QptQSy0aQI1ESte', 14='xFiiV7F5x7fzKv1cmeiUnj7Ja8YNfRDsKFThBs7p1ILYqaMxnDLEp3u3yjL3ka1dTyOIagySKjuyDrr06TkIeZroMD9jIogz1icBzErdC_n5aRO3TPnnXaE6Kdd2hS5KxenbFKBFC08HZxHXuAUCtMywcpdqgO6hBHdVO1', 15='Hnxbu1lirw', 16='nT8k9RiiweO3D8FBqcjX_7fhmUOILpfp7IdYji5VFENw71YjkFUzZbNVBiHc9TBOnnp7noM3im7ID8cJ6SAHNTKn6jtf_3A5uGSiwnDOF5yiV0tGaMgJwnlNs_l9avyHCctVVJhajlwpB1EEdC6T3aYVxc8W8eqcb', 17='vqaQ98adCShrZ2wox54FH6PLC2q38JbXzqoB0XSENaew3Q', 18='Uh53jcZP', 19='sPdyHqyhvm0vLW4AhhSEb18Tu7NK0zhV6DGiiybwJjBHdsMgzuIIb3gSNuK6YTSCG97iTYKTjxOB9fZlQsmvBEjNy66wBbxC3rAfBj8ss92IfpLMGQiKPzJlcd3byfDnLgJsrBTPwFx8lYXHWXkHCiOjLncZOajTkKnslo7y_qMmsrTEx5gy5FQoqmsTBpmLLMm_sARQQUJSg5AzLNCfYDN1sjsuO1cnsensXJOvx6Yffiv38IrfAeG', 20='bntUYrlkCwkWTb9xbS6mV2CUlsFf2AJAn2AqV3OMtWk4lA9P', 21=56, 22='', 23='DSJhVvn4Rw', 24='yqTQOXHdMpWkM8MRQ4c', 25='MCKEl6hMOe', 26='n5VnCO', 27='o2_hMs', 28='pXGEplsk', 29='hsxs6d', 30='DeOgCD01Wtr1SKYOraMafMIc9CunUnuIpHVBepXrhwSFaQuurMK7Qb5hHVSFYZMe1Buc8maMr9_6FYXNxgj1udChCuY__FHkimdHbN5m1BZDCCvvS2MxBay6GOwVKzygchoJaP9PCTMXTg0UKFGUdjwcY8T6WVFgSnVRPoOhweIuUym7dHh6W4_g1aHI0eN9OLxvaoO4ZIDGd4DeR3', 31='O15Xgh2kA', 32='esAtWtG3Vo', 33='2020-01-31', 34='2020-01-31', 35='2020-01-31', 36='2020-01-31', 37=undef, 38='BXq2R24Cemae3ZSEWGHF4nAlDGspvadGk2S9ku3W6eILaK9LeaYk33ASJHrjneiK7zS1qohjhRvVUGPkQCl8j2lGTuLJZnZDXuD4_w3m5hK83KkSMBfoxU_ejqjrIfY5PCwfITmBrNMuy4Px_U_Kkwgt5qOCttmbEDtgXYFytHWTvh5YyEsEXMmh0eglmtYcLqqfJFKQWGIxd', 39='BfKNDY6N', 40='xJn3_EftwI', 41='UwRvRu9', 42='e0eCU28XIv', 43=1714647854, 44=undef, 45=1954786734, 46='UrQkAgLU0', 47='KyxyqZ3Enl2H7O7oz8', 48='2020-01-31 21:14:38', 49=0, 50=undef, 51='Ibpe0jP90_9g89G6nzikMttyeq_UXk7l4SAZxMcqyhU4k91o', 52='SmgzznOrL', 53='aWZwno', 54='puTMcILF', 55='ygiTDJwvBllqExheHESxfaEYF3sOdBVzp2mI4H', 56='w_GhFSP', 57='YRx8C6ECo4', 58=1639155680, 59=6, 60='dCd5tojJQRqjPPl80wkdfezqdGcNfv5pWNKg0zx0jPXL9XjBEuX25Xv', 61='r', 62='5', 63='KnBQOrV3VAh7nJtmT_ChLplMPYrgdHfku1pRID', 64='UUJAorN7pcaewsmuCNJMSaA36VY8mgEtFamIfk4cFtumSvG3d6T7FuQtqOwtN', 65='gsgbBTxsN0RW3WPlwq4O2hkKJN9TKljl4js4sdOb2929bajH4We3oNCO', 66='T66bhpE', 67='jnbyDwz', 68='Y51EY5pVjPYLWNlt8_iyqTXokQas1oYRrEaak9vydyAEhSw85', 69='YEKi_OwDG', 70='AX7qez', 71='2020-01-31 21:14:38', 72='c6wOzffrV8eOg6D0FltAURYZqSy0eiXcTWWrrvCf7daQNzYu4XhQWGql3O7pBCd4_sbcP5ui_D', 73='bqqDQdFa5RrvkAufRbZuPdt'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
DBIx::Class::Storage::DBI::_dbh_execute(): Unknown column 'guarantorid' in 'field list' at /kohadevbox/koha/t/lib/TestBuilder.pm line 358
# Looks like your test exited with 255 before it could output anything.
t/db_dependent/Koha/Patron/Messages.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 12/12 subtests
Comment 17 Lucas Gass 2020-01-31 22:54:52 UTC
please ignore my previous comment. I wonked my database, this patchset works
Comment 18 Lucas Gass 2020-02-05 20:43:48 UTC
backported to 19.05.x for 19.05.08
Comment 19 Hayley Pelham 2020-02-20 03:47:37 UTC
Backported to 18.11.x for 18.11.14