User:Katie Horn/TODO DI oid refactor

From Wikitech
Jump to navigation Jump to search

2014 January 06 - DonationInterface - Order ID assignment refactoring

  • Torpedo i_order_id. If we need an internal order id, we use ctid.
    • The orphan slayer seems to use i_order_id in batch mode. Hit it until it stops.
      • $this->batch = true; is set in the orphan adapter, but that doesn't even seem to matter because reAddHardData() is overriding everything.
    • remove i_order_id from all forms
    • remove i_order_id from stomp messages, if it even makes it that far.
    • remove any straggling references to i_order_id from civicrm
  • setNormalizedOrderIDs() - Should take a cue from the gateway object, that tells the DonationData class where it should look for the order_id.
    • $_GET with a keyname, for things like resultswitchers
    • $_POST with a keyname, same deal.
    • $_SESSION['Donor'] or whatever.
    • Directive to make one ourselves
    • Directive to wait for the gateway to go get it
    • Some mechanism to handle freaky batch mode (probably closely related to the last item).
      • Description of Freaky Batch Mode:
        • $this->adapter = new GlobalCollectOrphanAdapter(array('external_data' => $data)); in orphans.php
        • After an evil extract() in the constructor... $this->dataObj = new DonationData( $this, self::getGlobal( 'Test' ), $external_data );
        • $this->populateData( $test, $data ); in DonationData.
        • $this->normalized = $external_data; in populateData, eventually followed by a $this->normalize...
        • ...which gets us to setNormalizedOrderIDs(), where it sends in the override if the gateway is in batch mode. Whew.
      • Oops. unstage_data in the orphan adapter is just sitting there looking weird. This concerns me because I changed the card_type staging function.
        • Uhm, make that "Really weird". It's recursive. Unsurprisingly, my name is the only one within 13 lines of this thing.
        • I take it all back. Not only do I stand by that function, but I think we should move it to the main gateweay adapter. Later. Also, this is totally barking up the wrong codepath: This unstage function only gets called by parse_files mode, and we've been running orphan_stomp mode forEVAR.
      • Oh good. $this->batch = true; gets set by the orphan_adapter when we go to loadDataAndReInit().
        • Same function also blows away DD entirely and sends the data in the 3rd param. Good... good.
    • Look at all the places you're already having to short-circuit (regen id), and bring those back in to the main process.
      • These will need to do something funky with the OID meta so we know we want to regen (or re-null if we are waiting for the gateway?)
      • resetOrderId looks like it's there just to... reset the OID. Wrong object, though... I think.
  • Post data defaults.
    • /me facepalms loudly - This is going to hurt.
    • Grep&destroy:
      • setPostDefaults -> setGatewayDefaults (intended to be for gateway specific things, and not universal defaults)
      • postdatadefaults
      • postDefaults - ARGHARGH, stupid extract( $options ) is still on the table! Kill it with as much fire as you can generate.
        • Anything we still really need to default for the gateway, can go in a new function called... er. setGatewayDefaults. Why not.
      • card_type. So it's come to this.
  • getAnnoyingOrderIDLogLinePrefix(), getLogMessagePrefix() - Set it up so we can stop making these optional in any way. This should just happen all the time in the adapter log functions.
    • But, if you're deferring order_id retrieval, just prefix with ctid.
  • Consider doing something like a sanity check before do_transaction will work...
    • Maybe add a bit of transaction meta that designates a certain function to be the one that retrieves the order id, and if transactions don't have that designation, throw an error if you don't have one yet and you're running in retrieval mode.
      • Meh. The gateway will do this for us... but we should probably cram an error in something if we go through the normalize function and still have no oid if we're generating.
  • Find diagram for transactionConfirm_CreditCard(); Kick self repeatedly while trying to think of something less insane to do with freaky batch operations. <-- turns out this is unnecessarily violent at self. The insanity stands.
  • loadDataAndReInit in the Orphan adapter:
  • See if there is any place we can add ctid in to outgoing transactions, for great happiness. MERCHANTREFERENCE in GC... others elsewhere.
  • BT, RTBT, and DD mid-flow processing (return step?): WTH, man? Hargh. Had to comment out a GET_ORDERSTATUS that happened on every pageload. Blowing it for cc transactions, but probably essential for those to go through.

Email-based CR on sandbox branch, round 1 (awight)

