diff options
23 files changed, 321 insertions, 116 deletions
diff --git a/CHANGELOG b/CHANGELOG index 70405957be9..f5a7cccdd5f 100644 --- a/CHANGELOG +++ b/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 diff --git a/app/assets/javascripts/gl_dropdown.js.coffee b/app/assets/javascripts/gl_dropdown.js.coffee index 960585245d7..4b78bcde774 100644 --- a/app/assets/javascripts/gl_dropdown.js.coffee +++ b/app/assets/javascripts/gl_dropdown.js.coffee @@ -1,13 +1,29 @@ 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) -> diff --git a/app/assets/javascripts/labels_select.js.coffee b/app/assets/javascripts/labels_select.js.coffee index f3cb1e3bc09..e08648d583b 100644 --- a/app/assets/javascripts/labels_select.js.coffee +++ b/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>" diff --git a/app/assets/javascripts/users_select.js.coffee b/app/assets/javascripts/users_select.js.coffee index 3d6452d2f46..84193400890 100644 --- a/app/assets/javascripts/users_select.js.coffee +++ b/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) => diff --git a/app/assets/stylesheets/framework/common.scss b/app/assets/stylesheets/framework/common.scss index 8625817fdab..9b676d759e0 100644 --- a/app/assets/stylesheets/framework/common.scss +++ b/app/assets/stylesheets/framework/common.scss @@ -378,6 +378,7 @@ table { position: absolute; top: 0; right: 0; + min-width: 250px; visibility: hidden; } } diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index d92cf6e6c44..2d616fc660c 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/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; } diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss index be626678bd7..61e0dd4d672 100644 --- a/app/assets/stylesheets/framework/variables.scss +++ b/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); diff --git a/app/assets/stylesheets/pages/labels.scss b/app/assets/stylesheets/pages/labels.scss index 3c13573c8fe..4e02ec4e891 100644 --- a/app/assets/stylesheets/pages/labels.scss +++ b/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; diff --git a/app/assets/stylesheets/pages/profile.scss b/app/assets/stylesheets/pages/profile.scss index d8c991356af..a9656e5cae7 100644 --- a/app/assets/stylesheets/pages/profile.scss +++ b/app/assets/stylesheets/pages/profile.scss @@ -214,7 +214,7 @@ } .crop-controls { - padding: 10px 0 0 0; + padding: 10px 0 0; text-align: center; } } diff --git a/app/helpers/dropdowns_helper.rb b/app/helpers/dropdowns_helper.rb index ceff1fbb161..316a10b7da3 100644 --- a/app/helpers/dropdowns_helper.rb +++ b/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 diff --git a/app/helpers/events_helper.rb b/app/helpers/events_helper.rb index a67a6b208e2..d3e5e3aa8b9 100644 --- a/app/helpers/events_helper.rb +++ b/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) diff --git a/app/models/event.rb b/app/models/event.rb index a5cfeaf388e..12183524b79 100644 --- a/app/models/event.rb +++ b/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 diff --git a/app/views/events/_event.html.haml b/app/views/events/_event.html.haml index 2d9d9dd6342..42c2764e7e2 100644 --- a/app/views/events/_event.html.haml +++ b/app/views/events/_event.html.haml @@ -1,4 +1,4 @@ -- 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)} diff --git a/app/views/profiles/passwords/edit.html.haml b/app/views/profiles/passwords/edit.html.haml index afd4f996b62..44d758dceb3 100644 --- a/app/views/profiles/passwords/edit.html.haml +++ b/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" diff --git a/app/views/projects/issues/_new_branch.html.haml b/app/views/projects/issues/_new_branch.html.haml index e66e4669d48..6da8e4f33a9 100644 --- a/app/views/projects/issues/_new_branch.html.haml +++ b/app/views/projects/issues/_new_branch.html.haml @@ -1,5 +1,5 @@ - 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 diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index b01a36265f9..6a42a6e3f58 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/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' diff --git a/app/views/shared/issuable/_label_dropdown.html.haml b/app/views/shared/issuable/_label_dropdown.html.haml index 186087e8f89..006a34a11e3 100644 --- a/app/views/shared/issuable/_label_dropdown.html.haml +++ b/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   - %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 diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index 5b2772de3f1..0e20e86356d 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/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) diff --git a/doc/development/scss_styleguide.md b/doc/development/scss_styleguide.md index 6c48c25448b..a79f4073cde 100644 --- a/doc/development/scss_styleguide.md +++ b/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 diff --git a/features/steps/dashboard/issues.rb b/features/steps/dashboard/issues.rb index f4a56865532..93aa77589be 100644 --- a/features/steps/dashboard/issues.rb +++ b/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) diff --git a/lib/gitlab/exclusive_lease.rb b/lib/gitlab/exclusive_lease.rb index 2ef50286b1d..c73eca832d7 100644 --- a/lib/gitlab/exclusive_lease.rb +++ b/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 diff --git a/spec/features/issues/update_issues_spec.rb b/spec/features/issues/update_issues_spec.rb index 121954fabca..3eb903a93fe 100644 --- a/spec/features/issues/update_issues_spec.rb +++ b/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 diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index 5fe44246738..89909c2bcd7 100644 --- a/spec/models/event_spec.rb +++ b/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 |