[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.
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.
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.
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.
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.
@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 |
Summary
What to test?
Steps to reproduce:
Expected behavior:
Ticket Link
Fixes #670