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 satisfaction survey link to help menu #3676

Merged

Conversation

abbyhu2000
Copy link
Member

@abbyhu2000 abbyhu2000 commented Mar 23, 2023

Add a link to the survey under the help menu on the top navigation bar. Also change the helper menu UI according to issue #3521

The link can be configurable in config/opensearch_dashboards.yml
#opensearchDashboards.survey.url: "https://survey.opensearch.org"

  1. The default is to show the survey Give feedback in the helper menu
    Screen Shot 2023-03-30 at 4 31 10 PM

  2. If admin wishes to disable the survey, they can edit the value in opensearch_dashboards.yml and set:
    opensearchDashboards.survey.url: ""
    Screen Shot 2023-03-30 at 4 31 41 PM

  3. If admin wishes to configure the survey, they can change the link in opensearch_dashboards.yml and set:
    opensearchDashboards.survey.url: "/newSurveyLink"

Issues Resolved

resolves #3521

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

@abbyhu2000 abbyhu2000 self-assigned this Mar 23, 2023
@abbyhu2000 abbyhu2000 force-pushed the add_survey_link_dashboard branch from 0887733 to 01e1d58 Compare March 23, 2023 19:28
@abbyhu2000 abbyhu2000 marked this pull request as ready for review March 23, 2023 19:29
@abbyhu2000 abbyhu2000 requested a review from a team as a code owner March 23, 2023 19:29
ashwin-pc
ashwin-pc previously approved these changes Mar 23, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 23, 2023

Codecov Report

Merging #3676 (04cb82c) into main (4e14aae) will increase coverage by 0.00%.
The diff coverage is 66.66%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main    #3676   +/-   ##
=======================================
  Coverage   66.41%   66.41%           
=======================================
  Files        3209     3209           
  Lines       61732    61733    +1     
  Branches     9533     9534    +1     
=======================================
+ Hits        41002    41003    +1     
  Misses      18442    18442           
  Partials     2288     2288           
Flag Coverage Δ
Linux 66.36% <66.66%> (+<0.01%) ⬆️
Windows 66.37% <66.66%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
src/core/public/chrome/chrome_service.tsx 91.66% <ø> (ø)
src/core/public/chrome/ui/header/header.tsx 73.33% <ø> (ø)
...njected_metadata/injected_metadata_service.mock.ts 100.00% <ø> (ø)
...lic/injected_metadata/injected_metadata_service.ts 56.25% <0.00%> (-3.75%) ⬇️
src/core/server/opensearch_dashboards_config.ts 100.00% <ø> (ø)
src/core/server/rendering/rendering_service.tsx 66.23% <ø> (ø)
src/legacy/server/config/schema.js 100.00% <ø> (ø)
...ation/angular/doc_table/create_doc_table_react.tsx 0.00% <ø> (ø)
...public/application/doc_views/doc_views_helpers.tsx 0.00% <ø> (ø)
src/core/public/chrome/constants.ts 100.00% <100.00%> (ø)
... and 1 more

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

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

I think I would want the ability to at minimum disable this feature.

The next level would be able to configure the link utilized.

@kavilla kavilla dismissed their stale review March 23, 2023 20:54

Not really a reason to block

@abbyhu2000 abbyhu2000 force-pushed the add_survey_link_dashboard branch 2 times, most recently from cd1f61c to 065f298 Compare March 24, 2023 18:25
@abbyhu2000
Copy link
Member Author

I think I would want the ability to at minimum disable this feature.

The next level would be able to configure the link utilized.

Added a config #opensearchDashboards.isSurveyAllowed: true in the yml file to enable/disable the survey link. @kavilla

@abbyhu2000 abbyhu2000 force-pushed the add_survey_link_dashboard branch from 065f298 to ac8f8da Compare March 24, 2023 18:28
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.

Some minor changes requested

CHANGELOG.md Show resolved Hide resolved
@@ -328,7 +332,7 @@ class HeaderHelpMenuUI extends Component<Props, State> {
})}
onClick={this.onMenuButtonClick}
>
<EuiIcon type="help" size="m" />
<EuiIcon type="questionInCircle" size="l" />
Copy link
Member

Choose a reason for hiding this comment

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

Did you validate this icon size across various break points? If the UX guidance is to take this from m to l, we should also definitely update https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/core/public/chrome/ui/header/home_icon.tsx#L40-L41 and maybe check any other icons in the menu.

