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

Added send location feature #2635

Merged
merged 9 commits into from
Nov 28, 2023
Merged

Added send location feature #2635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0547476
added send location feature
mdshamoon Nov 23, 2023
6847868
Merge branch 'master' of https://github.com/glific/glific-frontend in…
mdshamoon Nov 23, 2023
9f43928
added list change fix
mdshamoon Nov 23, 2023
26cd674
fixed test cases
mdshamoon Nov 23, 2023
549093d
added optimizations
mdshamoon Nov 24, 2023
5c0db11
added review
mdshamoon Nov 24, 2023
60115a3
commented out
mdshamoon Nov 24, 2023
92327ca
added one test case
mdshamoon Nov 27, 2023
ac5ba62
fixed translations
mdshamoon Nov 27, 2023
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"@emoji-mart/react": "^1.1.1",
"@emotion/react": "^11.11.1",
"@emotion/styled": "^11.11.0",
"@glific/flow-editor": "^1.19.3",
"@glific/flow-editor": "^1.19.3-1",
"@mui/icons-material": "^5.14.16",
"@mui/material": "^5.14.16",
"@mui/x-date-pickers": "^6.17.0",
Expand Down
10 changes: 3 additions & 7 deletions src/common/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ export const GUPSHUP_QUICK_REPLY =
export const CALL_TO_ACTION = 'CALL_TO_ACTION';
export const LIST = 'LIST';
export const QUICK_REPLY = 'QUICK_REPLY';
export const LOCATION_REQUEST = 'LOCATION_REQUEST_MESSAGE';
Copy link

Choose a reason for hiding this comment

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

It's usually better to prefix constants with a common namespace to avoid naming collisions especially with generic terms like LOCATION_REQUEST. Consider using a more explicit constant name like MESSAGE_TYPE_LOCATION_REQUEST.

export const TERMS_OF_USE_LINK = 'https://tides.coloredcow.com/terms-of-use';
export const COMPACT_MESSAGE_LENGTH = 35;
Copy link

Choose a reason for hiding this comment

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

Defining the COMPACT_MESSAGE_LENGTH as a constant is good, but make sure to use it wherever appropriate throughout the application to maintain consistency.


// Gupshup sample media
export const SAMPLE_MEDIA_FOR_SIMULATOR = [
Expand Down Expand Up @@ -209,10 +212,3 @@ export const yupPasswordValidation = (t: any) =>
)
.min(10, t('Password must be at least 10 characters long.'))
.required(t('Input required'));

export const INTERACTIVE_QUICK_REPLY = 'QUICK_REPLY';
export const INTERACTIVE_LIST = 'LIST';

export const TERMS_OF_USE_LINK = 'https://tides.coloredcow.com/terms-of-use';

export const COMPACT_MESSAGE_LENGTH = 35;
2 changes: 2 additions & 0 deletions src/common/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,8 @@
default:
break;
}
} else if (interactiveJSON.type === 'location_request_message') {
Copy link

Choose a reason for hiding this comment

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

The usage of string literal 'location_request_message' should be replaced by the LOCATION_REQUEST constant to maintain consistency and ease of code maintenance.

messageBody = interactiveJSON.body.text;

Check warning on line 138 in src/common/utils.ts

View check run for this annotation

Codecov / codecov/patch

src/common/utils.ts#L138 

Added line #L138 was not covered by tests
}

return messageBody;
Expand Down
8 changes: 5 additions & 3 deletions src/components/UI/Pager/Pager.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,14 @@ const collapsedRowData = (dataObj: any, columnStyles: any, recordId: any) => {
</TableRow>
);
}
console.log(dataObj);

