Skip to content
This repository has been archived by the owner on Sep 20, 2023. It is now read-only.

Fix #2769 #2770

Merged
merged 6 commits into from
Jun 11, 2019
Merged

Fix #2769 #2770

merged 6 commits into from
Jun 11, 2019

Conversation

LucianoPAlmeida
Copy link
Collaborator

@LucianoPAlmeida LucianoPAlmeida commented Jun 6, 2019

Fix #2769

  • Show repo when the notification is a security vulnerability type.
  • And also since it is not an issue, maybe when should not show the number

so from:
Screen Shot 2019-06-06 at 00 10 09
to:
Screen Shot 2019-06-06 at 00 12 16

@@ -81,6 +81,14 @@ final class NotificationSectionController: ListSwiftSectionController<Notificati

BadgeNotifications.clear(for: model)

if model.type == .securityVulnerability {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like a good case for a guard 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure thing :))

Copy link
Collaborator

@jdisho jdisho left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you @LucianoPAlmeida 😄

(I will leave the PR open for now in case someone else wants to have a look)

@jdisho jdisho merged commit 1d0c6c9 into GitHawkApp:master Jun 11, 2019
@LucianoPAlmeida LucianoPAlmeida deleted the fix-vulnerability-feed branch June 11, 2019 15:47
@BasThomas
Copy link
Collaborator

Awesome, thanks @LucianoPAlmeida (and for reviewing/merging, @jdisho)!

@j-f1
Copy link

j-f1 commented Jun 20, 2019

Shouldn’t this open the URL of the vulnerability in a Safari View Controller so the user can view the report itself?

@LucianoPAlmeida
Copy link
Collaborator Author

@j-f1 that's a good point 👍
What do you think @BasThomas @jdisho

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secure vulnerability notification is showing a issue detail view with failure
4 participants