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

Jump to bottom

Add localization, call validation and use call type on subpath #5221

Merged
merged 12 commits into from
Mar 16, 2021
Merged

Add localization, call validation and use call type on subpath #5221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6229a23
Add localization, call validation and use call type on subpath
larkox Mar 12, 2021
f28b4dd
Add more localization to the parser and bring fixes from webapp
larkox Mar 12, 2021
cde4f45
Fix ephemerals and channel header / post options crashes
larkox Mar 12, 2021
dcfddad
fix app command parser deps and alert messages
mickmister Mar 15, 2021
474763d
Improve suggestion handling
larkox Mar 15, 2021
67ce2f3
Fix test
larkox Mar 15, 2021
9f7bf74
Fix lint
larkox Mar 15, 2021
5bbf084
Return errors as error
larkox Mar 15, 2021
769e7eb
Merge branch 'feature/cloud-apps' into TypeOnPath
larkox Mar 15, 2021
620c4f1
Address PR feedback
larkox Mar 15, 2021
13c02d9
return error property
mickmister Mar 15, 2021
32798ac
fix error string wordings
mickmister Mar 16, 2021
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
66 changes: 40 additions & 26 deletions app/actions/apps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,65 +3,79 @@

import {Client4} from '@mm-redux/client';
import {ActionFunc} from '@mm-redux/types/actions';
import {AppCallResponse, AppForm, AppCallRequest} from '@mm-redux/types/apps';
import {AppCallResponse, AppForm, AppCallRequest, AppCallType} from '@mm-redux/types/apps';
import {AppCallTypes, AppCallResponseTypes} from '@mm-redux/constants/apps';
import {sendEphemeralPost} from './views/post';
import {handleGotoLocation} from '@mm-redux/actions/integrations';
import {showModal} from './navigation';
import {Theme} from '@mm-redux/types/preferences';
import CompassIcon from '@components/compass_icon';
import {getTheme} from '@mm-redux/selectors/entities/preferences';
import EphemeralStore from '@store/ephemeral_store';
import {makeCallErrorResponse} from '@utils/apps';

export function doAppCall<Res=unknown>(call: AppCallRequest, intl: any): ActionFunc {
export function doAppCall<Res=unknown>(call: AppCallRequest, type: AppCallType, intl: any): ActionFunc {
return async (dispatch, getState) => {
const ephemeral = (text: string) => dispatch(sendEphemeralPost(text, call?.context.channel_id, call?.context.root_id));
try {
const res = await Client4.executeAppCall(call) as AppCallResponse<Res>;
const res = await Client4.executeAppCall(call, type) as AppCallResponse<Res>;
const responseType = res.type || AppCallResponseTypes.OK;

switch (responseType) {
case AppCallResponseTypes.OK:
if (res.markdown) {
if (!call.context.channel_id) {
return {data: res};
}

ephemeral(res.markdown);
}
return {data: res};
case AppCallResponseTypes.ERROR:
return {data: res, error: {message: res.error}};
return {error: res};
case AppCallResponseTypes.FORM: {
if (!res.form) {
const errMsg = 'An error has occurred. Please contact the App developer. Details: Response type is `form`, but no form was included in response.';

ephemeral(errMsg);
return {data: res};
const errMsg = intl.formatMessage({
id: 'apps.error.responses.form.no_form',
defaultMessage: 'Response type is `form`, but no form was included in response.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if this errors are shown to the user somehow, if they are.. do they make sense for an end user? they seem more relevant to the plugin developer / sysadmin, I doubt an end user will understand the meaning of this type of errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We feel the error should somehow be delivered specially for debugging. What do you think about a "default error" and a console.log statement here?

Copy link
Contributor

@enahum enahum Mar 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

console output is removed from the code base in production builds, see https://reactnative.dev/docs/next/performance#using-consolelog-statements

});
return {error: makeCallErrorResponse(errMsg)};
}

const screen = EphemeralStore.getNavigationTopComponentId();
if (call.type === AppCallTypes.SUBMIT && screen !== 'AppForm') {
if (type === AppCallTypes.SUBMIT && screen !== 'AppForm') {
showAppForm(res.form, call, getTheme(getState()));
}

return {data: res};
}
case AppCallResponseTypes.NAVIGATE:
if (!res.url) {
const errMsg = 'An error has occurred. Please contact the App developer. Details: Response type is `navigate`, but no url was included in response.';
if (!res.navigate_to_url) {
const errMsg = intl.formatMessage({
id: 'apps.error.responses.navigate.no_url',
defaultMessage: 'Response type is `navigate`, but no url was included in response.',
});
return {error: makeCallErrorResponse(errMsg)};
}

ephemeral(errMsg);
return {data: res};
if (type !== AppCallTypes.SUBMIT) {
const errMsg = intl.formatMessage({
id: 'apps.error.responses.navigate.no_submit',
defaultMessage: 'Response type is `navigate`, but the call was not a submission.',
});
return {error: makeCallErrorResponse(errMsg)};
}
dispatch(handleGotoLocation(res.url, intl));

dispatch(handleGotoLocation(res.navigate_to_url, intl));

return {data: res};
default: {
const errMsg = intl.formatMessage({
id: 'apps.error.responses.unknown_type',
defaultMessage: 'App response type not supported. Response type: {type}.',
}, {
type: responseType,
});
return {error: makeCallErrorResponse(errMsg)};
}
}

return {data: res};
} catch (error) {
return {error};
const errMsg = error.message || intl.formatMessage({
id: 'apps.error.responses.unexpected_error',
defaultMessage: 'Received an unexpected error.',
});
return {error: makeCallErrorResponse(errMsg)};
}
};
}
Expand Down
43 changes: 38 additions & 5 deletions app/actions/views/command.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@ import {intlShape} from 'react-intl';
import {IntegrationTypes} from '@mm-redux/action_types';
import {executeCommand as executeCommandService} from '@mm-redux/actions/integrations';
import {getCurrentTeamId} from '@mm-redux/selectors/entities/teams';
import {AppCallTypes} from '@mm-redux/constants/apps';
import {AppCallResponseTypes, AppCallTypes} from '@mm-redux/constants/apps';
import {DispatchFunc, GetStateFunc, ActionFunc} from '@mm-redux/types/actions';

import {AppCommandParser} from '@components/autocomplete/slash_suggestion/app_command_parser/app_command_parser';

import {doAppCall} from '@actions/apps';
import {appsEnabled} from '@utils/apps';
import {AppCallResponse} from '@mm-redux/types/apps';
import {sendEphemeralPost} from './post';

export function executeCommand(message: string, channelId: string, rootId: string, intl: typeof intlShape): ActionFunc {
return async (dispatch: DispatchFunc, getState: GetStateFunc) => {
Expand All @@ -39,15 +41,46 @@ export function executeCommand(message: string, channelId: string, rootId: strin

const appsAreEnabled = appsEnabled(state);
if (appsAreEnabled) {
const parser = new AppCommandParser({dispatch, getState}, args.channel_id, args.root_id);
const parser = new AppCommandParser({dispatch, getState}, intl, args.channel_id, args.root_id);
if (parser.isAppCommand(msg)) {
const call = await parser.composeCallFromCommand(message);
const createErrorMessage = (errMessage: string) => {
return {error: {message: errMessage}};
};

if (!call) {
return {error: {message: 'Error submitting command'}};
return createErrorMessage(intl.formatMessage({
id: 'mobile.commands.error_title',
defaultMessage: 'Error Executing Command',
}));
}

call.type = AppCallTypes.SUBMIT;
return dispatch(doAppCall(call, intl));
const res = await dispatch(doAppCall(call, AppCallTypes.SUBMIT, intl));
if (res.error) {
const errorResponse = res.error as AppCallResponse;
return createErrorMessage(errorResponse.error || intl.formatMessage({
id: 'apps.error.unknown',
defaultMessage: 'Unknown error.',
}));
}
const callResp = res.data as AppCallResponse;
switch (callResp.type) {
case AppCallResponseTypes.OK:
if (callResp.markdown) {
dispatch(sendEphemeralPost(callResp.markdown, args.channel_id, args.parent_id));
}
return {data: {}};
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this returning an empty object in data? would it b sufficient to return {data: true} ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The expected data to be returned from here is an Slash Command Response (https://developers.mattermost.com/integrate/slash-commands/). Right now is not being used, so {} and true would mean almost the same, but I preferred to keep the empty object.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as the empty object is being handled correctly I have no problem with it

case AppCallResponseTypes.FORM:
case AppCallResponseTypes.NAVIGATE:
return {data: {}};
default:
return createErrorMessage(intl.formatMessage({
id: 'apps.error.responses.unknown_type',
defaultMessage: 'App response type not supported. Response type: {type}.',
Copy link

Choose a reason for hiding this comment

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

Should we eventually (not now) add links to supported App response types.

}, {
type: callResp.type,
}));
}
}
}

Expand Down
Loading