Direct EDI generation for ACQ orders -- AKA kill ruby webrick

Bug #1373690 reported by Bill Erickson
30
This bug affects 4 people
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
Wishlist
Unassigned

Bug Description

In bug #1065270 we replaced the external Ruby layer for EDI parsing with an internal Perl parser. Given some recent complications with installing the Ruby dependencies (Bug #1342227) AND given that removing the Ruby layer entirely would simplify installation and maintenance... Maybe now is the time to kill it.

To remove it, we need only to be able to produce EDI ORDER records natively in Evergreen. Currently for orders, we generate JEDI, a custom JSON representation of EDI. The Ruby layer translates the JEDI to EDI. Instead of doing this, we could just produce EDI text instead from a new action/trigger template, then skip the Ruby translation.

Once the A/T event produces EDI, edi_pusher can skip the ruby step and push the output directly to the vendor.

I've done some simple proof of concept work on this. Working branch forthcoming.

Revision history for this message
Bill Erickson (berick) wrote :
Revision history for this message
Bill Erickson (berick) wrote :

Code is still unsquashed and in progress, but much testing has occurred as a result of the hackfest (and beyond). It's not ready for a pullrequest yet, but I'd like to encourage anyone who can to test.

Testing by comparing new EDI to existing EDI for the same order:

 * Install the branch and run the DB upgrade (XXXX.data.acq-order-edi.sql).

 * Reset an activated PO so it can be re-activated. For this example, assume PO ID is 3.

UPDATE acq.purchase_order SET order_date = null, state = 'pending' WHERE id = 3

 * Load PO #3 in the interface and activate it.

 * Wait for action_trigger_runner.pl to process the event or run it manually. This will result in EDI output for the action/trigger event.

 * Locate the EDI output for the re-activated PO.

SELECT out.data FROM action_trigger.event_output out JOIN action_trigger.event evt ON (evt.template_output = out.id) WHERE evt.event_def = 54 AND evt.target = 3;

* Compare the old EDI (from acq.edi_message) with the new EDI (from above) OR paste the values somewhere and point me at them.

* If you run edi_pusher.pl on the test machine, avoid any unexpected behavior (trying to re-send the order -- which would fail for parsing reasons), set the state of the new event to 'error':

UPDATE action_trigger.event SET state = 'error' WHERE event_def = 54 AND target = 3;

Caveats:

 * The template is based on the stock seed JEDI template (event_def 23), which may have settings that differ from your local template (e.g. INC_COPIES = true/false).

 * This code does not disable the JEDI template. It will still operate as before.

 * Some fields contain dates and will generally differ across versions if comparing via 'diff', etc.

Revision history for this message
Bill Erickson (berick) wrote :

One more caveat... the new code is a little more aggressive about scrubbing special characters (?,:, etc.) from text fields, which primarily affects titles and authors, so some text values may look a little different.

Revision history for this message
Bill Erickson (berick) wrote :

I've decided to take this a step further. In combination with bug #1513872 (moving EDI toggles into the database), I'm working on a branch to generate EDI directly within the Perl code, no Ruby or Action/Trigger required.

The code is far enough along at this point to generate what appears to be sane and correct EDI. It is not yet integrated into the EDI delivery process.

To test:

1. Install the new code and SQL changes
2. cd Open-ILS/src/support-scripts/test-scripts
3. ./edi_writer.pl --po-id <test-PO-ID-of-choice>
4. The script prints EDI to STDOUT.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1513872-acq-edi-account-attrs-wip

==

By moving everything in the Perl code, we lose the flexibility we had using A/T templates. The goal is to represent all vendor-specific tweaks as edi_attr's (from the new SQL) so that local template tweaks are not required. (For my part, editing Perl instead of TT templates and removing the A/T steps make building and testing this code significantly faster and easier).

Question for the room... How did you modify your EDI (event_def 23) templates from the stock version?

Revision history for this message
Bill Erickson (berick) wrote :

More code pushed with attrs to handle all EDI quirks I'm aware of.

I also pushed a companion to edi_pusher.pl called (for now, anyway) edi_order_pusher.pl. I used a different name hoping to deprecate the current EDI generation code over a period of releases and not simply cut it off.

Test it like so:

$ ./edi_order_pusher.pl --test-mode --po-id 12345

With no --po-id, all pending EDI orders will be processed. In --test-mode, no data is written or delivered, only printed to STDOUT.

TODO:
- copy upgrade SQL to baseline
- figure out what to do about unit tests
- more human testing w/ different vendors and environments.

Revision history for this message
Bill Erickson (berick) wrote :

Looking for volunteers to test and compare EDI data from the wild using the new code.

How to test and compare:

0. Install the working branch, including SQL upgrade script.

