Cht-core: Targets are not always calculated correctly

[SCdF]

Initial discussion on #2064 .

Some CHPs have shown us that every time they refresh they will get different target values for their targets. Another has shown us that a value is always 0, even though she has shown that she has filled out the requisite followup reports from the history page.

So there is clearly something funky going on with the rules.

[garethbowen]

This is probably related to #2074 as tasks and targets are generated the same way.

[SCdF]

@garethbowen so for my clarity this is because of that refactor that happened awhile ago but this area was missed?

[garethbowen]

Yes I think that's where the bug was introduced. The root cause was always calling session.modify even when the fact was new and should be session.asserted. I think nools was then ignoring the modify because it didn't know about the fact.

This particularly affects first run as replication causes many changes to fire, so I managed to reproduce it by clearing my local db.

[SCdF]

Code looks good.

[alxndrsn]

How do changes to TaskGeneratorSrv make _Targets_ more reliable?

[SCdF]

Alex makes a good point. Just re-opening for @garethbowen to confirm how this works. I made a mistake when closing and reviewed the code (looks good!) and did not reflect on how that interacts with the actual ticket.

[garethbowen]

The TargetGenerator service depends on the TaskGenerator service. This is because Targets are emitted from the same nools configuration as Tasks. The current naming is an artefact of the order the services were developed - maybe we should consider renaming TaskGenerator to RulesEngine and then having a TaskGenerator and TargetGenerator which both depend on it.

I traced one incarnation of this issue back to the fact not being modified during replication, which meant the Target nools object wasn't being emitted, which meant the TargetGenerator wasn't being notified, and the UI left stale.

This hopefully also solves issues we've had with Tasks being out of date: #2074

[SCdF]

OK I see how this works. I agree that it's incredibly confusing that this still talks about creating tasks. I'm going to stare at it a bit and then attempt the smallest refactor possible to rename it to be clearer.

[SCdF]

@garethbowen made some naming changes in a branch. I looked at adding a TaskGenerator but I couldn't really see what it would do. I imagine it might help with clarity if we could work out what to split out into it, but I confess I'm not too hot on how LiveList works.

[garethbowen]

Looks good. There's an obvious comment to remove.

The only benefit I could see to pulling out a TaskGenerator service would be so you don't need to pass in the magic "task" string, but that's probably not worth the overhead of another service!

[sglangevin]

I'm still seeing a mismatch between submitted forms and what's displaying on the Targets tab. Reopening.

[sglangevin]

Here's what I then saw after a reload:

[sglangevin]

Note that I have also seen targets calculate correctly, but this is not consistent and usually occurs after leaving the app sitting for a while, or after I view reports on the history tab. Viewing reports on the history tab seems to trigger the targets to recalculate somehow.

[sglangevin]

More screenshots - here's what I saw after waiting a while (top) then after a browser refresh. Going to About and clicking reload brought me back to the image above, the one I got after reloading before.

[sglangevin]

One last comment: I've seen that the pregnancy target usually updates itself after a reload, but isn't 100% accurate on first load. The assessments, newborn care visit and on-time follow-up targets are very inconsistent. This means there may be a config issue @abbyad

[sglangevin]

With the newly updated app_settings that @garethbowen uploaded to lg.dev, I am only getting accurate targets when I use the browser refresh button in Chrome. Clicking reload from the about page clears the targets for assessments but updates the pregnancy registration target. I'm wondering if this is an issue with the UI updating because I see browser refresh working in Chrome but the reload button doesn't seem to bring the accurate targets. Hard closing the app and reopening, I get an unpredictable number of assessments listed in Targets.

[sglangevin]

Final comment for today. After some additional testing and help from @estellecomment and @abbyad we have discovered that the only way to get correct targets is to refresh from the Targets tab on web or mobile. Refreshing from other pages doesn't do the trick.

[estellecomment]

Logging what comes out of the TargetGenerator (here).

