Merge branch 'master' into add-ability-to-archive-a-project-via-api-14296

CHANGELOG

@@ -6,6 +6,9 @@ v 8.7.0 (unreleased)
   - Fix avatar stretching by providing a cropping feature
   - Add endpoints to archive or unarchive a project !3372
 
+v 8.6.2 (unreleased)
+  - Comments on confidential issues don't show up in activity feed to non-members
+
 v 8.6.1
   - Add option to reload the schema before restoring a database backup. !2807
   - Display navigation controls on mobile. !3214

app/assets/javascripts/gl_dropdown.js.coffee

 class GitLabDropdownFilter
   BLUR_KEYCODES = [27, 40]
+  HAS_VALUE_CLASS = "has-value"
 
-  constructor: (@dropdown, @options) ->
-    @input = @dropdown.find(".dropdown-input .dropdown-input-field")
+  constructor: (@input, @options) ->
+    $inputContainer = @input.parent()
+    $clearButton = $inputContainer.find('.js-dropdown-input-clear')
+
+    # Clear click
+    $clearButton.on 'click', (e) =>
+      e.preventDefault()
+      e.stopPropagation()
+      @input
+        .val('')
+        .trigger('keyup')
+        .focus()
 
     # Key events
     timeout = ""
     @input.on "keyup", (e) =>
-      if e.keyCode is 13 && @input.val() isnt ""
+      if @input.val() isnt "" and !$inputContainer.hasClass HAS_VALUE_CLASS
+        $inputContainer.addClass HAS_VALUE_CLASS
+      else if @input.val() is "" and $inputContainer.hasClass HAS_VALUE_CLASS
+        $inputContainer.removeClass HAS_VALUE_CLASS
+
+      if e.keyCode is 13 and @input.val() isnt ""
         if @options.enterCallback
           @options.enterCallback()
         return
@@ -95,7 +111,9 @@ class GitLabDropdown
 
     # Init filiterable
     if @options.filterable
-      @filter = new GitLabDropdownFilter @dropdown,
+      @input = @dropdown.find('.dropdown-input .dropdown-input-field')
+
+      @filter = new GitLabDropdownFilter @input,
         remote: @options.filterRemote
         query: @options.data
         keys: @options.search.fields
@@ -103,6 +121,7 @@ class GitLabDropdown
           return @fullData
         callback: (data) =>
           @parseData data
+          @highlightRow 1
         enterCallback: =>
           @selectFirstRow()
 
@@ -224,11 +243,19 @@ class GitLabDropdown
 
   noResults: ->
     html = "<li>"
-    html += "<a href='#' class='is-focused'>"
+    html += "<a href='#' class='dropdown-menu-empty-link is-focused'>"
     html += "No matching results."
     html += "</a>"
     html += "</li>"
 
+  highlightRow: (index) ->
+    if @input.val() isnt ""
+      selector = '.dropdown-content li:first-child a'
+      if @dropdown.find(".dropdown-toggle-page").length
+        selector = ".dropdown-page-one .dropdown-content li:first-child a"
+
+      $(selector).addClass 'is-focused'
+
   rowClicked: (el) ->
     fieldName = @options.fieldName
     field = @dropdown.parent().find("input[name='#{fieldName}']")
@@ -272,7 +299,7 @@ class GitLabDropdown
     if @dropdown.find(".dropdown-toggle-page").length
       selector = ".dropdown-page-one .dropdown-content li:first-child a"
 
-    # similute a click on the first link
+    # simulate a click on the first link
     $(selector).trigger "click"
 
 $.fn.glDropdown = (opts) ->

app/assets/javascripts/labels_select.js.coffee

@@ -6,7 +6,7 @@ class @LabelsSelect
       labelUrl = $dropdown.data('labels')
       selectedLabel = $dropdown.data('selected')
       if selectedLabel
-        selectedLabel = selectedLabel.split(',')
+        selectedLabel = selectedLabel.toString().split(',')
       newLabelField = $('#new_label_name')
       newColorField = $('#new_label_color')
       showNo = $dropdown.data('show-no')
@@ -14,38 +14,81 @@ class @LabelsSelect
       defaultLabel = $dropdown.data('default-label')
 
       if newLabelField.length
