-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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 Objects and assets from Source settings to Synchronization section #113982
[Workplace Search] Migrate Objects and assets from Source settings to Synchronization section #113982
Conversation
We have to set the source from the sync logic file and this naming makes more sense
Because we have a child logic file, SynchronizationLogic, we have to pass the content source into it for reading its values from SourceLogic. There are 3 ways to do this: 1. Access the source directly at SourceLogic.values.contentSource - This how we normally do it. The problem here is that SourceLogic is not mounted when the default values are set in the reducers. This caused the UI to break and I could not find a way to safely mount SourceLogic before this logic file needed it. 2. Use the connect property and connect to Sourcelogic to access contentSource - This actually worked great but our test helper does not work well with it and after an hour or so trying to make it work, I punted and decided to go with #3 below. 3. Pass the contentSource as a prop - This works great and is easy to test. The only drawback is that all other components that use the SynchronizationLogic file have to also pass in the content source. This commit does just that.
This is slightly beyond the scope of this PR but trying to make the final PR more manageable. There is an edge case where a running job lists the nextStart in the past if it is is running. After a lengthy Slack convo, it was decided to catch these in the UI and show a fallback string instead of something like “Next run 3 hours ago”
From previous PR feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just 1 minor typo:
@@ -711,3 +711,17 @@ export const SYNC_DISABLED_MESSAGE = i18n.translate( | |||
defaultMessage: 'Source synchronization disabled.', | |||
} | |||
); | |||
|
|||
export const SYNC_SETTINGS_UPDATED_MESSAGE = i18n.translate( | |||
'xpack.enterpriseSearch.workplaceSearch.sources.sync_settings_updated_message', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use camelCase here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
… Synchronization section (elastic#113982) * Rename method We have to set the source from the sync logic file and this naming makes more sense * Wire up Enable Synchronization toggle * Remove sync controls from source settings * Refactor to pass in contentSource as prop Because we have a child logic file, SynchronizationLogic, we have to pass the content source into it for reading its values from SourceLogic. There are 3 ways to do this: 1. Access the source directly at SourceLogic.values.contentSource - This how we normally do it. The problem here is that SourceLogic is not mounted when the default values are set in the reducers. This caused the UI to break and I could not find a way to safely mount SourceLogic before this logic file needed it. 2. Use the connect property and connect to Sourcelogic to access contentSource - This actually worked great but our test helper does not work well with it and after an hour or so trying to make it work, I punted and decided to go with elastic#3 below. 3. Pass the contentSource as a prop - This works great and is easy to test. The only drawback is that all other components that use the SynchronizationLogic file have to also pass in the content source. This commit does just that. * Add logic for Objects and assets view * Add content to Objects and assets view * Add fallback for `nextStart` that is in the past This is slightly beyond the scope of this PR but trying to make the final PR more manageable. There is an edge case where a running job lists the nextStart in the past if it is is running. After a lengthy Slack convo, it was decided to catch these in the UI and show a fallback string instead of something like “Next run 3 hours ago” * reduce -> map From previous PR feedback * Fix casing on i18n ID
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
… Synchronization section (#113982) (#114015) * Rename method We have to set the source from the sync logic file and this naming makes more sense * Wire up Enable Synchronization toggle * Remove sync controls from source settings * Refactor to pass in contentSource as prop Because we have a child logic file, SynchronizationLogic, we have to pass the content source into it for reading its values from SourceLogic. There are 3 ways to do this: 1. Access the source directly at SourceLogic.values.contentSource - This how we normally do it. The problem here is that SourceLogic is not mounted when the default values are set in the reducers. This caused the UI to break and I could not find a way to safely mount SourceLogic before this logic file needed it. 2. Use the connect property and connect to Sourcelogic to access contentSource - This actually worked great but our test helper does not work well with it and after an hour or so trying to make it work, I punted and decided to go with #3 below. 3. Pass the contentSource as a prop - This works great and is easy to test. The only drawback is that all other components that use the SynchronizationLogic file have to also pass in the content source. This commit does just that. * Add logic for Objects and assets view * Add content to Objects and assets view * Add fallback for `nextStart` that is in the past This is slightly beyond the scope of this PR but trying to make the final PR more manageable. There is an edge case where a running job lists the nextStart in the past if it is is running. After a lengthy Slack convo, it was decided to catch these in the UI and show a fallback string instead of something like “Next run 3 hours ago” * reduce -> map From previous PR feedback * Fix casing on i18n ID Co-authored-by: Scotty Bollinger <[email protected]>
💔 Build Failed
Failed CI Steps
Test FailuresKibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/index_lifecycle_management/home_page·ts.Index Lifecycle Management app Home page Create new policy with all PhasesStandard Out
Stack Trace
Kibana Pipeline / general / Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/index_lifecycle_management/home_page·ts.Index Lifecycle Management app Home page "after all" hook for "Create new policy with all Phases"Standard Out
Stack Trace
Kibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/management/index_lifecycle_management/policies·js.apis management index lifecycle management policies list should add the indices linked to the policiesStandard Out
Stack Trace
and 4 more failures, only showing the first 3. Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
closes https://github.com/elastic/workplace-search-team/issues/2041
closes https://github.com/elastic/workplace-search-team/issues/2059
Summary
This PR is part of adding Sync Scheduling to the Workplace Search admin dashboard. This PR:
nextStart
is in the pastNot done
2021-10-05_12-57-00.mp4
Checklist