Add localization, call validation and use call type on subpath #5221
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
enahum
requested changes
Mar 12, 2021
There was a problem hiding this comment.
the error handler is throwing me off., why is it under data? instead of being its own error property.
Before I continue with the review please fix the error handlers data, also have the @aaronrothschild reviewed the error messages?
app/actions/apps.ts
Outdated
| return {data: res}; | ||
| case AppCallResponseTypes.ERROR: | ||
| return {data: res, error: {message: res.error}}; | ||
| return {data: res}; |
There was a problem hiding this comment.
why error is returning data instead of error
There was a problem hiding this comment.
This part of the code is dealing all the time with the AppCallResponse structure. The AppCallResponse already has an error type and field and may have extra context for the element that will handle the error. I understand this is not very semantic, but dividing here (and potentially in many other places down the road) the structure to handle Errors and the rest of the results separately will generate more confusion, I think.
There was a problem hiding this comment.
the structure to handle Errors and the rest of the results separately will generate more confusion, I think.
I agree, though per PR feedback I did start using the error return value in some places where doAppCall is called, to try to conform to the rest of the mobile app's error handling. So those instances (particularly in the apps modal form) will need to be reverted to avoid using that field and instead use the call response's error field.
There was a problem hiding this comment.
you decide what is best but obviously this Pr is impacting the changes done in the other PR. In my opinion it should follow the {data: ..., error: ... } pattern and I don't see why it should change unless there is a better reason than having to deconstruct the response from the server.
app/actions/apps.ts
Outdated
| id: 'apps.error.responses.form.no_form', | ||
| defaultMessage: 'Response type is `form`, but no form was included in response.', | ||
| }); | ||
| return {data: makeCallErrorResponse(errMsg)}; |
There was a problem hiding this comment.
Suggested change
| return {data: makeCallErrorResponse(errMsg)}; | |
| return {error: makeCallErrorResponse(errMsg)}; |
app/actions/apps.ts
Outdated
| id: 'apps.error.responses.navigate.no_url', | ||
| defaultMessage: 'Response type is `navigate`, but no url was included in response.', | ||
| }); | ||
| return {data: makeCallErrorResponse(errMsg)}; |
There was a problem hiding this comment.
Suggested change
| return {data: makeCallErrorResponse(errMsg)}; | |
| return {error: makeCallErrorResponse(errMsg)}; |
app/actions/apps.ts
Outdated
| id: 'apps.error.responses.navigate.no_submit', | ||
| defaultMessage: 'Response type is `navigate`, but the call was not a submission.', | ||
| }); | ||
| return {data: makeCallErrorResponse(errMsg)}; |
There was a problem hiding this comment.
Suggested change
| return {data: makeCallErrorResponse(errMsg)}; | |
| return {error: makeCallErrorResponse(errMsg)}; |
app/actions/apps.ts
Outdated
| }, { | ||
| type: responseType, | ||
| }); | ||
| return {data: makeCallErrorResponse(errMsg)}; |
There was a problem hiding this comment.
Suggested change
| return {data: makeCallErrorResponse(errMsg)}; | |
| return {error: makeCallErrorResponse(errMsg)}; |
app/actions/apps.ts
Outdated
| id: 'apps.error.responses.unexpected_error', | ||
| defaultMessage: 'Received an unexpected error.', | ||
| }); | ||
| return {data: makeCallErrorResponse(errMsg)}; |
There was a problem hiding this comment.
Suggested change
| return {data: makeCallErrorResponse(errMsg)}; | |
| return {error: makeCallErrorResponse(errMsg)}; |
app/actions/views/command.ts
Outdated
| const createErrorMessage = (errMessage: string) => { | ||
| return {error: {message: errMessage}}; | ||
| }; | ||
|
|
||
| if (!call) { | ||
| return {error: {message: 'Error submitting command'}}; |
| if (callResp.markdown) { | ||
| sendEphemeralPost(callResp.markdown, args.channel_id, args.parent_id); | ||
| } | ||
| return {data: {}}; |
There was a problem hiding this comment.
why is this returning an empty object in data? would it b sufficient to return {data: true} ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
As long as the empty object is being handled correctly I have no problem with it
mickmister
reviewed
Mar 12, 2021
app/components/autocomplete/slash_suggestion/app_command_parser/app_command_parser.ts
Outdated
Show resolved
Hide resolved
DHaussermann
added
the
Build Apps for PR
|
Building app in separate branch. |
mattermod
removed
the
Build Apps for PR
|
If I have a user field in a command, and I spell the username wrong, when I try to autocomplete a dynamic argument later in the command, it fails to parse the form because the user fetch request fails. I'm wondering if we should have "forgiving" behavior during the autocomplete phase (versus submit), or show the error to the user when this sort of error happens. |
mickmister
reviewed
Mar 15, 2021
|
@larkox going to review now, but it seems this PR has a conflict. |
|
@enahum conflict solved |
enahum
reviewed
Mar 15, 2021
There was a problem hiding this comment.
Added some comments, Also will approve after @aaronrothschild gives the green light on error text
| 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.', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
console output is removed from the code base in production builds, see https://reactnative.dev/docs/next/performance#using-consolelog-statements
| if (callResp.markdown) { | ||
| sendEphemeralPost(callResp.markdown, args.channel_id, args.parent_id); | ||
| } | ||
| return {data: {}}; |
There was a problem hiding this comment.
As long as the empty object is being handled correctly I have no problem with it
| const {data} = await this.props.actions.getChannel(post.channel_id) as {data?: any; error?: any}; | ||
| if (data) { | ||
| const channel = data as Channel; | ||
| teamID = channel.team_id; |
There was a problem hiding this comment.
can this be a DM/GM where the channel does not belong to a team?
| binding.app_id, | ||
| AppBindingLocations.IN_POST + binding.location, | ||
| post.channel_id, | ||
| teamID, |
There was a problem hiding this comment.
if teamID is undefined or empty you should consider using the currentTeamId
| binding.app_id, | ||
| AppBindingLocations.IN_POST + binding.location, | ||
| post.channel_id, | ||
| teamID, |
There was a problem hiding this comment.
same, unless passing an empty string is already handled
mickmister
reviewed
Mar 15, 2021
|
@enahum @larkox As I review this - should I be expecting that these errors will be returned to an end-user ("Hey something went wrong, a developer needs to fix it") with sufficient context for a developer to fix it ("...and send them this info SEGFAULT XYZ blah blah"). OR are these errors being returned to the developer via a return response in our API? Or both? I ask because it will determine how I look at the error messages. |
|
@aaron.rothschild these errors are being shown to the user. Nothing is getting automatically sent our way. There are some errors that wouldn't be that useful to see as a user. Like |
aaronrothschild
approved these changes
Mar 15, 2021
| default: | ||
| return createErrorMessage(intl.formatMessage({ | ||
| id: 'apps.error.responses.unknown_type', | ||
| defaultMessage: 'App response type not supported. Response type: {type}.', |
There was a problem hiding this comment.
Should we eventually (not now) add links to supported App response types.
app/components/autocomplete/slash_suggestion/app_command_parser/app_command_parser.ts
Outdated
Show resolved
Hide resolved
app/components/autocomplete/slash_suggestion/app_command_parser/app_command_parser.ts
Outdated
Show resolved
Hide resolved
| return []; | ||
| return this.makeSuggestionError(this.intl.formatMessage({ | ||
| id: 'apps.error.lookup.cannot_compose', | ||
| defaultMessage: 'Cannot compose lookup call.', |
app/components/autocomplete/slash_suggestion/app_command_parser/app_command_parser.ts
Outdated
Show resolved
Hide resolved
mickmister
approved these changes
Mar 16, 2021
migbot
approved these changes
Mar 16, 2021
enahum
approved these changes
Mar 16, 2021
larkox
added
4: Reviews Complete
mickmister
added a commit
that referenced
this pull request
Mar 23, 2021
* Apps bindings support (#5012) * Add redux related information * Add binding loading on channel refresh * Add channel header and post option bindings * Fix test * Minor fixes * Fix snapshots * Handle errors and show bindings only on main channel * Update Expand Levels keys and values to match latest changes * Add NAVIGATE call response handling. * Add more isolation to apps related code * Add defaults to send ephemeral * Update variable naming * Rename shouldProcessApps by a more meaningful appsEnabled * Embedded forms (#5169) * Add redux related information * Add binding loading on channel refresh * Add channel header and post option bindings * Fix test * Minor fixes * Fix snapshots * Handle errors and show bindings only on main channel * Update Expand Levels keys and values to match latest changes * Add NAVIGATE call response handling. * Add more isolation to apps related code * Add Embedded Forms * Fix snapshots * Add Embedded Forms * Improve naming, change the root element to be a binding and improve binding handling * Get post down on the buttons, remove unneeded variables and minor fixes from the review * Allow undefined bindings to fillBindingsInformation and add logging for error. * Address review feedback * Add App Forms (#5177) * Add App Forms * Address feedback and self review * Add dynamic select * Fixes and improvements * Add the ability to refresh on submit. * Use AppFormValue instead of redoing the type * Address feedback * [MM-31508] Rename URL to Path in Call (#5186) * Add user agent to call context (#5193) * Remove unneeded state reference (#5196) * Change user agent query on fetch bindings (#5201) * Add refresh websocket event to refetch bindings (#5194) * Add refresh websocket event to refetch bindings * Add missing changes * Declare socket event constant and separate the handler to a different function * Add binding validation on binding fetch (#5200) * Apps commands (#5107) * Add redux related information * Add binding loading on channel refresh * Add channel header and post option bindings * Fix test * Minor fixes * Fix snapshots * apps modals draft * Handle errors and show bindings only on main channel * Update Expand Levels keys and values to match latest changes * Add NAVIGATE call response handling. * Add more isolation to apps related code * reuse command parser throughout slash_suggestion component's lifecycle * fix prop and lint * using alert to show error message. need another way * duplicate import * types * types * types * rename file * copy webapp parser and test * dependencies moved. tests pass * move app command parser into its own folder * converted to typescript, all tests are passing * automated and manual tests work * lint * lint * remove fall throughs with blocks * types * doAppCall type * extract displayError to deps file * test types * lint * fix tests * unused import * PR feedback * fix imports and show errors * types * remove execute suggestion for mobile * watch feature flag * fix tests * change form text arugment behavior to show user input and not hint * return call response error in doAppCall * update tests to remove hint from text field suggestions * lint * use new base command structure * fix tests * wrap appsEnabled * typescript actions/command.ts * update app constants * PR feedback * fix error handling from doAppCall action * Use App CallRequest structure (#5212) * error handling * move test files * remove unused import Co-authored-by: Daniel Espino García <larkox@gmail.com> * Add feature flag (#5207) * Add feature flag * Simplify return * Add localization, call validation and use call type on subpath (#5221) * Add localization, call validation and use call type on subpath * Add more localization to the parser and bring fixes from webapp * Fix ephemerals and channel header / post options crashes * fix app command parser deps and alert messages * Improve suggestion handling * Fix test * Fix lint * Return errors as error * Address PR feedback * return error property * fix error string wordings Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com> * update mobile command parser with webapp fixes Co-authored-by: Daniel Espino García <larkox@gmail.com> Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>
larkox
added a commit
that referenced
this pull request
Mar 23, 2021
…5229) * Apps bindings support (#5012) * Add redux related information * Add binding loading on channel refresh * Add channel header and post option bindings * Fix test * Minor fixes * Fix snapshots * Handle errors and show bindings only on main channel * Update Expand Levels keys and values to match latest changes * Add NAVIGATE call response handling. * Add more isolation to apps related code * Add defaults to send ephemeral * Update variable naming * Rename shouldProcessApps by a more meaningful appsEnabled * Embedded forms (#5169) * Add redux related information * Add binding loading on channel refresh * Add channel header and post option bindings * Fix test * Minor fixes * Fix snapshots * Handle errors and show bindings only on main channel * Update Expand Levels keys and values to match latest changes * Add NAVIGATE call response handling. * Add more isolation to apps related code * Add Embedded Forms * Fix snapshots * Add Embedded Forms * Improve naming, change the root element to be a binding and improve binding handling * Get post down on the buttons, remove unneeded variables and minor fixes from the review * Allow undefined bindings to fillBindingsInformation and add logging for error. * Address review feedback * Add App Forms (#5177) * Add App Forms * Address feedback and self review * Add dynamic select * Fixes and improvements * Add the ability to refresh on submit. * Use AppFormValue instead of redoing the type * Address feedback * [MM-31508] Rename URL to Path in Call (#5186) * Add user agent to call context (#5193) * Remove unneeded state reference (#5196) * Change user agent query on fetch bindings (#5201) * Add refresh websocket event to refetch bindings (#5194) * Add refresh websocket event to refetch bindings * Add missing changes * Declare socket event constant and separate the handler to a different function * Add binding validation on binding fetch (#5200) * Apps commands (#5107) * Add redux related information * Add binding loading on channel refresh * Add channel header and post option bindings * Fix test * Minor fixes * Fix snapshots * apps modals draft * Handle errors and show bindings only on main channel * Update Expand Levels keys and values to match latest changes * Add NAVIGATE call response handling. * Add more isolation to apps related code * reuse command parser throughout slash_suggestion component's lifecycle * fix prop and lint * using alert to show error message. need another way * duplicate import * types * types * types * rename file * copy webapp parser and test * dependencies moved. tests pass * move app command parser into its own folder * converted to typescript, all tests are passing * automated and manual tests work * lint * lint * remove fall throughs with blocks * types * doAppCall type * extract displayError to deps file * test types * lint * fix tests * unused import * PR feedback * fix imports and show errors * types * remove execute suggestion for mobile * watch feature flag * fix tests * change form text arugment behavior to show user input and not hint * return call response error in doAppCall * update tests to remove hint from text field suggestions * lint * use new base command structure * fix tests * wrap appsEnabled * typescript actions/command.ts * update app constants * PR feedback * fix error handling from doAppCall action * Use App CallRequest structure (#5212) * error handling * move test files * remove unused import Co-authored-by: Daniel Espino García <larkox@gmail.com> * Add feature flag (#5207) * Add feature flag * Simplify return * Add localization, call validation and use call type on subpath (#5221) * Add localization, call validation and use call type on subpath * Add more localization to the parser and bring fixes from webapp * Fix ephemerals and channel header / post options crashes * fix app command parser deps and alert messages * Improve suggestion handling * Fix test * Fix lint * Return errors as error * Address PR feedback * return error property * fix error string wordings Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com> * migrate app selector to AutocompleteSelector * add if statement to avoid missing prop * add tests for dynamic values * clean up * remove old files Co-authored-by: Daniel Espino García <larkox@gmail.com> Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>
mickmister
added a commit
that referenced
this pull request
Mar 24, 2021
* Apps bindings support (#5012) * Add redux related information * Add binding loading on channel refresh * Add channel header and post option bindings * Fix test * Minor fixes * Fix snapshots * Handle errors and show bindings only on main channel * Update Expand Levels keys and values to match latest changes * Add NAVIGATE call response handling. * Add more isolation to apps related code * Add defaults to send ephemeral * Update variable naming * Rename shouldProcessApps by a more meaningful appsEnabled * Embedded forms (#5169) * Add redux related information * Add binding loading on channel refresh * Add channel header and post option bindings * Fix test * Minor fixes * Fix snapshots * Handle errors and show bindings only on main channel * Update Expand Levels keys and values to match latest changes * Add NAVIGATE call response handling. * Add more isolation to apps related code * Add Embedded Forms * Fix snapshots * Add Embedded Forms * Improve naming, change the root element to be a binding and improve binding handling * Get post down on the buttons, remove unneeded variables and minor fixes from the review * Allow undefined bindings to fillBindingsInformation and add logging for error. * Address review feedback * Add App Forms (#5177) * Add App Forms * Address feedback and self review * Add dynamic select * Fixes and improvements * Add the ability to refresh on submit. * Use AppFormValue instead of redoing the type * Address feedback * [MM-31508] Rename URL to Path in Call (#5186) * Add user agent to call context (#5193) * Remove unneeded state reference (#5196) * Change user agent query on fetch bindings (#5201) * Add refresh websocket event to refetch bindings (#5194) * Add refresh websocket event to refetch bindings * Add missing changes * Declare socket event constant and separate the handler to a different function * Add binding validation on binding fetch (#5200) * Apps commands (#5107) * Add redux related information * Add binding loading on channel refresh * Add channel header and post option bindings * Fix test * Minor fixes * Fix snapshots * apps modals draft * Handle errors and show bindings only on main channel * Update Expand Levels keys and values to match latest changes * Add NAVIGATE call response handling. * Add more isolation to apps related code * reuse command parser throughout slash_suggestion component's lifecycle * fix prop and lint * using alert to show error message. need another way * duplicate import * types * types * types * rename file * copy webapp parser and test * dependencies moved. tests pass * move app command parser into its own folder * converted to typescript, all tests are passing * automated and manual tests work * lint * lint * remove fall throughs with blocks * types * doAppCall type * extract displayError to deps file * test types * lint * fix tests * unused import * PR feedback * fix imports and show errors * types * remove execute suggestion for mobile * watch feature flag * fix tests * change form text arugment behavior to show user input and not hint * return call response error in doAppCall * update tests to remove hint from text field suggestions * lint * use new base command structure * fix tests * wrap appsEnabled * typescript actions/command.ts * update app constants * PR feedback * fix error handling from doAppCall action * Use App CallRequest structure (#5212) * error handling * move test files * remove unused import Co-authored-by: Daniel Espino García <larkox@gmail.com> * Add feature flag (#5207) * Add feature flag * Simplify return * Add localization, call validation and use call type on subpath (#5221) * Add localization, call validation and use call type on subpath * Add more localization to the parser and bring fixes from webapp * Fix ephemerals and channel header / post options crashes * fix app command parser deps and alert messages * Improve suggestion handling * Fix test * Fix lint * Return errors as error * Address PR feedback * return error property * fix error string wordings Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com> * leave the channel info screen when the call is successful. stay if there is an error * fix intl in tests Co-authored-by: Daniel Espino García <larkox@gmail.com> Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com> Co-authored-by: Mattermod <mattermod@users.noreply.github.com>
mickmister
added a commit
that referenced
this pull request
Mar 24, 2021
* Apps bindings support (#5012) * Add redux related information * Add binding loading on channel refresh * Add channel header and post option bindings * Fix test * Minor fixes * Fix snapshots * Handle errors and show bindings only on main channel * Update Expand Levels keys and values to match latest changes * Add NAVIGATE call response handling. * Add more isolation to apps related code * Add defaults to send ephemeral * Update variable naming * Rename shouldProcessApps by a more meaningful appsEnabled * Embedded forms (#5169) * Add redux related information * Add binding loading on channel refresh * Add channel header and post option bindings * Fix test * Minor fixes * Fix snapshots * Handle errors and show bindings only on main channel * Update Expand Levels keys and values to match latest changes * Add NAVIGATE call response handling. * Add more isolation to apps related code * Add Embedded Forms * Fix snapshots * Add Embedded Forms * Improve naming, change the root element to be a binding and improve binding handling * Get post down on the buttons, remove unneeded variables and minor fixes from the review * Allow undefined bindings to fillBindingsInformation and add logging for error. * Address review feedback * Add App Forms (#5177) * Add App Forms * Address feedback and self review * Add dynamic select * Fixes and improvements * Add the ability to refresh on submit. * Use AppFormValue instead of redoing the type * Address feedback * [MM-31508] Rename URL to Path in Call (#5186) * Add user agent to call context (#5193) * Remove unneeded state reference (#5196) * Change user agent query on fetch bindings (#5201) * Add refresh websocket event to refetch bindings (#5194) * Add refresh websocket event to refetch bindings * Add missing changes * Declare socket event constant and separate the handler to a different function * Add binding validation on binding fetch (#5200) * Apps commands (#5107) * Add redux related information * Add binding loading on channel refresh * Add channel header and post option bindings * Fix test * Minor fixes * Fix snapshots * apps modals draft * Handle errors and show bindings only on main channel * Update Expand Levels keys and values to match latest changes * Add NAVIGATE call response handling. * Add more isolation to apps related code * reuse command parser throughout slash_suggestion component's lifecycle * fix prop and lint * using alert to show error message. need another way * duplicate import * types * types * types * rename file * copy webapp parser and test * dependencies moved. tests pass * move app command parser into its own folder * converted to typescript, all tests are passing * automated and manual tests work * lint * lint * remove fall throughs with blocks * types * doAppCall type * extract displayError to deps file * test types * lint * fix tests * unused import * PR feedback * fix imports and show errors * types * remove execute suggestion for mobile * watch feature flag * fix tests * change form text arugment behavior to show user input and not hint * return call response error in doAppCall * update tests to remove hint from text field suggestions * lint * use new base command structure * fix tests * wrap appsEnabled * typescript actions/command.ts * update app constants * PR feedback * fix error handling from doAppCall action * Use App CallRequest structure (#5212) * error handling * move test files * remove unused import Co-authored-by: Daniel Espino García <larkox@gmail.com> * Add feature flag (#5207) * Add feature flag * Simplify return * Add localization, call validation and use call type on subpath (#5221) * Add localization, call validation and use call type on subpath * Add more localization to the parser and bring fixes from webapp * Fix ephemerals and channel header / post options crashes * fix app command parser deps and alert messages * Improve suggestion handling * Fix test * Fix lint * Return errors as error * Address PR feedback * return error property * fix error string wordings Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com> * show bindings in threads and all post menus * unused import * fallback on post id in case this is the root post * fix ephemeral reply for modal submissions * fix merge typo Co-authored-by: Daniel Espino García <larkox@gmail.com> Co-authored-by: Ben Schumacher <ben.schumacher@mattermost.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR is part of the sync with Webapp code. It tackles mainly three topics:
Ticket Link
None
Related Pull Requests
Webapp: mattermost/mattermost-webapp#7679
Redux: mattermost/mattermost-redux#1406
Proxy: mattermost/mattermost-plugin-apps#99