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

Refactor apps modal autocomplete into AutocompleteSelector component #5229

Merged
merged 23 commits into from
Mar 23, 2021

Conversation

mickmister
Copy link
Contributor

@mickmister mickmister commented Mar 19, 2021

Summary

There was originally code copied to make the AppFormSelector work, mostly from the AutocompleteSelector and SelectorScreen components. Most of the code carried over had to do with the channel and user selectors, and the functionality for static and dynamic fields wasn't working correctly.

This PR removes the AppFormSelector, and instead extends the AutocompleteSelector to support dynamic values, so the AppFormField can provide dynamic lookup values. This way there is no code duplication, and any fixes made to AutocompleteSelector will be applied to the apps modal.

Tests have been added for the dynamic select that ensures the values are fetched on mount and used. Also that values are fetched on search and used.

Ticket Link

Fixes https://mattermost.atlassian.net/browse/MM-33718
Fixes https://mattermost.atlassian.net/browse/MM-33928

Awaiting merge of feature/cloud-apps -> master #5226

larkox and others added 21 commits February 10, 2021 15:23
* 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
* 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

* 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
* Add refresh websocket event to refetch bindings

* Add missing changes

* Declare socket event constant and separate the handler to a different function
* 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

* Simplify return
* 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>
@mickmister mickmister added 2: Dev Review Requires review by a core commiter Build Apps for PR Build the mobile app for iOS and Android to test 3: QA Review Requires review by a QA tester labels Mar 19, 2021
@mattermod
Copy link
Contributor

Building app in separate branch.

@mattermod mattermod removed the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 19, 2021
@mattermod
Copy link
Contributor

@mickmister mickmister requested a review from migbot March 19, 2021 15:31
@DHaussermann DHaussermann added the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 19, 2021
@mattermod
Copy link
Contributor

Building app in separate branch.

@mattermod mattermod removed the Build Apps for PR Build the mobile app for iOS and Android to test label Mar 19, 2021
@mattermod
Copy link
Contributor

@mickmister mickmister added the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Mar 19, 2021
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

LGTM, just two minor comments.

};
export type InteractiveDialogConfig = {
app_id: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems unrelated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure when this field should be removed. It is an artifact of an early implementation of the apps modal

break;
case AppFieldTypes.STATIC_SELECT:
if (field.options) {
options = field.options.map((option) => ({text: option.label, value: option.value}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simplify to field.options?.map(...)?

Copy link
Contributor Author

@mickmister mickmister Mar 23, 2021

Choose a reason for hiding this comment

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

I'm getting a type error when having it like this:

Type '{ text: string; value: string; }[] | undefined' is not assignable to type 'DialogOption[]'.
  Type 'undefined' is not assignable to type 'DialogOption[]'

I'll keep it with the if statement

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Reviewed both Jira issues related and testes with test and htpp-hello app
  • Static select is resolved
  • Dynamic options in command parser is resolved
  • Confirmed there is test coverage in Zephyr for both of these issues

LGTM!

@DHaussermann DHaussermann removed the 3: QA Review Requires review by a QA tester label Mar 22, 2021
@migbot migbot added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels Mar 22, 2021
Base automatically changed from feature/cloud-apps to master March 22, 2021 22:02
@mickmister mickmister removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Mar 23, 2021
@mickmister mickmister requested a review from larkox March 23, 2021 13:27
@larkox larkox merged commit 11a9590 into master Mar 23, 2021
@larkox larkox deleted the app-modals-select-fix branch March 23, 2021 18:53
@amyblais amyblais added this to the v1.42.0 milestone Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants