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

[Workplace Search] Migrate SourceLogic from ent-search #83593

Merged

Conversation

scottybollinger
Copy link
Contributor

@scottybollinger scottybollinger commented Nov 17, 2020

Summary

This PR migrates SourceLogic to Kibana. The first commit is merely a copy/paste of the existing file followed by the following:

  • Remove routes/http in favor of HttpLogic
  • Remove local flash messages in favor of global messages
  • Update paths to imports
  • Remove "I"s from interface names
  • Varions type fixes
  • Adds i18n support

Only changed lodash imports and import order for linting
Renamed IMeta -> Meta
Used object instead of IObject
- All instances of flashAPIErrors(e) are only placeholders until the later commit removing axios.

- buttonLoading was set to false when the error flash messages were set. For now I added a `setButtonNotLoading` action to do this manually in a finally block. This will be refactored once axios is removed.

- SourcesLogic is no longer needed because we set a queued flash message instead of trying to set it in SourcesLogic, which no longer has local flash messages
According to the API info getSourceReConnectData is supposed to send the source ID and not the service type. In the template, we are actually sending the ID but the logic file parameterizes it as serviceType. This is fixed here.

Usage: https://github.com/elastic/ent-search/blob/master/app/javascript/workplace_search/ContentSources/components/AddSource/ReAuthenticate.tsx#L38
Also removes using history in favor of KibanaLogic’s navigateToUrl
This selector is actually an array of strings
Previously in `ent-search`, we had a generic `IObject` interface that we could use on keyed objects. It was not migrated over since it uses `any` and Kibana has a generic `object` type we can use in most situations. However, when we are checking for keys in our code, `object` does not work. This commit is an attempt at making a generic interface we can use.
@scottybollinger scottybollinger added Feature:Plugins v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Nov 17, 2020
@scottybollinger scottybollinger requested a review from a team as a code owner November 17, 2020 22:21
...state,
...{ [option]: !state[option] },
}),
resetSourceState: () => [],
resetSourceState: () => ({}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was wrong in ent-search 🤦

Removes GenericObject from last commit and adds stricter local typing
Also added for already-merged SourcesLogic
Copy link
Contributor

@yakhinvadim yakhinvadim left a comment

Choose a reason for hiding this comment

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

Thanks for atomic commit history!
I have 2 minor comments:

const route = isOrganization
? routes.formCreateFritoPieOrganizationContentSourcesPath()
: routes.formCreateFritoPieAccountContentSourcesPath();
const route = isOrganization ? ORG_CREATE_SOURCE_ROUTE : ACCOUNT_CREATE_SOURCE_ROUTE;
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 inline these variables? They are not used anywhere else.

successCallback();
} catch (e) {
flashAPIErrors(e);
actions.setButtonNotLoading();
Copy link
Contributor

Choose a reason for hiding this comment

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

This was in finally block before. Now it's only in catch block. Should we move it below the try-catch block?

Copy link
Contributor

@yakhinvadim yakhinvadim left a comment

Choose a reason for hiding this comment

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

🎉

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
enterpriseSearch 692.0KB 692.0KB +28.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@scottybollinger scottybollinger merged commit e7ff3a6 into elastic:master Nov 18, 2020
@scottybollinger scottybollinger deleted the scottybollinger/source-logic branch November 18, 2020 19:05
phillipb added a commit to phillipb/kibana that referenced this pull request Nov 19, 2020
… into add-logs-to-node-details

* 'add-logs-to-node-details' of github.com:phillipb/kibana: (87 commits)
  [Maps] Add 'crossed' & 'exited' events to tracking alert (elastic#82463)
  Updating code-owners to use new core/app-services team names (elastic#83731)
  Add Managed label to data streams and a view switch for the table (elastic#83049)
  [Maps] Add query bar inputs to geo threshold alerts tracked points & boundaries (elastic#80871)
  fix(NA): search examples kibana version declaration (elastic#83182)
  Fixed console error, which appears when saving changes in Edit Alert flyout (elastic#83610)
  [Alerting] Add `alert.updatedAt` field to represent date of last user edit (elastic#83578)
  Not resetting server log level if level is defined (elastic#83651)
  disable incremenetal build for legacy tsconfig.json (elastic#82986)
  [Workplace Search] Migrate SourceLogic from ent-search (elastic#83593)
  [Workplace Search] Port Box changes from ent-search (elastic#83675)
  [APM] Improve router types (elastic#83620)
  Bump flat to v4.1.1 (elastic#83647)
  Bump y18n@5 to v5.0.5 (elastic#83644)
  Bump jsonpointer to v4.1.0 (elastic#83641)
  Bump is-my-json-valid to v2.20.5 (elastic#83642)
  [Telemetry] Move Monitoring collection strategy to a collector (elastic#82638)
  Update typescript eslint to v4.8 (elastic#83520)
  [ML] Persist URL state for Anomaly detection jobs using metric function (elastic#83507)
  [ML] Performance improvements to annotations editing in Single Metric Viewer & buttons placement (elastic#83216)
  ...
chrisronline pushed a commit to chrisronline/kibana that referenced this pull request Nov 19, 2020
* Initial copy/paste of source logic

Only changed lodash imports and import order for linting

* Add types and route

* Update paths and typings

Renamed IMeta -> Meta
Used object instead of IObject

* Remove internal flash messages in favor of globals

- All instances of flashAPIErrors(e) are only placeholders until the later commit removing axios.

- buttonLoading was set to false when the error flash messages were set. For now I added a `setButtonNotLoading` action to do this manually in a finally block. This will be refactored once axios is removed.

- SourcesLogic is no longer needed because we set a queued flash message instead of trying to set it in SourcesLogic, which no longer has local flash messages

* Add return types to callback definitions

* Update routes

According to the API info getSourceReConnectData is supposed to send the source ID and not the service type. In the template, we are actually sending the ID but the logic file parameterizes it as serviceType. This is fixed here.

Usage: https://github.com/elastic/ent-search/blob/master/app/javascript/workplace_search/ContentSources/components/AddSource/ReAuthenticate.tsx#L38

* Replace axios with HttpLogic

Also removes using history in favor of KibanaLogic’s navigateToUrl

* Fix incorrect type

This selector is actually an array of strings

* Create GenericObject to satisfy TypeScript

Previously in `ent-search`, we had a generic `IObject` interface that we could use on keyed objects. It was not migrated over since it uses `any` and Kibana has a generic `object` type we can use in most situations. However, when we are checking for keys in our code, `object` does not work. This commit is an attempt at making a generic interface we can use.

* More strict object typing

Removes GenericObject from last commit and adds stricter local typing

* Add i18n

Also added for already-merged SourcesLogic

* Move button loading action to finally block

* Move route strings to inline
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 20, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 83593 or prevent reminders by adding the backport:skip label.

@scottybollinger scottybollinger added the backport:skip This commit does not require backporting label Nov 20, 2020
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants