fix: remove group by on event trigger value

[gulfaraz]

Describe your changes

Restrict to one event per region.

• This causes event names to be duplicated.

Checklist before requesting a review

• I have performed a self-review of my code
• I have added tests wherever relevant
• I have made sure that all automated checks pass before requesting a review

[gulfaraz]

@gal-agmon This morning you mentioned for a region we should not show trigger and warning.

My interpretation is that if there is both trigger and warning then we should only show trigger. Is this correct?

This change also affects all other countries and hazard types. In the case of PHL typhoon we currently have,

This will become,

Checking to make sure this change is intended.

[gulfaraz]

> test:api:all
> node --expose-gc node_modules/.bin/jest --config=jest.api.config.js --runInBand --detectOpenHandles --logHeapUsage

ts-jest[versions] (WARN) Version 4.9.5 of typescript installed has not been tested with ts-jest. If you're experiencing issues, consider using a supported version (>=2.7.0 <4.0.0). Please do not report issues in ts-jest if you are using unsupported versions.
 PASS  test/email/typhoon/email-phl-typhoon.test.ts (10.067s, 217 MB heap size)
  Should send an email for phl typhoon
    ✓ default (8359ms)

 PASS  test/email/flash-flood/email-mwi-flash-flood.test.ts (29.832s, 213 MB heap size)
  Should send an email for mwi flash flood
    ✓ default (6842ms)
    ✓ no-trigger (22903ms)

 PASS  test/email/floods/email-uga-floods.test.ts (29.106s, 214 MB heap size)
  Should send an email for uga floods
    ✓ default (6238ms)
    ✓ warning (7355ms)
    ✓ warning-to-trigger (7515ms)
    ✓ no-trigger (7916ms)

 PASS  test/email/dengue/email-phl-dengue.test.ts (15.547s, 223 MB heap size)
  Should send an email for phl dengue
    ✓ default (8082ms)
    ✓ no-trigger (7382ms)

 PASS  test/email/malaria/email-eth-malaria.test.ts (18.229s, 214 MB heap size)
  Should send an email for eth malaria
    ✓ default (8941ms)
    ✓ no-trigger (9207ms)

 PASS  test/email/drought/email-uga-drought.test.ts (16.178s, 213 MB heap size)
  Should send an email for uga drought
    ✓ triggered in january (8392ms)
    ✓ non triggered any month (7697ms)

 PASS  test/email/floods/email-ssd-floods.test.ts (17.699s, 223 MB heap size)
  Should send an email for ssd floods
    ✓ default (9628ms)
    ✓ no-trigger (7974ms)

Test Suites: 7 passed, 7 total
Tests:       15 passed, 15 total
Snapshots:   0 total
Time:        137.249s, estimated 139s
Ran all test suites.

[gal-agmon]

@gulfaraz I don't understand in what situation the same area would be affected by both a warning and a trigger at the same time? according to the screenshosts these looks like exactly the same event

[gulfaraz]

I agree. I find it weird that this is consistent across multiple countries and hazard types. You can see this behaviour in ibf-demo and then confirm if this change can be merged.

[gulfaraz]

I've identified other bugs fixed by this PR,

[jannisvisser]

@gulfaraz I am a bit hesitant if this is OK to just change, and doesn't have adverse affects in other scenarios. This was put in like this I think because of the multi-threshold work in e.g. UGA floods, where it is used to distinguish events of different magnitude? I guess as I'm writing this that they should already be separated by eventName (= glofasStation in that case). But did you check this in that scenario, and others?

[jannisvisser]

OK, I've tested the UGA floods case myself. And have also read up now on the other comments in this PR (e.g. regarding typhoon). I do think I can confirm this change is OK to merge, but I am afraid that it just solves a symptom of wider-spread disease. Because why is e.g. the typhoon being stored as something that can come out as trigger+warning? So we can either just merge it and treat this as step-by-step iterative solving, or we should dive deeper into it..

[jannisvisser]

@gulfaraz probably not related but is the [object Object] bug in the typhoon-screenshots, right above the area bullet list (but seen everywhere on local/test/demo), a known bug taken up somewhere?

[gulfaraz]

they should already be separated by eventName (= glofasStation in that case)

In case there are duplicate event names with different magnitudes, then the expectation is that we only show the highest magnitude. i.e.) If we received 3 flood forecasts,

1. a warning forecast for G5100
2. a trigger forecast for G5100
3. a warning forecast for G1967

We expect to see 2 flood events,

1. a trigger for G5100
2. a warning for G1967

Because why is e.g. the typhoon being stored as something that can come out as trigger+warning?

Yes, this is unclear to me as well. The example I described for flood forecasts should also apply for other countries and hazards. In PHL typhoon, we should not see a warning and a trigger event simultaneously for the same regions. But this has been the case for a while, so I assumed this is the desired behaviour. If this is undesired behaviour, this PR will change it.

I requested input on this PR to check if this behaviour is desired. So far we're aligned that this change is needed.

So we can either just merge it and treat this as step-by-step iterative solving, or we should dive deeper into it.

I prefer to merge this as it fixes at least 4 known issues. We can do a deeper dive into it if we can identify at least one undesired behaviour caused by this change.

the [object Object] bug

This was an object I added for debugging but forgot to remove. It is removed in this commit. It does not occur in ibf-test anymore.

[jannisvisser]

@gulfaraz clear, approved in that case.