Bug 15630

Summary: Make Edifact module pluggable
Product: Koha Reporter: Kyle M Hall <kyle>
Component: AcquisitionsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: amy, brendan, jonathan.druart, m.de.rooy, martin.renvoize, nicole
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 7736    
Bug Blocks: 16354    
Attachments: Bug 15630 - Make Edifact module pluggable
Bug 15630 - Make Edifact module pluggable
Bug 15630 - Make Edifact module pluggable
Bug 15630 - Make Edifact module pluggable
Bug 15630 - Make Edifact module pluggable

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!