1. Find a PO that has an edi_message with message_type = ORDERS.

2. Apply an EDI Attribute Set to the EDI account which was used to generate the PO EDI in question.

2a) View stock attribute sets:

select * from acq.edi_attr_set;

2b) From above, get the ID of the attribute set that best matches the edi_account by vendor name. Then link an attribute set to the edi_account in question.

update acq.edi_account set attr_set = <attr_set_id> where id = <edi_account_id>;

3. Generate EDI for said PO using the new code:
  $ ./edi_order_pusher.pl --test-mode --po-id 12345

4. Compare the new EDI to the EDI from the existing edi_message.

Fields containing dates (UNB, DTM) will differ to some degree, but other fields and overall message structure should match exactly.

5. Do this with various vendors / accounts.

The code has base attribute sets for known vendors matching the existing stock A/T JEDI template. Local modifications or vendors that are not represented may require either a change to values within the new EDI Attribute Set tables or new code.

Post tests, discrepancies, questions, etc. here.

Thanks to all.

Revision history for this message
Bill Erickson (berick) wrote :

Rebased to current master and did some heavy squashing.

Revision history for this message
Bill Erickson (berick) wrote :

Pushed a commit to propagate SQL changes to base schema.

When comparing EDI messages, I find it easiest to copy them to a text file then search/replace "'" with "\n" so there's one line per EDI segment instead of one line per message.

Revision history for this message
Bill Erickson (berick) wrote :

Regarding the admin UI for the new EDI attributes, my plan was to wait until we have some agreement that we want to adopt these features. I was also only planning to build a webstaff version, no XUL UI.

Bill Erickson (berick)
Changed in evergreen:
assignee: Bill Erickson (berick) → nobody
Revision history for this message
Bill Erickson (berick) wrote :
Revision history for this message
Chris Sharp (chrissharp123) wrote :

I have tested this branch with POs from PINES libraries to Brodart, Ingram, and Midwest Tape and have had success with all three.

Revision history for this message
Bill Erickson (berick) wrote :

TODO: More B&T data testing.

Galen Charlton (gmc)
Changed in evergreen:
importance: Undecided → Wishlist
Revision history for this message
Jeanette Lundgren (jlundgren) wrote :

Adding a note that C/W MARS EDI libraries need ability to support local PO names as optional, alternate PO identifier. Our libraries replace the po_number with the po_name when generating EDI line item data for all vendors.

Revision history for this message
Bill Erickson (berick) wrote :

Hi Jeanette, thanks for the input! The behavior of the existing EDI template variable INC_PO_NAME has been replicated in the new code. (The change replaces the PO ID with the PO name in the RFF+LI segment). So, unless you have made additional local changes to your openils-mapper Ruby libraries, you should be covered with the new code.

Revision history for this message
Bill Erickson (berick) wrote :

Rebased branch (user/berick/lp1513872-acq-edi-account-attrs) to current master.

Revision history for this message
Bill Erickson (berick) wrote :

And to avoid confusion with the mixed bug numbers, I pushed a new branch w/ more generic name, matching the number from this bug:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1373690-acq-edi-perl-attrs

Revision history for this message
Bill Erickson (berick) wrote :

I've started a parallel branch with a web-based admin UI:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1373690-acq-edi-perl-attrs-ui

At present, it allows the for setting and un-setting attributes on existing attribute sets, but it does not yet support creating, deleting, or renaming attribute sets. Leaving this branch separate until it's done, since it's not 100% required for testing the main branch.

Revision history for this message
Bill Erickson (berick) wrote :

UI branch now supports create and modify of attr sets. Will merge with main branch after I update the release notes.

Revision history for this message
Bill Erickson (berick) wrote :

UI code and updated release notes merged back into the (new) primary branch:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1373690-acq-edi-perl-attrs

FYI, I added some UNIQUE constraints to the schema, so anyone who previously merged the DB code should consider:

alter table acq.edi_attr_set add constraint edi_attr_set_label_key unique (label);
alter table acq.edi_attr add constraint edi_attr_label_key unique (label);

I'm done with this code for now, pending broader testing.

Revision history for this message
Jason Stephenson (jstephenson) wrote :

Feedback fest is over, but I want to take a look at this.

Changed in evergreen:
assignee: nobody → Jason Stephenson (jstephenson)
Revision history for this message
Bill Erickson (berick) wrote :

Fixed a merge conflict. Rebased to master.

Bill Erickson (berick)
Changed in evergreen:
assignee: Jason Stephenson (jstephenson) → nobody
Revision history for this message
Bill Erickson (berick) wrote :

Rebased to master. Pushed another commit to add a sample EDI attribute set for Midwest Library Service. This includes 2 additional attributes to support their EDI tweaks.

