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] Add read views for Source Sync Scheduling #113199

Merged

Conversation

scottybollinger
Copy link
Contributor

@scottybollinger scottybollinger commented Sep 27, 2021

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

Summary

This is the first PR for adding Sync Scheduling to the Workplace Search admin dashboard. This PR adds read views for the feature, to help split it into manageable chunks.

Done

  • Adds source name to the top of the sub nav
  • Adds read view for Sync, Freq, and tabs for Sync frequency & Blocked time windows
  • Adds stubbed out Objects and assets page for routing purposes only

Not done

  • Placeholder copy in place for descriptions
  • TODOs for link hrefs
  • All form interactions for views and persisting to the server including logic for input state changes
  • Move 'Objects and assets' from Settings section to new sub-nav item for Sync
  • 100% unit test coverage. Stubbed out methods will be replaced with Kea actions and tested later
  • a11y testing. Will do this once the entire feature is wired up
ss1.mp4

Also in this PR, we account for when source sync has been disabled in the config (a7a86d6):
image

Checklist

Placeholders in place whle content is being written
The weird typing around `DaysOfWeek` was taken from this SO answer to get an array from a union type:

https://stackoverflow.com/a/45486495/1949235
These components are the repeatable components in each of the frequency tabs.

- FrequencyItem
- BlockedWindowItem

Form changes methods are stubbed for now.
This is just the basics. More will be added in a future PR
This is merely a placeholder page so the routes could be built out. Section will be moved from settings in a future PR
@scottybollinger scottybollinger added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Sep 27, 2021
@scottybollinger scottybollinger requested review from a team September 27, 2021 22:45
@scottybollinger
Copy link
Contributor Author

@elasticmachine merge upstream

Comment on lines 109 to 110
itemClassName="scottyItem"
popoverClassName="scottyPopover"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in a future commit 🌮

There was a very long discussion about the edge case that is covered here.

https://github.com/elastic/ent-search/pull/4715

Basically here is what we landed on:

In most cases, the user will use the form to set the sync frequency, in which case the duration will be in the format of "PT3D" (ISO 8601). However, if an operator has set the sync frequency via the API, the duration could be a complex format, such as "P1DT2H3M4S". It was decided that in this case, we should omit seconds and go with the least common denominator from minutes.

Example: "P1DT2H3M4S" -> "1563 Minutes"
@scottybollinger scottybollinger force-pushed the scottybollinger/sync-sched-1 branch from c30cbe5 to ef95030 Compare September 28, 2021 19:58
@scottybollinger scottybollinger force-pushed the scottybollinger/sync-sched-1 branch from ef95030 to 07a68fa Compare September 28, 2021 21:41
@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.

This looks great! 🥇
I only have minor comments:

Comment on lines 551 to 573
export const SOURCE_SYNCRONIZATION_DESCRIPTION = i18n.translate(
'xpack.enterpriseSearch.workplaceSearch.sources.sourceSyncronizationDescription',
{
defaultMessage:
'Sync chupa chups dragée gummi bears jelly beans brownie. Fruitcake pie chocolate cake caramels carrot cake cotton candy dragée sweet roll soufflé.',
}
);

export const SOURCE_FREQUENCY_DESCRIPTION = i18n.translate(
'xpack.enterpriseSearch.workplaceSearch.sources.sourceFrequencyDescription',
{
defaultMessage:
'Frequency chupa chups dragée gummi bears jelly beans brownie. Fruitcake pie chocolate cake caramels carrot cake cotton candy dragée sweet roll soufflé.',
}
);

export const SOURCE_OBJECTS_AND_ASSETS_DESCRIPTION = i18n.translate(
'xpack.enterpriseSearch.workplaceSearch.sources.sourceObjectsAndAssetsDescription',
{
defaultMessage:
'Objects chupa chups dragée gummi bears jelly beans brownie. Fruitcake pie chocolate cake caramels carrot cake cotton candy dragée sweet roll soufflé.',
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid translators being confused about these messages, maybe we could add something like this at the beginning of the text?

"DO NOT TRANSLATE, temporary placeholder:"

Comment on lines +151 to +159
export const DAYS_OF_WEEK_VALUES = [
'sunday',
'monday',
'tuesday',
'wednesday',
'thursday',
'friday',
'saturday',
] as const;
Copy link
Contributor

Choose a reason for hiding this comment

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

@bhuvanaurora in the Day of week multiselect, what day should be the first one in the list? Sunday or Monday?
I'm talking about this multiselect:
image

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 is possibly going away, as the API does not support it.

Comment on lines +84 to +90
const dayPickerOptions = DAYS_OF_WEEK_VALUES.reduce((options, day) => {
options.push({
label: DAYS_OF_WEEK_LABELS[day.toUpperCase() as keyof typeof DAYS_OF_WEEK_LABELS],
value: day,
});
return options;
}, [] as Array<EuiComboBoxOptionOption<string>>);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to use map here? I find it easier to understand than reduce.
If not possible feel free to resolve this comment.

<EuiFlexItem grow={false}>
<EuiText>{BLOCK_LABEL}</EuiText>
</EuiFlexItem>
<EuiFlexItem grow={false} style={{ width: 175 }} className="blockedItemSyncSelect">
Copy link
Contributor

@yakhinvadim yakhinvadim Oct 5, 2021

Choose a reason for hiding this comment

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

My local setup is broken, so I can't check why custom styles are needed. I'm assuming to keep everything in one line? If so, maybe we could remove those? I don't think it's realistic for design to be perfect on all resolutions. Also, we recently went over the plugin codebase to remove custom styles, so bringing new ones feels like a step back.

Same question for hardcoded values in frequency_item.tsx component.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got my local setup working and now see that it's not used to keep things on one line, ignore this comment. 🙈

@yakhinvadim
Copy link
Contributor

@scottybollinger I'm going to approve this to unblock the following PR. Feel free to provide fixes separately or in this PR.

@scottybollinger scottybollinger enabled auto-merge (squash) October 5, 2021 01:41
@scottybollinger scottybollinger merged commit 8f7dd3f into elastic:master Oct 5, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Oct 5, 2021
…#113199)

* Add constants

Placeholders in place whle content is being written

* Update mock to match API and add types

The weird typing around `DaysOfWeek` was taken from this SO answer to get an array from a union type:

https://stackoverflow.com/a/45486495/1949235

* Add routes and stubbed docs urls

* Add components for list items

These components are the repeatable components in each of the frequency tabs.

- FrequencyItem
- BlockedWindowItem

Form changes methods are stubbed for now.

* Add tab components for Frequency page

* Add Frequency page component

* Add synchronization logic

This is just the basics. More will be added in a future PR

* Add Synchronization op-level page

* Add Synchronization router and subnav

* Add `Objects and assets` page stub

This is merely a placeholder page so the routes could be built out. Section will be moved from settings in a future PR

* Add name and new nav item to source sub nav

* Add SynchronizationRouter nav to Source router

* Fix a couple of typos

* Add callout and disable subnav for disabled sync

This was added to the API after the rest of the work was done, so adding it here.

elastic/workplace-search-team#2043

* Update frequency item to account for edge case

There was a very long discussion about the edge case that is covered here.

elastic/ent-search#4715

Basically here is what we landed on:

In most cases, the user will use the form to set the sync frequency, in which case the duration will be in the format of "PT3D" (ISO 8601). However, if an operator has set the sync frequency via the API, the duration could be a complex format, such as "P1DT2H3M4S". It was decided that in this case, we should omit seconds and go with the least common denominator from minutes.

Example: "P1DT2H3M4S" -> "1563 Minutes"

* Fix failing tests and add key

* Update constants with note for translators

* Fix typo

Co-authored-by: Vadim Yakhin <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Vadim Yakhin <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Oct 5, 2021
#113889)

* Add constants

Placeholders in place whle content is being written

* Update mock to match API and add types

The weird typing around `DaysOfWeek` was taken from this SO answer to get an array from a union type:

https://stackoverflow.com/a/45486495/1949235

* Add routes and stubbed docs urls

* Add components for list items

These components are the repeatable components in each of the frequency tabs.

- FrequencyItem
- BlockedWindowItem

Form changes methods are stubbed for now.

* Add tab components for Frequency page

* Add Frequency page component

* Add synchronization logic

This is just the basics. More will be added in a future PR

* Add Synchronization op-level page

* Add Synchronization router and subnav

* Add `Objects and assets` page stub

This is merely a placeholder page so the routes could be built out. Section will be moved from settings in a future PR

* Add name and new nav item to source sub nav

* Add SynchronizationRouter nav to Source router

* Fix a couple of typos

* Add callout and disable subnav for disabled sync

This was added to the API after the rest of the work was done, so adding it here.

elastic/workplace-search-team#2043

* Update frequency item to account for edge case

There was a very long discussion about the edge case that is covered here.

elastic/ent-search#4715

Basically here is what we landed on:

In most cases, the user will use the form to set the sync frequency, in which case the duration will be in the format of "PT3D" (ISO 8601). However, if an operator has set the sync frequency via the API, the duration could be a complex format, such as "P1DT2H3M4S". It was decided that in this case, we should omit seconds and go with the least common denominator from minutes.

Example: "P1DT2H3M4S" -> "1563 Minutes"

* Fix failing tests and add key

* Update constants with note for translators

* Fix typo

Co-authored-by: Vadim Yakhin <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Vadim Yakhin <[email protected]>

Co-authored-by: Scotty Bollinger <[email protected]>
Co-authored-by: Vadim Yakhin <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
enterpriseSearch 1503 1514 +11

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.3MB 1.3MB +15.5KB

History

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

@scottybollinger scottybollinger deleted the scottybollinger/sync-sched-1 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 release_note:skip Skip the PR/issue when compiling release notes v7.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants