Comment 31 for bug 800478

Finally getting a chance to review these changes. I have a few comments, Liam:

1. A more focused and community-generic commit message would definitely be appreciated as this is quite a lot to sift through.

2. In a previous comment you said you were planning to recover the ability to specify the destination fund amount (i.e. external currency conversions), but it looks like the latest code still removes that.

3. I'd suggest giving the SQL upgrade scripts prefixes which guarantee their install order (XXXA.foo.sql, XXXB.foo.sql, .. or some such). They may apply in the right order as-is, but it's nice to see at a glance the order is well defined.

4. The upgrade scripts should use the new "SELECT evergreen.upgrade_deps_block_check(‘XXXX’, :eg_version);" instead of inserting directly into config.upgrade_log.

5. Please leave my PgTap test commit as a standalone commit (with me as the author) just because that's the right thing to do.

6. FWIW it's not essential that these commits all be squashed into one. It's OK if they are, but if it's easier to keep them separate, that's fine too, as long as each commit is tagged with the LP#.

7. Finally and most importantly, when I reached the 3rd file in the upgrades (XXXX.schema.acq.current_fund_allocation.sql), I started getting syntax errors and was unable to proceed with applying upgrades, so I was unable to test the code.