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

Added send location feature #2635

Merged
merged 9 commits into from
Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
22 changes: 22 additions & 0 deletions .github/workflows/gpt-review.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
name: AI Code Reviewer

on:
pull_request:
types:
- opened
- synchronize
permissions: write-all

Choose a reason for hiding this comment

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

The permissions: write-all setting gives all permissions to all actions. This can be a security risk. Please consider limiting the permissions to only what is necessary for the workflow to function.

Choose a reason for hiding this comment

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

The types field should not be restricted to only the labeled event type if we want to review all pull requests. Consider adding more event types such as opened, synchronize, or reopened, unless there's a specific workflow that only requires the labeled event.

jobs:
review:
runs-on: ubuntu-latest
steps:

Choose a reason for hiding this comment

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

Using runs-on: ubuntu-latest is not always recommended for reproducible builds since 'latest' will always point to the latest stable version of Ubuntu. Specify an explicit version of the ubuntu runner like ubuntu-20.04 or ubuntu-22.04 to ensure consistent behavior as newer versions are released.

- name: Checkout Repo
uses: actions/checkout@v3

- name: AI Code Reviewer
uses: glific/ai-codereviewer@main
with:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
OPENAI_API_MODEL: "gpt-4" # Optional: defaults to "gpt-4"
exclude: "**/*.json, **/*.md" # Optional: exclude patterns separated by commas

Choose a reason for hiding this comment

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

The exclude pattern is excluding all .json and .md files. If there are any relevant files with these extensions that need to be included in the workflow, please adjust the pattern accordingly.

2 changes: 1 addition & 1 deletion package.json
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
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';

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;

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
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') {

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
21 changes: 16 additions & 5 deletions src/components/simulator/Simulator.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,10 @@
import {
TIME_FORMAT,
SAMPLE_MEDIA_FOR_SIMULATOR,

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 @@
/>
);
}

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 = (

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

View check run for this annotation

Codecov / codecov/patch

src/components/simulator/Simulator.tsx#L329

Added line #L329 was not covered by tests

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

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
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,

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';

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;

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
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;

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

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;
}
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 (

Check warning on line 32 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#L31-L32

Added lines #L31 - L32 were not covered by tests
<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}

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