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

[GH-939]: Updated the message for webhookURL in step 2 of plugin setup #983

Merged
merged 5 commits into from
Dec 5, 2023

Conversation

Kshitij-Katiyar
Copy link
Contributor

Summary

  • Updated the message in webhookURL modal in the second step of the plugin setup

Screenshot

Old

Screenshot from 2023-10-10 13-45-05

New

Screenshot from 2023-10-10 13-43-30

Issue link

#939

What to test

  • Content of the modal in step 2 of plugin setup(webhook URL step)

How to test

  • Install the plugin
  • run jira setup
  • open the JIRA bot dm
  • Complete Step 1
  • Modal to be tested will be visible at the end of step 2
    Screenshot from 2023-10-10 13-48-04

@codecov-commenter
Copy link

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6109662) 29.65% compared to head (6820a1c) 29.65%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #983      +/-   ##
==========================================
- Coverage   29.65%   29.65%   -0.01%     
==========================================
  Files          52       52              
  Lines        7792     7794       +2     
==========================================
  Hits         2311     2311              
- Misses       5285     5287       +2     
  Partials      196      196              
Files Coverage Δ
server/setup_flow.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -400,7 +405,7 @@ func (p *Plugin) stepWebhook() flow.Step {
Color: flow.ColorPrimary,
Dialog: &model.Dialog{
Title: "Jira Webhook URL",
IntroductionText: "Please scroll to select the entire URL if necessary.\n\n```{{.WebhookURL}}```\n\nOnce you have entered all options and the webhook URL, select **Create**",
IntroductionText: fmt.Sprintf("Please copy and use the link below as webhook URL. Once you have entered all options and the webhook URL, select **Create**. %s", lineBreak) + fmt.Sprintf("%s %s", lineBreak, webhookURL),
Copy link
Member

Choose a reason for hiding this comment

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

Putting a line break into a variable seems kind of overkill to me. Is this a functional requirement or for readability? The developer has to trace positional %s's to determine where those are being used. This is a small piece of code so it doesn't matter that much but just bringing it up for discussion

@@ -400,7 +405,7 @@ func (p *Plugin) stepWebhook() flow.Step {
Color: flow.ColorPrimary,
Dialog: &model.Dialog{
Title: "Jira Webhook URL",
IntroductionText: "Please scroll to select the entire URL if necessary.\n\n```{{.WebhookURL}}```\n\nOnce you have entered all options and the webhook URL, select **Create**",
IntroductionText: fmt.Sprintf("Please copy and use the link below as webhook URL. Once you have entered all options and the webhook URL, select **Create**. %s", lineBreak) + fmt.Sprintf("%s %s", lineBreak, webhookURL),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
IntroductionText: fmt.Sprintf("Please copy and use the link below as webhook URL. Once you have entered all options and the webhook URL, select **Create**. %s", lineBreak) + fmt.Sprintf("%s %s", lineBreak, webhookURL),
IntroductionText: fmt.Sprintf("Please copy the link below as the webhook URL. Once you have entered all options and the webhook URL, select **Create**. %s", lineBreak) + fmt.Sprintf("%s %s", lineBreak, webhookURL),

Copy link
Member

Choose a reason for hiding this comment

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

@Kshitij-Katiyar - The updated text doesn't match the the popup dialog itself. The user only has 2 options: Cancel or Continue. The user isn't able to see any fields to populate.

I recognize that your PR hasn't made changes to that existing text. Are you open to updating your proposed text to provide more guidance to the user who encounters this dialog? If so, what would you recommend? What instructions on this dialog would help guide the user interested in setting up their Jira Webhook?

Copy link
Member

Choose a reason for hiding this comment

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

@cwarnermm This dialog is actually instructing the admin to interact with a form in Jira's UI, for installing the integration on that end. There is indeed no indication on the dialog itself, but I believe there is an instruction in a post that opens the dialog.

WithPretext("##### :white_check_mark: Step 3: Setup Jira Subscriptions Webhook").
WithText(`To receive Jira event notifications in Mattermost Channels, you need to set up a single global ` +
"webhook, configured for all possible event triggers that you would like to be pushed into " +
"Mattermost. The plugin processes all data from the global webhook, and then routes the events " +
"to channels and users based on your subscriptions.\n\n" +
"1. Navigate to [Jira System Settings/Webhooks]({{.ManageWebhooksURL}}) (see _screenshot_), select **Create a WebHook** in the top right corner.\n" +
"2. Give your webhook a meaningful **Name** of your choice.\n" +
"3. **Status**: Enabled.\n" +
"4. Leave **URL** blank for the moment. Once you are done configuring the webhook options, come back " +
"here and select **View Webhook URL** to see the confidential URL.\n" +
"5. **Issue related events**: we recommend leaving the query at **All Issues**. Check **Comment**, " +
"**Attachment**, and **Issue** events. We recommend checking all of these boxes. These events will be " +
"further filtered by Mattermost subscriptions. Leave **Entity property**, **Worklog**, and **Issue " +
"link** events unchecked, they are not yet supported.\n" +
"6. Leave all other checkboxes blank.\n" +
"7. Select **View Webhook URL** to see the secret **URL** to enter in Jira, and continue.\n").

The last line "Select View Webhook URL" is referring to the button below the post that opens the dialog

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @mickmister, for this context.
@Kshitij-Katiyar - I approve this PR assuming the suggested change is applied to the dialog text. Thanks!

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 thinking we should specify that the create button is in Jira though. For example:

Once you have entered all options and the webhook URL, select Create on the Jira create webhook page.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with your proposal, @mickmister! Thank you! @Kshitij-Katiyar - Please note and update the text as proposed.

Copy link
Member

Choose a reason for hiding this comment

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

FYI there's an unresolved request in this thread, to make sure we mention the Create button is in Jira's UI

Copy link
Member

Choose a reason for hiding this comment

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

@Kshitij-Katiyar - Is this update something you're open to creating a PR for?

@hanzei hanzei added the 4: Reviews Complete All reviewers have approved the pull request label Dec 5, 2023
@hanzei hanzei added this to the v4.1.0 milestone Dec 5, 2023
@hanzei hanzei merged commit e1430fa into mattermost:master Dec 5, 2023
9 checks passed
@mickmister mickmister mentioned this pull request Dec 20, 2023
@raghavaggarwal2308 raghavaggarwal2308 deleted the GH-939 branch July 25, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants