@awalkowiak lets chat about this PR either during tech huddle or a JEAM meeting. I'm hoping we can refine this a bit since it's pretty crude in its current state. Maybe we can build a lot of this logic into the PrepaymentInvoiceService (or maybe a new PrepaymentInvoiceMigrationService) and unit test the heck out of it to know that it works.

It also doesn't seem to run on app startup so I need to figure out why...

@ewaterman

It also doesn't seem to run on app startup so I need to figure out why...

You have to add this migration file in the 0.9.x/changelog.xml. Looks like (at least in this PR) it is missing there.

Did you try to run it locally? How was the performance?

@awalkowiak I haven't been able to performance test this yet since I haven't tried setting my local up with prod data. Maybe we can chat tomorrow about what it takes to get that set up (and I can document the process for the future).

@awalkowiak @alannadolny

The change runs the full way through with my latest commit. Looks like it had something to do with the way the code was looping over the prepayment invoices. Maybe something to do with iterator vs loops, I'm not totally sure.

Here's the output I got on my local with stage data:

Prepayment Invoice migration completed in 12755 milliseconds!
761 prepayment invoices/orders were found.
694 orders were migrated.
6 were skipped due to no migration being needed
0 were skipped due to a bad order.

which is much faster than I thought it would be... I'm slightly suspicious and also wondering where the other 61 prepayment invoices are (I have a log statement for each case so I have no idea...)

I still have to check how the invoices are looking in the frontend (my local is behaving all wonky with the stage data).

I poked around in the UI for some invoices that were migrated and all the ones I looked at seem to look correct to me. That said, I'm noticing the migration is not setting the amount field for any old non-prepay, non-inverse invoice items, would this be a problem?

I'd appreciate if someone else could run the migration locally to confirm it works for them as well. @awalkowiak @alannadolny

Artur, I know you wanted to revisit the topic of whether we should not do this migration, so if you have that conversation Monday please loop me in (I'll be OOO),

@alannadolny to answer an earlier question, I thought about it, but it's quite difficult to construct a reasonable precondition for these migrations. For step 1, we could technically add a check like:

    preconditions(onFail:'MARK_RAN') {
            sqlCheck(expectedResult: '0') {
                <count all invoice items where amount is null joined in invoice where invoice type is prepayment>
            }
    }

but that's doing essentially the same lookup that the migration is doing and so wouldn't really save us much. And step 2 of the migration is even harder to build a precondition for. Anyways, if there's nothing to migrate, the script will likely run fast enough since it's just doing a few selects, but it is a valid concern.

@awalkowiak I'm going to remove the do not merge flag in case any decisions are made on Monday, but I'd like at the very least to have one more person test on their local first.

@jmiranda

3 - That's a fair concern. It might be somewhat complex to determine since we'd need to fetch all invoice items of an invoice at once and see if there's a matching inverse for each item, but maybe it's worth it to do that extra work just to be sure we don't ever get in a halfway state. If this is all working in a transaction, it shouldn't be an issue since it should get rolled back. I'll check.

5 - Good question. I'll test.

6 - I'm not sure it's safe to rollback by deleting inverse items. I don't think the system could handle that properly since it now expects the inverse items to be there. Maybe it'd be fine though and we can keep generating inverse items the old way if there aren't already inverses. Maybe @awalkowiak can comment on this.

Okay I tested the code and it does seem to be properly transactional (I threw an error at the end of the migration and all amount fields were set back to null).

I believe this solves concerns 3 since if there's ever an error (there shouldn't be), the script will rollback all inverse items it created and so would be impossible to get stuck in a place where some of a prepayment invoice's items have an inverse and some don't.

As far as I'm concerned I think this can be merged once we've had a chance to retest against Manon's data on stage.