https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22818
Katrin Fischer <katrin.fisc...@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off --- Comment #50 from Katrin Fischer <katrin.fisc...@bsz-bw.de> --- I am still working on this, but adding some first notes and questions: 1) QA test tools FAIL Koha/MessageAttributes.pm FAIL forbidden patterns forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545) (line 16) FAIL Koha/MessageAttribute.pm FAIL forbidden patterns forbidden pattern: Incorrect license statement (using postal address), may be a false positive if the file is coming from outside Koha (bug 24545) (line 16) 2) Unit tests While tests pass, they produce some non-good looking output: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Illrequests.t t/db_dependent/Illrequests.t .. 7/12 Use of uninitialized value $staff_to_send in pattern match (m//) at /home/vagrant/kohaclone/Koha/Illrequest.pm line 1522. Koha::Illrequest::send_staff_notice(Koha::Illrequest=HASH(0x55b06c17ba40), "ILL_REQUEST_CANCEL") called at /home/vagrant/kohaclone/Koha/Illrequest.pm line 288 Koha::Illrequest::status(Koha::Illrequest=HASH(0x55b06c17ba40), "CANCREQ") called at t/db_dependent/Illrequests.t line 788 main::__ANON__() called at /usr/share/perl5/Test/Builder.pm line 310 eval {...} called at /usr/share/perl5/Test/Builder.pm line 310 Test::Builder::subtest(Test::Builder=HASH(0x55b06bac3bf8), "Helpers", CODE(0x55b063112be8)) called at /usr/share/perl5/Test/More.pm line 807 Test::More::subtest("Helpers", CODE(0x55b063112be8)) called at t/db_dependent/Illrequests.t line 934 t/db_dependent/Illrequests.t .. ok All tests successful. Files=1, Tests=12, 32 wallclock secs ( 0.09 usr 0.03 sys + 21.40 cusr 6.88 csys = 28.40 CPU) Result: PASS 3) Code Review 3.1) Email addressing I feel like here is a general issue here in that we will have problems with spam protection if the mail server is not in the same domain as the branchillemail given. Is the the new ILL staff email is to be used as the sender or the recipient of the ILL emails to staff? It looks like it's actually used as both? As the pref only reads "to", maybe we could just keep the current from and it would resolve a lot of the possible issues with spam. I think we need to at least make use of $library->inbound_email_address in a couple of cases replacing the branchemail here: + my $from = $branch->branchillemail || $branch->branchemail; get_staff_to_address seems ok in that regard, but send_staff_notice from_adress does not. Reply-to and From are the spam critical ones. Use of Koha::Email is removed? 3.2) Translatability The "N/A" here are problematic. I think it would be better to just create an empty string? With TT libraries can still resolve that to another text. + substitute => { + ill_bib_title => $title ? $title->value : 'N/A', + ill_bib_author => $author ? $author->value : 'N/A', 3.3) Sample letters I need to take a closer look for typos and such, but the sample letters need to also be added to the new sample .yml and to the translated installers. 3.4) Sysprefs.sql New prefs are in the wrong alphabetic spot :) 3.5) Notices & slips The module in the letters summary table is not translatable yet and shows as "ill". ------------- Most of those are not bad ones, only 3.1 Email addressing worries me a fair bit. Clearing up intentions and intended behaviour would be a good firs step here. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list Koha-bugs@lists.koha-community.org https://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/