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.

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

Improve charts partially resolves #80 #91

Merged
merged 8 commits into from
Apr 22, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
54 changes: 47 additions & 7 deletions server/src/bots/telegram/botResponse/trendResponse.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,27 @@
import { getCovidTrends } from '../../../services/api/api-chart';
import { addDays, Now } from '../../../utils/dateUtils';
import { CountrySituationInfo } from '../../../models/covid19.models';
import { Transform } from '../../../services/domain/chart';
import {
Transform,
enrichWithTitle,
enrichWithType,
} from '../../../services/domain/chart';
import { catchAsyncError } from '../../../utils/catchError';
import { getRequestedCountry } from '../../../services/domain/countries';
import { logger } from '../../../utils/logger';
import { CallBackQueryHandlerWithCommandArgument } from '../models';
import * as TelegramBot from 'node-telegram-bot-api';
import { Frequency } from './../../../models/constants';

export const trendsByCountryResponse: CallBackQueryHandlerWithCommandArgument = async (
bot: TelegramBot,
message: TelegramBot.Message,
chatId: number,
requestedCountry?: string | undefined
requestedCountry?: string | undefined,
requestedFrequency?: Frequency | undefined
Copy link
Owner

Choose a reason for hiding this comment

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

I know that you just followed the pattern, but I recently realized that It means
requestedFrequency might be present and might not
AND
when it presents it could be Frequency and could be undefined.

If that's interned, then go for that :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@danbilokha
Could you please paste what I should do with it?
Are you saying that undefined is redundant?
And I should remove it to be requestedFrequency?: Frequency

Copy link
Owner

Choose a reason for hiding this comment

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

just told you how it works so you were aware of that.

): Promise<TelegramBot.Message> => {
const ferequency = requestedFrequency || Frequency.Weekly;

const [err, [foundCountry, foundSituation]] = await catchAsyncError(
getRequestedCountry(requestedCountry)
);
Expand All @@ -25,12 +33,44 @@ export const trendsByCountryResponse: CallBackQueryHandlerWithCommandArgument =

return bot.sendMessage(chatId, err.message);
}
const lastWeekSituation = foundSituation.filter(
(c: CountrySituationInfo) => {

let startDate: Date;
let hasFilter = true;
switch (ferequency) {
case Frequency.Weekly:
startDate = addDays(Now, -7);
break;
case Frequency.Monthly:
startDate = addDays(Now, -30);
break;
case Frequency.WholePeriod:
hasFilter = false;
break;
}

let periodSituation = foundSituation;
if (hasFilter) {
periodSituation = periodSituation.filter((c: CountrySituationInfo) => {
const date = new Date(c.date);
return date < Now && date > addDays(Now, -7);
}
return date < Now && date > startDate;
});
}

const frequencyName =
ferequency === Frequency.WholePeriod
? 'Whole period'
: capitalize(ferequency);

let model = enrichWithTitle(
Copy link
Owner

Choose a reason for hiding this comment

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

is that really a model? Maybe a chart? Does it have a type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Types
Types

Types

Types

Types

Types

kript

kript

kript

kript

kript
kript

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@danbilokha
There is no need to add any type there.
TypeScript is clever enough to identify and follow the type by return type of the function

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@danbilokha try to hover your mouse on the variable. And you'll see the macig.
I know I promised to provide with a nice article on that point.
But so far I've just found. this
image

Transform(periodSituation),
`${frequencyName} trends for ${capitalize(requestedCountry)}`
);
if (ferequency === Frequency.Weekly) {
model = enrichWithType(model, 'barStacked');
}

return bot.sendPhoto(chatId, getCovidTrends(Transform(lastWeekSituation)));
return bot.sendPhoto(chatId, getCovidTrends(model));
};

const capitalize = (input: string): string =>
Copy link
Owner

Choose a reason for hiding this comment

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

that function should go to utils

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lovely utils

input.charAt(0).toUpperCase() + input.substring(1).toLowerCase();
6 changes: 5 additions & 1 deletion server/src/bots/telegram/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
} from './botResponse/subscribeResponse';
import { SubscriptionType } from '../../models/subscription.models';
import { MessageHandlerRegistry } from './services/registry/messageHandlerRegistry';
import { withTwoArgumentsAfterCommand } from './services/registry/withTwoArgumentsAfterCommand';
import { subscriptionNotifierHandler } from './services/subscriptionNotifierManager';
import { unsubscribeStrategyResponse } from './botResponse/unsubscribeResponse';
import { trendsByCountryResponse } from './botResponse/trendResponse';
Expand Down Expand Up @@ -109,7 +110,10 @@ export function runTelegramBot(
],
unsubscribeStrategyResponse
)
.registerMessageHandler([UserRegExps.Trends], trendsByCountryResponse);
.registerMessageHandler(
[UserRegExps.Trends],
withTwoArgumentsAfterCommand(trendsByCountryResponse)
danbilokha marked this conversation as resolved.
Show resolved Hide resolved
);

// Message handler for feature Countries / Country
for (const continent of Object.keys(Continents)) {
Expand Down
19 changes: 15 additions & 4 deletions server/src/bots/telegram/services/keyboard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
CustomSubscriptions,
UserMessages,
UserRegExps,
Frequency,
} from '../../../models/constants';
import { InlineKeyboard, ReplyKeyboard } from 'node-telegram-keyboard-wrapper';
import { UNSUBSCRIPTIONS_ROW_ITEMS_NUMBER } from '../models';
Expand Down Expand Up @@ -33,10 +34,20 @@ export const getAfterCountryResponseInlineKeyboard = (
ik.addRow({
text: `${CustomSubscriptions.SubscribeMeOn} ${country}`,
callback_data: `${UserRegExps.Subscribe} ${country}`,
}).addRow({
text: 'Show weekly chart',
callback_data: `${UserRegExps.Trends} ${country}`,
});
}).addRow(
{
text: 'Show weekly chart',
callback_data: `${UserRegExps.Trends} ${country}`,
},
{
text: 'Show monthly chart',
callback_data: `${UserRegExps.Trends} ${country} ${Frequency.Monthly}`,
},
{
text: 'Show whole period chart',
callback_data: `${UserRegExps.Trends} ${country} ${Frequency.WholePeriod}`,
}
);

