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.

Sign up for GitHub

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

Jump to bottom

OWL-884 feat: add learn jit to webpanel #187

Merged
merged 6 commits into from
Apr 19, 2022
Merged

OWL-884 feat: add learn jit to webpanel #187

merged 6 commits into from
Apr 19, 2022

Conversation

denis-snyk
Copy link
Contributor

@denis-snyk denis-snyk commented Mar 25, 2022 •
edited
Loading

What is this PR

This PR adds a Snyk learn link to Snyk OSS and Snyk Code Suggestion Webviews
The logic follows:

  • If the issue is not a vulnerability (e.g. code quality issue). Don't show a link
  • If Snyk Learn has a lesson matching the issue CWE, show a link to the lesson
  • If Snyk Learn doesn't have a lesson matching the issue CWE, show a link to Snyk Learn's homepage

Considerations

  • I didn't want this code to be non-blocking to existing functionality - hence the independent postMessage events
  • We don't want the appearance of the learn link to be jankey since it renders slightly after the rest of the webview
  • I didn't want to tightly intertwine the learn link into existing functionality - we might rip it out in favour of a back-end approach if we find that it is high value

Screenshots:
Snyk OSS
image
Snyk Code
image

@denis-snyk denis-snyk self-assigned this Mar 25, 2022
@CLAassistant
Copy link

CLAassistant commented Mar 25, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@denis-snyk denis-snyk marked this pull request as ready for review March 25, 2022 16:12
@denis-snyk denis-snyk requested a review from a team as a code owner March 25, 2022 16:12
alexander-kraev-snyk
alexander-kraev-snyk reviewed Mar 28, 2022
View reviewed changes
.vscode/settings.json Outdated Show resolved Hide resolved
src/snyk/common/services/learnService.ts Outdated Show resolved Hide resolved
src/snyk/common/services/learnService.ts Outdated Show resolved Hide resolved
@denis-snyk denis-snyk force-pushed the feat/learn-jit branch 3 times, most recently from 1a786c1 to d4bdad3 Compare March 29, 2022 13:57
michelkaporin
michelkaporin requested changes Apr 1, 2022
View reviewed changes
Copy link
Contributor

@michelkaporin michelkaporin left a comment

Choose a reason for hiding this comment

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

Great work @denis-snyk! I've left some minor comments to address.

Something I am not sure about is whether Learn about this vulnerability link would always bring a user to a learn doc. I've tested goof and opened Directory Traversal issue that upon opening a link brought me to the generic Snyk Learn page. Same with "Arbitrary Code Execution" from OSS results. Is there some logic bug maybe?

Screenshot 2022-04-01 at 11 51 40

Screenshot 2022-04-01 at 11 53 25

Also couldn't find any links to test from Code Security results.

.vscode/settings.json Outdated Show resolved Hide resolved
media/views/common/suggestionHeader.scss Outdated Show resolved Hide resolved
media/views/snykCode/suggestion/suggestion.scss Outdated Show resolved Hide resolved
src/snyk/common/services/learnService.ts Outdated Show resolved Hide resolved
src/snyk/common/services/learnService.ts Show resolved Hide resolved
src/snyk/snykOss/views/suggestion/ossSuggestionWebviewProvider.ts Outdated Show resolved Hide resolved
src/snyk/snykOss/views/suggestion/ossSuggestionWebviewProvider.ts Outdated Show resolved Hide resolved
src/snyk/snykOss/views/suggestion/ossSuggestionWebviewProvider.ts Outdated Show resolved Hide resolved
src/snyk/snykOss/views/suggestion/ossSuggestionWebviewProvider.ts Outdated Show resolved Hide resolved
src/test/unit/common/services/learnService.test.ts Outdated Show resolved Hide resolved
@Geit Geit self-assigned this Apr 13, 2022
@Geit Geit requested a review from michelkaporin April 14, 2022 11:12
michelkaporin
michelkaporin reviewed Apr 14, 2022
View reviewed changes
src/snyk/common/services/learnService.ts Outdated
return cacheResult.data;
} else {
const res = await axios.get<Lesson[]>('/lessons', {
baseURL: 'https://api.snyk.io/v1/learn',
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to declare URL as const and pull the host from configuration.baseApiUrl from configuration.ts

@michelkaporin
Copy link
Contributor

@Geit @denis-snyk Also Changelog.md, Readme.md and Snyk Docs should be updated accordingly.

michelkaporin
michelkaporin reviewed Apr 19, 2022
View reviewed changes
src/snyk/common/configuration/configuration.ts Outdated
@@ -154,6 +156,10 @@ export class Configuration implements IConfiguration {
return this.defaultOssApiEndpoint;
}

get snykLearnEndpoint(): string {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to have this as a constant on LearnService instead, since it's the only place where it gets called from :)

CHANGELOG.md Show resolved Hide resolved
@michelkaporin
Copy link
Contributor

Great one, folks 👏

@michelkaporin michelkaporin merged commit 6daa113 into main Apr 19, 2022
@michelkaporin michelkaporin deleted the feat/learn-jit branch April 19, 2022 09:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-kraev-snyk alexander-kraev-snyk alexander-kraev-snyk left review comments

@KobyDamari KobyDamari KobyDamari left review comments

@michelkaporin michelkaporin michelkaporin approved these changes

Assignees

@Geit Geit

@denis-snyk denis-snyk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@denis-snyk @CLAassistant @michelkaporin @KobyDamari @alexander-kraev-snyk @Geit