Copy link
Member Author

Choose a reason for hiding this comment

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

Need confirmation from the UX team, @KrooshalUX for his help menu icon, should we make it size 'm' or 'l'? Size 'm' looks kinda small to me

Copy link
Member Author

Choose a reason for hiding this comment

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

@kgcreative @joshuarrrr For reference, the first image is size 'l' and the second image is size 'm'.
Screen Shot 2023-04-05 at 4 04 00 PM
Screen Shot 2023-04-05 at 4 04 36 PM

defaultMessage="Give feedback"
/>
</EuiButtonEmpty>
{opensearchSurvey}
Copy link
Member

Choose a reason for hiding this comment

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

you could just conditionally render inline:

Suggested change
{opensearchSurvey}
{ surveyLink && (
<div>
<EuiButtonEmpty href={surveyLink} target="_blank" size="xs" flush="left">
<FormattedMessage
id="core.ui.chrome.headerGlobalNav.helpMenuSatisfactionSurveyTitle"
defaultMessage="Give feedback"
/>
</EuiButtonEmpty>
<EuiSpacer size="xs" />
</div>
)}

src/core/public/chrome/ui/header/header_help_menu.tsx Outdated Show resolved Hide resolved
@@ -75,6 +75,7 @@ function mockProps() {
mark: { defaultUrl: '/' },
applicationTitle: 'OpenSearch Dashboards',
},
survey: '/',
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what kind of unit tests we have for the header or help menu, but I'd expect our coverage to validate cases where the survey value is set and when it's undefined. Looks like we only have the first path covered.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no unit tests that are covering the helper menu itself. Added a test file header_help_menu.test.tsx and added two unit tests for displaying and hiding the survey link. @joshuarrrr

config/opensearch_dashboards.yml Outdated Show resolved Hide resolved
@seanneumann
Copy link
Contributor

Did we investigate setting the url to something like "survey.opensearch.org" so the survey endpoint can be updated via DNS?

@AMoo-Miki
Copy link
Collaborator

Did we investigate setting the url to something like "survey.opensearch.org" so the survey endpoint can be updated via DNS?

This would be exceptionally simple to do. Though, I would prefer some specificity in the URL so we can use the domain for other stuff too; so while the domain will be survey.opensearch.org, at runtime we would append osd-version=2.7.0 which will tell us that (1) it came from OSD and (2) it was OSD 2.7.0.

@seanneumann
Copy link
Contributor

I like it Miki. let's do it.

Any other params to append?

kavilla
kavilla previously approved these changes Apr 8, 2023
src/core/public/chrome/ui/header/header_help_menu.tsx Outdated Show resolved Hide resolved
@abbyhu2000 abbyhu2000 requested a review from manasvinibs as a code owner April 13, 2023 04:14
@abbyhu2000 abbyhu2000 force-pushed the add_survey_link_dashboard branch from 1dcd22a to 607844a Compare April 13, 2023 04:15
@abbyhu2000 abbyhu2000 requested a review from kavilla April 13, 2023 04:16
@abbyhu2000
Copy link
Member Author

Currently the cypress tests for multiple datasource are failing, added a line in cypress_workflow.yml to skip multiple data source cypress tests so the cypress tests action can pass. Created an issue to solve the cypress test failure.#3843
Screenshot 2023-04-13 at 1 17 41 PM

@@ -12,7 +12,7 @@ env:
START_CMD: 'node ../scripts/opensearch_dashboards --dev --no-base-path --no-watch'
OPENSEARCH_SNAPSHOT_CMD: 'node ../scripts/opensearch snapshot'
SPEC: 'cypress/integration/core-opensearch-dashboards/opensearch-dashboards/**/*.js,'
CYPRESS_ENV: 'env CYPRESS_VISBUILDER_ENABLED=true '
CYPRESS_ENV: 'env CYPRESS_VISBUILDER_ENABLED=true,DATASOURCE_MANAGEMENT_ENABLED=false'
Copy link
Member Author

Choose a reason for hiding this comment

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

This can be removed once the MD cypress tests are fixed. #3843

@abbyhu2000
Copy link
Member Author

Also Link checker fail because it references Roubenmeschian and Roubenmeschian server is down. Will delete Roubenmeschian reference in the code. (it exists in two comments in discover plugin)

