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

Overview page add state for missing data source #2237

Merged
merged 8 commits into from
Nov 12, 2024

Conversation

TackAdam
Copy link
Collaborator

@TackAdam TackAdam commented Oct 30, 2024

Description

Add a state for workspaces when there is no data source associated. A data source needs to be present for everything to function so the user is prompted to do so if they end up in a state where a data source is not present.

Screen.Recording.2024-10-30.at.2.36.54.PM.mov

Issues Resolved

Check List

  • New functionality includes testing.
    • [] All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@TackAdam TackAdam added enhancement New feature or request backport 2.x labels Oct 30, 2024
common/constants/shared.ts Outdated Show resolved Hide resolved

// Set to null if there are no data sources associated [Handle if dashboard was set, then datasource deleted]
if (savedObjectsArray.length === 0) {
await setObservabilityDashboardsId(null);
Copy link
Member

@ps48 ps48 Nov 11, 2024

Choose a reason for hiding this comment

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

For the refresh problem, I looked into the CMS implementation. It looks like this is an issue from the CMS plugin where the registration is only happening in plugin setup:

setOverviewPage(page);
May be we can add a call-back for this function. This callback can be called to run setupOverviewPage

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Screenshot 2024-11-11 at 10 46 31 AM Wasn't able to use a callback as the registration exist. Found a work around calling

getOverviewPage().removeSection(SECTIONS.DASHBOARD);

to remove the section getting it to re-render correctly.

@TackAdam TackAdam marked this pull request as ready for review November 11, 2024 23:09
<EuiFlexItem grow={false} style={{ maxWidth: '40%' }}>
<EuiText textAlign="center">
<p style={{ margin: 0 }}>
There are no data sources associated to the workspace. Associate data sources or
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor: add i18n please (can be followups)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

          {i18n.translate('traceAnalytics.noDataSourcesMessage', {
            defaultMessage:
              'There are no data sources associated to the workspace. Associate data sources or request your administrator to associate data sources for you to get started.',
          })}

@@ -92,6 +97,7 @@ export const Home = () => {
startDate: dashboardAttributes.timeFrom,
endDate: dashboardAttributes.timeTo,
};
setIsDashboardLoaded(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should be able to set this flag in finally block

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

      .finally(() => {
        setIsDashboardLoaded(true);
      });

Thanks for feedback

const isDashboardSelected = useObservable(ObsDashboardStateManager.isDashboardSelected$);
const dashboardState = useObservable(ObsDashboardStateManager.dashboardState$);

useEffect(() => {
const checkDataSource = async () => {
Copy link
Collaborator

@mengweieric mengweieric Nov 12, 2024

Choose a reason for hiding this comment

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

We should move this function definition out of the useEffect to make it cleaner

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -61,6 +63,7 @@ export function SelectDashboardFlyout({ closeFlyout, dashboardsSavedObjects, rel
const onClickAdd = async () => {
const selectedOption = options.find((option) => option.checked === 'on');
if (selectedOption && selectedOption.key) {
getOverviewPage().createSection(DASHBOARD_SECTION);
Copy link
Member

Choose a reason for hiding this comment

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

Does replace dashboard work as expected when the section is already created?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, haven't seen any problems in all my testing. Just added as a fail safe for if a user removed the datasource to ensure when it is added back it works without needing a hard refresh.

@@ -26,7 +26,7 @@ import SampleDataLightPNG from './assets/SampleDataLight.png';

export function AddDashboardCallout() {
const showFlyout = useObservable(ObsDashboardStateManager.showFlyout$);
const isDarkMode = uiSettingsService.get('theme:darkMode');
const isDarkMode = uiSettingsService?.get('theme:darkMode') ?? false;
Copy link
Member

Choose a reason for hiding this comment

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

nice

Copy link
Collaborator

@mengweieric mengweieric left a comment

Choose a reason for hiding this comment

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

Thanks

@TackAdam TackAdam merged commit 3631968 into opensearch-project:main Nov 12, 2024
11 of 18 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 12, 2024
* added callout for no data sources pathway in workspaces

Signed-off-by: Adam Tackett <[email protected]>

* reset dashboard if datasource is deleted

Signed-off-by: Adam Tackett <[email protected]>

* handle refresh, adjust naming, unit test

Signed-off-by: Adam Tackett <[email protected]>

* add i18n, finally block, move function

Signed-off-by: Adam Tackett <[email protected]>

---------

Signed-off-by: Adam Tackett <[email protected]>
Co-authored-by: Adam Tackett <[email protected]>
(cherry picked from commit 3631968)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
mengweieric pushed a commit that referenced this pull request Nov 12, 2024
* added callout for no data sources pathway in workspaces



* reset dashboard if datasource is deleted



* handle refresh, adjust naming, unit test



* add i18n, finally block, move function



---------



(cherry picked from commit 3631968)

Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Adam Tackett <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants