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

Add a new augment-vis saved object type #3109

Merged

Conversation

ohltyler
Copy link
Member

@ohltyler ohltyler commented Dec 19, 2022

Description

This PR adds a new augment-vis saved object type. Details on the design can be found in the related issue

Specifically, this PR makes the following changes:

Client-side:

  • creates & defines an augment vis saved object class
  • creates helper fns to convert from client-side definition (with no references) to server-side definition (with references) & vice-versa
  • introduce a saved object loader for the new saved object type. This loader provides a clean way to interact with all saved objects of this type, and can be consumed in dependent plugins for performing CRUD operations on saved objects of this type
  • creates and sets the new loader in the vis_augmenter plugin's start lifecycle step

Server-side:

  • creates & defines a augment vis saved object type + index mapping
  • registers the new type in the vis_augmenter plugin's setup lifecycle step

It also registers the saved object in the Saved Objects Management plugin, by:

  • adding a management section in the augment-vis saved object server-side definition,
  • registering a capabilities provider to allow some of the actions to work in the management plugin, and
  • adding the type to the plugin registry

Screenshots of an example augment-vis type in the Saved Objects Management plugin:

Fig. 1: new object is highlighted (note there is no link to the title since there is no dedicated plugin for this new saved object):
Screen Shot 2023-01-04 at 2 02 25 PM

Fig. 2: viewing the relationship of the saved object, showing the visualization saved object as the child:
Screen Shot 2023-01-04 at 2 02 52 PM

Issues Resolved

Closes #2893

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

@ohltyler ohltyler requested a review from a team as a code owner December 19, 2022 23:27
@ohltyler ohltyler marked this pull request as draft December 19, 2022 23:34
@ohltyler
Copy link
Member Author

Keeping as draft until #3108 has been reviewed & merged; I will then rebase this PR

@ohltyler ohltyler changed the base branch from main to feature/feature-anywhere December 19, 2022 23:34
@ohltyler ohltyler changed the title Add saved obj Add a new vis-integration saved object type Dec 19, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2022

Codecov Report

Merging #3109 (f1b08ad) into feature/feature-anywhere (e78ccbf) will increase coverage by 0.02%.
The diff coverage is 84.05%.

@@                     Coverage Diff                      @@
##           feature/feature-anywhere    #3109      +/-   ##
============================================================
+ Coverage                     66.67%   66.70%   +0.02%     
============================================================
  Files                          3220     3226       +6     
  Lines                         61454    61522      +68     
  Branches                       9417     9429      +12     
============================================================
+ Hits                          40977    41038      +61     
- Misses                        18232    18237       +5     
- Partials                       2245     2247       +2     
Impacted Files Coverage Δ
.../plugins/saved_objects_management/public/plugin.ts 63.15% <ø> (ø)
...ved_objects_management/public/register_services.ts 16.66% <33.33%> (-3.34%) ⬇️
...ugmenter/public/saved_augment_vis/utils/helpers.ts 33.33% <33.33%> (ø)
...ter/public/saved_augment_vis/_saved_augment_vis.ts 45.45% <45.45%> (ø)
.../saved_augment_vis/saved_augment_vis_references.ts 92.85% <92.85%> (ø)
.../saved_objects_management/public/lib/in_app_url.ts 100.00% <100.00%> (ø)
...nter/public/saved_augment_vis/saved_augment_vis.ts 100.00% <100.00%> (ø)
...ter/public/saved_augment_vis/utils/test_helpers.ts 100.00% <100.00%> (ø)
src/plugins/vis_augmenter/public/services.ts 100.00% <100.00%> (ø)
...ic/application/models/sense_editor/sense_editor.ts 64.00% <0.00%> (-1.78%) ⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ohltyler ohltyler force-pushed the add-saved-obj branch 3 times, most recently from 272d45d to 06aefaa Compare December 22, 2022 20:09
@ohltyler ohltyler changed the title Add a new vis-integration saved object type Add a new augment-vis saved object type Dec 22, 2022
@ohltyler ohltyler added the Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry label Dec 22, 2022
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

Not a comprehensive review yet, but this is looking good so far. Just a few minor comments and questions.

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
…mments

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler
Copy link
Member Author

ohltyler commented Jan 4, 2023

@ashwin-pc @joshuarrrr the latest commit registers the new type in the management plugin. I've added details and screenshots in the PR description as well. The plugin framework provides all of the functionality out-of-the-box but I've also manually confirmed the CRUD operations, inspection, viewing relationships, import/export, searching, etc. work as expected. The main changes needed were adding a management section in the server-side definition, registering a capabilities provider to allow some of the actions to work in the management plugin, and adding the type to the plugin registry.

I also added a helper formatTitle fn to clean up formatting of the toast when there is no title attribute on the saved object, since I noticed this issue when testing out my changes. Before it would show undefined

One open question: how do we want to generate names for the saved objects? Right now, I'm defaulting to Augment-<vis-name>, but I haven't given it much thought yet.

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ashwin-pc
Copy link
Member

ashwin-pc commented Jan 5, 2023

@ohltyler pretty neat implementation, can you also as a part of this PR document the process to create a new saved object type? (optional but highly appreciated!)

@abbyhu2000 Can you take a look at this PR too since you have a lot of context about saved objects?

@ohltyler
Copy link
Member Author

ohltyler commented Jan 5, 2023

@ohltyler pretty neat implementation, can you also as a part of this PR document the process to create a new saved object type? (optional but highly appreciated!)

Sure, I'll go ahead and add a section in the README 😄

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@ohltyler
Copy link
Member Author

ohltyler commented Feb 3, 2023

@ashwin-pc @kristenTian @joshuarrrr can I get another look at this PR? I'd like to get this one merged next to help unblock the plugin teams.

src/plugins/visualize/public/application/types.ts Outdated Show resolved Hide resolved
src/plugins/saved_objects/README.md Show resolved Hide resolved
*/
export const createAugmentVisSavedObject = async (AugmentVis: ISavedAugmentVis): Promise<any> => {
const loader = getSavedAugmentVisLoader();
return await loader.get((AugmentVis as any) as Record<string, unknown>);
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I like the usage of ISavedAugmentVis. Just seems like it should be simpler for the typescript compiler to understand that it's compatible with Record<string, unknown>.

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
@joshuarrrr
Copy link
Member

merging despite build and verify failure on windows. See #3400

@joshuarrrr joshuarrrr merged commit eeed599 into opensearch-project:feature/feature-anywhere Feb 8, 2023
@ohltyler ohltyler deleted the add-saved-obj branch February 9, 2023 00:01
@joshuarrrr joshuarrrr removed the v2.6.0 label Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

[Feature Anywhere] Add a augment-vis saved object type
5 participants