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

Avoided panic on empty body of PR and Issue #351

Merged
merged 2 commits into from
Aug 18, 2020
Merged

Avoided panic on empty body of PR and Issue #351

merged 2 commits into from
Aug 18, 2020

Conversation

abdulsmapara
Copy link
Contributor

Summary

Ticket Link

Extension to the PR #326 that fixes #312

@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2020

Codecov Report

Merging #351 into master will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #351      +/-   ##
==========================================
- Coverage   21.68%   21.66%   -0.02%     
==========================================
  Files          10       10              
  Lines        2638     2640       +2     
==========================================
  Hits          572      572              
- Misses       2029     2031       +2     
  Partials       37       37              
Impacted Files Coverage Δ
server/plugin/api.go 8.73% <0.00%> (-0.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a4792f...d261f52. Read the comment docs.

Copy link
Contributor

@hanzei hanzei 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 the fix 👍

Indeed, result should never be nil if err != nil

@hanzei hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Aug 13, 2020
@hanzei hanzei added this to the v1.1.0 milestone Aug 13, 2020
@hanzei hanzei requested a review from larkox August 13, 2020 07:46
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix!

@larkox larkox removed the 2: Dev Review Requires review by a core committer label Aug 13, 2020
@hanzei
Copy link
Contributor

hanzei commented Aug 13, 2020

@DHaussermann I'm happy to review this one myself, given that I also found the bug.

@DHaussermann
Copy link

@hanzei or @larkox If you have a bit of time, could one of you peer-test this PR using the peer testing process documented here https://mattermost.atlassian.net/wiki/spaces/INT/pages/619937892/Peer+Testing+Process

Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Scope

Empty body of PRs and issues should not crash plugin

Testing notes

  • Links to PRs and issues with empty bodies don't crash the plugin any longer
  • Tooltip for these are correctly shown
  • Removing of HTML comments still work

Follow-up concerns

None

@hanzei
Copy link
Contributor

hanzei commented Aug 14, 2020

@DHaussermann ☝️

@hanzei hanzei mentioned this pull request Aug 14, 2020
17 tasks
@DHaussermann
Copy link

Thanks for testing @hanzei. Follow-up on this So I can make sure there is appropriate release testing in place. How is it possible to create a PR where Body is empty i.e. nil?
I have tested with and an empty description field but this is different?

@hanzei
Copy link
Contributor

hanzei commented Aug 15, 2020

/update-branch

@hanzei
Copy link
Contributor

hanzei commented Aug 16, 2020

@DHaussermann E.g. mattermost/mattermost-plugin-demo#112 is a PR without a description, which would cause a panic. I haven't found an issue to reproduce the issue for far.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

Thanks @hanzei for peer testing. I have added release coverage for the end to end testing.

Huge thanks @abdulsmapara for fixing this! Much appreciated.

@DHaussermann DHaussermann added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Aug 17, 2020
@hanzei hanzei merged commit 2061de8 into mattermost:master Aug 18, 2020
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.

Remove commented text in Tooltip link body
6 participants