fix: compact number in email should consider format

[gulfaraz]

Describe your changes

This PR is a result of feedback from @gal-agmon on #1515

Compact numbers in email consider number format. This changes PHL typhoon emails,

Before this PR, https://us18.campaign-archive.com/?u=e71f3b134823403aa6fe0c6dc&id=ce2d4c8cfb
After this PR, https://us18.campaign-archive.com/?u=e71f3b134823403aa6fe0c6dc&id=44951c89fb

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

[jannisvisser]

@gulfaraz so from 10% onwards it's rounded to tens of percentages? This is not what you said in the meeting, nor what Gal Said in the meeting right?

[jannisvisser]

@gulfaraz also, this is not correct in the new email

[gulfaraz]

@jannisvisser Thanks! Not much was said in the meeting. These changes are based on a review from weeks ago. I'll try to find some notes and update this PR.

[jannisvisser]

@gulfaraz this is not fully correct. You have to take the weighted average, in this case through 'weightVar=total_houses', which is also an indicator-attribute. In aggregates.service in the front-end there is an example of how this is used. You can maybe argue this is already better than the sum we had before, but this will lead to differing numbers between email and portal. If too much work, you could yet again opt to omit the total (the whole line) in case of percentage. It's even arguable that this total percentage doesn't have much value, because it depends on how you go with including admin-areas in this set that are far from the track and have 1% damage, which is a semi-random rule anyway..

[gulfaraz]

I tried and failed to understand the weighted var and weighted average so I fell back to a simple average. I want the email and portal values to match. The calculation on the frontend is coupled with place codes and layers so it's not easy to extract the underlying calculation.

Can you give a pseudo-code explanation of the correct calculation? I'm lost on which variables mean what and how they are connected.

[jannisvisser]

Please realize that having this total figure in the email is a new feature, that is not currently not on production. So we're losing nothing with just omitting it.

Pseudocode formula is just something like: total-% = (area1-%*area1-total_houses + area2-%*area2-total_houses) / (area1-total_houses + area2-total_houses).

You probably don't have this additional data on 'total_houses' per area readily available at the place where you need it.

[gulfaraz]

Please realize that having this total figure in the email is a new feature, that is not currently not on production. So we're losing nothing with just omitting it.

Noted.

Thank you @jannisvisser

[jannisvisser]

@gulfaraz I see your new commit. To be honest I was actually meaning to only omit the line in the percentage case. Because it does work correctly in the sum-case, which is the vast majority of all implementations (population_affected). I guess you would maybe judge this unclean code and therefore prefer to remove it altogether? But in this particular case, it might be better to split it with a clear comment in the code explaining why.

[gulfaraz]

To be honest I was actually meaning to only omit the line in the percentage case.

This wasn't clear before and is now perplexing. Did the feature requirements describe this exception?

Because it does work correctly in the sum-case, which is the vast majority of all implementations (population_affected).

We agreed to have shared behaviour across countries and hazards without exceptions like these. I understand we implemented this feature before our agreement, but we're deciding to sign off on the feature now.

The requirement is to show the same aggregate value in the email as in the dashboard. If the suggestion is to show a value in the dashboard but not in the email, I advise to not ship a partial implementation. Let's refine this feature when it returns as feedback.

But in this particular case, it might be better to split it with a clear comment in the code explaining why.

This addresses the discrepancy for us devs. However, it does not explain why the value is missing to the end-user, those demoing IBF, design, and other stakeholders.

We have the opportunity to,

1. Avoid the effort to document this exception
2. Avoid communicating this exception to the relevant people mentioned above
3. Avoid a future refactor to remove this exception

[jannisvisser]

Ok, I'm convinced by your argumentation, but do not appreciate the unfriendly tone of the choice of the word 'perplexing' (unless I'm misinterpreting somehow).

[gulfaraz]

By perplexing, I meant confusing. I apologize if it came across as unfriendly. It was very neutral.

[jannisvisser]

Ok, clear, thanks :)

[gulfaraz]

The indicator value shown in the email event is incorrect. See review here and IBF Dev Chat for details.

In our emails we used to show the total indicator as % of houses exposed: Approximately 7,23 % of houses exposed
This is a total of all the percentage values [0.53, 0.51, 0.5, 0.47, 0.45, 0.39, 0.3, 0.3, 0.3, 0.26, 0.25, 0.17, 0.17, 0.16, 0.16, 0.14, 0.14, 0.12, 0.12, 0.12, 0.12, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.08, 0.07, 0.07, 0.06, 0.05, 0.05, 0.05, 0.05, 0.03, 0.03, 0.03, 0.03, 0.03, 0.03, 0.03, 0.02, 0.02, 0.01, 0.01, 0.01, 0.01, 0.01, 0.01, 0.01, 0.01, 0.01, 0.01, 0.01, 0.01, 0.01, 0.01, 0.01, 0.01] = 7.229999999999999

Which should be interpreted as 723%.

The old indicator is wrong and there is no easy way to calculate the correct value without significant refactoring. So I removed the indicator following @jannisvisser's advice. After this change, the emails look like,

1. PHL typhoon https://us18.campaign-archive.com/?u=e71f3b134823403aa6fe0c6dc&id=d31bea6478
2. PHL flood https://us18.campaign-archive.com/?u=e71f3b134823403aa6fe0c6dc&id=591b54ea33
3. UGA flood https://us18.campaign-archive.com/?u=e71f3b134823403aa6fe0c6dc&id=625a1ea130