[Koha-bugs] [Bug 20478] Advance notices: send separate digest messages per branch

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

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

Martin Renvoize <martin.renvoize at ptfs-europe.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Signed Off                  |Passed QA
                 CC|                            |martin.renvoize at ptfs-europe
                   |                            |.com

--- Comment #23 from Martin Renvoize <martin.renvoize at ptfs-europe.com> ---
(In reply to Jonathan Druart from comment #18)
> Great to see tests for this code, even if done in an unusual way.
> 
> Some quick remarks:
> 1. Transactions must be done using txn_begin (also note that you are setting
> AutoCommit off in the sub and rollback at the end => no sense)
> 
> 2. $ENV{"OVERRIDE_SYSPREF_dateformat"} = 'metric';
> Why not set_preference? Note that for test we usually use mock_preference
> (from t::lib::Mocks) to avoid to mess with the cache.
> 
> 3. You could use build_sample_item from bug 21971 to simplify the objects
> creation (not pushed yet)
> 
> 4. use Koha::DateUtils instead of DateTime directly
> 
> 5. You could use File::Slurp::read_file to simplify a bit the code
> 
> I would have preferred to see the code moved to a module. It would have
> eased the write of the tests and made the code reusable.
> 
> Keeping the SO status to get other QA opinions.

I've cleaned up the test a little to make it a bit more 'koha', but I've not
gone as far as Jonathan suggests above.. I feel these additional clean ups
could be handled separately, like the factoring out of code into a module for
example.

Code works and passes all tests.. It's also great to see a cronscript with
tests.

Passing QA

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