[Koha-bugs] [Bug 22818] ILL should be able to send notices

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22818

Katrin Fischer <katrin.fischer at bsz-bw.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Patch doesn't apply         |Signed Off

--- Comment #50 from Katrin Fischer <katrin.fischer at 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 at 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.