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-1003] Update Readme 'What are notifications?' section screenshot #1005

Conversation

chriswachira
Copy link
Contributor

@chriswachira chriswachira commented Dec 13, 2023

Summary

This PR updates the screenshot in the 'What are notifications?' section which shows the new location of Jira actions at the Message actions button.

Ticket Link

Fixes #1003

@mattermost-build
Copy link
Contributor

Hello @chriswachira,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@chriswachira chriswachira force-pushed the update-readme-notifications-screenshot branch from cfe65b6 to 25479a3 Compare December 13, 2023 23:40
@hanzei hanzei requested a review from cwarnermm December 14, 2023 08:43
@hanzei hanzei added 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer labels Dec 14, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9950c0e) 33.32% compared to head (b88a59f) 33.32%.
Report is 7 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1005   +/-   ##
=======================================
  Coverage   33.32%   33.32%           
=======================================
  Files          53       53           
  Lines        7944     7944           
=======================================
  Hits         2647     2647           
  Misses       5070     5070           
  Partials      227      227           

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

@chriswachira
Copy link
Contributor Author

@cwarnermm Thank you for your approval. Is it possible to retry the failed job from your end?

Copy link
Contributor

@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.

Thanks for this contribution @chriswachira! I have just one suggestion to avoid putting user-identifying data in the screenshots

Copy link
Contributor

@mickmister mickmister Dec 14, 2023

Choose a reason for hiding this comment

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

Not sure if we have a direct policy on this, but ideally we don't have user-identifiable data in the screenshots. In this case, your username. Maybe we can instead have sysadmin or something like that. LMK what you think @chriswachira. Not a requirement to change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, Mick. This is understandable. Fixing in a few...

readme.md Outdated
@@ -159,7 +159,7 @@ Jira notifications are messages sent to a Mattermost channel when a particular e

Notifications and webhooks can be used together or you can opt for one of them.