Screenshot 2023-04-13 at 1 36 06 PM
Screenshot 2023-04-13 at 1 37 46 PM

@abbyhu2000 abbyhu2000 force-pushed the add_survey_link_dashboard branch 2 times, most recently from e3f0f3e to 04cb82c Compare April 14, 2023 14:53
kavilla
kavilla previously approved these changes Apr 14, 2023
Add a link to the survey under the help menu on the top navigation bar

Signed-off-by: abbyhu2000 <[email protected]>
User can disable the display of the survey in the helper menu.

Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: abbyhu2000 <[email protected]>
Add a new config in the yml file so user can enable/disable the survey link in the helper menu.

Signed-off-by: abbyhu2000 <[email protected]>
@abbyhu2000 abbyhu2000 merged commit 685c911 into opensearch-project:main Apr 14, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 14, 2023
* Add satisfaction survey link to help menu

Add a link to the survey under the help menu on the top navigation bar

Signed-off-by: abbyhu2000 <[email protected]>

* Make survey link configurable

User can disable the display of the survey in the helper menu.

Signed-off-by: abbyhu2000 <[email protected]>

* update snapshot

Signed-off-by: abbyhu2000 <[email protected]>

* modify config

Signed-off-by: abbyhu2000 <[email protected]>

* Make survey link configuration

Add a new config in the yml file so user can enable/disable the survey link in the helper menu.

Signed-off-by: abbyhu2000 <[email protected]>

* address comments and add unit test for helper menu

Signed-off-by: abbyhu2000 <[email protected]>

* change survey link to custom domain

Signed-off-by: abbyhu2000 <[email protected]>

* disable md cypress tests for now

Signed-off-by: abbyhu2000 <[email protected]>

---------

Signed-off-by: abbyhu2000 <[email protected]>
(cherry picked from commit 685c911)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
AMoo-Miki pushed a commit that referenced this pull request Apr 15, 2023
* Add satisfaction survey link to help menu (#3676)

* Add satisfaction survey link to help menu

Add a link to the survey under the help menu on the top navigation bar

Signed-off-by: abbyhu2000 <[email protected]>

* Make survey link configurable

User can disable the display of the survey in the helper menu.

Signed-off-by: abbyhu2000 <[email protected]>

* update snapshot

Signed-off-by: abbyhu2000 <[email protected]>

* modify config

Signed-off-by: abbyhu2000 <[email protected]>

* Make survey link configuration

Add a new config in the yml file so user can enable/disable the survey link in the helper menu.

Signed-off-by: abbyhu2000 <[email protected]>

* address comments and add unit test for helper menu

Signed-off-by: abbyhu2000 <[email protected]>

* change survey link to custom domain

Signed-off-by: abbyhu2000 <[email protected]>

* disable md cypress tests for now

Signed-off-by: abbyhu2000 <[email protected]>

---------

Signed-off-by: abbyhu2000 <[email protected]>
(cherry picked from commit 685c911)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

* add changelog

Signed-off-by: Josh Romero <[email protected]>

---------

Signed-off-by: Josh Romero <[email protected]>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Josh Romero <[email protected]>
sikhote pushed a commit to sikhote/OpenSearch-Dashboards that referenced this pull request Apr 24, 2023
* Add satisfaction survey link to help menu

Add a link to the survey under the help menu on the top navigation bar

Signed-off-by: abbyhu2000 <[email protected]>

* Make survey link configurable

User can disable the display of the survey in the helper menu.

Signed-off-by: abbyhu2000 <[email protected]>

* update snapshot

Signed-off-by: abbyhu2000 <[email protected]>

* modify config

Signed-off-by: abbyhu2000 <[email protected]>

* Make survey link configuration

Add a new config in the yml file so user can enable/disable the survey link in the helper menu.

Signed-off-by: abbyhu2000 <[email protected]>

* address comments and add unit test for helper menu

Signed-off-by: abbyhu2000 <[email protected]>

* change survey link to custom domain

Signed-off-by: abbyhu2000 <[email protected]>

* disable md cypress tests for now

Signed-off-by: abbyhu2000 <[email protected]>

---------

Signed-off-by: abbyhu2000 <[email protected]>
Signed-off-by: David Sinclair <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Link to UX survey into Dashboards
8 participants