-
Notifications
You must be signed in to change notification settings - Fork 148
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-670] Fix issue: Code previews not working for branches #767
Conversation
@raghavaggarwal2308 Can you please share a example permalink that now works? |
@hanzei Here is an example: https://github.com/mattermost/mattermost/blob/master/server/public/Makefile#L7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature doesn't seem to work with branches that are not master
. e.g. https://github.com/mattermost/mattermost-plugin-github/blob/playwright-shared-repo/.gitmodules#L2 doesn't create a preview.
Can you please check what is going on?
@hanzei I checked and found about the regex we are using to compare with the link and extract data is not accepting characters other than alphabets and numbers in the branch name. |
@raghavaggarwal2308 Let's support dashes and underscores here. It's okay if we don't support slashes because of the complications. We'll want to make sure there is no UX impact when the branch has a slash though. It should be as if it's not a link to a file, rather than failing to load it |
// quick bailout if the commit hash is not proper. | ||
if _, err := hex.DecodeString(r.permalinkInfo.commit); err != nil { | ||
p.client.Log.Warn("Bad git commit hash in permalink", "error", err.Error(), "hash", r.permalinkInfo.commit) | ||
continue | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the implications of removing this? Can we instead be a bit more permissive with the restriction to allow for the new kinds of links? I'm concerned that removing this may be too permissive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this as it is commit specific and our regex will handle all the other possible cases. I don't think there will be any implications here. @mickmister
server/plugin/permalinks_test.go
Outdated
output: "start https://github.com/mattermost/mattermost-server/blob/badhash/app/authentication.go#L15-L22 lorem ipsum", | ||
name: "link with branch name", | ||
input: "start https://github.com/mattermost/mattermost-server/blob/master/app/authentication.go#L15-L22 lorem ipsum", | ||
output: "start \n[mattermost/mattermost-server/app/authentication.go](https://github.com/mattermost/mattermost-server/blob/master/app/authentication.go#L15-L22)\n```go\ntype TokenLocation int\n\nconst (\n\tTokenLocationNotFound TokenLocation = iota\n\tTokenLocationHeader\n\tTokenLocationCookie\n\tTokenLocationQueryString\n)\n```\n lorem ipsum", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test require connecting to the internet? Ideally this isn't the case, though idk if we have any sort of mocking in place for the requests to fetch code from GitHub. The other tests have a valid URL that points to this code, though this master
branch one is a broken link. How does this test end up gathering this TokenLocation
code for the test here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mickmister This test does not require internet connectivity. we have to mock in the place of request. mocking
the link is used in the mock function to return the intended value so it works on a broken link too.
@mickmister Added the support for hyphen, Hope it aligns with your suggestion. |
@hanzei @mickmister Working fine now. |
@Kshitij-Katiyar I think underscores are commonly used in branch names too. Can we support branch names with underscores? Does it support capital letters as well? Maybe a unit test that tests using them together |
…h various charecter
@mickmister the regex now in use supports |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The above PR has been tested for the following scenarios:
- Tested the code preview for permalinks with branch names
The PR was working fine for the above mentioned condition, LGTM. Approved.
Summary
What to test?
Steps to reproduce:
Expected behavior:
Ticket Link
Fixes #670