Changed in evergreen:
status: New → Confirmed
Revision history for this message
Mike Rylander (mrylander) wrote :

Bill, comment 22 brings up a question for me: do we have a "catalog" of required tweaks, and (even if not) are we confident that the tweaks required in the wild are covered at this point?

Thanks! SOOOO looking forward to this.

Revision history for this message
Bill Erickson (berick) wrote :

Mike, for the catalog of required tweaks are you thinking something like this?

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=blob;f=Open-ILS/src/sql/Pg/950.data.seed-values.sql;h=7772cb767f5d8b1f208dfcde1ce216c7ec1b5f73;hb=refs/heads/user/berick/lp1373690-acq-edi-perl-attrs#l16969

These are the stock EDI attribute sets created to match the tweaks in the current stock template (plus new configs for MLS). To my knowledge, this covers all known options and vendor-specific requirements.

Before use, local changes will still be required to link each edi_account to an attr_set. And some attr_set's will undoubtedly need changing and/or new attr_sets will need to be created to accommodate local variation.

Revision history for this message
Bill Erickson (berick) wrote :

Maybe a long shot for 3.0, but I've added a pullrequest tag.

Code is rebased to master.

To sweeten the pot, I have pushed another commit to make is possible for sites to adopt the new EDI attributes setup on a per-account basis. From the modified release notes:

EDI accounts have a new boolean field "Use EDI Attributes" (use_attrs) that specifies whether PO's generated via the account should be built using EDI attributes or fall back to traditional JEDI A/T template generation.

This allows sites to activate EDI attributes on a per-account basis, making it possible to migrate piecemeal to EDI attributes. For the initial roll out of this new features, no accounts will be configured to use EDI attributes by default.

tags: added: acq pullrequest
Changed in evergreen:
milestone: none → 3.0-alpha
Galen Charlton (gmc)
Changed in evergreen:
assignee: nobody → Galen Charlton (gmc)
Revision history for this message
Galen Charlton (gmc) wrote :

Some initial comments:

- Overall looks promising
- The optional nature of this is crucial for it making it into 3.0; there's going to be a ton of detailed testing required. Announcing a deprecation of the Ruby stuff in 3.1 and removing it for 3.2 would be nice... but potentially optimistic.
- Another tweak I know of that's worth adding an EDI attribute for: an option to emit the ID of the owning library rather than the short name, as a work-around for B&T's five-character limit
- new code squashes away : and ' in titles rather than quoting them; while that may or may not be something that the materials vendors actually care about, it does make direct comparison of old vs. new output more time-consuming
- Typos in the seed data: 'Oder' and 'Feld'

Revision history for this message
Galen Charlton (gmc) wrote :

- It would be handy for the admin interface to have a 'Clone attribute set' option, as I suspect that a common scenario would be taking a stock attribute set and modifying it

Revision history for this message
Galen Charlton (gmc) wrote :

The chunking for IMD values is not quite right: one IMD can have up to two item description data element of maximum length 35, so it should be

IMD+F+BTI+:::Happy Birthday, Princess! (Disney P:rincess)'

not

IMD+F+BTI+:::Happy Birthday, Princess! (Disney Princess)'

Revision history for this message
Galen Charlton (gmc) wrote :

I've created the user/gmcharlt/lp1373690-acq-edi-perl-attrs-follow-ups branch containing follow-ups for the issues I care most about at the moment:

- adding a USE_ID_FOR_OWNING_LIB EDI attribute
- improving how various attributes emitted in IMD segments are normalized
- fixing the typos

I'm not treating this as a signoff at this point, and am releasing this bug for further testing. However, absent somebody finding a showstopper, my inclination is to include this in 3.0, even if it ends up being marked as experimental.

Changed in evergreen:
assignee: Galen Charlton (gmc) → nobody
Revision history for this message
Bill Erickson (berick) wrote :

Thanks Galen,

I have pushed a new branch:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1373690-acq-edi-perl-attrs-follow-ups-2

1. Tested USE_ID_FOR_OWNING_LIB and confirmed it's behaving as expected.
2. Confirmed long title (IMD) values encode and escape correctly
3. Added sign-off's for Galen's commits.
4. Added support for a "Clone" operation (under Create operation) in the attr set UI.

Changed in evergreen:
assignee: nobody → Mike Rylander (mrylander)
Revision history for this message
Mike Rylander (mrylander) wrote :

Pushing this to master makes my heart super happy. Thanks, Bill and Galen, and all testers!

Changed in evergreen:
status: Confirmed → Fix Committed
Changed in evergreen:
status: Fix Committed → Fix Released
assignee: Mike Rylander (mrylander) → nobody
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.