const additionalRowInformation = Object.keys(dataObj).map((key, index) => {
const rowIdentifier = `collapsedRowData-${recordId}-${index}`;

// This is for location translation type where the text is inside body.
const body = typeof dataObj[key].body === 'string' ? dataObj[key].body : dataObj[key].body.text;

return (
<TableRow className={styles.CollapseTableRow} key={rowIdentifier}>
<TableCell className={`${styles.TableCell} ${columnStyles ? columnStyles[0] : null}`}>
Expand All @@ -58,11 +62,9 @@ const collapsedRowData = (dataObj: any, columnStyles: any, recordId: any) => {
</TableCell>
<TableCell className={`${styles.TableCell} ${columnStyles ? columnStyles[1] : null}`}>
<div>
<p className={styles.TableText}>{dataObj[key].body}</p>
<p className={styles.TableText}>{body}</p>
</div>
</TableCell>
<TableCell className={`${styles.TableCell} ${columnStyles ? columnStyles[2] : null}`} />
<TableCell className={`${styles.TableCell} ${columnStyles ? columnStyles[3] : null}`} />
</TableRow>
);
});
Expand Down
21 changes: 16 additions & 5 deletions src/components/simulator/Simulator.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,10 @@
import {
TIME_FORMAT,
SAMPLE_MEDIA_FOR_SIMULATOR,
Copy link

Choose a reason for hiding this comment

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

The LocationRequestTemplate import path should be verified to make sure it refers to the correct file location. Since LocationRequestTemplate is a React component, check that the actual file is located within the specified import path.

INTERACTIVE_LIST,
INTERACTIVE_QUICK_REPLY,
LIST,
QUICK_REPLY,
DEFAULT_MESSAGE_LIMIT,
LOCATION_REQUEST,
} from 'common/constants';
import { GUPSHUP_CALLBACK_URL } from 'config';
import { ChatMessageType } from 'containers/Chat/ChatMessages/ChatMessage/ChatMessageType/ChatMessageType';
Expand Down Expand Up @@ -54,6 +55,7 @@
} from 'graphql/subscriptions/Simulator';
import { updateSimulatorConversations } from 'services/SubscriptionService';
import styles from './Simulator.module.css';
import { LocationRequestTemplate } from 'containers/Chat/ChatMessages/ChatMessage/LocationRequestTemplate/LocationRequestTemplate';