All resultant changes reflected in 258edf4b57e8c9edffe2-something-something

  • payment_product and contribution_tracking_id patch should be its own thing (predecessor) so we can rollback independently :p
    • There are reasons that I had to address this with the current patch, and I probably could have done it backwards once I noticed the issues. Sadly, the timing was such that it would have been a nontrivial amount of time to go back in time and rebase.
  • order id meta alt_locations documentation needs to explain more. What is "dataset" (new term), why "name" and "key", how is this used to produce order_id. Also, 'generate' is not documented. Nor 'final'...
    • Addressed in new commit.
  • Big picture thing. I'm torn about the new order_id_meta-- this would be a good time to stop introducing new ad-hoc arrays, and wrap the thing you care about in a first-class object: in this case, it would be an OrderIdGenerator or manager or something. We can still give it array constructor parameters, which might come from a declarative config file, but object-ness gives us much better documentation, validation, and will allow us to factor chunks of functionality out of this mammoth Adapter hierarchy. It also makes it clear exactly which data from the transaction is accessed during order_id generation. Yeah, the more I read of the patch, the more I come back to this paragraph. No worries if you don't want to go there on this revision though, it's not a blocker.
    • Adding a lengthy @TODO to everything that I intended to be the seed of a new way to keep track of everything, in a new commit. Also I think you added a mingle card.
  • Is the base class definition of defineOrderIDMeta() actually a shortcut for: function defineOrderIDMeta() {} ? cos to the untrained eye, it looks like there is no default implementation and therefore PayPal and Amazon will die.
    • This is true, and intentional. Every gateway will have to be tickled in this manner before I am done here.
  • gateway_adapter line 279: please do not abbreviate "DD", it is ambiguous
    • Damn, my line numbers went all whack. I just killed several long-standing abbreviations for DonationData, though...
  • You removed $postdatadefaults from the base class, but it is still a thing.
    • I have repeatedly grepped everything for this and all variations I had on record, and I can't find it anywhere. Where are you still seeing it?
  • What's with the new logline style in buildRequestXML?
    • Huh? Oh. That was a debugging thing. Removed.
  • Better name for regenerateOrderID would be invalidateOrderId
    • You know, I actually thought about that and deliberately didn't do it. They would actually be two different things... generate implies we're running in generate mode. Invalidate should be something that *triggers* a generate when we are running in generate mode. To put it another way, invalidate should be ok to do from either mode when some piece of data that the current data depends on, changes.
  • I think buildOrderIDSources is really way overly meta. Why don't you just iterate over actual GPS values, then search through alt_locations? It would a lot more readable. I don't see a use case for persisting all possible candidates, either. If you really want to add a generic function to dereference novel variable addressing schemes, please split it into a separate class which is called from buildOrderIdSources.
    • G...PS?
      So, this is definitely another one of those things that is totally the precursor to the class that I've been thinking about since 2011. If I had a nickel for every time I would have been saved if I'd just stored this meta and been able to override...
      Guess I should have been writing that down, huh? I have definitely seen numerous use-cases for keeping it all, though. But, it should totally be tucked away in a class, and used everywhere.
  • Why is validateDataConstraintsMet not calling DataValidator? Also, there is no final return statement in the case where dataConstraints do not exist.
    • Because: Derp. Both questions.
      Alternate answer: Because DataValidator::validate_numeric was (oddly enough) protected.
      I have decided to @TODO myself for porting the majority of the function to the validator, and leave it at that for now.
  • Document normalizeOrderId( $override parameter. sorry, I know you will document later. projection much?
    • Done.
      ...cleaned up the return on that one, too. Derp again.
  • Yeah there should be no range defined in the default implementation of generateOrderID, it should be pure virtual or at least require the range to be configured per processor IMO.
    • Probably not obvious due to all the code churn, but that is legacy behavior that I didn't want to change in this revision (copied straight out of DonationData).
  • getOrderIdMeta does not make sense. What is a raw/non-array order_id_meta?
    • This statement broke my internal parser.
      In other words: What?
  • GlobalCollectAdapter line 1161: random error_log
    • Kill'd.
  • GlobalCollectAdapter, line 2307: ooo, what is going on? I see it was not the fault of this rev, but why would we have to check the returnto for order_id? Can't we just build it from scratch each time through the function?
    • Argh, again, line numbers are f'd now... but I know what you're taking about, I think. stage_returnto()?
      So, those have hella problems. There's a @TODO about figuring out why we can't use unstaged in the staging functions somewhere. I have been trying to sweep that one under something for a long time. This is one of the things that my Data Item class plans would fix forever, though...
  • globalcollect_resultswitcher.body.php: TODO: Let's have the adapter decide what to do with the incoming URL parameters like order_id and REF? What is "REF" and why is there no other code that references it?
    1. absolutely agree that this work should be done. Just not in this already-monstrous rev, because the work there is way bigger on the inside.
    2. REF is a param that only GC passes back to us on iframe returns, that only ever comes to the resultswitcher.


Order of Things To Do

...probably.

  1. destroy i_order_id from all of everything, forever
  2. Add mechanism to specify order_id [generation|retrieval] per gateweay, and transaction meta that designates transactions to be the ones that go get it.
  3. implement in GC, and make sure it works either way (with unit tests)
  4. Unkill batch mode (which has probably been wrecked at this point) and verify with unit tests
  5. Unwreck all other gateways (paypal, amazon, adyen)
  6. payments deploy
  7. tackle wr1 log parser: Have it prefer the proposed new efficient log prefix style, but still work in either case
  8. civi deploy
  9. redo all log lines with appropriate prefixes
  10. payments deploy
  11. Happy Dance

Necessary assertions

  • Order IDs can be generated
    • And only regen at appropriate times
  • Order ID generation can be deferred to the gateway
  • Both self-generation and gateway generation still work for the following:
    • Batch mode (orphan slaying) still works (<-- hey self: Lousy wording. What you really mean is "orphan slaying is unchanged, regardless of the chosen setting)).
    • Verify that all gateways are still operational
    • Verify that all major payment methods within every gateway still function (<-- Ha, now you have to do it)

TODO immediately after

  • Collapse all the getLogMessagePrefix() nonsense.
    • You're going to have to add some logic to the wr1 parser to... take both old-style and new-style logs. Should actually speed up the parsing (because getting the ctid used to be a pita...)
  • Do that thing you've always wanted to do, and keep metadata on all fields through DD.
  • card_type must die, from all forms and... everything. This is now being normalized into payment_submethod. Will (possibly) require some involvement from production.
  • See what else you should be doing for the SEPA deadline
  • Revisit adapter construction. This is nuts, and makes testing... more or less impossible in places.