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] Fix Source Settings bug #90242

Conversation

scottybollinger
Copy link
Contributor

@scottybollinger scottybollinger commented Feb 3, 2021

Summary

Back in this PR, I made the decision to duplicate the getSourceConfigData functionality to completely separate the functionality from settings (updating) and adding a source. From that PR:

NOTE: Both AddSource and SourceSettings use a getSourceConfigData method to fetch config data. I toyed with using one of these to provide for the other and decided to just duplicate the method because of all the loading states in each tree that rely on this method.

Because of oauth issues I was not able to adequately test it and now realize it does not work because the shared SaveConfig pulls the data it needs from the logic file. I went ahead and removed the duplicate code and let the AddSourceLogic module be the single source of truth for both adding and updating a connector. In the future, we should rename AddSourceLogic to something more generic, but I wanted to try and get this in before 7.12 so we can address that later.

Also, my comment in that PR about " the loading states in each tree" is unwarranted because the only async call is for the AddSourceLogic endpoint.

Checklist

@scottybollinger scottybollinger added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Feb 3, 2021
@scottybollinger scottybollinger requested a review from a team February 3, 2021 22:11
@scottybollinger scottybollinger changed the title [Workplace Search] Fix Srouce Settings bug [Workplace Search] Fix Source Settings bug Feb 4, 2021
@scottybollinger
Copy link
Contributor Author

@elasticmachine merge upstream

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.

Sorry, I missed this PR at first! LGTM 👍

@scottybollinger
Copy link
Contributor Author

@elasticmachine merge upstream

@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 1.8MB 1.8MB -486.0B

History

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

@scottybollinger scottybollinger merged commit 284842d into elastic:master Feb 4, 2021
@scottybollinger scottybollinger deleted the scottybollinger/source-settings-bug branch February 4, 2021 19:58
scottybollinger added a commit to scottybollinger/kibana that referenced this pull request Feb 4, 2021
* Remove comment

Verified that this works as expected

* Replaces usage from SourceLogic to AddSourceLogic

* Remove unused duplicate code

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Feb 4, 2021
* master: (244 commits)
  [maps] Top hits per entity--change to title to use recent, minor edits (elastic#89254)
  [DOCS] Update installation details (elastic#90354)
  RFC for automatically generated typescript API documentation for every plugins public services, types, and functionality (elastic#86704)
  Elastic Maps Server config is `host` not `hostname` (elastic#90234)
  Use doc link services in index pattern management (elastic#89937)
  [Fleet] Managed Agent Policy (elastic#88688)
  [Workplace Search] Fix Source Settings bug  (elastic#90242)
  [Enterprise Search] Refactor MockRouter test helper to not store payload (elastic#90206)
  Use doc link service in more Stack Monitoring pages (elastic#89050)
  [App Search] Relevance Tuning logic - actions and selectors only, no listeners (elastic#89313)
  Remove UI filters from UI (elastic#89793)
  Use newfeed.service config for all newsfeeds (elastic#90252)
  skip flaky suite (elastic#85086)
  Add readme to geo containment alert covering test alert setup (elastic#89625)
  [APM] Enabling yesterday option when 24 hours is selected (elastic#90017)
  Test user for maps tests under import geoJSON tests (elastic#86015)
  [Lens] Hide column in table (elastic#88680)
  [Security Solution][Detections] Reduce detection engine reliance on _source (elastic#89371)
  [Discover] Minor cleanup (elastic#90260)
  [Search Session][Management] Rename "cancel" button and delete "Reload" button (elastic#90015)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 8, 2021
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

scottybollinger added a commit that referenced this pull request Feb 9, 2021
* Remove comment

Verified that this works as expected

* Replaces usage from SourceLogic to AddSourceLogic

* Remove unused duplicate code

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants