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

added point in time table #3264

Closed

Conversation

ajygupta
Copy link

Signed-off-by: Ajay Gupta ajyg@amazon.com

Description

Updated the Empty state with the point in time table. Added routing, breadcrumbs and data source selection support.
There is integration with the backend APIs and saved objects to fetch PITs and populated the table which would be done in following PRs.
image

Issues Resolved

#1998

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
    • yarn test:ftr
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

Signed-off-by: Ajay Gupta <ajyg@amazon.com>
@ajygupta ajygupta requested a review from a team as a code owner January 12, 2023 13:24

// TODO: use APIs to fetch PITs and update the table and message
const [pits] = useState([]);
const [message] = useState(<EmptyState />);

Choose a reason for hiding this comment

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

Why is this a component - not sure if we should use state to save components, can we use a boolean to either show zero state or table instead ?

Copy link
Author

Choose a reason for hiding this comment

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

So we will be adding and using setMessage to update the message based on the response from the get PIT API. There can be other states like error fetching data.

Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need to bring extra state management. In our core service, we provide an embedded PersistedState class.

This class is an implementation of a persistent state management system. It manages and persists the state of an application across reloads and page transitions.

The PersistedState class has get, set, and setSilent methods. You can use the set method to update the state of the PersistedState object.

<EuiPageContentBody>
<EuiInMemoryTable
items={pits}
itemId="name"

Choose a reason for hiding this comment

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

I see itemId="id" used in other pages - can we use the same for consistency?

Copy link
Author

Choose a reason for hiding this comment

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

Will update this in following PR based on the property name we have in the PIT object.

sort: string;
}

const PITTable = ({ history }: RouteComponentProps) => {

Choose a reason for hiding this comment

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

Can we please add unit tests for this component ?

Copy link
Author

Choose a reason for hiding this comment

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

Will add tests in the following PR when we add logic to fetch the PITs

Copy link
Member

Choose a reason for hiding this comment

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

Yes, pls add more unit tests.

@joshuarrrr joshuarrrr added the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Jan 20, 2023
@ajygupta
Copy link
Author

@joshuarrrr @kavilla @ashwin-pc can you please review this change? Thanks

Copy link
Member

@ananzh ananzh left a comment

Choose a reason for hiding this comment

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

The "Point in Time Management" plugin adds a new functionality to OpenSearch Dashboards, which allows users to take a snapshot of their data at a specific point in time and then perform queries against that snapshot. The changes made in the pull request seem to be related to the plugin's UI, specifically in the src/plugins/point_in_time_management/public directory.

Have talked to @ajygupta and the project is targeting v2.8 release. Please feel free to add more pull request to my tracking issue: #3589

Thank you!


export async function mountManagementSection(
getStartServices: StartServicesAccessor<PointInTimeManagementStartDependencies>,
params: ManagementAppMountParams
) {
const [{ chrome, application }] = await getStartServices();
const [{ chrome, application, savedObjects, notifications }] = await getStartServices();
const deps: PointInTimeManagementContext = {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Instead of passing params object to multiple functions and extracting its properties, maybe consider destructuring it at the beginning of the function to make the code cleaner.

const { setBreadcrumbs, history, element } = params;


// TODO: use APIs to fetch PITs and update the table and message
const [pits] = useState([]);
const [message] = useState(<EmptyState />);
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't need to bring extra state management. In our core service, we provide an embedded PersistedState class.

This class is an implementation of a persistent state management system. It manages and persists the state of an application across reloads and page transitions.

The PersistedState class has get, set, and setSilent methods. You can use the set method to update the state of the PersistedState object.

sort: string;
}

const PITTable = ({ history }: RouteComponentProps) => {
Copy link
Member

Choose a reason for hiding this comment

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

Yes, pls add more unit tests.

@ananzh
Copy link
Member

ananzh commented Mar 22, 2023

@ajygupta could you check my comments and make changes? thanks

@joshuarrrr
Copy link
Member

@ajygupta We'll move this to draft until you're able to get back to the changes requested.

@joshuarrrr joshuarrrr marked this pull request as draft April 10, 2023 21:58
@joshuarrrr joshuarrrr removed the v2.8.0 label May 25, 2023
@dblock
Copy link
Member

dblock commented Jul 8, 2024

Looks stalled, closing. @ajygupta please feel free to finish and reopen.

[Catch All Triage, attendees 1, 2, 3, 4, 5, 6, 7]

@dblock dblock closed this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-PIT Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants