From fd84e9f0346121afbc07be473259ede7e8a43e68 Mon Sep 17 00:00:00 2001 From: Regis Date: Mon, 7 Aug 2017 19:38:15 -0600 Subject: use border radisu scss var --- app/assets/stylesheets/pages/issuable.scss | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index b78db402c13..7365f96d041 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -8,13 +8,13 @@ .is-confidential { color: $orange-600; background-color: $orange-50; - border-radius: 3px; + border-radius: $border-radius-default; padding: 5px; margin: 0 3px 0 -4px; } .is-not-confidential { - border-radius: 3px; + border-radius: $border-radius-default; padding: 5px; margin: 0 3px 0 -4px; } -- cgit v1.2.3 From 3d08e47239ffd00856507b73f1ce36ffa1e05256 Mon Sep 17 00:00:00 2001 From: Mike Greiling Date: Mon, 7 Aug 2017 23:03:27 -0500 Subject: fix commit metadata in project:show page --- app/assets/javascripts/dispatcher.js | 3 +++ 1 file changed, 3 insertions(+) (limited to 'app') diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 265e304b957..745c3e892f3 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -343,6 +343,9 @@ import UserFeatureHelper from './helpers/user_feature_helper'; if ($('#tree-slider').length) new TreeView(); if ($('.blob-viewer').length) new BlobViewer(); + $('#tree-slider').waitForImages(function() { + gl.utils.ajaxGet(document.querySelector('.js-tree-content').dataset.logsPath); + }); break; case 'projects:edit': setupProjectEdit(); -- cgit v1.2.3 From 643355908762c591c53a77813edb60a3e7532923 Mon Sep 17 00:00:00 2001 From: winh Date: Wed, 26 Jul 2017 22:20:37 +0200 Subject: Make Git revision dropdown style consistent --- app/assets/stylesheets/framework/dropdowns.scss | 8 ++++---- app/assets/stylesheets/framework/typography.scss | 2 ++ 2 files changed, 6 insertions(+), 4 deletions(-) (limited to 'app') diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 02e0ba74158..1bb04b59a2a 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -725,9 +725,9 @@ } // TODO: change global style and remove mixin -@mixin new-style-dropdown { - .dropdown-menu, - .dropdown-menu-nav { +@mixin new-style-dropdown($selector: '') { + #{$selector}.dropdown-menu, + #{$selector}.dropdown-menu-nav { .divider { margin: 6px 0; } @@ -773,7 +773,7 @@ } } - .dropdown-menu-align-right { + #{$selector}.dropdown-menu-align-right { margin-top: 2px; } } diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss index bf5f124d142..96409b10b99 100644 --- a/app/assets/stylesheets/framework/typography.scss +++ b/app/assets/stylesheets/framework/typography.scss @@ -339,6 +339,8 @@ a > code { @extend .ref-name; } +@include new-style-dropdown('.git-revision-dropdown'); + /** * Apply Markdown typography * -- cgit v1.2.3 From 029fb98b02f00e55243eaa781dc2849e94f16ae5 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 9 Aug 2017 17:55:53 +0800 Subject: Detect if we didn't create the ref sooner --- app/models/repository.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'app') diff --git a/app/models/repository.rb b/app/models/repository.rb index 049bebdbe42..02f7f70ecb0 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -999,7 +999,7 @@ class Repository yield(commit(branch_name_or_sha)) ensure - rugged.references.delete(tmp_ref) if tmp_ref + rugged.references.delete(tmp_ref) if tmp_ref && ref_exists?(tmp_ref) end def add_remote(name, url) @@ -1022,6 +1022,9 @@ class Repository def fetch_ref(source_path, source_ref, target_ref) args = %W(fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref}) run_git(args) + + # Make sure ref was created, and raise Rugged::ReferenceError when not + raise Rugged::ReferenceError unless ref_exists?(target_ref) end def create_ref(ref, ref_path) -- cgit v1.2.3 From 412db1874fbf2847ad9d84e9d2344d4c4d4b9fef Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Wed, 9 Aug 2017 21:41:45 +0800 Subject: Fix some tests and report the error message --- app/models/repository.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/models/repository.rb b/app/models/repository.rb index 02f7f70ecb0..c032baa5d2c 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1021,10 +1021,10 @@ class Repository def fetch_ref(source_path, source_ref, target_ref) args = %W(fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref}) - run_git(args) + message, status = run_git(args) # Make sure ref was created, and raise Rugged::ReferenceError when not - raise Rugged::ReferenceError unless ref_exists?(target_ref) + raise Rugged::ReferenceError, message unless ref_exists?(target_ref) end def create_ref(ref, ref_path) -- cgit v1.2.3 From 8858ddaf83e57adc6c003e03e72929f6068a6bfa Mon Sep 17 00:00:00 2001 From: Simon Knox Date: Tue, 18 Jul 2017 15:09:04 +1000 Subject: take edit note button out of dropdown --- app/assets/stylesheets/pages/notes.scss | 56 +++++++++++----------- app/views/projects/notes/_actions.html.haml | 38 +++++++++------ .../notes/_more_actions_dropdown.html.haml | 9 ++-- app/views/shared/icons/_ellipsis_v.svg | 1 + app/views/snippets/notes/_actions.html.haml | 17 +++++-- 5 files changed, 68 insertions(+), 53 deletions(-) create mode 100644 app/views/shared/icons/_ellipsis_v.svg (limited to 'app') diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 2bb867052f6..0a194f3707f 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -453,7 +453,10 @@ ul.notes { } .note-actions { + align-self: flex-start; flex-shrink: 0; + display: inline-flex; + align-items: center; // For PhantomJS that does not support flex float: right; margin-left: 10px; @@ -463,18 +466,12 @@ ul.notes { float: none; margin-left: 0; } - - .note-action-button { - margin-left: 8px; - } - - .more-actions-toggle { - margin-left: 2px; - } } .more-actions { - display: inline-block; + float: right; // phantomjs fallback + display: flex; + align-items: flex-end; .tooltip { white-space: nowrap; @@ -482,16 +479,10 @@ ul.notes { } .more-actions-toggle { - padding: 0; - &:hover .icon, &:focus .icon { color: $blue-600; } - - .icon { - padding: 0 6px; - } } .more-actions-dropdown { @@ -519,28 +510,42 @@ ul.notes { @include notes-media('max', $screen-md-max) { float: none; margin-left: 0; + } +} - .note-action-button { - margin-left: 0; - } +.note-actions-item { + margin-left: 15px; + display: flex; + align-items: center; + + &.more-actions { + // compensate for narrow icon + margin-left: 10px; } } .note-action-button { - display: inline; - line-height: 20px; + line-height: 1; + padding: 0; + min-width: 16px; + color: $gray-darkest; .fa { - color: $gray-darkest; position: relative; - font-size: 17px; + font-size: 16px; } + + svg { height: 16px; width: 16px; - fill: $gray-darkest; + top: 0; vertical-align: text-top; + + path { + fill: currentColor; + } } .award-control-icon-positive, @@ -613,10 +618,7 @@ ul.notes { .note-role { position: relative; - top: -2px; - display: inline-block; - padding-left: 7px; - padding-right: 7px; + padding: 0 7px; color: $notes-role-color; font-size: 12px; line-height: 20px; diff --git a/app/views/projects/notes/_actions.html.haml b/app/views/projects/notes/_actions.html.haml index 9c42be4e0ff..cb737d129f0 100644 --- a/app/views/projects/notes/_actions.html.haml +++ b/app/views/projects/notes/_actions.html.haml @@ -17,24 +17,32 @@ "inline-template" => true, "ref" => "note_#{note.id}" } - %button.note-action-button.line-resolve-btn{ type: "button", - class: ("is-disabled" unless can_resolve), - ":class" => "{ 'is-active': isResolved }", - ":aria-label" => "buttonText", - "@click" => "resolve", - ":title" => "buttonText", - ":ref" => "'button'" } + .note-actions-item + %button.note-action-button.line-resolve-btn{ type: "button", + class: ("is-disabled" unless can_resolve), + ":class" => "{ 'is-active': isResolved }", + ":aria-label" => "buttonText", + "@click" => "resolve", + ":title" => "buttonText", + ":ref" => "'button'" } - = icon('spin spinner', 'v-show' => 'loading', class: 'loading', 'aria-hidden' => 'true', 'aria-label' => 'Loading') - %div{ 'v-show' => '!loading' }= render 'shared/icons/icon_status_success.svg' + = icon('spin spinner', 'v-show' => 'loading', class: 'loading', 'aria-hidden' => 'true', 'aria-label' => 'Loading') + %div{ 'v-show' => '!loading' }= render 'shared/icons/icon_status_success.svg' - if current_user - if note.emoji_awardable? - user_authored = note.user_authored?(current_user) - = link_to '#', title: 'Add reaction', class: "note-action-button note-emoji-button js-add-award js-note-emoji #{'js-user-authored' if user_authored} has-tooltip", data: { position: 'right' } do - = icon('spinner spin') - %span{ class: 'link-highlight award-control-icon-neutral' }= custom_icon('emoji_slightly_smiling_face') - %span{ class: 'link-highlight award-control-icon-positive' }= custom_icon('emoji_smiley') - %span{ class: 'link-highlight award-control-icon-super-positive' }= custom_icon('emoji_smile') + .note-actions-item + = button_tag title: 'Add reaction', class: "note-action-button note-emoji-button js-add-award js-note-emoji #{'js-user-authored' if user_authored} has-tooltip btn btn-transparent", data: { position: 'right', container: 'body' } do + = icon('spinner spin') + %span{ class: 'link-highlight award-control-icon-neutral' }= custom_icon('emoji_slightly_smiling_face') + %span{ class: 'link-highlight award-control-icon-positive' }= custom_icon('emoji_smiley') + %span{ class: 'link-highlight award-control-icon-super-positive' }= custom_icon('emoji_smile') - = render 'projects/notes/more_actions_dropdown', note: note, note_editable: note_editable + - if note_editable + .note-actions-item + = button_tag title: 'Edit comment', class: 'note-action-button js-note-edit has-tooltip btn btn-transparent', data: { container: 'body' } do + %span.link-highlight + = custom_icon('icon_pencil') + + = render 'projects/notes/more_actions_dropdown', note: note, note_editable: note_editable diff --git a/app/views/projects/notes/_more_actions_dropdown.html.haml b/app/views/projects/notes/_more_actions_dropdown.html.haml index 75a4687e1e3..5930209a682 100644 --- a/app/views/projects/notes/_more_actions_dropdown.html.haml +++ b/app/views/projects/notes/_more_actions_dropdown.html.haml @@ -1,14 +1,11 @@ - is_current_user = current_user == note.author - if note_editable || !is_current_user - .dropdown.more-actions + .dropdown.more-actions.note-actions-item = button_tag title: 'More actions', class: 'note-action-button more-actions-toggle has-tooltip btn btn-transparent', data: { toggle: 'dropdown', container: 'body' } do - = icon('ellipsis-v', class: 'icon') + %span.icon + = custom_icon('ellipsis_v') %ul.dropdown-menu.more-actions-dropdown.dropdown-open-left - - if note_editable - %li - = button_tag 'Edit comment', class: 'js-note-edit btn btn-transparent' - %li.divider - unless is_current_user %li = link_to new_abuse_report_path(user_id: note.author.id, ref_url: noteable_note_url(note)) do diff --git a/app/views/shared/icons/_ellipsis_v.svg b/app/views/shared/icons/_ellipsis_v.svg new file mode 100644 index 00000000000..9117a9bb9ec --- /dev/null +++ b/app/views/shared/icons/_ellipsis_v.svg @@ -0,0 +1 @@ + diff --git a/app/views/snippets/notes/_actions.html.haml b/app/views/snippets/notes/_actions.html.haml index 098a88c48c5..3a50324770d 100644 --- a/app/views/snippets/notes/_actions.html.haml +++ b/app/views/snippets/notes/_actions.html.haml @@ -1,10 +1,17 @@ - if current_user - if note.emoji_awardable? - user_authored = note.user_authored?(current_user) - = link_to '#', title: 'Add reaction', class: "note-action-button note-emoji-button js-add-award js-note-emoji #{'js-user-authored' if user_authored} has-tooltip", data: { position: 'right' } do - = icon('spinner spin') - %span{ class: 'link-highlight award-control-icon-neutral' }= custom_icon('emoji_slightly_smiling_face') - %span{ class: 'link-highlight award-control-icon-positive' }= custom_icon('emoji_smiley') - %span{ class: 'link-highlight award-control-icon-super-positive' }= custom_icon('emoji_smile') + .note-actions-item + = link_to '#', title: 'Add reaction', class: "note-action-button note-emoji-button js-add-award js-note-emoji #{'js-user-authored' if user_authored} has-tooltip", data: { position: 'right' } do + = icon('spinner spin') + %span{ class: 'link-highlight award-control-icon-neutral' }= custom_icon('emoji_slightly_smiling_face') + %span{ class: 'link-highlight award-control-icon-positive' }= custom_icon('emoji_smiley') + %span{ class: 'link-highlight award-control-icon-super-positive' }= custom_icon('emoji_smile') + + - if note_editable + .note-actions-item + = button_tag title: 'Edit comment', class: 'note-action-button js-note-edit has-tooltip btn btn-transparent', data: { container: 'body' } do + %span.link-highlight + = custom_icon('icon_pencil') = render 'projects/notes/more_actions_dropdown', note: note, note_editable: note_editable -- cgit v1.2.3 From 023d6749c24741dd1ac065b7c9d4413acb9aa320 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 10 Aug 2017 18:08:58 +0800 Subject: Just detect exit status rather than check ref Feedback: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/13416#note_37193731 So we just try a cheaper way to detect it if it works or not --- app/models/repository.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app') diff --git a/app/models/repository.rb b/app/models/repository.rb index c032baa5d2c..f8139ff595e 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -1024,7 +1024,7 @@ class Repository message, status = run_git(args) # Make sure ref was created, and raise Rugged::ReferenceError when not - raise Rugged::ReferenceError, message unless ref_exists?(target_ref) + raise Rugged::ReferenceError, message if status != 0 end def create_ref(ref, ref_path) -- cgit v1.2.3 From 26bb50412ce44be434d7bb86a952397b7983deb5 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Wed, 9 Aug 2017 16:41:51 +0200 Subject: Cache Appearance instances in Redis This caches the result of Appearance.first in a similar fashion to how ApplicationSetting instances are cached. We also add some NOT NULL constraints to the table and correct the timestamp types. Fixes gitlab-org/gitlab-ce#36066, fixes gitlab-org/gitlab-ce#31698 --- app/controllers/admin/appearances_controller.rb | 2 +- app/helpers/appearances_helper.rb | 2 +- app/models/appearance.rb | 20 ++++++++++++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/controllers/admin/appearances_controller.rb b/app/controllers/admin/appearances_controller.rb index 4b0ec54b3f4..92df1c8dff0 100644 --- a/app/controllers/admin/appearances_controller.rb +++ b/app/controllers/admin/appearances_controller.rb @@ -45,7 +45,7 @@ class Admin::AppearancesController < Admin::ApplicationController # Use callbacks to share common setup or constraints between actions. def set_appearance - @appearance = Appearance.last || Appearance.new + @appearance = Appearance.current || Appearance.new end # Only allow a trusted parameter "white list" through. diff --git a/app/helpers/appearances_helper.rb b/app/helpers/appearances_helper.rb index 16136d02530..cdf5fa5d4b7 100644 --- a/app/helpers/appearances_helper.rb +++ b/app/helpers/appearances_helper.rb @@ -20,7 +20,7 @@ module AppearancesHelper end def brand_item - @appearance ||= Appearance.first + @appearance ||= Appearance.current end def brand_header_logo diff --git a/app/models/appearance.rb b/app/models/appearance.rb index f9c48482be7..ff15689ecac 100644 --- a/app/models/appearance.rb +++ b/app/models/appearance.rb @@ -8,7 +8,27 @@ class Appearance < ActiveRecord::Base validates :logo, file_size: { maximum: 1.megabyte } validates :header_logo, file_size: { maximum: 1.megabyte } + validate :single_appearance_row, on: :create + mount_uploader :logo, AttachmentUploader mount_uploader :header_logo, AttachmentUploader has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + + CACHE_KEY = 'current_appearance'.freeze + + after_commit :flush_redis_cache + + def self.current + Rails.cache.fetch(CACHE_KEY) { first } + end + + def flush_redis_cache + Rails.cache.delete(CACHE_KEY) + end + + def single_appearance_row + if self.class.any? + errors.add(:single_appearance_row, 'Only 1 appearances row can exist') + end + end end -- cgit v1.2.3 From 64e13d1958eac06185866e050ef83678ed2c4fd9 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 10 Aug 2017 22:19:55 +0800 Subject: Avoid ambiguity, which happened in a single test run --- app/controllers/projects/issues_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app') diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index e2ccabb22db..917ee3955e2 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -212,7 +212,7 @@ class Projects::IssuesController < Projects::ApplicationController end def create_merge_request - result = MergeRequests::CreateFromIssueService.new(project, current_user, issue_iid: issue.iid).execute + result = ::MergeRequests::CreateFromIssueService.new(project, current_user, issue_iid: issue.iid).execute if result[:status] == :success render json: MergeRequestCreateSerializer.new.represent(result[:merge_request]) -- cgit v1.2.3 From c772464b68ad87860206e1ad341cca69e301a483 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 10 Aug 2017 22:20:53 +0800 Subject: Introduce MergeRequest#write_ref and Repository#write_ref so that we don't have to fetch it for non-forks --- app/models/merge_request.rb | 20 +++++++++++++++----- app/models/project.rb | 4 +--- app/models/repository.rb | 6 +++++- 3 files changed, 21 insertions(+), 9 deletions(-) (limited to 'app') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index f90194041b1..b6c9b8f21c7 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -792,11 +792,7 @@ class MergeRequest < ActiveRecord::Base end def fetch_ref - target_project.repository.fetch_ref( - source_project.repository.path_to_repo, - "refs/heads/#{source_branch}", - ref_path - ) + write_ref update_column(:ref_fetched, true) end @@ -939,4 +935,18 @@ class MergeRequest < ActiveRecord::Base true end + + private + + def write_ref + if for_fork? + target_project.repository.fetch_ref( + source_project.repository.path_to_repo, + "refs/heads/#{source_branch}", + ref_path + ) + else + source_project.repository.write_ref(ref_path, source_branch_sha) + end + end end diff --git a/app/models/project.rb b/app/models/project.rb index 7010664e1c8..7cdd00bc17b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1048,9 +1048,7 @@ class Project < ActiveRecord::Base def change_head(branch) if repository.branch_exists?(branch) repository.before_change_head - repository.rugged.references.create('HEAD', - "refs/heads/#{branch}", - force: true) + repository.write_ref('HEAD', "refs/heads/#{branch}") repository.copy_gitattributes(branch) repository.after_change_head reload_default_branch diff --git a/app/models/repository.rb b/app/models/repository.rb index f8139ff595e..04403cdd035 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -224,7 +224,7 @@ class Repository # This will still fail if the file is corrupted (e.g. 0 bytes) begin - rugged.references.create(keep_around_ref_name(sha), sha, force: true) + write_ref(keep_around_ref_name(sha), sha) rescue Rugged::ReferenceError => ex Rails.logger.error "Unable to create keep-around reference for repository #{path}: #{ex}" rescue Rugged::OSError => ex @@ -237,6 +237,10 @@ class Repository ref_exists?(keep_around_ref_name(sha)) end + def write_ref(ref_path, sha) + rugged.references.create(ref_path, sha, force: true) + end + def diverging_commit_counts(branch) root_ref_hash = raw_repository.rev_parse_target(root_ref).oid cache.fetch(:"diverging_commit_counts_#{branch.name}") do -- cgit v1.2.3 From 0395c47193b3bbf6b4f060f28c9f632580313a35 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 10 Jul 2017 17:43:57 +0200 Subject: Migrate events into a new format This commit migrates events data in such a way that push events are stored much more efficiently. This is done by creating a shadow table called "events_for_migration", and a table called "push_event_payloads" which is used for storing push data of push events. The background migration in this commit will copy events from the "events" table into the "events_for_migration" table, push events in will also have a row created in "push_event_payloads". This approach allows us to reclaim space in the next release by simply swapping the "events" and "events_for_migration" tables, then dropping the old events (now "events_for_migration") table. The new table structure is also optimised for storage space, and does not include the unused "title" column nor the "data" column (since this data is moved to "push_event_payloads"). == Newly Created Events Newly created events are inserted into both "events" and "events_for_migration", both using the exact same primary key value. The table "push_event_payloads" in turn has a foreign key to the _shadow_ table. This removes the need for recreating and validating the foreign key after swapping the tables. Since the shadow table also has a foreign key to "projects.id" we also don't have to worry about orphaned rows. This approach however does require some additional storage as we're duplicating a portion of the events data for at least 1 release. The exact amount is hard to estimate, but for GitLab.com this is expected to be between 10 and 20 GB at most. The background migration in this commit deliberately does _not_ update the "events" table as doing so would put a lot of pressure on PostgreSQL's auto vacuuming system. == Supporting Both Old And New Events Application code has also been adjusted to support push events using both the old and new data formats. This is done by creating a PushEvent class which extends the regular Event class. Using Rails' Single Table Inheritance system we can ensure the right class is used for the right data, which in this case is based on the value of `events.action`. To support displaying old and new data at the same time the PushEvent class re-defines a few methods of the Event class, falling back to their original implementations for push events in the old format. Once all existing events have been migrated the various push event related methods can be removed from the Event model, and the calls to `super` can be removed from the methods in the PushEvent model. The UI and event atom feed have also been slightly changed to better handle this new setup, fortunately only a few changes were necessary to make this work. == API Changes The API only displays push data of events in the new format. Supporting both formats in the API is a bit more difficult compared to the UI. Since the old push data was not really well documented (apart from one example that used an incorrect "action" nmae) I decided that supporting both was not worth the effort, especially since events will be migrated in a few days _and_ new events are created in the correct format. --- app/models/event.rb | 50 +++++++++++- app/models/event_for_migration.rb | 5 ++ app/models/push_event.rb | 126 +++++++++++++++++++++++++++++ app/models/push_event_payload.rb | 22 +++++ app/services/event_create_service.rb | 9 ++- app/services/push_event_payload_service.rb | 120 +++++++++++++++++++++++++++ app/views/events/_commit.html.haml | 4 +- app/views/events/_event_push.atom.haml | 19 +++-- app/views/events/event/_push.html.haml | 13 +-- 9 files changed, 345 insertions(+), 23 deletions(-) create mode 100644 app/models/event_for_migration.rb create mode 100644 app/models/push_event.rb create mode 100644 app/models/push_event_payload.rb create mode 100644 app/services/push_event_payload_service.rb (limited to 'app') diff --git a/app/models/event.rb b/app/models/event.rb index 8d93a228494..a598eb08e82 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -48,6 +48,7 @@ class Event < ActiveRecord::Base belongs_to :author, class_name: "User" belongs_to :project belongs_to :target, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations + has_one :push_event_payload, foreign_key: :event_id # For Hash only serialize :data # rubocop:disable Cop/ActiveRecordSerialize @@ -55,6 +56,7 @@ class Event < ActiveRecord::Base # Callbacks after_create :reset_project_activity after_create :set_last_repository_updated_at, if: :push? + after_create :replicate_event_for_push_events_migration # Scopes scope :recent, -> { reorder(id: :desc) } @@ -64,10 +66,36 @@ class Event < ActiveRecord::Base where(project_id: projects.pluck(:id)).recent end - scope :with_associations, -> { includes(:author, :project, project: :namespace).preload(:target) } + scope :with_associations, -> do + # We're using preload for "push_event_payload" as otherwise the association + # is not always available (depending on the query being built). + includes(:author, :project, project: :namespace) + .preload(:target, :push_event_payload) + end + scope :for_milestone_id, ->(milestone_id) { where(target_type: "Milestone", target_id: milestone_id) } + self.inheritance_column = 'action' + class << self + def find_sti_class(action) + if action.to_i == PUSHED + PushEvent + else + Event + end + end + + def subclass_from_attributes(attrs) + # Without this Rails will keep calling this method on the returned class, + # resulting in an infinite loop. + return unless self == Event + + action = attrs.with_indifferent_access[inheritance_column].to_i + + PushEvent if action == PUSHED + end + # Update Gitlab::ContributionsCalendar#activity_dates if this changes def contributions where("action = ? OR (target_type IN (?) AND action IN (?)) OR (target_type = ? AND action = ?)", @@ -290,6 +318,16 @@ class Event < ActiveRecord::Base @commits ||= (data[:commits] || []).reverse end + def commit_title + commit = commits.last + + commit[:message] if commit + end + + def commit_id + commit_to || commit_from + end + def commits_count data[:total_commits_count] || commits.count || 0 end @@ -385,6 +423,16 @@ class Event < ActiveRecord::Base user ? author_id == user.id : false end + # We're manually replicating data into the new table since database triggers + # are not dumped to db/schema.rb. This could mean that a new installation + # would not have the triggers in place, thus losing events data in GitLab + # 10.0. + def replicate_event_for_push_events_migration + new_attributes = attributes.with_indifferent_access.except(:title, :data) + + EventForMigration.create!(new_attributes) + end + private def recent_update? diff --git a/app/models/event_for_migration.rb b/app/models/event_for_migration.rb new file mode 100644 index 00000000000..a1672da5eec --- /dev/null +++ b/app/models/event_for_migration.rb @@ -0,0 +1,5 @@ +# This model is used to replicate events between the old "events" table and the +# new "events_for_migration" table that will replace "events" in GitLab 10.0. +class EventForMigration < ActiveRecord::Base + self.table_name = 'events_for_migration' +end diff --git a/app/models/push_event.rb b/app/models/push_event.rb new file mode 100644 index 00000000000..3f1ff979de6 --- /dev/null +++ b/app/models/push_event.rb @@ -0,0 +1,126 @@ +class PushEvent < Event + # This validation exists so we can't accidentally use PushEvent with a + # different "action" value. + validate :validate_push_action + + # Authors are required as they're used to display who pushed data. + # + # We're just validating the presence of the ID here as foreign key constraints + # should ensure the ID points to a valid user. + validates :author_id, presence: true + + # The project is required to build links to commits, commit ranges, etc. + # + # We're just validating the presence of the ID here as foreign key constraints + # should ensure the ID points to a valid project. + validates :project_id, presence: true + + # The "data" field must not be set for push events since it's not used and a + # waste of space. + validates :data, absence: true + + # These fields are also not used for push events, thus storing them would be a + # waste. + validates :target_id, absence: true + validates :target_type, absence: true + + def self.sti_name + PUSHED + end + + def push? + true + end + + def push_with_commits? + !!(commit_from && commit_to) + end + + def tag? + return super unless push_event_payload + + push_event_payload.tag? + end + + def branch? + return super unless push_event_payload + + push_event_payload.branch? + end + + def valid_push? + return super unless push_event_payload + + push_event_payload.ref.present? + end + + def new_ref? + return super unless push_event_payload + + push_event_payload.created? + end + + def rm_ref? + return super unless push_event_payload + + push_event_payload.removed? + end + + def commit_from + return super unless push_event_payload + + push_event_payload.commit_from + end + + def commit_to + return super unless push_event_payload + + push_event_payload.commit_to + end + + def ref_name + return super unless push_event_payload + + push_event_payload.ref + end + + def ref_type + return super unless push_event_payload + + push_event_payload.ref_type + end + + def branch_name + return super unless push_event_payload + + ref_name + end + + def tag_name + return super unless push_event_payload + + ref_name + end + + def commit_title + return super unless push_event_payload + + push_event_payload.commit_title + end + + def commit_id + commit_to || commit_from + end + + def commits_count + return super unless push_event_payload + + push_event_payload.commit_count + end + + def validate_push_action + return if action == PUSHED + + errors.add(:action, "the action #{action.inspect} is not valid") + end +end diff --git a/app/models/push_event_payload.rb b/app/models/push_event_payload.rb new file mode 100644 index 00000000000..6cdb1cd4fe9 --- /dev/null +++ b/app/models/push_event_payload.rb @@ -0,0 +1,22 @@ +class PushEventPayload < ActiveRecord::Base + include ShaAttribute + + belongs_to :event, inverse_of: :push_event_payload + + validates :event_id, :commit_count, :action, :ref_type, presence: true + validates :commit_title, length: { maximum: 70 } + + sha_attribute :commit_from + sha_attribute :commit_to + + enum action: { + created: 0, + removed: 1, + pushed: 2 + } + + enum ref_type: { + branch: 0, + tag: 1 + } +end diff --git a/app/services/event_create_service.rb b/app/services/event_create_service.rb index 0f3a485a3fd..0b7e4f187f7 100644 --- a/app/services/event_create_service.rb +++ b/app/services/event_create_service.rb @@ -71,7 +71,14 @@ class EventCreateService end def push(project, current_user, push_data) - create_event(project, current_user, Event::PUSHED, data: push_data) + # We're using an explicit transaction here so that any errors that may occur + # when creating push payload data will result in the event creation being + # rolled back as well. + Event.transaction do + event = create_event(project, current_user, Event::PUSHED) + + PushEventPayloadService.new(event, push_data).execute + end Users::ActivityService.new(current_user, 'push').execute end diff --git a/app/services/push_event_payload_service.rb b/app/services/push_event_payload_service.rb new file mode 100644 index 00000000000..b0a389c85f9 --- /dev/null +++ b/app/services/push_event_payload_service.rb @@ -0,0 +1,120 @@ +# Service class for creating push event payloads as stored in the +# "push_event_payloads" table. +# +# Example: +# +# data = Gitlab::DataBuilder::Push.build(...) +# event = Event.create(...) +# +# PushEventPayloadService.new(event, data).execute +class PushEventPayloadService + # event - The event this push payload belongs to. + # push_data - A Hash produced by `Gitlab::DataBuilder::Push.build` to use for + # building the push payload. + def initialize(event, push_data) + @event = event + @push_data = push_data + end + + # Creates and returns a new PushEventPayload row. + # + # This method will raise upon encountering validation errors. + # + # Returns an instance of PushEventPayload. + def execute + @event.build_push_event_payload( + commit_count: commit_count, + action: action, + ref_type: ref_type, + commit_from: commit_from_id, + commit_to: commit_to_id, + ref: trimmed_ref, + commit_title: commit_title, + event_id: @event.id + ) + + @event.push_event_payload.save! + @event.push_event_payload + end + + # Returns the commit title to use. + # + # The commit title is limited to the first line and a maximum of 70 + # characters. + def commit_title + commit = @push_data.fetch(:commits).last + + return nil unless commit && commit[:message] + + raw_msg = commit[:message] + + # Find where the first line ends, without turning the entire message into an + # Array of lines (this is a waste of memory for large commit messages). + index = raw_msg.index("\n") + message = index ? raw_msg[0..index] : raw_msg + + message.strip.truncate(70) + end + + def commit_from_id + if create? + nil + else + revision_before + end + end + + def commit_to_id + if remove? + nil + else + revision_after + end + end + + def commit_count + @push_data.fetch(:total_commits_count) + end + + def ref + @push_data.fetch(:ref) + end + + def revision_before + @push_data.fetch(:before) + end + + def revision_after + @push_data.fetch(:after) + end + + def trimmed_ref + Gitlab::Git.ref_name(ref) + end + + def create? + Gitlab::Git.blank_ref?(revision_before) + end + + def remove? + Gitlab::Git.blank_ref?(revision_after) + end + + def action + if create? + :created + elsif remove? + :removed + else + :pushed + end + end + + def ref_type + if Gitlab::Git.tag_ref?(ref) + :tag + else + :branch + end + end +end diff --git a/app/views/events/_commit.html.haml b/app/views/events/_commit.html.haml index ad434a64556..98cdcca3ecc 100644 --- a/app/views/events/_commit.html.haml +++ b/app/views/events/_commit.html.haml @@ -1,5 +1,5 @@ %li.commit .commit-row-title - = link_to truncate_sha(commit[:id]), project_commit_path(project, commit[:id]), class: "commit-sha", alt: '', title: truncate_sha(commit[:id]) + = link_to truncate_sha(event.commit_id), project_commit_path(project, event.commit_id), class: "commit-sha", alt: '', title: truncate_sha(event.commit_id) · - = markdown event_commit_title(commit[:message]), project: project, pipeline: :single_line, author: event.author + = markdown event_commit_title(event.commit_title), project: project, pipeline: :single_line, author: event.author diff --git a/app/views/events/_event_push.atom.haml b/app/views/events/_event_push.atom.haml index 9fcacfbbf36..bf655f9d21a 100644 --- a/app/views/events/_event_push.atom.haml +++ b/app/views/events/_event_push.atom.haml @@ -1,14 +1,13 @@ %div{ xmlns: "http://www.w3.org/1999/xhtml" } - - event.commits.first(15).each do |commit| - %p - %strong= commit[:author][:name] - = link_to "(##{truncate_sha(commit[:id])})", project_commit_path(event.project, id: commit[:id]) - %i - at - = commit[:timestamp].to_time.to_s(:short) - %blockquote= markdown(escape_once(commit[:message]), pipeline: :atom, project: event.project, author: event.author) - - if event.commits_count > 15 + %p + %strong= event.author_name + = link_to "(#{truncate_sha(event.commit_id)})", project_commit_path(event.project, event.commit_id) + %i + at + = event.created_at.to_s(:short) + %blockquote= markdown(escape_once(event.commit_title), pipeline: :atom, project: event.project, author: event.author) + - if event.commits_count > 1 %p %i \... and - = pluralize(event.commits_count - 15, "more commit") + = pluralize(event.commits_count - 1, "more commit") diff --git a/app/views/events/event/_push.html.haml b/app/views/events/event/_push.html.haml index 54b414cc62a..973c652ad88 100644 --- a/app/views/events/event/_push.html.haml +++ b/app/views/events/event/_push.html.haml @@ -14,9 +14,7 @@ - if event.push_with_commits? .event-body %ul.well-list.event_commits - - few_commits = event.commits[0...2] - - few_commits.each do |commit| - = render "events/commit", commit: commit, project: project, event: event + = render "events/commit", project: project, event: event - create_mr = event.new_ref? && create_mr_button?(project.default_branch, event.ref_name, project) && event.authored_by?(current_user) - if event.commits_count > 1 @@ -44,9 +42,6 @@ = link_to create_mr_path(project.default_branch, event.ref_name, project) do Create Merge Request - elsif event.rm_ref? - - repository = project.repository - - last_commit = repository.commit(event.commit_from) - - if last_commit - .event-body - %ul.well-list.event_commits - = render "events/commit", commit: last_commit, project: project, event: event + .event-body + %ul.well-list.event_commits + = render "events/commit", project: project, event: event -- cgit v1.2.3 From aac1de46c9be659b74da12f704412f38292974db Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 27 Jul 2017 19:42:15 +0200 Subject: Use a specialized class for querying events This changes various controllers to use the new EventCollection class for retrieving events. This class uses a JOIN LATERAL query on PostgreSQL to retrieve queries in a more efficient way, while falling back to a simpler / less efficient query for MySQL. The EventCollection class also includes a limit on the number of events to display to prevent malicious users from cycling through all events, as doing so could put a lot of pressure on the database. JOIN LATERAL is only supported on PostgreSQL starting with version 9.3.0 and as such this optimisation is only used when using PostgreSQL 9.3 or newer. --- app/controllers/dashboard/projects_controller.rb | 8 +- app/controllers/dashboard_controller.rb | 6 +- app/controllers/groups_controller.rb | 6 +- app/controllers/projects_controller.rb | 9 ++- app/models/event.rb | 9 ++- app/models/event_collection.rb | 98 ++++++++++++++++++++++++ 6 files changed, 121 insertions(+), 15 deletions(-) create mode 100644 app/models/event_collection.rb (limited to 'app') diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index 74fe45e1ff6..f71ab702e71 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -52,8 +52,10 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController end def load_events - @events = Event.in_projects(load_projects(params.merge(non_public: true))) - @events = event_filter.apply_filter(@events).with_associations - @events = @events.limit(20).offset(params[:offset] || 0) + projects = load_projects(params.merge(non_public: true)) + + @events = EventCollection + .new(projects, offset: params[:offset].to_i, filter: event_filter) + .to_a end end diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index f9c31920302..19a5db6fd17 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -29,9 +29,9 @@ class DashboardController < Dashboard::ApplicationController current_user.authorized_projects end - @events = Event.in_projects(projects) - @events = @event_filter.apply_filter(@events).with_associations - @events = @events.limit(20).offset(params[:offset] || 0) + @events = EventCollection + .new(projects, offset: params[:offset].to_i, filter: @event_filter) + .to_a end def set_show_full_reference diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 27137ffde54..f76b3f69e9e 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -160,9 +160,9 @@ class GroupsController < Groups::ApplicationController end def load_events - @events = Event.in_projects(@projects) - @events = event_filter.apply_filter(@events).with_associations - @events = @events.limit(20).offset(params[:offset] || 0) + @events = EventCollection + .new(@projects, offset: params[:offset].to_i, filter: event_filter) + .to_a end def user_actions diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index 8dfe0f51709..e93f34498d6 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -301,10 +301,11 @@ class ProjectsController < Projects::ApplicationController end def load_events - @events = @project.events.recent - @events = event_filter.apply_filter(@events).with_associations - limit = (params[:limit] || 20).to_i - @events = @events.limit(limit).offset(params[:offset] || 0) + projects = Project.where(id: @project.id) + + @events = EventCollection + .new(projects, offset: params[:offset].to_i, filter: event_filter) + .to_a end def project_params diff --git a/app/models/event.rb b/app/models/event.rb index a598eb08e82..f2a560a6b56 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -62,8 +62,13 @@ class Event < ActiveRecord::Base scope :recent, -> { reorder(id: :desc) } scope :code_push, -> { where(action: PUSHED) } - scope :in_projects, ->(projects) do - where(project_id: projects.pluck(:id)).recent + scope :in_projects, -> (projects) do + sub_query = projects + .except(:order) + .select(1) + .where('projects.id = events.project_id') + + where('EXISTS (?)', sub_query).recent end scope :with_associations, -> do diff --git a/app/models/event_collection.rb b/app/models/event_collection.rb new file mode 100644 index 00000000000..8b8244314af --- /dev/null +++ b/app/models/event_collection.rb @@ -0,0 +1,98 @@ +# A collection of events to display in an event list. +# +# An EventCollection is meant to be used for displaying events to a user (e.g. +# in a controller), it's not suitable for building queries that are used for +# building other queries. +class EventCollection + # To prevent users from putting too much pressure on the database by cycling + # through thousands of events we put a limit on the number of pages. + MAX_PAGE = 10 + + # projects - An ActiveRecord::Relation object that returns the projects for + # which to retrieve events. + # filter - An EventFilter instance to use for filtering events. + def initialize(projects, limit: 20, offset: 0, filter: nil) + @projects = projects + @limit = limit + @offset = offset + @filter = filter + end + + # Returns an Array containing the events. + def to_a + return [] if current_page > MAX_PAGE + + relation = if Gitlab::Database.join_lateral_supported? + relation_with_join_lateral + else + relation_without_join_lateral + end + + relation.with_associations.to_a + end + + private + + # Returns the events relation to use when JOIN LATERAL is not supported. + # + # This relation simply gets all the events for all authorized projects, then + # limits that set. + def relation_without_join_lateral + events = filtered_events.in_projects(projects) + + paginate_events(events) + end + + # Returns the events relation to use when JOIN LATERAL is supported. + # + # This relation is built using JOIN LATERAL, producing faster queries than a + # regular LIMIT + OFFSET approach. + def relation_with_join_lateral + projects_for_lateral = projects.select(:id).to_sql + + lateral = filtered_events + .limit(limit_for_join_lateral) + .where('events.project_id = projects_for_lateral.id') + .to_sql + + # The outer query does not need to re-apply the filters since the JOIN + # LATERAL body already takes care of this. + outer = base_relation + .from("(#{projects_for_lateral}) projects_for_lateral") + .joins("JOIN LATERAL (#{lateral}) AS #{Event.table_name} ON true") + + paginate_events(outer) + end + + def filtered_events + @filter ? @filter.apply_filter(base_relation) : base_relation + end + + def paginate_events(events) + events.limit(@limit).offset(@offset) + end + + def base_relation + # We want to have absolute control over the event queries being built, thus + # we're explicitly opting out of any default scopes that may be set. + Event.unscoped.recent + end + + def limit_for_join_lateral + # Applying the OFFSET on the inside of a JOIN LATERAL leads to incorrect + # results. To work around this we need to increase the inner limit for every + # page. + # + # This means that on page 1 we use LIMIT 20, and an outer OFFSET of 0. On + # page 2 we use LIMIT 40 and an outer OFFSET of 20. + @limit + @offset + end + + def current_page + (@offset / @limit) + 1 + end + + def projects + @projects.except(:order) + end +end -- cgit v1.2.3 From 41a5adca7514ced023a2708ab26666db560b58a3 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Thu, 10 Aug 2017 23:53:55 +0800 Subject: Don't try to create diffs if one of the branch is missing Also fix a few tests --- app/models/merge_request.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'app') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index b6c9b8f21c7..daee7c93995 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -443,7 +443,8 @@ class MergeRequest < ActiveRecord::Base end def reload_diff_if_branch_changed - if source_branch_changed? || target_branch_changed? + if (source_branch_changed? || target_branch_changed?) && + (source_branch_head && target_branch_head) reload_diff end end -- cgit v1.2.3 From d59aed94e7ec441f44301a55e0529a9c34a01fd2 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 10 Aug 2017 13:05:04 -0500 Subject: Move syntax highlighting into a method Fix https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12198#note_37142936 --- app/assets/javascripts/repo/components/repo_preview.vue | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/assets/javascripts/repo/components/repo_preview.vue b/app/assets/javascripts/repo/components/repo_preview.vue index d8de022335b..0caa3a4551a 100644 --- a/app/assets/javascripts/repo/components/repo_preview.vue +++ b/app/assets/javascripts/repo/components/repo_preview.vue @@ -4,7 +4,7 @@ import Store from '../stores/repo_store'; export default { data: () => Store, mounted() { - $(this.$el).find('.file-content').syntaxHighlight(); + this.highlightFile(); }, computed: { html() { @@ -12,10 +12,16 @@ export default { }, }, + methods: { + highlightFile() { + $(this.$el).find('.file-content').syntaxHighlight(); + }, + }, + watch: { html() { this.$nextTick(() => { - $(this.$el).find('.file-content').syntaxHighlight(); + this.highlightFile(); }); }, }, -- cgit v1.2.3 From a0d55bd97a6476627a45f5b3f315c785f2823709 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 11 Aug 2017 09:35:06 +0100 Subject: Fixes the correct import modal window not showing Closes #36318 --- app/assets/javascripts/projects/project_new.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app') diff --git a/app/assets/javascripts/projects/project_new.js b/app/assets/javascripts/projects/project_new.js index 985521aef34..7f972b6f6ee 100644 --- a/app/assets/javascripts/projects/project_new.js +++ b/app/assets/javascripts/projects/project_new.js @@ -36,7 +36,7 @@ const bindEvents = () => { $('.how_to_import_link').on('click', (e) => { e.preventDefault(); - $('.how_to_import_link').next('.modal').show(); + $(e.currentTarget).next('.modal').show(); }); $('.modal-header .close').on('click', () => { -- cgit v1.2.3 From 21066e827ab6b957b0948b025745b563f01a00d5 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Tue, 8 Aug 2017 17:40:22 +0100 Subject: Pending delete projects no longer return 500 error in Admins projects view --- app/finders/admin/projects_finder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app') diff --git a/app/finders/admin/projects_finder.rb b/app/finders/admin/projects_finder.rb index a5ba791a513..7176bfe22d6 100644 --- a/app/finders/admin/projects_finder.rb +++ b/app/finders/admin/projects_finder.rb @@ -18,7 +18,7 @@ class Admin::ProjectsFinder end def execute - items = Project.with_statistics + items = Project.without_deleted.with_statistics items = items.in_namespace(namespace_id) if namespace_id.present? items = items.where(visibility_level: visibility_level) if visibility_level.present? items = items.with_push if with_push.present? -- cgit v1.2.3 From 8bfae74e9c6b6dde6f2e33d9ea45e43c8c4004a7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 11 Aug 2017 10:54:03 +0000 Subject: Delete correct key from `session` after authenticating using U2F --- app/controllers/concerns/authenticates_with_two_factor.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app') diff --git a/app/controllers/concerns/authenticates_with_two_factor.rb b/app/controllers/concerns/authenticates_with_two_factor.rb index ea441b1736b..b75e401a8df 100644 --- a/app/controllers/concerns/authenticates_with_two_factor.rb +++ b/app/controllers/concerns/authenticates_with_two_factor.rb @@ -69,7 +69,7 @@ module AuthenticatesWithTwoFactor if U2fRegistration.authenticate(user, u2f_app_id, user_params[:device_response], session[:challenge]) # Remove any lingering user data from login session.delete(:otp_user_id) - session.delete(:challenges) + session.delete(:challenge) remember_me(user) if user_params[:remember_me] == '1' sign_in(user) -- cgit v1.2.3 From ca685f80e0fffa60827fe0da7cff1192cef701de Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Fri, 11 Aug 2017 20:12:17 +0800 Subject: Since now fetch_ref is reliable, we could just rely on it --- app/models/repository.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'app') diff --git a/app/models/repository.rb b/app/models/repository.rb index 04403cdd035..a761302b06b 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -989,12 +989,10 @@ class Repository if start_repository == self start_branch_name else - tmp_ref = "refs/tmp/#{SecureRandom.hex}/head" - - fetch_ref( + tmp_ref = fetch_ref( start_repository.path_to_repo, "#{Gitlab::Git::BRANCH_REF_PREFIX}#{start_branch_name}", - tmp_ref + "refs/tmp/#{SecureRandom.hex}/head" ) start_repository.commit(start_branch_name).sha @@ -1003,7 +1001,7 @@ class Repository yield(commit(branch_name_or_sha)) ensure - rugged.references.delete(tmp_ref) if tmp_ref && ref_exists?(tmp_ref) + rugged.references.delete(tmp_ref) if tmp_ref end def add_remote(name, url) @@ -1029,6 +1027,8 @@ class Repository # Make sure ref was created, and raise Rugged::ReferenceError when not raise Rugged::ReferenceError, message if status != 0 + + target_ref end def create_ref(ref, ref_path) -- cgit v1.2.3 From f05bfef70cb1e22942c7cdf0e3432a984281020a Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Fri, 11 Aug 2017 09:30:48 -0500 Subject: Increase z-index of pipeline dropdown --- app/assets/stylesheets/pages/pipelines.scss | 1 + 1 file changed, 1 insertion(+) (limited to 'app') diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss index 6185342b495..85d1905ad40 100644 --- a/app/assets/stylesheets/pages/pipelines.scss +++ b/app/assets/stylesheets/pages/pipelines.scss @@ -824,6 +824,7 @@ button.mini-pipeline-graph-dropdown-toggle { * Top arrow in the dropdown in the mini pipeline graph */ .mini-pipeline-graph-dropdown-menu { + z-index: 200; &::before, &::after { -- cgit v1.2.3 From a5c8a52782ae6e948adbbba77c0ec702ffe28ae1 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Wed, 9 Aug 2017 15:49:30 +0200 Subject: Better caching and indexing of broadcast messages Caching of BroadcastMessage instances has been changed so a cache stays valid as long as the default cache expiration time permits, instead of the cache being expired after 1 minute. When modifying broadcast messages the cache is flushed automatically. To remove the need for performing sequence scans on the "broadcast_messages" table we also add an index on (starts_at, ends_at, id), permitting PostgreSQL to use an index scan to get all necessary data. Finally this commit adds a few NOT NULL constraints to the table to match the Rails validations. Fixes gitlab-org/gitlab-ce#31706 --- app/models/broadcast_message.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/models/broadcast_message.rb b/app/models/broadcast_message.rb index 944725d91c3..3692bcc680d 100644 --- a/app/models/broadcast_message.rb +++ b/app/models/broadcast_message.rb @@ -14,9 +14,15 @@ class BroadcastMessage < ActiveRecord::Base default_value_for :color, '#E75E40' default_value_for :font, '#FFFFFF' + CACHE_KEY = 'broadcast_message_current'.freeze + + after_commit :flush_redis_cache + def self.current - Rails.cache.fetch("broadcast_message_current", expires_in: 1.minute) do - where('ends_at > :now AND starts_at <= :now', now: Time.zone.now).order([:created_at, :id]).to_a + Rails.cache.fetch(CACHE_KEY) do + where('ends_at > :now AND starts_at <= :now', now: Time.zone.now) + .reorder(id: :asc) + .to_a end end @@ -31,4 +37,8 @@ class BroadcastMessage < ActiveRecord::Base def ended? ends_at < Time.zone.now end + + def flush_redis_cache + Rails.cache.delete(CACHE_KEY) + end end -- cgit v1.2.3 From d0622b79d8d011c80f63e71c96e69754a5b0ec16 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Thu, 10 Aug 2017 19:09:14 -0400 Subject: Better categorize test coverage results Also marks a few things as uncovered, and removes an unused class. --- app/controllers/unicorn_test_controller.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'app') diff --git a/app/controllers/unicorn_test_controller.rb b/app/controllers/unicorn_test_controller.rb index b7a1a046be0..ed04bd1f77d 100644 --- a/app/controllers/unicorn_test_controller.rb +++ b/app/controllers/unicorn_test_controller.rb @@ -1,12 +1,14 @@ +# :nocov: if Rails.env.test? class UnicornTestController < ActionController::Base def pid render plain: Process.pid.to_s end - + def kill Process.kill(params[:signal], Process.pid) render plain: 'Bye!' end end end +# :nocov: -- cgit v1.2.3 From 180de2d20127f79773bf661f88cd7556b191d0b9 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 7 Aug 2017 19:43:11 +0200 Subject: Make sure uploads for personal snippets are correctly rendered --- app/uploaders/personal_file_uploader.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app') diff --git a/app/uploaders/personal_file_uploader.rb b/app/uploaders/personal_file_uploader.rb index ef70871624b..3298ad104ec 100644 --- a/app/uploaders/personal_file_uploader.rb +++ b/app/uploaders/personal_file_uploader.rb @@ -4,7 +4,7 @@ class PersonalFileUploader < FileUploader end def self.base_dir - File.join(root_dir, 'system') + File.join(root_dir, '-', 'system') end private -- cgit v1.2.3 From 649d042dbc9e2bfda96bb98b0eabd4b00ea2daff Mon Sep 17 00:00:00 2001 From: Robin Bobbitt Date: Mon, 31 Jul 2017 17:34:47 -0400 Subject: Add option to disable project export on instance --- app/controllers/projects_controller.rb | 5 +++ app/helpers/application_settings_helper.rb | 1 + app/models/application_setting.rb | 1 + .../admin/application_settings/_form.html.haml | 6 ++++ app/views/projects/_export.html.haml | 41 ++++++++++++++++++++++ app/views/projects/edit.html.haml | 37 +------------------ 6 files changed, 55 insertions(+), 36 deletions(-) create mode 100644 app/views/projects/_export.html.haml (limited to 'app') diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index e93f34498d6..1d24563a6a6 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -7,6 +7,7 @@ class ProjectsController < Projects::ApplicationController before_action :repository, except: [:index, :new, :create] before_action :assign_ref_vars, only: [:show], if: :repo_exists? before_action :tree, only: [:show], if: [:repo_exists?, :project_view_files?] + before_action :project_export_enabled, only: [:export, :download_export, :remove_export, :generate_new_export] # Authorize before_action :authorize_admin_project!, only: [:edit, :update, :housekeeping, :download_export, :export, :remove_export, :generate_new_export] @@ -390,4 +391,8 @@ class ProjectsController < Projects::ApplicationController url_for(params) end + + def project_export_enabled + render_404 unless current_application_settings.project_export_enabled? + end end diff --git a/app/helpers/application_settings_helper.rb b/app/helpers/application_settings_helper.rb index 6825adcb39f..150188f0b65 100644 --- a/app/helpers/application_settings_helper.rb +++ b/app/helpers/application_settings_helper.rb @@ -146,6 +146,7 @@ module ApplicationSettingsHelper :plantuml_enabled, :plantuml_url, :polling_interval_multiplier, + :project_export_enabled, :prometheus_metrics_enabled, :recaptcha_enabled, :recaptcha_private_key, diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index bd7c4cd45ea..8e446ff6dd8 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -241,6 +241,7 @@ class ApplicationSetting < ActiveRecord::Base performance_bar_allowed_group_id: nil, plantuml_enabled: false, plantuml_url: nil, + project_export_enabled: true, recaptcha_enabled: false, repository_checks_enabled: true, repository_storages: ['default'], diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index a4f49d3f6d7..8bf6556079b 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -48,6 +48,12 @@ = select(:application_setting, :enabled_git_access_protocol, [['Both SSH and HTTP(S)', nil], ['Only SSH', 'ssh'], ['Only HTTP(S)', 'http']], {}, class: 'form-control') %span.help-block#clone-protocol-help Allow only the selected protocols to be used for Git access. + .form-group + .col-sm-offset-2.col-sm-10 + .checkbox + = f.label :project_export_enabled do + = f.check_box :project_export_enabled + Project export enabled %fieldset %legend Account and Limit Settings diff --git a/app/views/projects/_export.html.haml b/app/views/projects/_export.html.haml new file mode 100644 index 00000000000..623d3bc91c6 --- /dev/null +++ b/app/views/projects/_export.html.haml @@ -0,0 +1,41 @@ +- return unless current_application_settings.project_export_enabled? + +- project = local_assigns.fetch(:project) +- expanded = Rails.env.test? + +%section.settings + .settings-header + %h4 + Export project + %button.btn.js-settings-toggle + = expanded ? 'Collapse' : 'Expand' + %p + Export this project with all its related data in order to move your project to a new GitLab instance. Once the export is finished, you can import the file from the "New Project" page. + .settings-content.no-animate{ class: ('expanded' if expanded) } + .bs-callout.bs-callout-info + %p.append-bottom-0 + %p + The following items will be exported: + %ul + %li Project and wiki repositories + %li Project uploads + %li Project configuration including web hooks and services + %li Issues with comments, merge requests with diffs and comments, labels, milestones, snippets, and other project entities + %p + The following items will NOT be exported: + %ul + %li Job traces and artifacts + %li LFS objects + %li Container registry images + %li CI variables + %li Any encrypted tokens + %p + Once the exported file is ready, you will receive a notification email with a download link. + - if project.export_project_path + = link_to 'Download export', download_export_project_path(project), + rel: 'nofollow', download: '', method: :get, class: "btn btn-default" + = link_to 'Generate new export', generate_new_export_project_path(project), + method: :post, class: "btn btn-default" + - else + = link_to 'Export project', export_project_path(project), + method: :post, class: "btn btn-default" diff --git a/app/views/projects/edit.html.haml b/app/views/projects/edit.html.haml index c2794f8aaa8..6178abe9160 100644 --- a/app/views/projects/edit.html.haml +++ b/app/views/projects/edit.html.haml @@ -161,42 +161,7 @@ = render 'merge_request_settings', form: f = f.submit 'Save changes', class: "btn btn-save" - %section.settings - .settings-header - %h4 - Export project - %button.btn.js-settings-toggle - = expanded ? 'Collapse' : 'Expand' - %p - Export this project with all its related data in order to move your project to a new GitLab instance. Once the export is finished, you can import the file from the "New Project" page. - .settings-content.no-animate{ class: ('expanded' if expanded) } - .bs-callout.bs-callout-info - %p.append-bottom-0 - %p - The following items will be exported: - %ul - %li Project and wiki repositories - %li Project uploads - %li Project configuration including web hooks and services - %li Issues with comments, merge requests with diffs and comments, labels, milestones, snippets, and other project entities - %p - The following items will NOT be exported: - %ul - %li Job traces and artifacts - %li LFS objects - %li Container registry images - %li CI variables - %li Any encrypted tokens - %p - Once the exported file is ready, you will receive a notification email with a download link. - - if @project.export_project_path - = link_to 'Download export', download_export_project_path(@project), - rel: 'nofollow', download: '', method: :get, class: "btn btn-default" - = link_to 'Generate new export', generate_new_export_project_path(@project), - method: :post, class: "btn btn-default" - - else - = link_to 'Export project', export_project_path(@project), - method: :post, class: "btn btn-default" + = render 'export', project: @project %section.settings.advanced-settings .settings-header -- cgit v1.2.3 From 64073185adcb3eec40eda05e11f9bf47f646bf9d Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Fri, 11 Aug 2017 13:27:38 -0400 Subject: Remove `username` from `User#sanitize_attrs` callback This attribute is since validated against `DynamicPathValidator`, which has strict requirements for the characters allowed, and should no longer need to be sanitized in a callback before saving. This has additional benefits in our test suite, where every creation of a `User` record was calling `Sanitize.clean` on a username value that was always clean, since we're the ones generating it. --- app/models/user.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'app') diff --git a/app/models/user.rb b/app/models/user.rb index 7935b89662b..42a1ac40c6c 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -726,9 +726,9 @@ class User < ActiveRecord::Base end def sanitize_attrs - %w[username skype linkedin twitter].each do |attr| - value = public_send(attr) # rubocop:disable GitlabSecurity/PublicSend - public_send("#{attr}=", Sanitize.clean(value)) if value.present? # rubocop:disable GitlabSecurity/PublicSend + %i[skype linkedin twitter].each do |attr| + value = self[attr] + self[attr] = Sanitize.clean(value) if value.present? end end -- cgit v1.2.3 From 6e3ca79dedbe27f64d12dbf391d0823137dcc610 Mon Sep 17 00:00:00 2001 From: Mehdi Lahmam Date: Thu, 10 Aug 2017 08:42:28 +0200 Subject: Add a `Last 7 days` option for Cycle Analytics view --- app/controllers/concerns/cycle_analytics_params.rb | 9 ++++++++- app/views/projects/cycle_analytics/show.html.haml | 3 +++ 2 files changed, 11 insertions(+), 1 deletion(-) (limited to 'app') diff --git a/app/controllers/concerns/cycle_analytics_params.rb b/app/controllers/concerns/cycle_analytics_params.rb index 52e06f4945a..1ab107168c0 100644 --- a/app/controllers/concerns/cycle_analytics_params.rb +++ b/app/controllers/concerns/cycle_analytics_params.rb @@ -6,6 +6,13 @@ module CycleAnalyticsParams end def start_date(params) - params[:start_date] == '30' ? 30.days.ago : 90.days.ago + case params[:start_date] + when '7' + 7.days.ago + when '30' + 30.days.ago + else + 90.days.ago + end end end diff --git a/app/views/projects/cycle_analytics/show.html.haml b/app/views/projects/cycle_analytics/show.html.haml index c704635ead3..3467e357c49 100644 --- a/app/views/projects/cycle_analytics/show.html.haml +++ b/app/views/projects/cycle_analytics/show.html.haml @@ -39,6 +39,9 @@ %span.dropdown-label {{ n__('Last %d day', 'Last %d days', 30) }} %i.fa.fa-chevron-down %ul.dropdown-menu.dropdown-menu-align-right + %li + %a{ "href" => "#", "data-value" => "7" } + {{ n__('Last %d day', 'Last %d days', 7) }} %li %a{ "href" => "#", "data-value" => "30" } {{ n__('Last %d day', 'Last %d days', 30) }} -- cgit v1.2.3 From e184dfc3c02e942fae0cc264770ffaa4b0201616 Mon Sep 17 00:00:00 2001 From: Regis Boudinot Date: Fri, 11 Aug 2017 21:55:36 +0000 Subject: fix confidential border issue as well as confidential styles leaking on new MR --- app/assets/stylesheets/pages/note_form.scss | 20 ++++---------------- app/views/projects/_md_preview.html.haml | 4 +--- 2 files changed, 5 insertions(+), 19 deletions(-) (limited to 'app') diff --git a/app/assets/stylesheets/pages/note_form.scss b/app/assets/stylesheets/pages/note_form.scss index c90642178fc..b4468d6d0a2 100644 --- a/app/assets/stylesheets/pages/note_form.scss +++ b/app/assets/stylesheets/pages/note_form.scss @@ -108,6 +108,7 @@ background-color: $orange-50; border-radius: $border-radius-default $border-radius-default 0 0; border: 1px solid $border-gray-normal; + border-bottom: none; padding: 3px 12px; margin: auto; align-items: center; @@ -132,22 +133,9 @@ } } -.not-confidential { - padding: 0; - border-top: none; -} - -.right-sidebar-expanded { - .md-area { - border-radius: 0; - border-top: none; - } -} - -.right-sidebar-collapsed { - .confidential-issue-warning { - border-bottom: none; - } +.confidential-issue-warning + .md-area { + border-top-left-radius: 0; + border-top-right-radius: 0; } .discussion-form { diff --git a/app/views/projects/_md_preview.html.haml b/app/views/projects/_md_preview.html.haml index 6e13bf47ff6..97041b87c48 100644 --- a/app/views/projects/_md_preview.html.haml +++ b/app/views/projects/_md_preview.html.haml @@ -1,11 +1,9 @@ - referenced_users = local_assigns.fetch(:referenced_users, nil) - if defined?(@issue) && @issue.confidential? - %li.confidential-issue-warning + .confidential-issue-warning = confidential_icon(@issue) %span This is a confidential issue. Your comment will not be visible to the public. -- else - %li.confidential-issue-warning.not-confidential .md-area .md-header -- cgit v1.2.3 From 7e7f602d29fef237b5531795bd5c38541d14ea14 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Fri, 4 Aug 2017 11:53:17 -0700 Subject: make NotificationRecipient a little more customizable --- app/models/notification_recipient.rb | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) (limited to 'app') diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb index 418b42d8f1d..30fab33e5e0 100644 --- a/app/models/notification_recipient.rb +++ b/app/models/notification_recipient.rb @@ -5,12 +5,18 @@ class NotificationRecipient custom_action: nil, target: nil, acting_user: nil, - project: nil + project: nil, + group: nil ) + unless NotificationSetting.levels.key?(type) || type == :subscription + raise ArgumentError, "invalid type: #{type.inspect}" + end + @custom_action = custom_action @acting_user = acting_user @target = target - @project = project || @target&.project + @project = project || default_project + @group = group || @project&.group @user = user @type = type end @@ -111,12 +117,18 @@ class NotificationRecipient end end + def default_project + return nil if @target.nil? + return @target if @target.is_a?(Project) + return @target.project if @target.respond_to?(:project) + end + def find_notification_setting project_setting = @project && user.notification_settings_for(@project) return project_setting unless project_setting.nil? || project_setting.global? - group_setting = @project&.group && user.notification_settings_for(@project.group) + group_setting = @group && user.notification_settings_for(@group) return group_setting unless group_setting.nil? || group_setting.global? -- cgit v1.2.3 From d5054abfcc1b27c664bff46ae6dc1482c591e5a9 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Fri, 4 Aug 2017 11:53:36 -0700 Subject: add Member#notifiable?(type, opts) --- app/models/member.rb | 4 ++++ app/models/members/group_member.rb | 4 ++++ app/models/members/project_member.rb | 4 ++++ 3 files changed, 12 insertions(+) (limited to 'app') diff --git a/app/models/member.rb b/app/models/member.rb index dc9247bc9a0..b5f75c9bff0 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -332,4 +332,8 @@ class Member < ActiveRecord::Base def notification_service NotificationService.new end + + def notifiable?(type, opts={}) + raise 'abstract' + end end diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index 47040f95533..cf434ec070b 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -30,6 +30,10 @@ class GroupMember < Member 'Group' end + def notifiable?(type, opts={}) + NotificationRecipientService.notifiable?(user, type, { group: group }.merge(opts)) + end + private def send_invite diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb index c0e17f4bfc8..47e47caad82 100644 --- a/app/models/members/project_member.rb +++ b/app/models/members/project_member.rb @@ -87,6 +87,10 @@ class ProjectMember < Member project.owner == user end + def notifiable?(type, opts={}) + NotificationRecipientService.notifiable?(user, type, { project: project }.merge(opts)) + end + private def delete_member_todos -- cgit v1.2.3 From 0268fc2ffc7cdc12a6e1a0bf565fe70ff8541398 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Fri, 4 Aug 2017 11:54:19 -0700 Subject: check notifiability for more emails --- app/services/notification_service.rb | 51 +++++++++++++++++++++++++++++++----- 1 file changed, 44 insertions(+), 7 deletions(-) (limited to 'app') diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index df04b1a4fe3..d92282ac896 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -10,9 +10,11 @@ class NotificationService # only if ssh key is not deploy key # # This is security email so it will be sent - # even if user disabled notifications + # even if user disabled notifications. However, + # it won't be sent to internal users like the + # ghost user or the EE support bot. def new_key(key) - if key.user + if key.user&.can?(:receive_notifications) mailer.new_ssh_key_email(key.id).deliver_later end end @@ -22,14 +24,14 @@ class NotificationService # This is a security email so it will be sent even if the user user disabled # notifications def new_gpg_key(gpg_key) - if gpg_key.user + if gpg_key.user&.can?(:receive_notifications) mailer.new_gpg_key_email(gpg_key.id).deliver_later end end # Always notify user about email added to profile def new_email(email) - if email.user + if email.user&.can?(:receive_notifications) mailer.new_email_email(email.id).deliver_later end end @@ -185,6 +187,8 @@ class NotificationService # Notify new user with email after creation def new_user(user, token = nil) + return true unless notifiable?(user, :mention) + # Don't email omniauth created users mailer.new_user_email(user.id, token).deliver_later unless user.identities.any? end @@ -206,19 +210,27 @@ class NotificationService # Members def new_access_request(member) + return true unless member.notifiable?(:subscription) + mailer.member_access_requested_email(member.real_source_type, member.id).deliver_later end def decline_access_request(member) + return true unless member.notifiable?(:subscription) + mailer.member_access_denied_email(member.real_source_type, member.source_id, member.user_id).deliver_later end # Project invite def invite_project_member(project_member, token) + return true unless project_member.notifiable?(:subscription) + mailer.member_invited_email(project_member.real_source_type, project_member.id, token).deliver_later end def accept_project_invite(project_member) + return true unless project_member.notifiable?(:subscription) + mailer.member_invite_accepted_email(project_member.real_source_type, project_member.id).deliver_later end @@ -232,10 +244,14 @@ class NotificationService end def new_project_member(project_member) + return true unless project_member.notifiable?(:mention) + mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later end def update_project_member(project_member) + return true unless project_member.notifiable?(:mention) + mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later end @@ -249,6 +265,9 @@ class NotificationService end def decline_group_invite(group_member) + # always send this one, since it's a response to the user's own + # action + mailer.member_invite_declined_email( group_member.real_source_type, group_member.group.id, @@ -258,15 +277,19 @@ class NotificationService end def new_group_member(group_member) + return true unless group_member.notifiable?(:mention) + mailer.member_access_granted_email(group_member.real_source_type, group_member.id).deliver_later end def update_group_member(group_member) + return true unless group_member.notifiable?(:mention) + mailer.member_access_granted_email(group_member.real_source_type, group_member.id).deliver_later end def project_was_moved(project, old_path_with_namespace) - recipients = NotificationRecipientService.notifiable_users(project.team.members, :mention, project: project) + recipients = notifiable_users(project.team.members, :mention, project: project) recipients.each do |recipient| mailer.project_was_moved_email( @@ -288,10 +311,14 @@ class NotificationService end def project_exported(project, current_user) + return true unless notifiable?(current_user, :mention, project: project) + mailer.project_was_exported_email(current_user, project).deliver_later end def project_not_exported(project, current_user, errors) + return true unless notifiable?(current_user, :mention, project: project) + mailer.project_was_not_exported_email(current_user, project, errors).deliver_later end @@ -300,7 +327,7 @@ class NotificationService return unless mailer.respond_to?(email_template) - recipients ||= NotificationRecipientService.notifiable_users( + recipients ||= notifiable_users( [pipeline.user], :watch, custom_action: :"#{pipeline.status}_pipeline", target: pipeline @@ -369,7 +396,7 @@ class NotificationService def relabeled_resource_email(target, labels, current_user, method) recipients = labels.flat_map { |l| l.subscribers(target.project) } - recipients = NotificationRecipientService.notifiable_users( + recipients = notifiable_users( recipients, :subscription, target: target, acting_user: current_user @@ -401,4 +428,14 @@ class NotificationService object.previous_changes[attribute].first end end + + private + + def notifiable?(*args) + NotificationRecipientService.notifiable?(*args) + end + + def notifiable_users(*args) + NotificationRecipientService.notifiable_users(*args) + end end -- cgit v1.2.3 From 90dd3fb32c1205ddd31c1c2c89b297e9528e0da8 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Fri, 4 Aug 2017 13:56:33 -0700 Subject: a membership with no user is always notifiable since this is for user invites and the like. --- app/models/member.rb | 12 ++++++++++-- app/models/members/group_member.rb | 4 ++-- app/models/members/project_member.rb | 4 ++-- 3 files changed, 14 insertions(+), 6 deletions(-) (limited to 'app') diff --git a/app/models/member.rb b/app/models/member.rb index b5f75c9bff0..57f85a9adaf 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -276,6 +276,14 @@ class Member < ActiveRecord::Base @notification_setting ||= user.notification_settings_for(source) end + def notifiable?(type, opts={}) + # always notify when there isn't a user yet + return true if user.blank? + + NotificationRecipientService.notifiable?(user, type, notifiable_options.merge(opts)) + end + + private def send_invite @@ -333,7 +341,7 @@ class Member < ActiveRecord::Base NotificationService.new end - def notifiable?(type, opts={}) - raise 'abstract' + def notifiable_options + {} end end diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index cf434ec070b..661e668dbf9 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -30,8 +30,8 @@ class GroupMember < Member 'Group' end - def notifiable?(type, opts={}) - NotificationRecipientService.notifiable?(user, type, { group: group }.merge(opts)) + def notifiable_options + { group: group } end private diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb index 47e47caad82..b6f1dd272cd 100644 --- a/app/models/members/project_member.rb +++ b/app/models/members/project_member.rb @@ -87,8 +87,8 @@ class ProjectMember < Member project.owner == user end - def notifiable?(type, opts={}) - NotificationRecipientService.notifiable?(user, type, { project: project }.merge(opts)) + def notifiable_options + { project: project } end private -- cgit v1.2.3 From 7416fbb398e21cf6931265f7f95f97f5fe73e187 Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Mon, 7 Aug 2017 17:36:35 -0700 Subject: rubocop fix --- app/models/member.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'app') diff --git a/app/models/member.rb b/app/models/member.rb index 57f85a9adaf..b26b5017183 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -276,14 +276,13 @@ class Member < ActiveRecord::Base @notification_setting ||= user.notification_settings_for(source) end - def notifiable?(type, opts={}) + def notifiable?(type, opts = {}) # always notify when there isn't a user yet return true if user.blank? NotificationRecipientService.notifiable?(user, type, notifiable_options.merge(opts)) end - private def send_invite -- cgit v1.2.3 From 38737345abdf91ac2cb0b1cdff4eb7132b4104ee Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Thu, 10 Aug 2017 10:41:25 -0700 Subject: skip the :read_project check for new_project_member since we're just adding them as a member, the permission may still return false. --- app/models/notification_recipient.rb | 7 ++++++- app/services/notification_service.rb | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/models/notification_recipient.rb b/app/models/notification_recipient.rb index 30fab33e5e0..dc862565a71 100644 --- a/app/models/notification_recipient.rb +++ b/app/models/notification_recipient.rb @@ -6,7 +6,8 @@ class NotificationRecipient target: nil, acting_user: nil, project: nil, - group: nil + group: nil, + skip_read_ability: false ) unless NotificationSetting.levels.key?(type) || type == :subscription raise ArgumentError, "invalid type: #{type.inspect}" @@ -19,6 +20,7 @@ class NotificationRecipient @group = group || @project&.group @user = user @type = type + @skip_read_ability = skip_read_ability end def notification_setting @@ -83,6 +85,8 @@ class NotificationRecipient def has_access? DeclarativePolicy.subject_scope do return false unless user.can?(:receive_notifications) + return true if @skip_read_ability + return false if @project && !user.can?(:read_project, @project) return true unless read_ability @@ -102,6 +106,7 @@ class NotificationRecipient private def read_ability + return nil if @skip_read_ability return @read_ability if instance_variable_defined?(:@read_ability) @read_ability = diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index d92282ac896..4267879b03d 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -244,7 +244,7 @@ class NotificationService end def new_project_member(project_member) - return true unless project_member.notifiable?(:mention) + return true unless project_member.notifiable?(:mention, skip_read_ability: true) mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later end -- cgit v1.2.3 From 8f6205d1442cb6d88e992bff7a07c088ae92d93a Mon Sep 17 00:00:00 2001 From: "http://jneen.net/" Date: Thu, 10 Aug 2017 13:13:09 -0700 Subject: don't send devise notifications to the ghost user --- app/models/user.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'app') diff --git a/app/models/user.rb b/app/models/user.rb index 7935b89662b..a4615436245 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1069,6 +1069,7 @@ class User < ActiveRecord::Base # Added according to https://github.com/plataformatec/devise/blob/7df57d5081f9884849ca15e4fde179ef164a575f/README.md#activejob-integration def send_devise_notification(notification, *args) + return true unless can?(:receive_notifications) devise_mailer.send(notification, self, *args).deliver_later end -- cgit v1.2.3 From 640cbb00e30162e11f775d03175e931a4fd1ad70 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 4 Aug 2017 15:32:44 +0100 Subject: Add dynamic navigation tunnel to fly-out menus --- app/assets/javascripts/fly_out_nav.js | 137 +++++++++++++++++++++++++++----- app/assets/stylesheets/new_sidebar.scss | 24 +----- 2 files changed, 120 insertions(+), 41 deletions(-) (limited to 'app') diff --git a/app/assets/javascripts/fly_out_nav.js b/app/assets/javascripts/fly_out_nav.js index 56744a440e7..9012f8b1a14 100644 --- a/app/assets/javascripts/fly_out_nav.js +++ b/app/assets/javascripts/fly_out_nav.js @@ -1,6 +1,19 @@ import Cookies from 'js-cookie'; import bp from './breakpoints'; +const IS_OVER_CLASS = 'is-over'; +const IS_ABOVE_CLASS = 'is-above'; +const IS_SHOWING_FLY_OUT_CLASS = 'is-showing-fly-out'; +let currentOpenMenu = null; +let menuCornerLocs; +let timeoutId; + +export const mousePos = []; + +export const setOpenMenu = (menu) => { currentOpenMenu = menu; }; + +export const slope = (a, b) => (b.y - a.y) / (b.x - a.x); + export const canShowActiveSubItems = (el) => { const isHiddenByMedia = bp.getBreakpointSize() === 'sm' || bp.getBreakpointSize() === 'md'; @@ -10,8 +23,28 @@ export const canShowActiveSubItems = (el) => { return true; }; + export const canShowSubItems = () => bp.getBreakpointSize() === 'sm' || bp.getBreakpointSize() === 'md' || bp.getBreakpointSize() === 'lg'; +export const getHideSubItemsInterval = () => { + if (!currentOpenMenu) return 0; + + const currentMousePos = mousePos[mousePos.length - 1]; + const prevMousePos = mousePos[0]; + const currentMousePosY = currentMousePos.y; + const [menuTop, menuBottom] = menuCornerLocs; + + if (currentMousePosY < menuTop.y || + currentMousePosY > menuBottom.y) return 0; + + if (slope(prevMousePos, menuBottom) < slope(currentMousePos, menuBottom) && + slope(prevMousePos, menuTop) > slope(currentMousePos, menuTop)) { + return 300; + } + + return 0; +}; + export const calculateTop = (boundingRect, outerHeight) => { const windowHeight = window.innerHeight; const bottomOverflow = windowHeight - (boundingRect.top + outerHeight); @@ -20,45 +53,111 @@ export const calculateTop = (boundingRect, outerHeight) => { boundingRect.top; }; -export const showSubLevelItems = (el) => { - const subItems = el.querySelector('.sidebar-sub-level-items'); +export const hideMenu = (el) => { + if (!el) return; - if (!subItems || !canShowSubItems() || !canShowActiveSubItems(el)) return; + const parentEl = el.parentNode; - subItems.style.display = 'block'; - el.classList.add('is-showing-fly-out'); - el.classList.add('is-over'); + el.style.display = ''; // eslint-disable-line no-param-reassign + el.style.transform = ''; // eslint-disable-line no-param-reassign + el.classList.remove(IS_ABOVE_CLASS); + parentEl.classList.remove(IS_OVER_CLASS); + parentEl.classList.remove(IS_SHOWING_FLY_OUT_CLASS); + + setOpenMenu(null); +}; +export const moveSubItemsToPosition = (el, subItems) => { const boundingRect = el.getBoundingClientRect(); const top = calculateTop(boundingRect, subItems.offsetHeight); const isAbove = top < boundingRect.top; subItems.classList.add('fly-out-list'); - subItems.style.transform = `translate3d(0, ${Math.floor(top)}px, 0)`; + subItems.style.transform = `translate3d(0, ${Math.floor(top)}px, 0)`; // eslint-disable-line no-param-reassign + + const subItemsRect = subItems.getBoundingClientRect(); + + menuCornerLocs = [ + { + x: subItemsRect.left, // left position of the sub items + y: subItemsRect.top, // top position of the sub items + }, + { + x: subItemsRect.left, // left position of the sub items + y: subItemsRect.top + subItemsRect.height, // bottom position of the sub items + }, + ]; if (isAbove) { - subItems.classList.add('is-above'); + subItems.classList.add(IS_ABOVE_CLASS); } }; -export const hideSubLevelItems = (el) => { +export const showSubLevelItems = (el) => { + const subItems = el.querySelector('.sidebar-sub-level-items'); + + if (!canShowSubItems() || !canShowActiveSubItems(el)) return; + + el.classList.add(IS_OVER_CLASS); + + if (!subItems) return; + + subItems.style.display = 'block'; + el.classList.add(IS_SHOWING_FLY_OUT_CLASS); + + setOpenMenu(subItems); + moveSubItemsToPosition(el, subItems); +}; + +export const mouseEnterTopItems = (el) => { + clearTimeout(timeoutId); + + timeoutId = setTimeout(() => { + if (currentOpenMenu) hideMenu(currentOpenMenu); + + showSubLevelItems(el); + }, getHideSubItemsInterval()); +}; + +export const mouseLeaveTopItem = (el) => { const subItems = el.querySelector('.sidebar-sub-level-items'); - if (!subItems || !canShowSubItems() || !canShowActiveSubItems(el)) return; + if (!canShowSubItems() || !canShowActiveSubItems(el) || (subItems && subItems === currentOpenMenu)) return; - el.classList.remove('is-showing-fly-out'); - el.classList.remove('is-over'); - subItems.style.display = ''; - subItems.style.transform = ''; - subItems.classList.remove('is-above'); + el.classList.remove(IS_OVER_CLASS); +}; + +export const documentMouseMove = (e) => { + mousePos.push({ x: e.clientX, y: e.clientY }); + + if (mousePos.length > 6) mousePos.shift(); }; export default () => { - const items = [...document.querySelectorAll('.sidebar-top-level-items > li')] - .filter(el => el.querySelector('.sidebar-sub-level-items')); + const sidebar = document.querySelector('.sidebar-top-level-items'); + const items = [...sidebar.querySelectorAll('.sidebar-top-level-items > li')]; + + sidebar.addEventListener('mouseleave', () => { + clearTimeout(timeoutId); + + timeoutId = setTimeout(() => { + if (currentOpenMenu) hideMenu(currentOpenMenu); + }, getHideSubItemsInterval()); + }); items.forEach((el) => { - el.addEventListener('mouseenter', e => showSubLevelItems(e.currentTarget)); - el.addEventListener('mouseleave', e => hideSubLevelItems(e.currentTarget)); + const subItems = el.querySelector('.sidebar-sub-level-items'); + + if (subItems) { + subItems.addEventListener('mouseleave', () => { + clearTimeout(timeoutId); + hideMenu(currentOpenMenu); + }); + } + + el.addEventListener('mouseenter', e => mouseEnterTopItems(e.currentTarget)); + el.addEventListener('mouseleave', e => mouseLeaveTopItem(e.currentTarget)); }); + + document.addEventListener('mousemove', documentMouseMove); }; diff --git a/app/assets/stylesheets/new_sidebar.scss b/app/assets/stylesheets/new_sidebar.scss index d49f23b4f5a..faedd207e01 100644 --- a/app/assets/stylesheets/new_sidebar.scss +++ b/app/assets/stylesheets/new_sidebar.scss @@ -250,32 +250,13 @@ $new-sidebar-collapsed-width: 50px; position: absolute; top: -30px; bottom: -30px; - left: 0; + left: -10px; right: -30px; z-index: -1; } - &::after { - content: ""; - position: absolute; - top: 44px; - left: -30px; - right: 35px; - bottom: 0; - height: 100%; - max-height: 150px; - z-index: -1; - transform: skew(33deg); - } - &.is-above { margin-top: 1px; - - &::after { - top: auto; - bottom: 44px; - transform: skew(-30deg); - } } > .active { @@ -322,8 +303,7 @@ $new-sidebar-collapsed-width: 50px; } } - &:not(.active):hover > a, - > a:hover, + &.active > a:hover, &.is-over > a { background-color: $white-light; } -- cgit v1.2.3 From 80c788bbeb875e724230a6ce64f09f98bbfe6f40 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 8 Aug 2017 11:25:36 +0100 Subject: fixed JS error when no sidebar exists --- app/assets/javascripts/fly_out_nav.js | 3 +++ 1 file changed, 3 insertions(+) (limited to 'app') diff --git a/app/assets/javascripts/fly_out_nav.js b/app/assets/javascripts/fly_out_nav.js index 9012f8b1a14..9a2ab7f82b1 100644 --- a/app/assets/javascripts/fly_out_nav.js +++ b/app/assets/javascripts/fly_out_nav.js @@ -135,6 +135,9 @@ export const documentMouseMove = (e) => { export default () => { const sidebar = document.querySelector('.sidebar-top-level-items'); + + if (!sidebar) return; + const items = [...sidebar.querySelectorAll('.sidebar-top-level-items > li')]; sidebar.addEventListener('mouseleave', () => { -- cgit v1.2.3 From 449a84fe4063408b432d7bd59a399747a570fff1 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 10 Aug 2017 13:24:03 +0100 Subject: fixed up specs caused by left over elements in the body --- app/assets/javascripts/fly_out_nav.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'app') diff --git a/app/assets/javascripts/fly_out_nav.js b/app/assets/javascripts/fly_out_nav.js index 9a2ab7f82b1..7d128cbf5fb 100644 --- a/app/assets/javascripts/fly_out_nav.js +++ b/app/assets/javascripts/fly_out_nav.js @@ -122,7 +122,8 @@ export const mouseEnterTopItems = (el) => { export const mouseLeaveTopItem = (el) => { const subItems = el.querySelector('.sidebar-sub-level-items'); - if (!canShowSubItems() || !canShowActiveSubItems(el) || (subItems && subItems === currentOpenMenu)) return; + if (!canShowSubItems() || !canShowActiveSubItems(el) || + (subItems && subItems === currentOpenMenu)) return; el.classList.remove(IS_OVER_CLASS); }; -- cgit v1.2.3 From 56d114921caef8588891fa2090b00823ddc4ce9f Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 14 Aug 2017 08:58:22 +0100 Subject: moved timeout to a variable --- app/assets/javascripts/fly_out_nav.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'app') diff --git a/app/assets/javascripts/fly_out_nav.js b/app/assets/javascripts/fly_out_nav.js index 7d128cbf5fb..cbc3ad23990 100644 --- a/app/assets/javascripts/fly_out_nav.js +++ b/app/assets/javascripts/fly_out_nav.js @@ -1,6 +1,7 @@ import Cookies from 'js-cookie'; import bp from './breakpoints'; +const HIDE_INTERVAL_TIMEOUT = 300; const IS_OVER_CLASS = 'is-over'; const IS_ABOVE_CLASS = 'is-above'; const IS_SHOWING_FLY_OUT_CLASS = 'is-showing-fly-out'; @@ -10,7 +11,7 @@ let timeoutId; export const mousePos = []; -export const setOpenMenu = (menu) => { currentOpenMenu = menu; }; +export const setOpenMenu = (menu = null) => { currentOpenMenu = menu; }; export const slope = (a, b) => (b.y - a.y) / (b.x - a.x); @@ -39,7 +40,7 @@ export const getHideSubItemsInterval = () => { if (slope(prevMousePos, menuBottom) < slope(currentMousePos, menuBottom) && slope(prevMousePos, menuTop) > slope(currentMousePos, menuTop)) { - return 300; + return HIDE_INTERVAL_TIMEOUT; } return 0; @@ -64,7 +65,7 @@ export const hideMenu = (el) => { parentEl.classList.remove(IS_OVER_CLASS); parentEl.classList.remove(IS_SHOWING_FLY_OUT_CLASS); - setOpenMenu(null); + setOpenMenu(); }; export const moveSubItemsToPosition = (el, subItems) => { @@ -129,7 +130,10 @@ export const mouseLeaveTopItem = (el) => { }; export const documentMouseMove = (e) => { - mousePos.push({ x: e.clientX, y: e.clientY }); + mousePos.push({ + x: e.clientX, + y: e.clientY, + }); if (mousePos.length > 6) mousePos.shift(); }; -- cgit v1.2.3 From 4d97843f3cfe94a3061bcb03aa0da6876b026bea Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 14 Aug 2017 13:35:28 +0200 Subject: Remove `\n` from translations They seem to confuse some translation tools and aren't rendered in HTML anyway. --- app/helpers/groups_helper.rb | 2 +- app/helpers/projects_helper.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/helpers/groups_helper.rb b/app/helpers/groups_helper.rb index 8cd61f738e1..4123a96911f 100644 --- a/app/helpers/groups_helper.rb +++ b/app/helpers/groups_helper.rb @@ -59,7 +59,7 @@ module GroupsHelper end def remove_group_message(group) - _("You are going to remove %{group_name}.\nRemoved groups CANNOT be restored!\nAre you ABSOLUTELY sure?") % + _("You are going to remove %{group_name}. Removed groups CANNOT be restored! Are you ABSOLUTELY sure?") % { group_name: group.name } end diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index a268413e84f..09cfd06dad3 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -80,7 +80,7 @@ module ProjectsHelper end def remove_project_message(project) - _("You are going to remove %{project_name_with_namespace}.\nRemoved project CANNOT be restored!\nAre you ABSOLUTELY sure?") % + _("You are going to remove %{project_name_with_namespace}. Removed project CANNOT be restored! Are you ABSOLUTELY sure?") % { project_name_with_namespace: project.name_with_namespace } end -- cgit v1.2.3 From d7b03c37f8346e29f21f0196fd3a532effa60ec3 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Fri, 11 Aug 2017 15:19:11 +0100 Subject: Speed up Group#user_ids_for_project_authorizations --- app/mailers/emails/members.rb | 4 ++-- app/models/group.rb | 30 ++++++++++++++++++++++++------ app/models/member.rb | 15 +++++++++++++-- app/models/namespace.rb | 14 ++++++++++++++ 4 files changed, 53 insertions(+), 10 deletions(-) (limited to 'app') diff --git a/app/mailers/emails/members.rb b/app/mailers/emails/members.rb index 7b617b359ea..d76c61c369f 100644 --- a/app/mailers/emails/members.rb +++ b/app/mailers/emails/members.rb @@ -11,11 +11,11 @@ module Emails @member_source_type = member_source_type @member_id = member_id - admins = member_source.members.owners_and_masters.includes(:user).pluck(:notification_email) + admins = member_source.members.owners_and_masters.pluck(:notification_email) # A project in a group can have no explicit owners/masters, in that case # we fallbacks to the group's owners/masters. if admins.empty? && member_source.respond_to?(:group) && member_source.group - admins = member_source.group.members.owners_and_masters.includes(:user).pluck(:notification_email) + admins = member_source.group.members.owners_and_masters.pluck(:notification_email) end mail(to: admins, diff --git a/app/models/group.rb b/app/models/group.rb index bd5735ed82e..2816a68257c 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -212,21 +212,39 @@ class Group < Namespace end def user_ids_for_project_authorizations - users_with_parents.pluck(:id) + members_with_parents.pluck(:user_id) end def members_with_parents - GroupMember.active.where(source_id: ancestors.pluck(:id).push(id)).where.not(user_id: nil) + # Avoids an unnecessary SELECT when the group has no parents + source_ids = + if parent_id + self_and_ancestors.reorder(nil).select(:id) + else + id + end + + GroupMember + .active_without_invites + .where(source_id: source_ids) + end + + def members_with_descendants + GroupMember + .active_without_invites + .where(source_id: self_and_descendants.reorder(nil).select(:id)) end def users_with_parents - User.where(id: members_with_parents.select(:user_id)) + User + .where(id: members_with_parents.select(:user_id)) + .reorder(nil) end def users_with_descendants - members_with_descendants = GroupMember.non_request.where(source_id: descendants.pluck(:id).push(id)) - - User.where(id: members_with_descendants.select(:user_id)) + User + .where(id: members_with_descendants.select(:user_id)) + .reorder(nil) end def max_member_access_for_user(user) diff --git a/app/models/member.rb b/app/models/member.rb index dc9247bc9a0..17e343c84d8 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -41,9 +41,20 @@ class Member < ActiveRecord::Base is_external_invite = arel_table[:user_id].eq(nil).and(arel_table[:invite_token].not_eq(nil)) user_is_active = User.arel_table[:state].eq(:active) - includes(:user).references(:users) - .where(is_external_invite.or(user_is_active)) + user_ok = Arel::Nodes::Grouping.new(is_external_invite).or(user_is_active) + + left_join_users + .where(user_ok) + .where(requested_at: nil) + .reorder(nil) + end + + # Like active, but without invites. For when a User is required. + scope :active_without_invites, -> do + left_join_users + .where(users: { state: 'active' }) .where(requested_at: nil) + .reorder(nil) end scope :invite, -> { where.not(invite_token: nil) } diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 6073fb94a3f..e7bc1d1b080 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -156,6 +156,14 @@ class Namespace < ActiveRecord::Base .base_and_ancestors end + def self_and_ancestors + return self.class.where(id: id) unless parent_id + + Gitlab::GroupHierarchy + .new(self.class.where(id: id)) + .base_and_ancestors + end + # Returns all the descendants of the current namespace. def descendants Gitlab::GroupHierarchy @@ -163,6 +171,12 @@ class Namespace < ActiveRecord::Base .base_and_descendants end + def self_and_descendants + Gitlab::GroupHierarchy + .new(self.class.where(id: id)) + .base_and_descendants + end + def user_ids_for_project_authorizations [owner_id] end -- cgit v1.2.3 From c1f9403e45e636651010929b6113add34f8e6a8a Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 10 Aug 2017 15:01:38 +0200 Subject: Use Prev/Next pagination for exploring projects This changes the pagination of the "Explore" pages so they use a simpler pagination system that only shows "Prev" and "Next" buttons. This removes the need for getting the total number of rows to display, a process that can easily take up to 2 seconds when browsing through a large list of projects. Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/27390 --- app/controllers/explore/projects_controller.rb | 11 +++++++---- app/helpers/pagination_helper.rb | 21 +++++++++++++++++++++ app/views/kaminari/gitlab/_without_count.html.haml | 8 ++++++++ app/views/shared/projects/_list.html.haml | 2 +- 4 files changed, 37 insertions(+), 5 deletions(-) create mode 100644 app/helpers/pagination_helper.rb create mode 100644 app/views/kaminari/gitlab/_without_count.html.haml (limited to 'app') diff --git a/app/controllers/explore/projects_controller.rb b/app/controllers/explore/projects_controller.rb index 741879dee35..762c6ebf3a3 100644 --- a/app/controllers/explore/projects_controller.rb +++ b/app/controllers/explore/projects_controller.rb @@ -6,7 +6,7 @@ class Explore::ProjectsController < Explore::ApplicationController def index params[:sort] ||= 'latest_activity_desc' @sort = params[:sort] - @projects = load_projects.page(params[:page]) + @projects = load_projects respond_to do |format| format.html @@ -21,7 +21,7 @@ class Explore::ProjectsController < Explore::ApplicationController def trending params[:trending] = true @sort = params[:sort] - @projects = load_projects.page(params[:page]) + @projects = load_projects respond_to do |format| format.html @@ -34,7 +34,7 @@ class Explore::ProjectsController < Explore::ApplicationController end def starred - @projects = load_projects.reorder('star_count DESC').page(params[:page]) + @projects = load_projects.reorder('star_count DESC') respond_to do |format| format.html @@ -50,6 +50,9 @@ class Explore::ProjectsController < Explore::ApplicationController def load_projects ProjectsFinder.new(current_user: current_user, params: params) - .execute.includes(:route, namespace: :route) + .execute + .includes(:route, namespace: :route) + .page(params[:page]) + .without_count end end diff --git a/app/helpers/pagination_helper.rb b/app/helpers/pagination_helper.rb new file mode 100644 index 00000000000..83dd76a01dd --- /dev/null +++ b/app/helpers/pagination_helper.rb @@ -0,0 +1,21 @@ +module PaginationHelper + def paginate_collection(collection, remote: nil) + if collection.is_a?(Kaminari::PaginatableWithoutCount) + paginate_without_count(collection) + elsif collection.respond_to?(:total_pages) + paginate_with_count(collection, remote: remote) + end + end + + def paginate_without_count(collection) + render( + 'kaminari/gitlab/without_count', + previous_path: path_to_prev_page(collection), + next_path: path_to_next_page(collection) + ) + end + + def paginate_with_count(collection, remote: nil) + paginate(collection, remote: remote, theme: 'gitlab') + end +end diff --git a/app/views/kaminari/gitlab/_without_count.html.haml b/app/views/kaminari/gitlab/_without_count.html.haml new file mode 100644 index 00000000000..250029c4475 --- /dev/null +++ b/app/views/kaminari/gitlab/_without_count.html.haml @@ -0,0 +1,8 @@ +.gl-pagination + %ul.pagination.clearfix + - if previous_path + %li.prev + = link_to(t('views.pagination.previous'), previous_path, rel: 'prev') + - if next_path + %li.next + = link_to(t('views.pagination.next'), next_path, rel: 'next') diff --git a/app/views/shared/projects/_list.html.haml b/app/views/shared/projects/_list.html.haml index 914506bf0ce..0bedfea3502 100644 --- a/app/views/shared/projects/_list.html.haml +++ b/app/views/shared/projects/_list.html.haml @@ -23,6 +23,6 @@ = icon('lock fw', base: 'circle', class: 'fa-lg private-fork-icon') %strong= pluralize(@private_forks_count, 'private fork') %span  you have no access to. - = paginate(projects, remote: remote, theme: "gitlab") if projects.respond_to? :total_pages + = paginate_collection(projects, remote: remote) - else .nothing-here-block No projects found -- cgit v1.2.3 From f01e34df3ffc4e38c51b23c0c62b04d25c7b3696 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Mon, 14 Aug 2017 14:52:04 +0100 Subject: Stops propagation for dropdown content in pipeline graph. Applies the same behavior of the mini graph --- .../components/graph/dropdown_job_component.vue | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) (limited to 'app') diff --git a/app/assets/javascripts/pipelines/components/graph/dropdown_job_component.vue b/app/assets/javascripts/pipelines/components/graph/dropdown_job_component.vue index 2944689a5a7..7695b04db74 100644 --- a/app/assets/javascripts/pipelines/components/graph/dropdown_job_component.vue +++ b/app/assets/javascripts/pipelines/components/graph/dropdown_job_component.vue @@ -48,6 +48,27 @@ return `${this.job.name} - ${this.job.status.label}`; }, }, + + methods: { + /** + * When the user right clicks or cmd/ctrl + click in the job name + * the dropdown should not be closed and the link should open in another tab, + * so we stop propagation of the click event inside the dropdown. + * + * Since this component is rendered multiple times per page we need to guarantee we only + * target the click event of this component. + */ + stopDropdownClickPropagation() { + $(this.$el.querySelectorAll('.js-grouped-pipeline-dropdown a.mini-pipeline-graph-dropdown-item')) + .on('click', (e) => { + e.stopPropagation(); + }); + }, + }, + + mounted() { + this.stopDropdownClickPropagation(); + }, }; \ No newline at end of file + -- cgit v1.2.3 From 06c330954e030e30e9e8284110907c53b206e447 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 10 Aug 2017 13:09:01 -0500 Subject: Use v-else instead of duplicating logic Fix http://192.168.1.135:3000/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/broadcast_message.js --- .../javascripts/repo/components/repo_preview.vue | 20 +++++++++++++++++--- app/assets/javascripts/repo/helpers/repo_helper.js | 2 +- 2 files changed, 18 insertions(+), 4 deletions(-) (limited to 'app') diff --git a/app/assets/javascripts/repo/components/repo_preview.vue b/app/assets/javascripts/repo/components/repo_preview.vue index 0caa3a4551a..2200754cbef 100644 --- a/app/assets/javascripts/repo/components/repo_preview.vue +++ b/app/assets/javascripts/repo/components/repo_preview.vue @@ -30,9 +30,23 @@ export default { diff --git a/app/assets/javascripts/repo/helpers/repo_helper.js b/app/assets/javascripts/repo/helpers/repo_helper.js index fee98c12592..b12ea92c17a 100644 --- a/app/assets/javascripts/repo/helpers/repo_helper.js +++ b/app/assets/javascripts/repo/helpers/repo_helper.js @@ -190,7 +190,7 @@ const RepoHelper = { newFile.url = file.url || location.pathname; newFile.url = file.url; - if (newFile.render_error === 'too_large') { + if (newFile.render_error === 'too_large' || newFile.render_error === 'collapsed') { newFile.tooLarge = true; } newFile.newContent = ''; -- cgit v1.2.3 From aef9f1eb9405e9bab92b15f5c99bf06eaf28a5d6 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 14 Aug 2017 15:22:09 +0200 Subject: Cache the number of forks of a project The number of forks of a project doesn't change very frequently and running a COUNT(*) every time this information is requested can be quite expensive. We also end up running such a COUNT(*) query at least twice on the homepage of a project. By caching this data and refreshing it when necessary we can reduce project homepage loading times by around 60 milliseconds (based on the timings of https://gitlab.com/gitlab-org/gitlab-ce). --- app/models/project.rb | 5 ++++- app/services/projects/destroy_service.rb | 2 ++ app/services/projects/fork_service.rb | 6 ++++++ app/services/projects/forks_count_service.rb | 30 ++++++++++++++++++++++++++++ app/services/projects/unlink_fork_service.rb | 6 ++++++ 5 files changed, 48 insertions(+), 1 deletion(-) create mode 100644 app/services/projects/forks_count_service.rb (limited to 'app') diff --git a/app/models/project.rb b/app/models/project.rb index 7010664e1c8..92ca126bafc 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -196,7 +196,6 @@ class Project < ActiveRecord::Base accepts_nested_attributes_for :import_data delegate :name, to: :owner, allow_nil: true, prefix: true - delegate :count, to: :forks, prefix: true delegate :members, to: :team, prefix: true delegate :add_user, :add_users, to: :team delegate :add_guest, :add_reporter, :add_developer, :add_master, to: :team @@ -1398,6 +1397,10 @@ class Project < ActiveRecord::Base # @deprecated cannot remove yet because it has an index with its name in elasticsearch alias_method :path_with_namespace, :full_path + def forks_count + Projects::ForksCountService.new(self).count + end + private def cross_namespace_reference?(from) diff --git a/app/services/projects/destroy_service.rb b/app/services/projects/destroy_service.rb index 11ad4838471..54eb75ab9bf 100644 --- a/app/services/projects/destroy_service.rb +++ b/app/services/projects/destroy_service.rb @@ -128,6 +128,8 @@ module Projects project.repository.before_delete Repository.new(wiki_path, project, disk_path: repo_path).before_delete + + Projects::ForksCountService.new(project).delete_cache end end end diff --git a/app/services/projects/fork_service.rb b/app/services/projects/fork_service.rb index a2b23ea6171..ad67e68a86a 100644 --- a/app/services/projects/fork_service.rb +++ b/app/services/projects/fork_service.rb @@ -21,11 +21,17 @@ module Projects builds_access_level = @project.project_feature.builds_access_level new_project.project_feature.update_attributes(builds_access_level: builds_access_level) + refresh_forks_count + new_project end private + def refresh_forks_count + Projects::ForksCountService.new(@project).refresh_cache + end + def allowed_visibility_level project_level = @project.visibility_level diff --git a/app/services/projects/forks_count_service.rb b/app/services/projects/forks_count_service.rb new file mode 100644 index 00000000000..e2e2b1da91d --- /dev/null +++ b/app/services/projects/forks_count_service.rb @@ -0,0 +1,30 @@ +module Projects + # Service class for getting and caching the number of forks of a project. + class ForksCountService + def initialize(project) + @project = project + end + + def count + Rails.cache.fetch(cache_key) { uncached_count } + end + + def refresh_cache + Rails.cache.write(cache_key, uncached_count) + end + + def delete_cache + Rails.cache.delete(cache_key) + end + + private + + def uncached_count + @project.forks.count + end + + def cache_key + ['projects', @project.id, 'forks_count'] + end + end +end diff --git a/app/services/projects/unlink_fork_service.rb b/app/services/projects/unlink_fork_service.rb index f385e426827..f30b40423c8 100644 --- a/app/services/projects/unlink_fork_service.rb +++ b/app/services/projects/unlink_fork_service.rb @@ -13,7 +13,13 @@ module Projects ::MergeRequests::CloseService.new(@project, @current_user).execute(mr) end + refresh_forks_count(@project.forked_from_project) + @project.forked_project_link.destroy end + + def refresh_forks_count(project) + Projects::ForksCountService.new(project).refresh_cache + end end end -- cgit v1.2.3 From f1e1113bf4d107c0ecf3f989f6110b00a83cef2d Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 10 Aug 2017 13:50:59 -0500 Subject: Split out linkClicked and add tests Fix https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12198#note_37143174 --- .../javascripts/repo/components/repo_sidebar.vue | 45 ++++++++++------------ 1 file changed, 21 insertions(+), 24 deletions(-) (limited to 'app') diff --git a/app/assets/javascripts/repo/components/repo_sidebar.vue b/app/assets/javascripts/repo/components/repo_sidebar.vue index d6d832efc49..ccc84c4ed7c 100644 --- a/app/assets/javascripts/repo/components/repo_sidebar.vue +++ b/app/assets/javascripts/repo/components/repo_sidebar.vue @@ -33,32 +33,29 @@ const RepoSidebar = { }); }, - linkClicked(clickedFile) { - let url = ''; + fileClicked(clickedFile) { let file = clickedFile; - if (typeof file === 'object') { - file.loading = true; - if (file.type === 'tree' && file.opened) { - file = Store.removeChildFilesOfTree(file); + + file.loading = true; + if (file.type === 'tree' && file.opened) { + file = Store.removeChildFilesOfTree(file); + file.loading = false; + } else { + Service.url = file.url; + // I need to refactor this to do the `then` here. + // Not a callback. For now this is good enough. + // it works. + Helper.getContent(file, () => { file.loading = false; - } else { - url = file.url; - Service.url = url; - // I need to refactor this to do the `then` here. - // Not a callback. For now this is good enough. - // it works. - Helper.getContent(file, () => { - file.loading = false; - Helper.scrollTabsRight(); - }); - } - } else if (typeof file === 'string') { - // go back - url = file; - Service.url = url; - Helper.getContent(null, () => Helper.scrollTabsRight()); + Helper.scrollTabsRight(); + }); } }, + + goToPreviousDirectoryClicked(prevURL) { + Service.url = prevURL; + Helper.getContent(null, () => Helper.scrollTabsRight()); + }, }, }; @@ -82,7 +79,7 @@ export default RepoSidebar; + @linkclicked="goToPreviousDirectoryClicked(prevURL)"/> -- cgit v1.2.3 From ecb7c534f60f89a57468148f543a6895692b6081 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 10 Aug 2017 14:01:32 -0500 Subject: Use promise syntax with Helper.getContent Fix https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12198#note_37143217 --- app/assets/javascripts/repo/components/repo_sidebar.vue | 17 +++++++++-------- app/assets/javascripts/repo/helpers/repo_helper.js | 3 +-- 2 files changed, 10 insertions(+), 10 deletions(-) (limited to 'app') diff --git a/app/assets/javascripts/repo/components/repo_sidebar.vue b/app/assets/javascripts/repo/components/repo_sidebar.vue index ccc84c4ed7c..0d4f8c6635e 100644 --- a/app/assets/javascripts/repo/components/repo_sidebar.vue +++ b/app/assets/javascripts/repo/components/repo_sidebar.vue @@ -42,19 +42,20 @@ const RepoSidebar = { file.loading = false; } else { Service.url = file.url; - // I need to refactor this to do the `then` here. - // Not a callback. For now this is good enough. - // it works. - Helper.getContent(file, () => { - file.loading = false; - Helper.scrollTabsRight(); - }); + Helper.getContent(file) + .then(() => { + file.loading = false; + Helper.scrollTabsRight(); + }) + .catch(Helper.loadingError); } }, goToPreviousDirectoryClicked(prevURL) { Service.url = prevURL; - Helper.getContent(null, () => Helper.scrollTabsRight()); + Helper.getContent(null) + .then(() => Helper.scrollTabsRight()) + .catch(Helper.loadingError); }, }, }; diff --git a/app/assets/javascripts/repo/helpers/repo_helper.js b/app/assets/javascripts/repo/helpers/repo_helper.js index b12ea92c17a..308ede5fba0 100644 --- a/app/assets/javascripts/repo/helpers/repo_helper.js +++ b/app/assets/javascripts/repo/helpers/repo_helper.js @@ -135,14 +135,13 @@ const RepoHelper = { return isRoot; }, - getContent(treeOrFile, cb) { + getContent(treeOrFile) { let file = treeOrFile; // const loadingData = RepoHelper.setLoading(true); return Service.getContent() .then((response) => { const data = response.data; // RepoHelper.setLoading(false, loadingData); - if (cb) cb(); Store.isTree = RepoHelper.isTree(data); if (!Store.isTree) { if (!file) file = data; -- cgit v1.2.3 From a081a60d89af1f05a8f6f243e073a96e35b2b349 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 10 Aug 2017 14:17:38 -0500 Subject: Make close/changed icon more accessible Fix https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12198#note_37143527 --- app/assets/javascripts/repo/components/repo_tab.vue | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) (limited to 'app') diff --git a/app/assets/javascripts/repo/components/repo_tab.vue b/app/assets/javascripts/repo/components/repo_tab.vue index 712d64c236f..b6a9948f487 100644 --- a/app/assets/javascripts/repo/components/repo_tab.vue +++ b/app/assets/javascripts/repo/components/repo_tab.vue @@ -10,6 +10,12 @@ const RepoTab = { }, computed: { + closeLabel() { + if (this.tab.changed) { + return `${this.tab.name} changed`; + } + return `Close ${this.tab.name}`; + }, changedClass() { const tabChangedObj = { 'fa-times': !this.tab.changed, @@ -34,8 +40,17 @@ export default RepoTab;