return ik.build();
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { CallBackQueryHandlerWithCommandArgument } from '../../models';
import * as TelegramBot from 'node-telegram-bot-api';
import { LogCategory } from '../../../../models/constants';
import { logger } from '../../../../utils/logger';
import { noResponse } from '../../botResponse/noResponse';

export const withTwoArgumentsAfterCommand = (
handlerFn: CallBackQueryHandlerWithCommandArgument
): CallBackQueryHandlerWithCommandArgument => {
return (
bot: TelegramBot,
message: TelegramBot.Message,
chatId: number,
ikCbData?: string
): Promise<TelegramBot.Message> => {
try {
const [arg1, arg2] = splitArgument(ikCbData);
Copy link
Owner

Choose a reason for hiding this comment

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

talked


return handlerFn.call(
this,
bot,
message,
chatId,
(arg1 && arg1.toLowerCase()) || ikCbData,
arg2 && arg2.toLowerCase()
);
} catch (err) {
logger.error(
`Error happend inside withTwoArgumentsAfterCommand() for ${chatId} with message: ${message.text} and ikCbData: ${ikCbData}`,
err,
LogCategory.Command,
chatId
);

return noResponse(this.bot, message, chatId);
}
};
};

const splitArgsRegexp = new RegExp(
'(?<firstArg>[^ ]+)[\\s.,;]+(?<secondArg>[^ ]+)'
);
function splitArgument(argument: string): [string, string] {
const match = splitArgsRegexp.exec(argument);
if (!match) {
return [undefined, undefined];
}

/* tslint:disable:no-string-literal */
return [match.groups['firstArg'], match.groups['secondArg']];
}
4 changes: 3 additions & 1 deletion server/src/models/chart.models.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ export type DataSet = {
label: string;
data: Array<number>;
fill: boolean;
borderColor: string;
backgroundColor?: string;
borderColor?: string;
};

type ChartData = {
Expand All @@ -13,4 +14,5 @@ type ChartData = {
export type ChartModel = {
type: string;
data: ChartData;
options: any;
};
8 changes: 7 additions & 1 deletion server/src/models/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,14 @@ export enum Continents {
Other = 'Other',
}

export enum Frequency {
Weekly = 'weekly',
Monthly = 'monthly',
WholePeriod = 'wholeperiod',
}

export enum Status {
Confirmed = 'confirmed',
Confirmed = 'active',
Copy link
Owner

Choose a reason for hiding this comment

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

Confirmed !== active

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My bad 😞

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@danbilokha Do you say I'm showing wrong data to user
OR I just need to rename Enum for consistancy?

Copy link
Owner

Choose a reason for hiding this comment

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

rename for consistency, I am not sure how you use that for showing the data

Deaths = 'deaths',
Recovered = 'recovered',
}
Expand Down
60 changes: 55 additions & 5 deletions server/src/services/domain/chart.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { Status } from '../../models/constants';
import { ChartModel } from '../../models/chart.models';
import { CountrySituationInfo } from '../../models/covid19.models';

import { Status } from '../../models/constants';
export const Transform = (situations: CountrySituationInfo[]): ChartModel => {
const days = situations.map((x) => x.date);
return {
Expand All @@ -13,21 +12,72 @@ export const Transform = (situations: CountrySituationInfo[]): ChartModel => {
label: Status.Confirmed,
data: situations.map((x) => x.confirmed),
fill: false,
borderColor: 'blue',
borderColor: 'rgb(230, 165, 96)',
backgroundColor: 'rgb(230, 165, 96)',
},
{
label: Status.Deaths,
data: situations.map((x) => x.deaths),
fill: false,
borderColor: 'red',
borderColor: 'rgb(222, 66, 91)',
backgroundColor: 'rgb(222, 66, 91)',
},
{
label: Status.Recovered,
data: situations.map((x) => x.recovered),
fill: false,
borderColor: 'green',
borderColor: 'rgb(62, 148, 107)',
backgroundColor: 'rgb(62, 148, 107)',
},
],
},
options: {},
};
};

export const enrichWithTitle = (
model: ChartModel,
title: string
): ChartModel => {
return {
...model,
options: {
...model.options,
title: {
display: true,
text: title,
fontColor: 'dark',
fontFamily: 'Helvetica',
fontSize: 28,
},
},
};
};

export const enrichWithType = (model: ChartModel, type: string): ChartModel => {
if (type === 'barStacked') {
Proladge marked this conversation as resolved.
Show resolved Hide resolved
return {
...model,
type: 'bar',
options: {
...model.options,
scales: {
xAxes: [
{
stacked: true,
},
],
yAxes: [
{
stacked: true,
},
],
},
},
};
}
return {
Proladge marked this conversation as resolved.
Show resolved Hide resolved
...model,
type,
};
};
2 changes: 1 addition & 1 deletion server/src/utils/dateUtils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export const addDays = (date: string | Date, days: number) => {
export const addDays = (date: string | Date, days: number): Date => {
Copy link
Owner

Choose a reason for hiding this comment

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

thanks 👍 really appreciate typings

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Types
Types

Types

Types

Types

Types

kript

kript

kript

kript

kript
kript

const result = new Date(date);
result.setDate(result.getDate() + days);
return result;
Expand Down