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.
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
See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24003#c3
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
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
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
I think I'm missing some context from here. Why manager id should be overwrite-able even if defined in userenv?
(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.
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>
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>
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>
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>
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>
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.
Nice work everyone! Pushed to master for 20.05
Pushed to 19.11.x branch for 19.11.02
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
please ignore my previous comment. I wonked my database, this patchset works
backported to 19.05.x for 19.05.08
Backported to 18.11.x for 18.11.14