Skip to content

[Workplace Search] Fix Chrome issues with GitHub sources#105680

Merged
scottybollinger merged 4 commits intoelastic:masterfrom
scottybollinger:scottybollinger/github-chrome-fix
Jul 14, 2021
Merged

[Workplace Search] Fix Chrome issues with GitHub sources#105680
scottybollinger merged 4 commits intoelastic:masterfrom
scottybollinger:scottybollinger/github-chrome-fix

Conversation

@scottybollinger
Copy link
Copy Markdown
Contributor

closes https://github.com/elastic/workplace-search-team/issues/1882

Summary

This PR addresses an edge case bug that occurs in the connection of Github sources. There were 3 issues that were fixed in this PR.

  1. We could not actually connect Github because the route had a required param that was not present. This had already been fixed for the organization source, but because the personal dashboard came later, it was not done at the same time and was missed. (27778fb)
  2. Connecting a private Github source redirected to the org repo selector page. This was, again, an oversight that came as a result of converting the org dashboard months before the private dashboard and not testing Github thoroughly in the personal dashboard migration (this was my bad). (0ad332f)
  3. Chrome was hidden on the org Github repo selector page. The issue here is a bit complicated but will explain it again. The personal dashboard requires that we hide the chrome. This is typically done by checking the URL to see if it is a personal dashboard route. However, we use the same route for ALL content sources and pass the context in a state param from the Rails server. Because the Kibana chrome fires before the useEffect call is made, there was a flash of Kibana chrome that was initially fixed by just hiding the chrome altogether and letting the app show the chrome where needed. Unfortunately, Github requires an extra redirect and that was throwing everything off. The solution was to move the logic of parsing the state from the query params to the template and hiding the chrome only where account was set in the state. (805711d)

Checklist

This param is not always required. Was already fixed for org version but the personal dashboard came later and was not fixed.

Original fix for org:
elastic@30d8b1d#diff-07f094b2a4719e8511f003d8e278a77cd6b808d11b14d1c528705f9b259c328fR373
Previously had the org route hard-coded
Because the useEffect call comes after the initial render, the chrome flashes. We originally got around this by hiding the chrome always because in non-github scenarios, this worked fine.

However, because the oauth plugin sends the state in the quert params and uses the same URL, we need to parse that to determine whether this is an org or accoutn route. We now do that logic in the template and set the chrome before calling the useEffect.

We still need to pass both the parsed params and the original quert string because the redirect passes that string to the next view.
@scottybollinger scottybollinger added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed v7.15.0 labels Jul 14, 2021
@scottybollinger scottybollinger requested a review from a team July 14, 2021 21:03
@scottybollinger scottybollinger changed the title Scottybollinger/GitHub chrome fix [Workplace Search] Fix Chrome issues with GitHub sources Jul 14, 2021
@scottybollinger scottybollinger enabled auto-merge (squash) July 14, 2021 21:16
Copy link
Copy Markdown
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.

Great work, thanks a ton for looking into it! 🙏

@kibanamachine
Copy link
Copy Markdown
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 2.1MB 2.1MB +177.0B

History

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

@scottybollinger scottybollinger merged commit 0f3e4f4 into elastic:master Jul 14, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 14, 2021
)

* Fix route validation

This param is not always required. Was already fixed for org version but the personal dashboard came later and was not fixed.

Original fix for org:
elastic@30d8b1d#diff-07f094b2a4719e8511f003d8e278a77cd6b808d11b14d1c528705f9b259c328fR373

* Fix route to account for private github route

Previously had the org route hard-coded

* Move the logic for parsing the query params to template

Because the useEffect call comes after the initial render, the chrome flashes. We originally got around this by hiding the chrome always because in non-github scenarios, this worked fine.

However, because the oauth plugin sends the state in the quert params and uses the same URL, we need to parse that to determine whether this is an org or accoutn route. We now do that logic in the template and set the chrome before calling the useEffect.

We still need to pass both the parsed params and the original quert string because the redirect passes that string to the next view.
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 14, 2021
)

* Fix route validation

This param is not always required. Was already fixed for org version but the personal dashboard came later and was not fixed.

Original fix for org:
elastic@30d8b1d#diff-07f094b2a4719e8511f003d8e278a77cd6b808d11b14d1c528705f9b259c328fR373

* Fix route to account for private github route

Previously had the org route hard-coded

* Move the logic for parsing the query params to template

Because the useEffect call comes after the initial render, the chrome flashes. We originally got around this by hiding the chrome always because in non-github scenarios, this worked fine.

However, because the oauth plugin sends the state in the quert params and uses the same URL, we need to parse that to determine whether this is an org or accoutn route. We now do that logic in the template and set the chrome before calling the useEffect.

We still need to pass both the parsed params and the original quert string because the redirect passes that string to the next view.
@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Backport successful

Status Branch Result
7.14
7.x

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jul 15, 2021
…105698)

* Fix route validation

This param is not always required. Was already fixed for org version but the personal dashboard came later and was not fixed.

Original fix for org:
30d8b1d#diff-07f094b2a4719e8511f003d8e278a77cd6b808d11b14d1c528705f9b259c328fR373

* Fix route to account for private github route

Previously had the org route hard-coded

* Move the logic for parsing the query params to template

Because the useEffect call comes after the initial render, the chrome flashes. We originally got around this by hiding the chrome always because in non-github scenarios, this worked fine.

However, because the oauth plugin sends the state in the quert params and uses the same URL, we need to parse that to determine whether this is an org or accoutn route. We now do that logic in the template and set the chrome before calling the useEffect.

We still need to pass both the parsed params and the original quert string because the redirect passes that string to the next view.

Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co>
kibanamachine added a commit that referenced this pull request Jul 15, 2021
…105699)

* Fix route validation

This param is not always required. Was already fixed for org version but the personal dashboard came later and was not fixed.

Original fix for org:
30d8b1d#diff-07f094b2a4719e8511f003d8e278a77cd6b808d11b14d1c528705f9b259c328fR373

* Fix route to account for private github route

Previously had the org route hard-coded

* Move the logic for parsing the query params to template

Because the useEffect call comes after the initial render, the chrome flashes. We originally got around this by hiding the chrome always because in non-github scenarios, this worked fine.

However, because the oauth plugin sends the state in the quert params and uses the same URL, we need to parse that to determine whether this is an org or accoutn route. We now do that logic in the template and set the chrome before calling the useEffect.

We still need to pass both the parsed params and the original quert string because the redirect passes that string to the next view.

Co-authored-by: Scotty Bollinger <scotty.bollinger@elastic.co>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 15, 2021
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (75 commits)
  [Search Sessions] Don’t try to delete errored searches (elastic#105434)
  [Search Sessions] fix saved object can be created even if courier:batchSearches is enabled (elastic#105407)
  [Remote Clusters] Fixed remote clusters details flyout for long strings (elastic#105592)
  [ML] Functional tests - re-activate a11y tests (elastic#105198)
  [APM] Typed client-side routing (elastic#104274)
  [Canvas] Expression error (elastic#103048)
  [ML] Fixing job wizard with missing description (elastic#105574)
  [Security Solution][Alerts] - Add alerts subfeature UI (elastic#105505)
  Upgrade EUI to v35.0.0 (elastic#105127)
  [Reporting] Clean up types for internal APIs needed for UI (elastic#105508)
  skip flaky suite (elastic#105087)
  [Workplace Search] Fix Chrome issues with GitHub sources (elastic#105680)
  [Fleet] Add containerized fleet server instructions to Fleet README (elastic#105669)
  [ML] Add api integration test for analytics map endpoint  (elastic#105531)
  Fixes cypress flake across two tests (elastic#105645)
  [Logs&Metrics UI] add owner properties to plugin manifest (elastic#105580)
  chore(NA): introduce preset for jest-integration tests on @kbn/test (elastic#105144)
  [Enterprise Search] Added Thumbnails to Search UI (elastic#104199)
  Translate App Search credentials list (elastic#105619)
  [APM] APM agent config created prior to Fleet migration is not injected into integration policy (elastic#105504)
  ...

# Conflicts:
#	x-pack/plugins/reporting/common/types.ts
#	x-pack/plugins/reporting/public/management/report_listing.test.tsx
@scottybollinger scottybollinger deleted the scottybollinger/github-chrome-fix branch March 15, 2022 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v7.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants