Invoke 'after' and 'enrich' interceptors only when context contains effects db. [fixes #447]

[zachcurry]

I thought I'd grab a ticket and give you a hand! See bug for context 447

I noticed that 'after' and 'enrich' both suffer from this issue, so I fixed them both with as little change as possible (no refactoring).

With regard to the tests, I wrote failing tests for each function, and each case, and they failed. After making them pass I noticed there was an error in an existing test case within 'test-enrich' such that it was not actually testing anything, so I fixed it as well! You will find a comment in the review highlighting this issue.

I made reference to my fix in CHANGES.md as per the contributing instructions. If that's not how it should be done, let me know and I'll fix/remove it.

[zachcurry]

This was the bad test I mentioned. It was looking inside 'ctx' for an ':after' key, and returning a function that was never invoked as a not-found value. This is fixed now.

[mike-thompson-day8]

@zacharycurry I appreciate you jumping in and helping, thanks. And I also appreciate the completeness of your PR (ie. including tests and an update Change.md).

But I'm thinking that both enrich and after should just do nothing when there's no :db effect. Ie. they should simply return context without doing anything. I don't think they should pluck out the original (coeffect) :db and process it..

Justification ...

Example after usecases:

1. after is used to write current value of :db to LocalStore. But if there's no new version of :db then there's no change, and no need to re-save to LocalStore. Hence "do nothing" is the right action.
2. after is used to check current :db against a spec for correctness, but if there's no change, then nothing new to check. Hence "do nothing" is the right action.

Example enrich usecases`:

1. enrich used to perform some cross-checks for new "warn-able" conditions. If there's no change to :db then there's nothing new to check for. Hence "do nothing" is the right action.

[zachcurry]

Thanks @mike-thompson-day8! I read the bugs description a tad too prescriptively and didn't think about what I was doing. Whoops! What you've said makes total sense.

I'll fix this up real quick...

[zachcurry]

If I understood you correctly @mike-thompson-day8 this PR should be exhibiting the desired behavior now. If you see anything else gone awry, let me know, I'll jump in there and fix it up :)

I'm new to open source dev, so if I do anything silly, or my PR is whack, please let me know so I can scrub some of this newb-smell off me ;)

[danielcompton]

I’ll take a look at this and compare to what I’ve done on re-frame-trace on Monday.

[mike-thompson-day8]

@danielcompton will leave this for you to merge (or not) and to then close #447

[zachcurry]

Hi @danielcompton ! I just heard you on the defn podcast. What a small world!

[zachcurry]

Thank you for helping me out with this @mike-thompson-day8 ! I really appreciate it!

Forgive me for asking the following newbie questions:

@danielcompton will leave this for you to merge...

I have merged master into my branch, is that what you mean? I don't think I have permissions to merge this to master myself (or do I?).

... and to then close #447

I don't think I have the permissions to close issues in this project (or do I?).

[danielcompton]

Thanks for your help here @zachcurry! I'll come back to the enrich test. I've opened #453 to discuss changing the behaviour of after and enrich, as it's a breaking API change.