export interface SimulatorProps {
showSimulator: boolean;
Expand Down Expand Up @@ -296,7 +298,7 @@
if (content) {
isInteractiveContentPresent = !!Object.entries(content).length;

if (isInteractiveContentPresent && messageType === INTERACTIVE_LIST) {
if (isInteractiveContentPresent && messageType === LIST) {
template = (
<>
<ListReplyTemplate
Expand All @@ -311,7 +313,7 @@
);
}

if (isInteractiveContentPresent && messageType === INTERACTIVE_QUICK_REPLY) {
if (isInteractiveContentPresent && messageType === QUICK_REPLY) {
template = (
<QuickReplyTemplate
{...content}
Expand All @@ -323,6 +325,15 @@
/>
);
}
Copy link

Choose a reason for hiding this comment

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

The renderMessage function call seems to use string literals for messageType comparisons, such as 'received'. Consider using or creating constants for these to reduce the possibility of errors due to typos.

if (isInteractiveContentPresent && messageType === LOCATION_REQUEST) {
template = (
Copy link

Choose a reason for hiding this comment

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

The inline creation of an object for the LocationRequestTemplate props, including content, isSimulator, and onSendLocationClick, influences readability and can be error-prone. Consider refactoring this to use a properly defined and typed object.

<LocationRequestTemplate
content={content}
isSimulator
onSendLocationClick={(payload: any) => sendMessage(sender, payload)}

Check warning on line 333 in src/components/simulator/Simulator.tsx

View check run for this annotation

Codecov / codecov/patch

src/components/simulator/Simulator.tsx#L333 

Added line #L333 was not covered by tests
/>
);
}
}

return (
Expand Down Expand Up @@ -386,7 +397,7 @@
const { templateType, interactiveContent } = interactiveMessage;
const previewMessage = renderMessage(interactiveMessage, 'received', 0, true);
setSimulatedMessage(previewMessage);
if (templateType === INTERACTIVE_LIST) {
if (templateType === LIST) {
Copy link

Choose a reason for hiding this comment

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

The JSON.parse(interactiveContent) function call has no accompanying validation or try-catch, which can cause uncaught exceptions if interactiveContent is not in a proper JSON format. Consider adding error handling to ensure robustness of the code.

const { items } = JSON.parse(interactiveContent);
setSelectedListTemplate(items);
} else {
Expand Down
14 changes: 10 additions & 4 deletions src/containers/Chat/ChatMessages/ChatMessage/ChatMessage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@
import {
DATE_FORMAT,
TIME_FORMAT,
INTERACTIVE_LIST,
INTERACTIVE_QUICK_REPLY,
LIST,
QUICK_REPLY,
VALID_URL_REGEX,
LOCATION_REQUEST,
Copy link

Choose a reason for hiding this comment

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

The imported constants LIST and QUICK_REPLY are used as string literals directly. It's recommended to use the constants instead of the raw strings for messageType comparisons.

} from 'common/constants';
import { WhatsAppToJsx, WhatsAppTemplateButton } from 'common/RichEditor';
import { Tooltip } from 'components/UI/Tooltip/Tooltip';
Expand All @@ -23,6 +24,7 @@
import { QuickReplyTemplate } from '../QuickReplyTemplate/QuickReplyTemplate';
import styles from './ChatMessage.module.css';
import { setNotification } from 'common/notification';
import { LocationRequestTemplate } from './LocationRequestTemplate/LocationRequestTemplate';
Copy link

Choose a reason for hiding this comment

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

The import statement for LocationRequestTemplate should be verified for correctness. Imports should reflect the accurate directory structure to prevent module resolution issues.


export interface ChatMessageProps {
id: number;
Expand Down Expand Up @@ -260,14 +262,18 @@
}

let template = null;
if (type === INTERACTIVE_LIST) {
if (type === LIST) {
template = <ListReplyTemplate {...content} disabled component={ChatTemplate} />;
}

if (type === INTERACTIVE_QUICK_REPLY) {
if (type === QUICK_REPLY) {
template = <QuickReplyTemplate {...content} disabled />;
}

if (type === LOCATION_REQUEST) {
template = <LocationRequestTemplate content={content} disabled />;

Check warning on line 274 in src/containers/Chat/ChatMessages/ChatMessage/ChatMessage.tsx

View check run for this annotation

Codecov / codecov/patch

src/containers/Chat/ChatMessages/ChatMessage/ChatMessage.tsx#L274 

Added line #L274 was not covered by tests
}

let displayLabel;
Copy link

Choose a reason for hiding this comment

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

The use of {...content} might not be the most appropriate way to pass props if the LocationRequestTemplate component expects specific properties. Be explicit about which properties are being passed down to child components.

if (flowLabel) {
const labels = flowLabel.split(',');
Expand Down
26 changes: 26 additions & 0 deletions .../Chat/ChatMessages/ChatMessage/LocationRequestTemplate/LocationRequestTemplate.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
.MessageContent {
max-width: 100%;
border-radius: 12px 12px 0px 0px;
font-family: 'Heebo', sans-serif, emoji;
box-shadow: 0 1px 2px rgba(0, 0, 0, 0.4);
clear: both;
max-width: 404px;
min-width: 85px;
padding: 10px;
Copy link

Choose a reason for hiding this comment

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

Verify that this CSS class .SimulatorButton and the custom property isSimulator have the same meaning and usage within the context of the LocationRequestTemplate component. It may lead to misunderstanding or misuse. Consider renaming it for clarity if there's a misalignment.

background: white;
border-bottom: 1px solid rgba(0, 0, 0, 0.1);
}

.SimulatorButton {
width: 100%;
text-transform: none !important;
border-radius: 0px 0px 15px 15px !important;
background: white !important;
border: 1px solid rgba(0, 0, 0, 0.1) !important;
border-top: none !important
Copy link

Choose a reason for hiding this comment

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

!important should be avoided in CSS as it makes debugging more difficult by breaking the natural cascading in your stylesheets. If overridden styles are indeed needed, look for a more specific selector or restructure your CSS to avoid using !important.

}

.ChatButton {
composes: SimulatorButton;
padding: 5px 20px !important;
}
48 changes: 48 additions & 0 deletions ...tainers/Chat/ChatMessages/ChatMessage/LocationRequestTemplate/LocationRequestTemplate.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { Button } from '@mui/material';
import LocationIconDark from 'assets/images/icons/Location/Dark.svg?react';
import styles from './LocationRequestTemplate.module.css';
import { ChatMessageType } from '../ChatMessageType/ChatMessageType';

export interface LocationRequestTemplateProps {
content: any;
disabled?: boolean;

isSimulator?: boolean;
onSendLocationClick?: any;
}

const locationPayload = {
type: 'location',
name: 'location',
id: 'LOCATION',
payload: {
latitude: '41.725556',
longitude: '-49.946944',
},
};

export const LocationRequestTemplate = ({
content,
disabled = false,
isSimulator = false,

onSendLocationClick = () => {},
}: LocationRequestTemplateProps) => {
const body = content.body.text;
return (
<div>
<div className={styles.MessageContent}>
<ChatMessageType type="TEXT" body={body} isSimulatedMessage={isSimulator} />
</div>
<Button
variant="text"
disabled={disabled}
startIcon={<LocationIconDark />}
onClick={() => onSendLocationClick({ payload: locationPayload })}

Check warning on line 41 in src/containers/Chat/ChatMessages/ChatMessage/LocationRequestTemplate/LocationRequestTemplate.tsx

View check run for this annotation

Codecov / codecov/patch

src/containers/Chat/ChatMessages/ChatMessage/LocationRequestTemplate/LocationRequestTemplate.tsx#L41 

Added line #L41 was not covered by tests
className={isSimulator ? styles.SimulatorButton : styles.ChatButton}
Copy link

Choose a reason for hiding this comment

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

Passing inline functions, such as () => onSendLocationClick({ payload: locationPayload }), to components can cause unnecessary re-renders as the function will be different on every render. Refactor this by using useCallback hook or defining the function outside of the render return.

>
Send Location
</Button>
</div>
);
};
Loading
Loading