Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: compact number in email should consider format #1656

Merged
merged 3 commits into from
Oct 21, 2024
Merged

fix: compact number in email should consider format #1656

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
780ad71
fix: compact number in email should consider format
gulfaraz Oct 15, 2024
1088ba8
fix: use average for percentage indicators in email body event
gulfaraz Oct 15, 2024
2c4117e
fix: remove event aggregate in email
gulfaraz Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion interfaces/IBF-dashboard/src/app/pipes/compact.pipe.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ export class CompactPipe implements PipeTransform {
}

const style = format === NumberFormat.perc ? 'percent' : 'decimal';
const maximumSignificantDigits = value > 100 ? 2 : 1;
const maximumSignificantDigits =
value > 100 || format === NumberFormat.perc ? 2 : 1;

let min = 0;
let prefix = '';
Expand Down
7 changes: 2 additions & 5 deletions services/API-service/src/api/notification/email/mjml/body-event.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
getTextElement,
getTimeFromNow,
getTimezoneDisplay,
getTotalAffected,
getTriangleIcon,
} from '../../helpers/mjml.helper';

Expand Down Expand Up @@ -87,9 +86,7 @@ const getMjmlBodyEvent = ({
contentContent.push(
`<strong>${indicatorLabel}:</strong> ${
totalAffected
? `Approximately ${toCompactNumberWithFormat(
totalAffected,
)} ${indicatorLabel.toLowerCase()}`
? `Approximately ${toCompactNumberWithFormat(totalAffected)}`
: 'Information is unavailable'
}`,
);
Expand Down Expand Up @@ -150,7 +147,7 @@ export const getMjmlEventListBody = (

// Indicator details
indicatorLabel: emailContent.indicatorMetadata.label,
totalAffected: getTotalAffected(event),
totalAffected: event.totalAffectedOfIndicator,
toCompactNumberWithFormat: (value: number) =>
toCompactNumber(
value,
Expand Down
11 changes: 1 addition & 10 deletions services/API-service/src/api/notification/helpers/mjml.helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@ import { format } from 'date-fns';
import { EapAlertClassKeyEnum } from '../../../shared/data.model';
import { LeadTime } from '../../admin-area-dynamic-data/enum/lead-time.enum';
import { CountryTimeZoneMapping } from '../../country/country-time-zone-mapping';
import {
NotificationDataPerEventDto,
TriggerStatusLabelEnum,
} from '../dto/notification-date-per-event.dto';
import { TriggerStatusLabelEnum } from '../dto/notification-date-per-event.dto';

interface AdminArea {
exposed?: string;
Expand Down Expand Up @@ -278,12 +275,6 @@ export const getTimeFromNow = (leadTime: LeadTime) => {
} from now`;
};

export const getTotalAffected = (
event: NotificationDataPerEventDto,
): number | null => {
return event.totalAffectedOfIndicator ?? null;
};

export const getTriangleIcon = (
eapAlertClassKey: EapAlertClassKeyEnum,
triggerStatusLabel: TriggerStatusLabelEnum,
Expand Down
27 changes: 21 additions & 6 deletions ...ces/API-service/src/api/notification/notification-content/notification-content.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { InjectRepository } from '@nestjs/typeorm';
import { Repository } from 'typeorm';

import { EventSummaryCountry, TriggeredArea } from '../../../shared/data.model';
import { NumberFormat } from '../../../shared/enums/number-format.enum';
import { HelperService } from '../../../shared/helper.service';
import { LeadTime } from '../../admin-area-dynamic-data/enum/lead-time.enum';
import { CountryEntity } from '../../country/country.entity';
Expand Down Expand Up @@ -194,7 +195,11 @@ export class NotificationContentService {
disasterType,
);

data.totalAffectedOfIndicator = this.getTotalAffected(data.triggeredAreas);
const indicatorMetadata = await this.getIndicatorMetadata(disasterType);
data.totalAffectedOfIndicator = this.getTotal(
data.triggeredAreas,
indicatorMetadata.numberFormatMap,
);
data.eapAlertClass = event.disasterSpecificProperties?.eapAlertClass;
return data;
}
Expand Down Expand Up @@ -261,12 +266,22 @@ export class NotificationContentService {
});
}

private getTotalAffected(triggeredAreas: TriggeredArea[]) {
return parseFloat(
triggeredAreas
.reduce((acc, triggeredArea) => acc + triggeredArea.actionsValue, 0)
.toFixed(2),
private getTotal(
triggeredAreas: TriggeredArea[],
numberFormat: NumberFormat,
) {
const total = triggeredAreas.reduce(
(acc, { actionsValue }) => acc + actionsValue,
0,
);

if (numberFormat === NumberFormat.perc) {
// return average for percentage
return total / triggeredAreas.length;
Copy link
Contributor

@jannisvisser jannisvisser Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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..

Copy link
Member Author

@gulfaraz gulfaraz Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jannisvisser jannisvisser Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, clear, thanks :)

}

// return sum
return total;
}

private async getFirstLeadTimeDate(
Expand Down
3 changes: 2 additions & 1 deletion services/API-service/src/shared/helper.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,8 @@ export class HelperService {
}

const style = format === NumberFormat.perc ? 'percent' : 'decimal';
const maximumSignificantDigits = value > 100 ? 2 : 1;
const maximumSignificantDigits =
value > 100 || format === NumberFormat.perc ? 2 : 1;

let min = 0;
let prefix = '';
Expand Down