Bug 16637 - Dependency for C4::Tags not listed
Summary: Dependency for C4::Tags not listed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: Main
Hardware: All All
: P5 - low trivial (vote)
Assignee: Mark Tompsett
QA Contact: Galen Charlton
URL:
Keywords:
Depends on: 16455
Blocks:
  Show dependency treegraph
 
Reported: 2016-06-02 01:29 UTC by Mark Tompsett
Modified: 2017-12-07 22:16 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16637: t/00-load.t warning from C4/Tags.pm (2.36 KB, patch)
2016-06-02 01:32 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16637: Optional Dependency not listed (2.50 KB, patch)
2016-06-04 15:32 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16637: Dependency for C4::Tags not listed (2.78 KB, patch)
2016-06-05 01:07 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16637: Dependency for C4::Tags not listed (3.07 KB, patch)
2016-06-05 07:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 16637: Dependency for C4::Tags not listed (3.09 KB, patch)
2016-06-06 20:01 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2016-06-02 01:29:53 UTC
Added optional dependency, so as to explain why testing
explodes when the Enhanced Content system preference
TagsExternalDictionary is set.

Since INIT runs after BEGIN, moved INIT logic into BEGIN in
order to remove INIT and thus eliminate the warning.

TEST PLAN
---------
1) prove t/00-load.t
   -- warning about INIT for C4::Tags
2) prove `git grep -l Tags | grep [.]t$`
   -- should all run okay
3) apply patch
4) repeat steps 1 and 2
   -- warning should be gone, and everything else run okay
5) run koha qa test tools
Comment 1 Mark Tompsett 2016-06-02 01:32:48 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-06-04 13:32:53 UTC
Is it a duplicate of bug 16455?
Comment 3 Mark Tompsett 2016-06-04 13:52:32 UTC
(In reply to Jonathan Druart from comment #2)
> Is it a duplicate of bug 16455?

Not quite. Because I also expressly add the PerlDependencies.pm which helps people realize things that could explode. I'll rework this to exclude the bug 16455 portion and be dependent on it.
Comment 4 Marcel de Rooy 2016-06-04 15:04:21 UTC
(In reply to M. Tompsett from comment #3)
> (In reply to Jonathan Druart from comment #2)
> > Is it a duplicate of bug 16455?
> 
> Not quite. Because I also expressly add the PerlDependencies.pm which helps
> people realize things that could explode. I'll rework this to exclude the
> bug 16455 portion and be dependent on it.

I passed QA already on 16455.
Please adjust this patch?
Comment 5 Mark Tompsett 2016-06-04 15:09:28 UTC
(In reply to Marcel de Rooy from comment #4)
> I passed QA already on 16455.
> Please adjust this patch?

I will be, but you'll notice I can't cleanly apply 16455. :)
Comment 6 Mark Tompsett 2016-06-04 15:32:40 UTC Comment hidden (obsolete)
Comment 7 Mark Tompsett 2016-06-04 15:36:18 UTC
This bug was initially triggered by what has been resolved in bug 16455. However, other minor side pieces were not dealt with in that bug, so this bug has been repurposed. The test plan in comment #6 reflects this.
Comment 8 Marcel de Rooy 2016-06-04 16:25:32 UTC
Mark,
What about this (without your patch):
If I clear the externaldict pref, the module Tags compiles ok even without the Ispell module.
If I enter a value in the pref, I have the Cant locate Ispell module error.
Sounds weird..

You skip testing now if the module is not installed. That is not taking into consideration the value of the pref, as above.
Comment 9 Marcel de Rooy 2016-06-04 16:28:36 UTC
(In reply to M. Tompsett from comment #5)
> (In reply to Marcel de Rooy from comment #4)
> > I passed QA already on 16455.
> > Please adjust this patch?
> 
> I will be, but you'll notice I can't cleanly apply 16455. :)

Did you git fetch or apply the dependency of 16455..
Comment 10 Mark Tompsett 2016-06-04 16:51:00 UTC
Oh... right... Shoot... tweaking still...
Comment 11 Mark Tompsett 2016-06-05 01:07:31 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2016-06-05 07:15:24 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2016-06-06 20:01:15 UTC
Created attachment 52100 [details] [review]
Bug 16637: Dependency for C4::Tags not listed

Added optional dependency, so as to explain why testing
explodes when the Enhanced Content system preference
TagsExternalDictionary is set. It is optional, because not only
does TagsExternalDictionary have to be set, but TagsEnabled
must be 'Allow'.

Also tweaked C4/Tags.pm to ignore TagsExternalDictionary,
if Lingua::Ispell is not installed. A warning is given.

TEST PLAN
---------
 1) Set the Enhanced Content system preference
    TagsExternalDictionary to /usr/bin/ispell

 2) sudo apt-get install liblingua-ispell-perl
    -- should be a new install

 3) prove t/db_dependent/Tags.t
    -- should work fine

 4) sudo apt-get remove liblingua-ispell-perl

 5) prove t/db_dependent/Tags.t
    -- should explode

 6) Clear the Enhanced Content system preference
    TagsExternalDictionary

 7) prove t/db_dependent/Tags.t
    -- should work fine

 8) apply patch

 9) prove t/db_dependent/Tags.t
    -- should work fine

10) Set the Enhanced Content system preference
    TagsExternalDictionary to /usr/bin/ispell

11) prove t/db_dependent/Tags.t
    -- should work, with warning.

12) sudo apt-get install liblingua-ispell-perl

13) prove t/db_dependent/Tags.t
    -- should work fine

14) run koha qa test tools.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Post-hackfest hotel Olympia lobby signoff. Kalimera!
Works as expected.
At this moment the Tags.t test does not need the database btw,
but the module should have much more test coverage.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Kyle M Hall 2016-06-10 18:05:34 UTC
Pushed to master for Koha 16.11, thanks Mark!
Comment 15 Frédéric Demians 2016-06-15 07:51:15 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 16 Julian Maurice 2016-06-16 10:07:53 UTC
Patch pushed to 3.22.x, will be in 3.22.8