Bug 15630 - Make Edifact module pluggable
Summary: Make Edifact module pluggable
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 7736
Blocks: 16354
  Show dependency treegraph
 
Reported: 2016-01-20 15:14 UTC by Kyle M Hall
Modified: 2017-06-14 22:15 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15630 - Make Edifact module pluggable (10.10 KB, patch)
2016-01-20 18:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15630 - Make Edifact module pluggable (10.05 KB, patch)
2016-01-20 18:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15630 - Make Edifact module pluggable (10.09 KB, patch)
2016-03-17 14:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 15630 - Make Edifact module pluggable (10.14 KB, patch)
2016-03-17 14:39 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 15630 - Make Edifact module pluggable (10.20 KB, patch)
2016-04-01 14:03 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2016-01-20 15:14:34 UTC
Bug 7736 introduces an EDIFACT module that works great for many European vendors, does not work will for US vendors, which have a much different interpretation of 'standard'. In fact, each vendor may pick require different arrangements of values in EDIFACT messages. It would be impossible to encompass all these requirements within Koha's EDIFACT module itself. Instead, we should allow the module to be pluggable, so versions of the module can be developed for vendors that require EDIFACT messages that don't conform to the standard set by Koha's EDIFACT module.
Comment 1 Kyle M Hall 2016-01-20 18:10:18 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2016-01-20 18:14:28 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2016-03-17 14:36:59 UTC Comment hidden (obsolete)
Comment 4 Nicole C. Engard 2016-03-17 14:39:43 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize 2016-04-01 14:03:55 UTC
Created attachment 49788 [details] [review]
Bug 15630 - Make Edifact module pluggable

Koha's EDIFACT module works great for many European vendors,
but does not work will for US vendors, which have a much different
interpretation of 'standard'. In fact, each vendor may require
different arrangements of values in EDIFACT messages. It would be
impossible to encompass all these requirements within Koha's EDIFACT
module itself. Instead, we should allow the module to be pluggable, so
versions of the module can be developed for vendors that require EDIFACT
messages that don't conform to the standard set by Koha's EDIFACT
module.

Test Plan:
1) Apply this patch
2) Run updatedatabase
3) Enable Koha plugins
4) Install the Edifact stub plugin available at
   https://github.com/bywatersolutions/koha-plugin-edifact-stub
5) Edit the EDI Vendor account, assign the plugin to a Vendor EDI account
6) Test EDI functionality ( ORDER, INVOICE ), there should be no errors
   or changes to the EDIFACT message input or output

Signed-off-by: Jason DeShaw <JDeShaw@cityoffargo.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2016-04-01 14:04:36 UTC
Nice enhancement, should make things much more extensible.. Go forth :)
Comment 7 Brendan Gallagher 2016-04-01 21:36:51 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!

RM note - I am now running the dbix schema update for both this and bug 7736 at the same time.
Comment 8 Jonathan Druart 2016-04-05 09:22:09 UTC
aqbooksellers.plugin has not been added to existing installations
Comment 9 Jonathan Druart 2016-04-05 09:22:24 UTC
(In reply to Jonathan Druart from comment #8)
> aqbooksellers.plugin has not been added to existing installations

vendor_edi_accounts.plugin!
Comment 10 Brendan Gallagher 2016-04-05 15:58:42 UTC
*Manually added in the missing updatedatabase.pl for .plugin
Comment 11 Jonathan Druart 2016-04-26 15:26:03 UTC
This patch makes the t/db_dependent/TestBuilder.t tests fail:

t/db_dependent/TestBuilder.t .. 3/41 DBD::mysql::st execute failed: Cannot delete or update a parent row: a foreign key constraint fails (`koha_empty`.`edifact_messages`, CONSTRAINT `emfk_basketno` FOREIGN KEY (`basketno`) REFERENCES `aqbasket` (`basketno`)) [for Statement "DELETE FROM `aqbasket` WHERE ( `basketno` = ? )" with ParamValues: 0='13'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.

the foreign keys don't not have a ON * CASCADE clause.
Comment 12 Kyle M Hall 2016-04-26 15:56:13 UTC
(In reply to Jonathan Druart from comment #11)
> This patch makes the t/db_dependent/TestBuilder.t tests fail:
> 
> t/db_dependent/TestBuilder.t .. 3/41 DBD::mysql::st execute failed: Cannot
> delete or update a parent row: a foreign key constraint fails
> (`koha_empty`.`edifact_messages`, CONSTRAINT `emfk_basketno` FOREIGN KEY
> (`basketno`) REFERENCES `aqbasket` (`basketno`)) [for Statement "DELETE FROM
> `aqbasket` WHERE ( `basketno` = ? )" with ParamValues: 0='13'] at
> /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1834.
> 
> the foreign keys don't not have a ON * CASCADE clause.

Filed bug 16354
Comment 13 David Cook 2016-05-30 06:11:41 UTC
Comment on attachment 49788 [details] [review]
Bug 15630 - Make Edifact module pluggable

Review of attachment 49788 [details] [review]:
-----------------------------------------------------------------

::: Koha/Plugins.pm
@@ +20,4 @@
>  use Modern::Perl;
>  
>  use Module::Load::Conditional qw(can_load);
> +use Module::Pluggable search_path => ['Koha::Plugin'], except => qr/::Edifact(|::Line|::Message|::Order|::Segment|::Transport)$/;

This seems suboptimal... since any plugin with additional packages would need an exception.

Wouldn't it make more sense to use "only" to only find plugins like ^Koha::Plugin::Com::CompanyName::PluginName$.

I suppose it might be too late for that now as there's any number of plugins that can be allowed so perhaps one does have to only use exceptions from now on... 

I'm thinking of making a plugin, but I'll probably just use a different namespace like Local::PluginName::Submodules...
Comment 14 Marcel de Rooy 2016-05-30 08:28:49 UTC
(In reply to David Cook from comment #13)

> > +use Module::Pluggable search_path => ['Koha::Plugin'], except => qr/::Edifact(|::Line|::Message|::Order|::Segment|::Transport)$/;
> 
> This seems suboptimal... since any plugin with additional packages would
> need an exception.

Have the same feeling when I see these lines..
Comment 15 Kyle M Hall 2016-06-07 16:28:55 UTC
(In reply to Marcel de Rooy from comment #14)
> (In reply to David Cook from comment #13)
> 
> > > +use Module::Pluggable search_path => ['Koha::Plugin'], except => qr/::Edifact(|::Line|::Message|::Order|::Segment|::Transport)$/;
> > 
> > This seems suboptimal... since any plugin with additional packages would
> > need an exception.
> 
> Have the same feeling when I see these lines..

I agree. I'm sure there is a better solution!