When you refresh on the Targets page, TargetGenerator outputs multiple times, gradually bigger counts, until it stops at the full value (ugly-format logs).
When you start on another tab, and then go to the Targets tab, it emits targets too but only once! And so the counts are too small.

Ha!

[garethbowen]

That's expected behaviour. The task generator emits each event as they're emitted from nools and caches the results. If you register your listener _before_ nools has emitted events then you get gradually incrementing numbers, if you register your listener _after_ nools has emitted events then you just get the total. The numbers should still be the same, however.

[garethbowen]

I found I could reproduce this by adding a delay to the target generator so it returns later. In this case I got 0 for all the targets. So I _think_ what was happening is, when just switching tabs the rendering completes when the target generator has only returned some of the targets, and then the UI doesn't get updated again when the rest are returned. If you reload on the targets page the rendering is slow enough that the targets are returned before rendering completes.

[estellecomment]

Hmmm. I don't get this. You should still see the TargetGenerator outputting things, even though they don't get updated in the UI. I was logging out the result of the TargetGenerator callback, and the numbers were too low.

Or is the callback also waiting on the digest cycle, so it wasn't running? That's timed by the digest cycle? I thought UI updates were done by the digest cycle. Shmrflblblwhaaaaat

It does seem to fix it though, so that's good :)

[sglangevin]

@garethbowen @estellecomment Perhaps I am missing something on my end, but I still only see the correctly updated tasks when I refresh the Targets tab. On first load and on reload, I have a mostly blank Targets tab. If I do an assessment, for example, the Targets page updates as expected - it seems to be working well. The lingering issue is on first load and on reload from the About page where my Targets get wiped and don't come back unless I refresh the Targets page. Reopening.

[estellecomment]

Ha. I was wondering how the fix fixed. I didn't see it any more, but lemme check more precisely.
Still on demo1 user? Did you try on admin?

[sglangevin]

@estellecomment I'm testing on alpha.dev with the demo user

[sglangevin]

Interestingly, if I log out and log back in on mobile, I see the correct values in Targets. This doesn't work on the web version.

[estellecomment]

Maybe a callback registration problem.

First emission from RulesEngine is just after init, nothing much happened yet. This always fires.

Subsequent ones are emitted from our custom events, and these only fire if you reloaded the targets page.

If you just navigated to targets without reloading, you just get the first callback, so not much emissions in it yet, so your counts are too low.

[estellecomment]

Found it!
The behavior above is fine : when you start a TaskGenerator, it doesn't restart the rules engine : if there's already a rules engine up, it gives you the emissions it has so far, then continues emitting and sends the new emissions in the callback.

HOWEVER, how it gives you the emissions it has so far is wrong. Because how it stores the emissions is wrong. Because it assumes that id is enough to identify an emission, when actually with the current config, you need id + targetType.
E.g.
{_id: "5D1090DB-7F57-E8CC-9E02-79355CF9EE8B", type: "assessments-u1", pass: false, date: 1457056538234} and
{_id: "5D1090DB-7F57-E8CC-9E02-79355CF9EE8B", type: "assessments-u5", pass: true, date: 1457056538234}
have the same id, so one will overwrite the other.

Working on fix now. Trying not to break tasks by fixing targets.

[abbyad]

Thanks for finding that @estellecomment! That matches how we create id for Tasks as well: _id: contact.contact._id + '-' + schedule.id.... is it possible to have the webapp combine the type and id to not leave that creation to tech leads and open to potential failures?

[garethbowen]

Nice one @estellecomment ! The TargetGenerator stores each emit by target type, but the RulesEngine doesn't. We could have some sort of id generation algorithm in the RulesEngine that knows how to handle Targets but I think requiring each ID to be unique in the config is the right approach.

[abbyad]

I simplified the target creation in the config to match @estellecomment's changes so that the person configuring the instance is less prone to making such a mistake again.

@sglangevin this is ready for acceptance testing now on alpha.dev

[sglangevin]

I'm excited to say this is working well! Moving to ready.