![This is a channel notification of a new bug that was created in Jira](https://github.com/mattermost/mattermost-plugin-jira/assets/74422101/e7020c3e-48f6-4825-8193-6a189f6c96eb)
![This is a channel notification of a new bug that was created in Jira](https://github.com/mattermost/mattermost-plugin-jira/assets/19710961/fc19bb59-260f-4904-8887-b8de41421bf6)
Copy link
Contributor

Choose a reason for hiding this comment

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

This screenshot is actually not showing a channel notification. It's instead showing the "Attach to Jira Issue" feature. So the original screenshot was incorrect for that reason as well. I think we can use the screenshot in the repo's README for this image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickmister I've updated the PR with your suggested changes.

Copy link
Member

Choose a reason for hiding this comment

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

@chriswachira - I believe 2 screenshots are expected as part of this PR. One which you've updated and then another showing a channel notification.

Copy link
Contributor Author

@chriswachira chriswachira Dec 15, 2023

Choose a reason for hiding this comment

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

@cwarnermm I've updated the PNG file under assets showing the new location for the Jira actions and I've also updated the screenshot link in the readme section to display the ticket-created.png image under assets which shows a Jira channel notification on . Please let me know if I'm missing something.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @chriswachira! It was a misunderstanding on my part! I expected to see two image assets, but was able to verify the second asset by URL. Nicely done on this PR, and thank you so much!

@chriswachira chriswachira force-pushed the update-readme-notifications-screenshot branch 2 times, most recently from e41482b to 2f6df2c Compare December 14, 2023 20:06
@chriswachira chriswachira force-pushed the update-readme-notifications-screenshot branch from 2f6df2c to be0e53c Compare December 14, 2023 20:09
@cwarnermm cwarnermm added Awaiting Submitter Action Blocked on the author 4: Reviews Complete All reviewers have approved the pull request and removed 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer Awaiting Submitter Action Blocked on the author labels Dec 14, 2023
@cwarnermm
Copy link
Member

@mickmister - Are you open to helping address the failing checks on this PR?

@mickmister
Copy link
Contributor

@toninis Any ideas on if something may have changed about how plugin-ci sets up its linting or something similar? There are a lot of (seemingly incorrect) linting errors that have come up recently in all the plugin repos

https://github.com/mattermost/mattermost-plugin-jira/actions/runs/7214030852/job/19655859450?pr=1005

golangci-lint run ./...
Error: build/deploy/main.go:116:8: undefined: archiver (typecheck)
	err = archiver.Unarchive(bundlePath, targetPath)
	      ^
Error: server/webhook_jira.go:18:15: undefined: jira (typecheck)
	Issue        jira.Issue   `json:"issue,omitempty"`
	             ^
Error: server/webhook_jira.go:19:15: undefined: jira (typecheck)
	User         jira.User    `json:"user,omitempty"`
	             ^
Error: server/webhook_jira.go:20:15: undefined: jira (typecheck)
	Comment      jira.Comment `json:"comment,omitempty"`
	             ^
Error: server/client_cloud.go:[63](https://github.com/mattermost/mattermost-plugin-jira/actions/runs/7214030852/job/19655859450?pr=1005#step:4:65):27: connection.AccountID undefined (type *Connection has no field or method AccountID) (typecheck)
		"accountId": connection.AccountID,

@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@chriswachira
Copy link
Contributor Author

Any updates on the CI issue?

@mickmister
Copy link
Contributor

/update-branch

readme.md Outdated
@@ -159,7 +159,7 @@ Jira notifications are messages sent to a Mattermost channel when a particular e

Notifications and webhooks can be used together or you can opt for one of them.

![This is a channel notification of a new bug that was created in Jira](https://github.com/mattermost/mattermost-plugin-jira/assets/74422101/e7020c3e-48f6-4825-8193-6a189f6c96eb)
![This is a channel notification of a new bug that was created in Jira](https://github.com/mattermost/mattermost-plugin-jira/assets/74422101/e15de4fe-1cb3-47d1-9b0d-538ab82ec91d)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious what's going on with this long hash here. @chriswachira Do you know how this URL is generated?

I wonder if this would also work:

Suggested change
![This is a channel notification of a new bug that was created in Jira](https://github.com/mattermost/mattermost-plugin-jira/assets/74422101/e15de4fe-1cb3-47d1-9b0d-538ab82ec91d)
![This is a channel notification of a new bug that was created in Jira](./assets/attach-from-post.png)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello and Happy New Year!

Uploading the image as an attachment on a PR description (even without posting the PR) generates such a URL when you select "Copy link address" from the browser's menu.

Yes, your suggestion also works.

Copy link
Contributor

Choose a reason for hiding this comment

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

@chriswachira Happy New Year! Hope you're doing well

On the subject of how to reference to the image, I think we would prefer the ./assets/attach-from-post.png approach, as it's easier to trace where the image is coming from. I applied the suggestion above, but the image is not shown properly in the readme, so I've reverted that change. The current change seems to not render the image properly either, when I look at the "rich diff" in the Files Changed view. @chriswachira Do you know why this might be the case?

CleanShot 2024-01-04 at 12 37 05

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've applied the relative path suggestion and it seems to be okay

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @mickmister! Kindly let me know if I should proceed to squash the commits for a final review.

readme.md Outdated Show resolved Hide resolved
@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@chriswachira
Copy link
Contributor Author

@mickmister @cwarnermm Hello to you all!

I believe this PR is complete and can be merged. Kindly review.

@mickmister
Copy link
Contributor

Hi @chriswachira, I see this blank image in the "rich diff" view of the PR. Do you know why this may be the case? @cwarnermm Do you have any thoughts on this?
CleanShot 2024-01-22 at 11 18 41

@@ -159,7 +159,7 @@ Jira notifications are messages sent to a Mattermost channel when a particular e

Notifications and webhooks can be used together or you can opt for one of them.

![This is a channel notification of a new bug that was created in Jira](https://github.com/mattermost/mattermost-plugin-jira/assets/74422101/e7020c3e-48f6-4825-8193-6a189f6c96eb)
![This is a channel notification of a new bug that was created in Jira](./assets/ticket-created.png)
Copy link
Member

Choose a reason for hiding this comment

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

Should this path point to the attach-from-post.png image file rather than ticket-created.png?

Copy link
Contributor Author

@chriswachira chriswachira Jan 22, 2024

Choose a reason for hiding this comment

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

I believe it should be the ticket-created.png image since we want to show a sample notification triggered from Jira

Copy link
Member

Choose a reason for hiding this comment

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

The image file committed to this PR is named attach-from-post.png, but the code to display an image is a different file name. An incorrect file name would be one reason we're seeing an image error placeholder in a rich preview.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR was supposed to have two changes:

  1. Change attach-from-post.png image to show the new location of the Jira actions buttons.
  2. Update the image in the "What are notifications?" section to show a notification triggered from Jira.

Please see this conversation we had earlier: #1005 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mickmister I'm not sure why the rich diff is displaying a broken image; the image is being rendered correctly if you click on "View file"

Copy link
Contributor

@mickmister mickmister Jan 24, 2024

Choose a reason for hiding this comment

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

Thanks I think I understand now. IIUC we now have a fixed attach-from-post.png file, but it's no longer being used as it was incorrectly being used?

I went ahead and put the image in the "attach message" section, and it looks good to me. What do you think?

CleanShot 2024-01-24 at 14 36 25

@mickmister mickmister requested a review from cwarnermm January 24, 2024 19:37
@cwarnermm cwarnermm merged commit 2ae8804 into mattermost:master Jan 24, 2024
9 checks passed
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 Contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Help Wanted: Screenshots in README are out of date
6 participants