[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

--- Comment #71 from Martin Renvoize <martin.renvoize at ptfs-europe.com> ---
(In reply to Katrin Fischer from comment #50)
> 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)

Fixed 

> 
> 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

Fixed

> 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?


Fixed

> 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',

Fixed

> 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. 

Converted to TT syntax and added to sampl_notices.yml

> 3.4) Sysprefs.sql
> 
> New prefs are in the wrong alphabetic spot :)

FIxed

> 3.5) Notices & slips
> 
> The module in the letters summary table is not translatable yet and shows as
> "ill".

Was a copy/paste error.. also fixed

> -------------
> 
> 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 first step
> here.

-- 
You are receiving this mail because:
You are watching all bug changes.