feature: new page for notebook create #1308

derek-ho · 2022-11-30T16:08:09Z

Signed-off-by: Derek Ho [email protected]

Description

Creates a new page (with unique URL) for dashboards integration.

Issues Resolved

relates to opensearch-project/dashboards-observability#110 ,
solves opensearch-project/dashboards-observability#9

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.

Signed-off-by: Derek Ho <[email protected]>

…o notebooks-integration

Signed-off-by: Derek Ho <[email protected]>

codecov-commenter · 2022-12-01T16:43:43Z

Codecov Report

Merging #1308 (be8b274) into 2.x (0fe1253) will decrease coverage by 4.37%.
The diff coverage is 64.70%.

@@             Coverage Diff              @@
##                2.x    opensearch-project/observability#1308      +/-   ##
============================================
- Coverage     41.71%   37.33%   -4.38%     
============================================
  Files           298      257      -41     
  Lines         17753    15377    -2376     
  Branches       4280     4039     -241     
============================================
- Hits           7405     5741    -1664     
+ Misses        10176     9605     -571     
+ Partials        172       31     -141

Flag	Coverage Δ
dashboards-observability	`37.33% <64.70%> (+0.07%)`	⬆️
opensearch-observability	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...lic/components/notebooks/components/note_table.tsx	`55.00% <ø> (+2.05%)`	⬆️
...ublic/components/notebooks/components/notebook.tsx	`30.14% <50.00%> (+0.66%)`	⬆️
.../public/components/notebooks/components/create.tsx	`65.62% <65.62%> (ø)`
...bility/public/components/metrics/helpers/utils.tsx	`74.28% <0.00%> (-1.43%)`	⬇️
...s/trace_analytics/components/common/search_bar.tsx	`82.35% <0.00%> (ø)`
...in/org/opensearch/observability/model/Timestamp.kt
...rvability/model/ObservabilityObjectSearchResult.kt
...ensearch/observability/model/SavedVisualization.kt
...ervability/action/GetObservabilityObjectRequest.kt
...rch/observability/model/ObservabilityObjectType.kt
... and 37 more

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

Signed-off-by: Derek Ho <[email protected]>

…lity into notebooks-integration

Signed-off-by: Derek Ho <[email protected]>

mengweieric · 2022-12-07T17:36:43Z

dashboards-observability/public/components/notebooks/components/__tests__/note_table.test.tsx

@@ -81,4 +81,39 @@ describe('<NoteTable /> spec', () => {
    utils.getByText('Actions').click();
    utils.getByText('Rename').click();
  });
-});
+
+  it('create notebook', () => {


Do we need to add or modify any cypress tests?

Good call out let me check

@mengweieric fixed in: 3dc8296. Do we want to run cypress test as a part of github action? It's a bit long but we should know about it somehow/they should be updated on any changes or flakiness or they should be removed in my opinion - why do we have tests that don't run on a regular basis?

Thanks for the changes, yes that's one task I currently have on plate, just need to resolve all the failing tests which may need some helping hands. Not sure if we should simply just add it to run on every PR or something like a nightly run.

@mengweieric I vote for nightly run 👍🏽 😄

Signed-off-by: Derek Ho <[email protected]>

dashboards-observability/.cypress/integration/2_notebooks.spec.js

ps48

I left some comments. In general, we need some UX feedback here. In my opinion, it is a bad experience to create a whole page for 1 input box.

ps48 · 2022-12-07T22:10:27Z

dashboards-observability/.cypress/integration/2_notebooks.spec.js

-      .click();
-    cy.wait(delay);
-
-    cy.get('.euiToastHeader__title').contains('Invalid notebook name').should('exist');


I think we should keep the invalid notebook toast here. Any reason we removed this?

Removed this because now the create button will not be clickable unless they have entered in some non-empty string. This is to keep the page on parity with the application analytics create page, as I was thinking to keep a consistent experience across the plugin, but I can change it back if folks think that's not appropriate. @ps48 @mengweieric

ps48 · 2022-12-07T22:20:43Z

dashboards-observability/public/components/notebooks/components/create.tsx

+
+    const onCreate = (name: string) => {
+      createNotebook(name);
+      sessionStorage.setItem('NotebooksName', '');


Is there a hard requirement to use sessionStorage here? Side Question: Why are we resetting NotebooksName in sessionStorage after creation?

No hard requirement, but again wanted to keep some parity between application analytics and notebooks. I think only reason is to not lose page content on refresh, but not sure how valuable that is. The reset is so that if they create something and then try to create another one they will not see their old notebook name when trying to create a new one.

ps48 · 2022-12-07T22:29:36Z

dashboards-observability/public/components/notebooks/components/create.tsx

+    renameNotebook: (name: string, id: string) => void
+  }
+
+  export const CreateNotebook = (props: CreateNotebookProps) => {


Let's change the name here CreateNotebook -> CreateNotebookPage Maybe? We already have createNotebook function. This would be confusing for other developers if not changed.

Sure. Changed in 1e6978c

derek-ho · 2022-12-07T22:41:29Z

I left some comments. In general, we need some UX feedback here. In my opinion, it is a bad experience to create a whole page for 1 input box.

@ps48 this PR is coming from opensearch-project/dashboards-observability#110, as part of the integration we need a new landing page when creating/editing notebooks. @pjfitzgibbons can you chime in here? I agree we may need UX on how to layout the page in a better way...

Signed-off-by: Derek Ho <[email protected]>

derek-ho added 7 commits November 30, 2022 11:07

initial commit to get new page rendered for notebook create

8e75537

Signed-off-by: Derek Ho <[email protected]>

clean up and new file for create

873efa3

Signed-off-by: Derek Ho <[email protected]>

get breadcrumb working and page styling

88d07f3

Signed-off-by: Derek Ho <[email protected]>

pushing work

e69530d

Signed-off-by: Derek Ho <[email protected]>

Merge branch '2.x' of github.com:opensearch-project/observability int…

b5a9934

…o notebooks-integration

get it mainly working

7fb33a4

Signed-off-by: Derek Ho <[email protected]>

fix existing test

14ff1c6

Signed-off-by: Derek Ho <[email protected]>

derek-ho added 4 commits December 1, 2022 12:00

clean up code and include two trivial tests

3f1e553

Signed-off-by: Derek Ho <[email protected]>

push work

dfd75bf

Signed-off-by: Derek Ho <[email protected]>

Merge branch 'notebooks-integration' of github.com:derek-ho/observabi…

68d1476

…lity into notebooks-integration

remove clone for now

550e76d

Signed-off-by: Derek Ho <[email protected]>

derek-ho marked this pull request as ready for review December 1, 2022 19:36

derek-ho requested a review from a team as a code owner December 1, 2022 19:36

fix component

d812da1

Signed-off-by: Derek Ho <[email protected]>

sejli previously approved these changes Dec 1, 2022

View reviewed changes

update a test

be8b274

Signed-off-by: Derek Ho <[email protected]>

derek-ho dismissed sejli’s stale review via be8b274 December 5, 2022 21:31

add test for create

ddfb8aa

Signed-off-by: Derek Ho <[email protected]>

mengweieric previously approved these changes Dec 7, 2022

View reviewed changes

fix cypress test

3dc8296

Signed-off-by: Derek Ho <[email protected]>

derek-ho dismissed mengweieric’s stale review via 3dc8296 December 7, 2022 18:45

mengweieric previously approved these changes Dec 7, 2022

View reviewed changes

ps48 reviewed Dec 7, 2022

View reviewed changes

dashboards-observability/.cypress/integration/2_notebooks.spec.js Outdated Show resolved Hide resolved

ps48 reviewed Dec 7, 2022

View reviewed changes

PR comment fixes

1e6978c

Signed-off-by: Derek Ho <[email protected]>

derek-ho dismissed mengweieric’s stale review via 1e6978c December 9, 2022 15:55

fix test

6868f74

Signed-off-by: Derek Ho <[email protected]>

pjfitzgibbons mentioned this pull request Dec 20, 2022

Dashboard Lists Integration opensearch-project/OpenSearch-Dashboards#3090

Merged

8 tasks

derek-ho closed this by deleting the head repository Jan 13, 2023

This was referenced Feb 1, 2023

Convert Notebooks - New into full-page form opensearch-project/dashboards-observability#9

Closed

Add new page/route for notebooks create and rename opensearch-project/dashboards-observability#152

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: new page for notebook create #1308

feature: new page for notebook create #1308

derek-ho commented Nov 30, 2022 •

edited

Loading

codecov-commenter commented Dec 1, 2022 •

edited

Loading

mengweieric Dec 7, 2022

derek-ho Dec 7, 2022

derek-ho Dec 7, 2022

mengweieric Dec 7, 2022

ps48 Dec 7, 2022

ps48 left a comment

ps48 Dec 7, 2022

derek-ho Dec 9, 2022

ps48 Dec 7, 2022

derek-ho Dec 9, 2022

ps48 Dec 7, 2022

derek-ho Dec 9, 2022

derek-ho commented Dec 7, 2022 •

edited

Loading

feature: new page for notebook create #1308

feature: new page for notebook create #1308

Conversation

derek-ho commented Nov 30, 2022 • edited Loading

Description

Issues Resolved

Check List

codecov-commenter commented Dec 1, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ps48 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derek-ho commented Dec 7, 2022 • edited Loading

derek-ho commented Nov 30, 2022 •

edited

Loading

codecov-commenter commented Dec 1, 2022 •

edited

Loading

derek-ho commented Dec 7, 2022 •

edited

Loading