+        $newLabelCreateButton = $('.js-new-label-btn')
+        $colorPreview = $('.js-dropdown-label-color-preview')
         $newLabelError = $dropdown.parent().find('.js-label-error')
         $newLabelError.hide()
 
+        # Suggested colors in the dropdown to chose from pre-chosen colors
         $('.suggest-colors-dropdown a').on 'click', (e) ->
           e.preventDefault()
           e.stopPropagation()
-          newColorField.val $(this).data('color')
-          $('.js-dropdown-label-color-preview')
+          newColorField
+            .val($(this).data('color'))
+            .trigger('change')
+          $colorPreview
             .css 'background-color', $(this).data('color')
+            .parent()
             .addClass 'is-active'
 
-        $('.js-new-label-btn').on 'click', (e) ->
+        # Cancel button takes back to first page
+        resetForm = ->
+          newLabelField
+            .val ''
+            .trigger 'change'
+          newColorField
+            .val ''
+            .trigger 'change'
+          $colorPreview
+            .css 'background-color', ''
+            .parent()
+            .removeClass 'is-active'
+
+        $('.dropdown-menu-back').on 'click', ->
+          resetForm()
+
+        $('.js-cancel-label-btn').on 'click', (e) ->
           e.preventDefault()
           e.stopPropagation()
+          resetForm()
+          $('.dropdown-menu-back', $dropdown.parent()).trigger 'click'
 
+        # Listen for change and keyup events on label and color field
+        # This allows us to enable the button when ready
+        enableLabelCreateButton = ->
           if newLabelField.val() isnt '' and newColorField.val() isnt ''
-            $newLabelError.hide()
-            $('.js-new-label-btn').disable()
-
-            # Create new label with API
-            Api.newLabel projectId, {
-              name: newLabelField.val()
-              color: newColorField.val()
-            }, (label) ->
-              $('.js-new-label-btn').enable()
-
-              if label.message?
-                $newLabelError
-                  .text label.message
-                  .show()
-              else
-                $('.dropdown-menu-back', $dropdown.parent()).trigger 'click'
+            $newLabelCreateButton.enable()
+          else
+            $newLabelCreateButton.disable()
+
+        newLabelField.on 'keyup change', enableLabelCreateButton
+
+        newColorField.on 'keyup change', enableLabelCreateButton
+
+        # Send the API call to create the label
+        $newLabelCreateButton
+          .disable()
+          .on 'click', (e) ->
+            e.preventDefault()
+            e.stopPropagation()
+
+            if newLabelField.val() isnt '' and newColorField.val() isnt ''
+              $newLabelError.hide()
+              $('.js-new-label-btn').disable()
+
+              # Create new label with API
+              Api.newLabel projectId, {
+                name: newLabelField.val()
+                color: newColorField.val()
+              }, (label) ->
+                $('.js-new-label-btn').enable()
+
+                if label.message?
+                  $newLabelError
+                    .text label.message
+                    .show()
+                else
+                  $('.dropdown-menu-back', $dropdown.parent()).trigger 'click'
 
       $dropdown.glDropdown(
         data: (term, callback) ->
@@ -78,8 +121,11 @@ class @LabelsSelect
           else
             selected = if label.title is selectedLabel then 'is-active' else ''
 
+          color = if label.color? then "<span class='dropdown-label-box' style='background-color: #{label.color}'></span>" else ""
+
           "<li>
             <a href='#' class='#{selected}'>
+              #{color}
               #{label.title}
             </a>
           </li>"

app/assets/javascripts/users_select.js.coffee

@@ -30,6 +30,7 @@ class @UsersSelect
               if showNullUser
                 showDivider += 1
                 users.unshift(
+                  beforeDivider: true
                   name: 'Unassigned',
                   id: 0
                 )
@@ -39,6 +40,7 @@ class @UsersSelect
                 name = showAnyUser
                 name = 'Any User' if name == true
                 anyUser = {
+                  beforeDivider: true
                   name: name,
                   id: null
                 }
@@ -75,20 +77,27 @@ class @UsersSelect
           selected = if user.id is selectedId then "is-active" else ""
           img = ""
 
-          if avatar
-            img = "<img src='#{avatar}' class='avatar avatar-inline' width='30' />"
-
-          "<li>
-            <a href='#' class='dropdown-menu-user-link #{selected}'>
-              #{img}
-              <strong class='dropdown-menu-user-full-name'>
+          if user.beforeDivider?
+            "<li>
+              <a href='#' class='#{selected}'>
                 #{user.name}
-              </strong>
-              <span class='dropdown-menu-user-username'>
-                #{username}
-              </span>
-            </a>
-          </li>"
+              </a>
+            </li>"
+          else
+            if avatar
+              img = "<img src='#{avatar}' class='avatar avatar-inline' width='30' />"
+
+            "<li>
+              <a href='#' class='dropdown-menu-user-link #{selected}'>
+                #{img}
+                <strong class='dropdown-menu-user-full-name'>
+                  #{user.name}
+                </strong>
+                <span class='dropdown-menu-user-username'>
+                  #{username}
+                </span>
+              </a>
+            </li>"
       )
 
     $('.ajax-users-select').each (i, select) =>

app/assets/stylesheets/framework/common.scss

@@ -378,6 +378,7 @@ table {
     position: absolute;
     top: 0;
     right: 0;
+    min-width: 250px;
     visibility: hidden;
   }
 }

app/assets/stylesheets/framework/dropdowns.scss

@@ -130,6 +130,12 @@
       text-decoration: none;
       outline: 0;
     }
+
+    &.dropdown-menu-empty-link {
+      &.is-focused {
+        background-color: $dropdown-empty-row-bg;
+      }
+    }
   }
 }
 
@@ -183,7 +189,7 @@
 }
 
 .dropdown-select {
-  width: 280px;
+  width: 300px;
 }
 
 .dropdown-menu-align-right {
@@ -237,7 +243,7 @@
 
 .dropdown-title-button {
   position: absolute;
-  top: -1px;
+  top: 0;
   padding: 0;
   color: $dropdown-title-btn-color;
   font-size: 14px;
@@ -270,6 +276,22 @@
     font-size: 12px;
     pointer-events: none;
   }
+
+  .dropdown-input-clear {
+    display: none;
+    cursor: pointer;
+    pointer-events: all;
+  }
+
+  &.has-value {
+    .dropdown-input-clear {
+      display: block;
+    }
+
+    .dropdown-input-search {
+      display: none;
+    }
+  }
 }
 
 .dropdown-input-field {
@@ -286,13 +308,13 @@
     border-color: $dropdown-input-focus-border;
     box-shadow: 0 0 4px $dropdown-input-focus-shadow;
 
-    + .fa {
+    ~ .fa {
       color: $dropdown-link-color;
     }
   }
 
   &:hover {
-    + .fa {
+    ~ .fa {
       color: $dropdown-link-color;
     }
   }
@@ -338,11 +360,12 @@
   }
 }
 
-.dropdown-menu-labels {
-  .label {
-    position: relative;
-    width: 30px;
-    margin-right: 5px;
-    text-indent: -99999px;
-  }
+.dropdown-label-box {
+  position: relative;
+  top: 3px;
+  margin-right: 5px;
+  display: inline-block;
+  width: 15px;
+  height: 15px;
+  border-radius: $border-radius-base;
 }

app/assets/stylesheets/framework/variables.scss

@@ -168,13 +168,14 @@ $regular_font: 'Source Sans Pro', "Helvetica Neue", Helvetica, Arial, sans-serif
 */
 $dropdown-bg: #fff;
 $dropdown-link-color: #555;
-$dropdown-link-hover-bg: rgba(#000, .04);
+$dropdown-link-hover-bg: $row-hover;
+$dropdown-empty-row-bg: rgba(#000, .04);
 $dropdown-border-color: rgba(#000, .1);
 $dropdown-shadow-color: rgba(#000, .1);
 $dropdown-divider-color: rgba(#000, .1);
 $dropdown-header-color: #959494;
 $dropdown-title-btn-color: #bfbfbf;
-$dropdown-input-color: #c7c7c7;
+$dropdown-input-color: #555;
 $dropdown-input-focus-border: rgb(58, 171, 240);
 $dropdown-input-focus-shadow: rgba(#000, .2);
 $dropdown-loading-bg: rgba(#fff, .6);

app/assets/stylesheets/pages/labels.scss

@@ -9,28 +9,45 @@
   }
 
   &.suggest-colors-dropdown {
-    margin-bottom: 5px;
+    margin-top: 10px;
+    margin-bottom: 10px;
+    border-radius: $border-radius-base;
+    overflow: hidden;
 
     a {
       @include border-radius(0);
-      width: 36.7px;
+      width: (100% / 7);
       margin-right: 0;
       margin-bottom: -5px;
     }
   }
 }
 
-.dropdown-label-color-preview {
-  display: none;
-  margin-top: 5px;
-  width: 100%;
-  height: 25px;
+.dropdown-new-label {
+  .dropdown-content {
+    max-height: 260px;
+  }
+}
+
+.dropdown-label-color-input {
+  position: relative;
+  margin-bottom: 10px;
 
   &.is-active {
-    display: block;
+    padding-left: 32px;
   }
 }
 
+.dropdown-label-color-preview {
+  position: absolute;
+  left: 0;
+  top: 0;
+  width: 32px;
+  height: 32px;
+  border-top-left-radius: $border-radius-base;
+  border-bottom-left-radius: $border-radius-base;
+}
+
 .label-row {
   .label {
     padding: 9px;

app/assets/stylesheets/pages/profile.scss

@@ -214,7 +214,7 @@
   }
 
   .crop-controls {
-    padding: 10px 0 0 0;
+    padding: 10px 0 0;
     text-align: center;
   }
 }

app/helpers/dropdowns_helper.rb

@@ -70,7 +70,8 @@ module DropdownsHelper
   def dropdown_filter(placeholder)
     content_tag :div, class: "dropdown-input" do
       filter_output = search_field_tag nil, nil, class: "dropdown-input-field", placeholder: placeholder
-      filter_output << icon('search')
+      filter_output << icon('search', class: "dropdown-input-search")
+      filter_output << icon('times', class: "dropdown-input-clear js-dropdown-input-clear", role: "button")
 
       filter_output.html_safe
     end

app/helpers/events_helper.rb

@@ -194,7 +194,7 @@ module EventsHelper
   end
 
   def event_to_atom(xml, event)
-    if event.proper?(current_user)
+    if event.visible_to_user?(current_user)
       xml.entry do
         event_link = event_feed_url(event)
         event_title = event_feed_title(event)

app/models/event.rb

@@ -73,15 +73,15 @@ class Event < ActiveRecord::Base
     end
   end
 
-  def proper?(user = nil)
+  def visible_to_user?(user = nil)
     if push?
       true
     elsif membership_changed?
       true
     elsif created_project?
       true
-    elsif issue?
-      Ability.abilities.allowed?(user, :read_issue, issue)
+    elsif issue? || issue_note?
+      Ability.abilities.allowed?(user, :read_issue, note? ? note_target : target)
     else
       ((merge_request? || note?) && target) || milestone?
     end
@@ -298,6 +298,10 @@ class Event < ActiveRecord::Base
     target.noteable_type == "Commit"
   end
 
+  def issue_note?
+    note? && target && target.noteable_type == "Issue"
+  end
+
   def note_project_snippet?
     target.noteable_type == "Snippet"
   end

app/views/events/_event.html.haml

-- if event.proper?(current_user)
+- if event.visible_to_user?(current_user)
   .event-item{class: "#{event.body? ? "event-block" : "event-inline" }"}
     .event-item-timestamp
       #{time_ago_with_tooltip(event.created_at)}

app/views/profiles/passwords/edit.html.haml

@@ -24,12 +24,13 @@
           = f.password_field :current_password, required: true, class: 'form-control'
           %p.help-block
             You must provide your current password in order to change it.
-        .form-group
-          = f.label :password, 'New password', class: 'label-light'
-          = f.password_field :password, required: true, class: 'form-control'
-        .form-group
-          = f.label :password_confirmation, class: 'label-light'
-          = f.password_field :password_confirmation, required: true, class: 'form-control'
-        .prepend-top-default.append-bottom-default
-          = f.submit 'Save password', class: "btn btn-create append-right-10"
+      .form-group
+        = f.label :password, 'New password', class: 'label-light'
+        = f.password_field :password, required: true, class: 'form-control'
+      .form-group
+        = f.label :password_confirmation, class: 'label-light'
+        = f.password_field :password_confirmation, required: true, class: 'form-control'
+      .prepend-top-default.append-bottom-default
+        = f.submit 'Save password', class: "btn btn-create append-right-10"
+        - unless @user.password_automatically_set?
           = link_to "I forgot my password", reset_profile_password_path, method: :put, class: "account-btn-link"

app/views/projects/issues/_new_branch.html.haml

 - if current_user && can?(current_user, :push_code, @project) && @issue.can_be_worked_on?(current_user)
   .pull-right
-    = link_to namespace_project_branches_path(@project.namespace, @project, branch_name: @issue.to_branch_name, issue_iid: @issue.iid), method: :post, class: 'btn', title: @issue.to_branch_name do
+    = link_to namespace_project_branches_path(@project.namespace, @project, branch_name: @issue.to_branch_name, issue_iid: @issue.iid), method: :post, class: 'btn has-tooltip', title: @issue.to_branch_name do
       = icon('code-fork')
       New Branch

app/views/shared/issuable/_form.html.haml

@@ -127,7 +127,7 @@
       for this project.
 
   - if issuable.new_record?
-    = link_to 'Cancel', namespace_project_issues_path(@project.namespace, @project), class: 'btn btn-cancel'
+    = link_to 'Cancel', polymorphic_path([@project.namespace, @project, issuable.class]), class: 'btn btn-cancel'
   - else
     .pull-right
       - if current_user.can?(:"destroy_#{issuable.to_ability_name}", @project)
@@ -135,4 +135,4 @@
                                                                                                   method: :delete, class: 'btn btn-grouped' do
           = icon('trash-o')
           Delete
-      = link_to 'Cancel', namespace_project_issue_path(@project.namespace, @project, issuable), class: 'btn btn-grouped btn-cancel'
+      = link_to 'Cancel', polymorphic_path([@project.namespace.becomes(Namespace), @project, issuable]), class: 'btn btn-grouped btn-cancel'

app/views/shared/issuable/_label_dropdown.html.haml

@@ -24,17 +24,21 @@
                 - else
                   View labels
     - if can? current_user, :admin_label, @project and @project
-      .dropdown-page-two
+      .dropdown-page-two.dropdown-new-label
         = dropdown_title("Create new label", back: true)
         = dropdown_content do
           .dropdown-labels-error.js-label-error
-          %input#new_label_color{type: "hidden"}
           %input#new_label_name.dropdown-input-field{type: "text", placeholder: "Name new label"}
-          .dropdown-label-color-preview.js-dropdown-label-color-preview
           .suggest-colors.suggest-colors-dropdown
             - suggested_colors.each do |color|
               = link_to '#', style: "background-color: #{color}", data: { color: color } do
                 &nbsp
-          %button.btn.btn-primary.js-new-label-btn{type: "button"}
-            Create
+          .dropdown-label-color-input
+            .dropdown-label-color-preview.js-dropdown-label-color-preview
+            %input#new_label_color.dropdown-input-field{ type: "text" }
+          .clearfix
+            %button.btn.btn-primary.pull-left.js-new-label-btn{type: "button"}
+              Create
+            %button.btn.btn-default.pull-right.js-cancel-label-btn{type: "button"}
+              Cancel
     = dropdown_loading

app/views/shared/issuable/_sidebar.html.haml

@@ -77,7 +77,7 @@
             Labels
             - if can?(current_user, :"admin_#{issuable.to_ability_name}", @project)
               = link_to 'Edit', '#', class: 'edit-link pull-right'
-          .value.issuable-show-labels.hide-collapsed{class: ("has-labels" if issuable.labels.any?)}
+          .value.bold.issuable-show-labels.hide-collapsed{ class: ("has-labels" if issuable.labels.any?) }
             - if issuable.labels.any?
               - issuable.labels.each do |label|
                 = link_to_label(label, type: issuable.to_ability_name)

doc/development/scss_styleguide.md

@@ -72,9 +72,9 @@ p { margin: 0; padding: 0; }
 
 ### Colors
 
-HEX (hexadecimal) colors short-form should use shortform where possible, and 
-should use lower case letters to differenciate between letters and numbers, e.
-g. `#E3E3E3` vs. `#e3e3e3`.
+HEX (hexadecimal) colors should use shorthand where possible, and should use 
+lower case letters to differentiate between letters and numbers, e.g. `#E3E3E3`
+vs. `#e3e3e3`.
 
 ```scss
 // Bad
@@ -160,6 +160,7 @@ is slightly more performant.
 ```
 
 ### Selectors with a `js-` Prefix
+
 Do not use any selector prefixed with `js-` for styling purposes. These 
 selectors are intended for use only with JavaScript to allow for removal or 
 renaming without breaking styling.
@@ -187,8 +188,28 @@ CSSComb globally (system-wide). Run it in the GitLab directory with
 
 Note that this won't fix every problem, but it should fix a majority.
 
+### Ignoring issues
+
+If you want a line or set of lines to be ignored by the linter, you can use 
+`// scss-lint:disable RuleName` ([more info][disabling-linters]):
+
+```scss
+// This lint rule is disabled because the class name comes from a gem.
+// scss-lint:disable SelectorFormat
+.ui_charcoal {
+  background-color: #333;
+}
+// scss-lint:enable SelectorFormat
+```
+
+Make sure a comment is added on the line above the `disable` rule, otherwise the
+linter will throw a warning. `DisableLinterReason` is enabled to make sure the 
+style guide isn't being ignored, and to communicate to others why the style 
+guide is ignored in this instance.
+
 [csscomb]: https://github.com/csscomb/csscomb.js
 [node]: https://github.com/nodejs/node
 [npm]: https://www.npmjs.com/
 [scss-lint]: https://github.com/brigade/scss-lint
 [scss-lint-documentation]: https://github.com/brigade/scss-lint/blob/master/lib/scss_lint/linter/README.md
+[disabling-linters]: https://github.com/brigade/scss-lint#disabling-linters-via-source

features/steps/dashboard/issues.rb

@@ -43,10 +43,10 @@ class Spinach::Features::DashboardIssues < Spinach::FeatureSteps
 
   step 'I click "All" link' do
     find('.js-author-search').click
-    find('.dropdown-menu-user-full-name', match: :first).click
+    find('.dropdown-content a', match: :first).click
 
     find('.js-assignee-search').click
-    find('.dropdown-menu-user-full-name', match: :first).click
+    find('.dropdown-content a', match: :first).click
   end
 
   def should_see(issue)

lib/gitlab/exclusive_lease.rb

@@ -15,6 +15,25 @@ module Gitlab
   # seconds then two overlapping operations may hold a lease for the same
   # key at the same time.
   #
+  # This class has no 'cancel' method. I originally decided against adding
+  # it because it would add complexity and a false sense of security. The
+  # complexity: instead of setting '1' we would have to set a UUID, and to
+  # delete it we would have to execute Lua on the Redis server to only
+  # delete the key if the value was our own UUID. Otherwise there is a
+  # chance that when you intend to cancel your lease you actually delete
+  # someone else's. The false sense of security: you cannot design your
+  # system to rely too much on the lease being cancelled after use because
+  # the calling (Ruby) process may crash or be killed. You _cannot_ count
+  # on begin/ensure blocks to cancel a lease, because the 'ensure' does
+  # not always run. Think of 'kill -9' from the Unicorn master for
+  # instance.
+  # 
+  # If you find that leases are getting in your way, ask yourself: would
+  # it be enough to lower the lease timeout? Another thing that might be
+  # appropriate is to only use a lease for bulk/automated operations, and
+  # to ignore the lease when you get a single 'manual' user request (a
+  # button click).
+  #
   class ExclusiveLease
     def initialize(key, timeout:)
       @key, @timeout = key, timeout
@@ -27,6 +46,8 @@ module Gitlab
       !!redis.set(redis_key, '1', nx: true, ex: @timeout)
     end
 
+    # No #cancel method. See comments above!
+
     private
 
     def redis

spec/features/issues/update_issues_spec.rb

@@ -59,7 +59,7 @@ feature 'Multiple issue updating from issues#index', feature: true do
       find('#check_all_issues').click
       find('.js-update-assignee').click
 
-      find('.dropdown-menu-user-link', text: "Unassigned").click
+      click_link 'Unassigned'
       click_update_issues_button
 
       within first('.issue .controls') do

spec/models/event_spec.rb

@@ -59,44 +59,70 @@ describe Event, models: true do
     end
 
     it { expect(@event.push?).to be_truthy }
-    it { expect(@event.proper?).to be_truthy }
+    it { expect(@event.visible_to_user?).to be_truthy }
     it { expect(@event.tag?).to be_falsey }
     it { expect(@event.branch_name).to eq("master") }
     it { expect(@event.author).to eq(@user) }
   end
 
-  describe '#proper?' do
-    context 'issue event' do
-      let(:project) { create(:empty_project, :public) }
-      let(:non_member) { create(:user) }
-      let(:member)  { create(:user) }
-      let(:author) { create(:author) }
-      let(:assignee) { create(:user) }
-      let(:admin) { create(:admin) }
-      let(:event) { Event.new(project: project, action: Event::CREATED, target: issue, author_id: author.id) }
-
-      before do
-        project.team << [member, :developer]
-      end
+  describe '#visible_to_user?' do
+    let(:project) { create(:empty_project, :public) }
+    let(:non_member) { create(:user) }
+    let(:member)  { create(:user) }
+    let(:author) { create(:author) }
+    let(:assignee) { create(:user) }
+    let(:admin) { create(:admin) }
+    let(:issue) { create(:issue, project: project, author: author, assignee: assignee) }
+    let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) }
+    let(:note_on_issue) { create(:note_on_issue, noteable: issue, project: project) }
+    let(:note_on_confidential_issue) { create(:note_on_issue, noteable: confidential_issue, project: project) }
+    let(:event) { Event.new(project: project, target: target, author_id: author.id) }
 
+    before do
+      project.team << [member, :developer]
+    end
+
+    context 'issue event' do
       context 'for non confidential issues' do
-        let(:issue) { create(:issue, project: project, author: author, assignee: assignee) }
+        let(:target) { issue }
 
-        it { expect(event.proper?(non_member)).to eq true }
-        it { expect(event.proper?(author)).to eq true }
-        it { expect(event.proper?(assignee)).to eq true }
-        it { expect(event.proper?(member)).to eq true }
-        it { expect(event.proper?(admin)).to eq true }
+        it { expect(event.visible_to_user?(non_member)).to eq true }
+        it { expect(event.visible_to_user?(author)).to eq true }
+        it { expect(event.visible_to_user?(assignee)).to eq true }
+        it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(admin)).to eq true }
       end
 
       context 'for confidential issues' do
-        let(:issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) }
+        let(:target) { confidential_issue }
+
+        it { expect(event.visible_to_user?(non_member)).to eq false }
+        it { expect(event.visible_to_user?(author)).to eq true }
+        it { expect(event.visible_to_user?(assignee)).to eq true }
+        it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(admin)).to eq true }
+      end
+    end
+
+    context 'note event' do
+      context 'on non confidential issues' do
+        let(:target) { note_on_issue }
+
+        it { expect(event.visible_to_user?(non_member)).to eq true }
+        it { expect(event.visible_to_user?(author)).to eq true }
+        it { expect(event.visible_to_user?(assignee)).to eq true }
+        it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(admin)).to eq true }
+      end
+
+      context 'on confidential issues' do
+        let(:target) { note_on_confidential_issue }
 
-        it { expect(event.proper?(non_member)).to eq false }
-        it { expect(event.proper?(author)).to eq true }
-        it { expect(event.proper?(assignee)).to eq true }
-        it { expect(event.proper?(member)).to eq true }
-        it { expect(event.proper?(admin)).to eq true }
+        it { expect(event.visible_to_user?(non_member)).to eq false }
+        it { expect(event.visible_to_user?(author)).to eq true }
+        it { expect(event.visible_to_user?(assignee)).to eq true }
+        it { expect(event.visible_to_user?(member)).to eq true }
+        it { expect(event.visible_to_user?(admin)).to eq true }
       end
     end
   end