diff options
36 files changed, 402 insertions, 171 deletions
@@ -407,4 +407,4 @@ gem 'flipper-active_record', '~> 0.10.2' # Structured logging gem 'lograge', '~> 0.5' -gem 'grape_logging', '~> 1.6' +gem 'grape_logging', '~> 1.7' diff --git a/Gemfile.lock b/Gemfile.lock index d7e1c7581d5..bcbe6b4f394 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -355,7 +355,7 @@ GEM activesupport grape (>= 0.16.0) rake - grape_logging (1.6.0) + grape_logging (1.7.0) grape grpc (1.4.5) google-protobuf (~> 3.1) @@ -1037,7 +1037,7 @@ DEPENDENCIES grape (~> 1.0) grape-entity (~> 0.6.0) grape-route-helpers (~> 2.1.0) - grape_logging (~> 1.6) + grape_logging (~> 1.7) haml_lint (~> 0.26.0) hamlit (~> 2.6.1) hashie-forbidden_attributes diff --git a/app/assets/javascripts/issue_show/components/app.vue b/app/assets/javascripts/issue_show/components/app.vue index e115ee40219..06f6ec241f4 100644 --- a/app/assets/javascripts/issue_show/components/app.vue +++ b/app/assets/javascripts/issue_show/components/app.vue @@ -72,10 +72,6 @@ export default { required: false, default: () => [], }, - isConfidential: { - type: Boolean, - required: true, - }, markdownPreviewPath: { type: String, required: true, @@ -131,7 +127,6 @@ export default { this.showForm = true; this.store.setFormState({ title: this.state.titleText, - confidential: this.isConfidential, description: this.state.descriptionText, lockedWarningVisible: false, updateLoading: false, @@ -147,8 +142,6 @@ export default { .then((data) => { if (location.pathname !== data.web_url) { gl.utils.visitUrl(data.web_url); - } else if (data.confidential !== this.isConfidential) { - gl.utils.visitUrl(location.pathname); } return this.service.getData(); diff --git a/app/assets/javascripts/issue_show/components/fields/confidential_checkbox.vue b/app/assets/javascripts/issue_show/components/fields/confidential_checkbox.vue deleted file mode 100644 index a0ff08e9111..00000000000 --- a/app/assets/javascripts/issue_show/components/fields/confidential_checkbox.vue +++ /dev/null @@ -1,23 +0,0 @@ -<script> - export default { - props: { - formState: { - type: Object, - required: true, - }, - }, - }; -</script> - -<template> - <fieldset class="checkbox"> - <label for="issue-confidential"> - <input - type="checkbox" - value="1" - id="issue-confidential" - v-model="formState.confidential" /> - This issue is confidential and should only be visible to team members with at least Reporter access. - </label> - </fieldset> -</template> diff --git a/app/assets/javascripts/issue_show/components/form.vue b/app/assets/javascripts/issue_show/components/form.vue index 6a2dd502fe2..28bf6c67ea5 100644 --- a/app/assets/javascripts/issue_show/components/form.vue +++ b/app/assets/javascripts/issue_show/components/form.vue @@ -4,7 +4,6 @@ import descriptionField from './fields/description.vue'; import editActions from './edit_actions.vue'; import descriptionTemplate from './fields/description_template.vue'; - import confidentialCheckbox from './fields/confidential_checkbox.vue'; export default { props: { @@ -44,7 +43,6 @@ descriptionField, descriptionTemplate, editActions, - confidentialCheckbox, }, computed: { hasIssuableTemplates() { @@ -81,8 +79,6 @@ :form-state="formState" :markdown-preview-path="markdownPreviewPath" :markdown-docs-path="markdownDocsPath" /> - <confidential-checkbox - :form-state="formState" /> <edit-actions :form-state="formState" :can-destroy="canDestroy" /> diff --git a/app/assets/javascripts/issue_show/index.js b/app/assets/javascripts/issue_show/index.js index 8053ef57e6c..aca9dec2a96 100644 --- a/app/assets/javascripts/issue_show/index.js +++ b/app/assets/javascripts/issue_show/index.js @@ -35,7 +35,6 @@ document.addEventListener('DOMContentLoaded', () => { initialDescriptionHtml: this.initialDescriptionHtml, initialDescriptionText: this.initialDescriptionText, issuableTemplates: this.issuableTemplates, - isConfidential: this.isConfidential, markdownPreviewPath: this.markdownPreviewPath, markdownDocsPath: this.markdownDocsPath, projectPath: this.projectPath, diff --git a/app/assets/javascripts/issue_show/stores/index.js b/app/assets/javascripts/issue_show/stores/index.js index f4639e9ed2a..af8b0414266 100644 --- a/app/assets/javascripts/issue_show/stores/index.js +++ b/app/assets/javascripts/issue_show/stores/index.js @@ -3,7 +3,6 @@ export default class Store { this.state = initialState; this.formState = { title: '', - confidential: false, description: '', lockedWarningVisible: false, updateLoading: false, diff --git a/app/assets/javascripts/new_sidebar.js b/app/assets/javascripts/new_sidebar.js index fbe474f2f61..cea4f35096a 100644 --- a/app/assets/javascripts/new_sidebar.js +++ b/app/assets/javascripts/new_sidebar.js @@ -68,7 +68,7 @@ export default class NewNavSidebar { if (breakpoint === 'sm' || breakpoint === 'md') { this.toggleCollapsedSidebar(true); } else if (breakpoint === 'lg') { - const collapse = this.$sidebar.hasClass('sidebar-icons-only'); + const collapse = Cookies.get('sidebar_collapsed') === 'true'; this.toggleCollapsedSidebar(collapse); } } diff --git a/app/assets/stylesheets/framework/files.scss b/app/assets/stylesheets/framework/files.scss index 8ad082f7a65..588ec1ff3bc 100644 --- a/app/assets/stylesheets/framework/files.scss +++ b/app/assets/stylesheets/framework/files.scss @@ -17,8 +17,11 @@ max-width: $limited-layout-width-sm; margin-left: auto; margin-right: auto; - padding-top: 64px; - padding-bottom: 64px; + + @media (min-width: $screen-md-min) { + padding-top: 64px; + padding-bottom: 64px; + } } } diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index a7acaf6c728..54c3c0173ae 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -608,7 +608,7 @@ + .files, + .alert { - margin-top: 30px; + margin-top: 32px; } } } diff --git a/app/helpers/issuables_helper.rb b/app/helpers/issuables_helper.rb index 66e1e607e01..df390dd5aab 100644 --- a/app/helpers/issuables_helper.rb +++ b/app/helpers/issuables_helper.rb @@ -213,7 +213,6 @@ module IssuablesHelper canUpdate: can?(current_user, :update_issue, issuable), canDestroy: can?(current_user, :destroy_issue, issuable), issuableRef: issuable.to_reference, - isConfidential: issuable.confidential, markdownPreviewPath: preview_markdown_path(@project), markdownDocsPath: help_page_path('user/markdown'), issuableTemplates: issuable_templates(issuable), diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index c0114dd0256..0c8cb9ba235 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -137,15 +137,7 @@ module ProjectsHelper end def last_push_event - return unless current_user - return current_user.recent_push unless @project - - project_ids = [@project.id] - if fork = current_user.fork_of(@project) - project_ids << fork.id - end - - current_user.recent_push(project_ids) + current_user&.recent_push(@project) end def project_feature_access_select(field) diff --git a/app/models/event.rb b/app/models/event.rb index c313bbb66f8..8e9490b66f4 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -49,7 +49,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 + has_one :push_event_payload # Callbacks after_create :reset_project_activity diff --git a/app/models/gpg_signature.rb b/app/models/gpg_signature.rb index 454c90d5fc4..1f047a32c84 100644 --- a/app/models/gpg_signature.rb +++ b/app/models/gpg_signature.rb @@ -1,8 +1,5 @@ class GpgSignature < ActiveRecord::Base include ShaAttribute - include IgnorableColumn - - ignore_column :valid_signature sha_attribute :commit_sha sha_attribute :gpg_key_primary_keyid diff --git a/app/models/push_event.rb b/app/models/push_event.rb index 23ffb0d4ea8..708513c7861 100644 --- a/app/models/push_event.rb +++ b/app/models/push_event.rb @@ -30,6 +30,44 @@ class PushEvent < Event delegate :commit_count, to: :push_event_payload alias_method :commits_count, :commit_count + # Returns events of pushes that either pushed to an existing ref or created a + # new one. + def self.created_or_pushed + actions = [ + PushEventPayload.actions[:pushed], + PushEventPayload.actions[:created] + ] + + joins(:push_event_payload) + .where(push_event_payloads: { action: actions }) + end + + # Returns events of pushes to a branch. + def self.branch_events + ref_type = PushEventPayload.ref_types[:branch] + + joins(:push_event_payload) + .where(push_event_payloads: { ref_type: ref_type }) + end + + # Returns PushEvent instances for which no merge requests have been created. + def self.without_existing_merge_requests + existing_mrs = MergeRequest.except(:order) + .select(1) + .where('merge_requests.source_project_id = events.project_id') + .where('merge_requests.source_branch = push_event_payloads.ref') + + # For reasons unknown the use of #eager_load will result in the + # "push_event_payload" association not being set. Because of this we're + # using "joins" here, which does mean an additional query needs to be + # executed in order to retrieve the "push_event_association" when the + # returned PushEvent is used. + joins(:push_event_payload) + .where('NOT EXISTS (?)', existing_mrs) + .created_or_pushed + .branch_events + end + def self.sti_name PUSHED end diff --git a/app/models/user.rb b/app/models/user.rb index 26b14ade5ca..358b04ac71f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -650,20 +650,13 @@ class User < ActiveRecord::Base @personal_projects_count ||= personal_projects.count end - def recent_push(project_ids = nil) - # Get push events not earlier than 2 hours ago - events = recent_events.code_push.where("created_at > ?", Time.now - 2.hours) - events = events.where(project_id: project_ids) if project_ids + def recent_push(project = nil) + service = Users::LastPushEventService.new(self) - # Use the latest event that has not been pushed or merged recently - events.includes(:project).recent.find do |event| - next unless event.project.repository.branch_exists?(event.branch_name) - - merge_requests = MergeRequest.where("created_at >= ?", event.created_at) - .where(source_project_id: event.project.id, - source_branch: event.branch_name) - - merge_requests.empty? + if project + service.last_event_for_project(project) + else + service.last_event_for_user end end diff --git a/app/services/event_create_service.rb b/app/services/event_create_service.rb index 0b7e4f187f7..6328d567a07 100644 --- a/app/services/event_create_service.rb +++ b/app/services/event_create_service.rb @@ -74,12 +74,19 @@ class EventCreateService # 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) + event = Event.transaction do + new_event = create_event(project, current_user, Event::PUSHED) - PushEventPayloadService.new(event, push_data).execute + PushEventPayloadService + .new(new_event, push_data) + .execute + + new_event end + Users::LastPushEventService.new(current_user) + .cache_last_push_event(event) + Users::ActivityService.new(current_user, 'push').execute end diff --git a/app/services/users/last_push_event_service.rb b/app/services/users/last_push_event_service.rb new file mode 100644 index 00000000000..f2bfb60604f --- /dev/null +++ b/app/services/users/last_push_event_service.rb @@ -0,0 +1,83 @@ +module Users + # Service class for caching and retrieving the last push event of a user. + class LastPushEventService + EXPIRATION = 2.hours + + def initialize(user) + @user = user + end + + # Caches the given push event for the current user in the Rails cache. + # + # event - An instance of PushEvent to cache. + def cache_last_push_event(event) + keys = [ + project_cache_key(event.project), + user_cache_key + ] + + if event.project.forked? + keys << project_cache_key(event.project.forked_from_project) + end + + keys.each { |key| set_key(key, event.id) } + end + + # Returns the last PushEvent for the current user. + # + # This method will return nil if no event was found. + def last_event_for_user + find_cached_event(user_cache_key) + end + + # Returns the last PushEvent for the current user and the given project. + # + # project - An instance of Project for which to retrieve the PushEvent. + # + # This method will return nil if no event was found. + def last_event_for_project(project) + find_cached_event(project_cache_key(project)) + end + + def find_cached_event(cache_key) + event_id = get_key(cache_key) + + return unless event_id + + unless (event = find_event_in_database(event_id)) + # We don't want to keep querying the same data over and over when a + # merge request has been created, thus we remove the key if no event + # (meaning an MR was created) is returned. + Rails.cache.delete(cache_key) + end + + event + end + + private + + def find_event_in_database(id) + PushEvent + .without_existing_merge_requests + .find_by(id: id) + end + + def user_cache_key + "last-push-event/#{@user.id}" + end + + def project_cache_key(project) + "last-push-event/#{@user.id}/#{project.id}" + end + + def get_key(key) + Rails.cache.read(key, raw: true) + end + + def set_key(key, value) + # We're using raw values here since this takes up less space and we don't + # store complex objects. + Rails.cache.write(key, value, raw: true, expires_in: EXPIRATION) + end + end +end diff --git a/app/views/layouts/nav/sidebar/_admin.html.haml b/app/views/layouts/nav/sidebar/_admin.html.haml index 8ab2b686f86..fcebb385a65 100644 --- a/app/views/layouts/nav/sidebar/_admin.html.haml +++ b/app/views/layouts/nav/sidebar/_admin.html.haml @@ -6,7 +6,7 @@ = icon('wrench') .sidebar-context-title Admin Area %ul.sidebar-top-level-items - = nav_link(controller: %w(dashboard admin projects users groups jobs runners cohorts), html_options: {class: 'home'}) do + = nav_link(controller: %w(dashboard admin projects users groups jobs runners cohorts conversational_development_index), html_options: {class: 'home'}) do = sidebar_link admin_root_path, title: _('Overview'), css: 'shortcuts-tree' do .nav-icon-container = custom_icon('overview') @@ -14,7 +14,7 @@ Overview %ul.sidebar-sub-level-items - = nav_link(controller: %w(dashboard admin projects users groups jobs runners cohorts), html_options: { class: "fly-out-top-item" } ) do + = nav_link(controller: %w(dashboard admin projects users groups jobs runners cohorts conversational_development_index), html_options: { class: "fly-out-top-item" } ) do = link_to admin_root_path do %strong.fly-out-top-item-name #{ _('Overview') } @@ -52,16 +52,16 @@ %span ConvDev Index - = nav_link(controller: %w(conversational_development_index system_info background_jobs logs health_check requests_profiles)) do - = sidebar_link admin_conversational_development_index_path, title: _('Monitoring') do + = nav_link(controller: %w(system_info background_jobs logs health_check requests_profiles)) do + = sidebar_link admin_system_info_path, title: _('Monitoring') do .nav-icon-container = custom_icon('monitoring') %span.nav-item-name Monitoring %ul.sidebar-sub-level-items - = nav_link(controller: %w(conversational_development_index system_info background_jobs logs health_check requests_profiles), html_options: { class: "fly-out-top-item" } ) do - = link_to admin_conversational_development_index_path do + = nav_link(controller: %w(system_info background_jobs logs health_check requests_profiles), html_options: { class: "fly-out-top-item" } ) do + = link_to admin_system_info_path do %strong.fly-out-top-item-name #{ _('Monitoring') } %li.divider.fly-out-top-item diff --git a/app/views/profiles/preferences/show.html.haml b/app/views/profiles/preferences/show.html.haml index 9e7fe556d88..352c2d66bab 100644 --- a/app/views/profiles/preferences/show.html.haml +++ b/app/views/profiles/preferences/show.html.haml @@ -16,10 +16,10 @@ .preview= image_tag "#{scheme.css_class}-scheme-preview.png" = f.radio_button :color_scheme_id, scheme.id = scheme.name + .col-sm-12 %hr - .col-sm-12 - %hr + .col-lg-4.profile-settings-sidebar %h4.prepend-top-0 Behavior diff --git a/changelogs/unreleased/changes-tab-jumping.yml b/changelogs/unreleased/changes-tab-jumping.yml new file mode 100644 index 00000000000..5740dfade9f --- /dev/null +++ b/changelogs/unreleased/changes-tab-jumping.yml @@ -0,0 +1,5 @@ +--- +title: Fixed merge request changes bar jumping +merge_request: +author: +type: fixed diff --git a/changelogs/unreleased/conv-dev-index-regression.yml b/changelogs/unreleased/conv-dev-index-regression.yml new file mode 100644 index 00000000000..799eafa4265 --- /dev/null +++ b/changelogs/unreleased/conv-dev-index-regression.yml @@ -0,0 +1,5 @@ +--- +title: Fix ConvDev Index nav item and Monitoring submenu regression +merge_request: !14124 +author: +type: fixed diff --git a/changelogs/unreleased/user-recent-push.yml b/changelogs/unreleased/user-recent-push.yml new file mode 100644 index 00000000000..defd5cdfd8e --- /dev/null +++ b/changelogs/unreleased/user-recent-push.yml @@ -0,0 +1,5 @@ +--- +title: Rework how recent push events are retrieved +merge_request: +author: +type: other diff --git a/doc/user/admin_area/monitoring/convdev.md b/doc/user/admin_area/monitoring/convdev.md index 3d93c7557a4..a98602c4d70 100644 --- a/doc/user/admin_area/monitoring/convdev.md +++ b/doc/user/admin_area/monitoring/convdev.md @@ -23,7 +23,7 @@ If you have just started using GitLab, it may take a few weeks for data to be collected before this feature is available. This feature is accessible only to a system admin, at -**Admin area > Monitoring > ConvDev Index**. +**Admin area > Overview > ConvDev Index**. [ce-30469]: https://gitlab.com/gitlab-org/gitlab-ce/issues/30469 [ping]: ../settings/usage_statistics.md#usage-ping diff --git a/doc/user/admin_area/monitoring/img/convdev_index.png b/doc/user/admin_area/monitoring/img/convdev_index.png Binary files differindex 4e47ff2228d..ffe18d76c96 100644 --- a/doc/user/admin_area/monitoring/img/convdev_index.png +++ b/doc/user/admin_area/monitoring/img/convdev_index.png diff --git a/spec/features/boards/boards_spec.rb b/spec/features/boards/boards_spec.rb index e010b5f3444..33aca6cb527 100644 --- a/spec/features/boards/boards_spec.rb +++ b/spec/features/boards/boards_spec.rb @@ -13,7 +13,7 @@ describe 'Issue Boards', js: true do project.team << [user, :master] project.team << [user2, :master] - allow_any_instance_of(ApplicationHelper).to receive(:collapsed_sidebar?).and_return(true) + page.driver.set_cookie('sidebar_collapsed', 'true') sign_in(user) end diff --git a/spec/features/dashboard/projects_spec.rb b/spec/features/dashboard/projects_spec.rb index 0613c158c54..9a7b8e3ba6b 100644 --- a/spec/features/dashboard/projects_spec.rb +++ b/spec/features/dashboard/projects_spec.rb @@ -83,12 +83,14 @@ feature 'Dashboard Projects' do end end - context 'last push widget' do + context 'last push widget', :use_clean_rails_memory_store_caching do before do event = create(:push_event, project: project, author: user) create(:push_event_payload, event: event, ref: 'feature', action: :created) + Users::LastPushEventService.new(user).cache_last_push_event(event) + visit dashboard_projects_path end diff --git a/spec/features/issues/filtered_search/visual_tokens_spec.rb b/spec/features/issues/filtered_search/visual_tokens_spec.rb index 4ae54fd6f4e..2b624f4842d 100644 --- a/spec/features/issues/filtered_search/visual_tokens_spec.rb +++ b/spec/features/issues/filtered_search/visual_tokens_spec.rb @@ -28,7 +28,7 @@ describe 'Visual tokens', js: true do sign_in(user) create(:issue, project: project) - allow_any_instance_of(ApplicationHelper).to receive(:collapsed_sidebar?).and_return(true) + page.driver.set_cookie('sidebar_collapsed', 'true') visit project_issues_path(project) end diff --git a/spec/features/merge_requests/diff_notes_avatars_spec.rb b/spec/features/merge_requests/diff_notes_avatars_spec.rb index ca536f2800c..9bcb78d5206 100644 --- a/spec/features/merge_requests/diff_notes_avatars_spec.rb +++ b/spec/features/merge_requests/diff_notes_avatars_spec.rb @@ -22,7 +22,7 @@ feature 'Diff note avatars', js: true do project.team << [user, :master] sign_in user - allow_any_instance_of(ApplicationHelper).to receive(:collapsed_sidebar?).and_return(true) + page.driver.set_cookie('sidebar_collapsed', 'true') end context 'discussion tab' do diff --git a/spec/features/merge_requests/user_posts_diff_notes_spec.rb b/spec/features/merge_requests/user_posts_diff_notes_spec.rb index 442ce14eb7e..2fb6d0b965f 100644 --- a/spec/features/merge_requests/user_posts_diff_notes_spec.rb +++ b/spec/features/merge_requests/user_posts_diff_notes_spec.rb @@ -6,7 +6,7 @@ feature 'Merge requests > User posts diff notes', :js do let(:project) { merge_request.source_project } before do - allow_any_instance_of(ApplicationHelper).to receive(:collapsed_sidebar?).and_return(true) + page.driver.set_cookie('sidebar_collapsed', 'true') project.add_developer(user) sign_in(user) diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 49cb7c954b4..1437479831e 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -313,23 +313,10 @@ describe ProjectsHelper do it 'returns recent push on the current project' do event = double(:event) - expect(user).to receive(:recent_push).with([project.id]).and_return(event) + expect(user).to receive(:recent_push).with(project).and_return(event) expect(helper.last_push_event).to eq(event) end - - context 'when current user has a fork of the current project' do - let(:fork) { double(:fork, id: 2) } - - it 'returns recent push considering fork events' do - expect(user).to receive(:fork_of).with(project).and_return(fork) - - event_on_fork = double(:event) - expect(user).to receive(:recent_push).with([project.id, fork.id]).and_return(event_on_fork) - - expect(helper.last_push_event).to eq(event_on_fork) - end - end end describe "#project_feature_access_select" do diff --git a/spec/javascripts/issue_show/components/app_spec.js b/spec/javascripts/issue_show/components/app_spec.js index 39065814bc2..583a3a74d77 100644 --- a/spec/javascripts/issue_show/components/app_spec.js +++ b/spec/javascripts/issue_show/components/app_spec.js @@ -42,7 +42,6 @@ describe('Issuable output', () => { initialDescriptionText: '', markdownPreviewPath: '/', markdownDocsPath: '/', - isConfidential: false, projectNamespace: '/', projectPath: '/', }, @@ -157,30 +156,6 @@ describe('Issuable output', () => { }); }); - it('reloads the page if the confidential status has changed', (done) => { - spyOn(gl.utils, 'visitUrl'); - spyOn(vm.service, 'updateIssuable').and.callFake(() => new Promise((resolve) => { - resolve({ - json() { - return { - confidential: true, - web_url: location.pathname, - }; - }, - }); - })); - - vm.updateIssuable(); - - setTimeout(() => { - expect( - gl.utils.visitUrl, - ).toHaveBeenCalledWith(location.pathname); - - done(); - }); - }); - it('correctly updates issuable data', (done) => { spyOn(vm.service, 'updateIssuable').and.callFake(() => new Promise((resolve) => { resolve(); diff --git a/spec/models/push_event_spec.rb b/spec/models/push_event_spec.rb index 532fb024261..ad3c3a406d9 100644 --- a/spec/models/push_event_spec.rb +++ b/spec/models/push_event_spec.rb @@ -11,6 +11,94 @@ describe PushEvent do event end + describe '.created_or_pushed' do + let(:event1) { create(:push_event) } + let(:event2) { create(:push_event) } + let(:event3) { create(:push_event) } + + before do + create(:push_event_payload, event: event1, action: :pushed) + create(:push_event_payload, event: event2, action: :created) + create(:push_event_payload, event: event3, action: :removed) + end + + let(:relation) { described_class.created_or_pushed } + + it 'includes events for pushing to existing refs' do + expect(relation).to include(event1) + end + + it 'includes events for creating new refs' do + expect(relation).to include(event2) + end + + it 'does not include events for removing refs' do + expect(relation).not_to include(event3) + end + end + + describe '.branch_events' do + let(:event1) { create(:push_event) } + let(:event2) { create(:push_event) } + + before do + create(:push_event_payload, event: event1, ref_type: :branch) + create(:push_event_payload, event: event2, ref_type: :tag) + end + + let(:relation) { described_class.branch_events } + + it 'includes events for branches' do + expect(relation).to include(event1) + end + + it 'does not include events for tags' do + expect(relation).not_to include(event2) + end + end + + describe '.without_existing_merge_requests' do + let(:project) { create(:project, :repository) } + let(:event1) { create(:push_event, project: project) } + let(:event2) { create(:push_event, project: project) } + let(:event3) { create(:push_event, project: project) } + let(:event4) { create(:push_event, project: project) } + + before do + create(:push_event_payload, event: event1, ref: 'foo', action: :created) + create(:push_event_payload, event: event2, ref: 'bar', action: :created) + create(:push_event_payload, event: event3, ref: 'baz', action: :removed) + create(:push_event_payload, event: event4, ref: 'baz', ref_type: :tag) + + project.repository.create_branch('bar', 'master') + + create( + :merge_request, + source_project: project, + target_project: project, + source_branch: 'bar' + ) + end + + let(:relation) { described_class.without_existing_merge_requests } + + it 'includes events that do not have a corresponding merge request' do + expect(relation).to include(event1) + end + + it 'does not include events that have a corresponding merge request' do + expect(relation).not_to include(event2) + end + + it 'does not include events for removed refs' do + expect(relation).not_to include(event3) + end + + it 'does not include events for pushing to tags' do + expect(relation).not_to include(event4) + end + end + describe '.sti_name' do it 'returns Event::PUSHED' do expect(described_class.sti_name).to eq(Event::PUSHED) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 36d1f6f3644..3ba01313efb 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1347,56 +1347,24 @@ describe User do end describe "#recent_push" do - subject { create(:user) } - let!(:project1) { create(:project, :repository) } - let!(:project2) { create(:project, :repository, forked_from_project: project1) } - - let!(:push_event) do - event = create(:push_event, project: project2, author: subject) - - create(:push_event_payload, - event: event, - commit_to: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', - commit_count: 0, - ref: 'master') - - event - end - - before do - project1.team << [subject, :master] - project2.team << [subject, :master] - end - - it "includes push event" do - expect(subject.recent_push).to eq(push_event) - end - - it "excludes push event if branch has been deleted" do - allow_any_instance_of(Repository).to receive(:branch_exists?).with('master').and_return(false) - - expect(subject.recent_push).to eq(nil) - end + let(:user) { build(:user) } + let(:project) { build(:project) } + let(:event) { build(:push_event) } - it "excludes push event if MR is opened for it" do - create(:merge_request, source_project: project2, target_project: project1, source_branch: project2.default_branch, target_branch: 'fix', author: subject) + it 'returns the last push event for the user' do + expect_any_instance_of(Users::LastPushEventService) + .to receive(:last_event_for_user) + .and_return(event) - expect(subject.recent_push).to eq(nil) + expect(user.recent_push).to eq(event) end - it "includes push events on any of the provided projects" do - expect(subject.recent_push(project1)).to eq(nil) - expect(subject.recent_push(project2)).to eq(push_event) - - push_event1 = create(:push_event, project: project1, author: subject) - - create(:push_event_payload, - event: push_event1, - commit_to: '1cf19a015df3523caf0a1f9d40c98a267d6a2fc2', - commit_count: 0, - ref: 'master') + it 'returns the last push event for a project when one is given' do + expect_any_instance_of(Users::LastPushEventService) + .to receive(:last_event_for_project) + .and_return(event) - expect(subject.recent_push([project1, project2])).to eq(push_event1) # Newest + expect(user.recent_push(project)).to eq(event) end end diff --git a/spec/services/event_create_service_spec.rb b/spec/services/event_create_service_spec.rb index 02d7ddeb86b..13395a7cac3 100644 --- a/spec/services/event_create_service_spec.rb +++ b/spec/services/event_create_service_spec.rb @@ -149,6 +149,14 @@ describe EventCreateService do .to change { user_activity(user) } end + it 'caches the last push event for the user' do + expect_any_instance_of(Users::LastPushEventService) + .to receive(:cache_last_push_event) + .with(an_instance_of(PushEvent)) + + service.push(project, user, push_data) + end + it 'does not create any event data when an error is raised' do payload_service = double(:service) diff --git a/spec/services/users/last_push_event_service_spec.rb b/spec/services/users/last_push_event_service_spec.rb new file mode 100644 index 00000000000..956358738fe --- /dev/null +++ b/spec/services/users/last_push_event_service_spec.rb @@ -0,0 +1,112 @@ +require 'spec_helper' + +describe Users::LastPushEventService do + let(:user) { build(:user, id: 1) } + let(:project) { build(:project, id: 2) } + let(:event) { build(:push_event, id: 3, author: user, project: project) } + let(:service) { described_class.new(user) } + + describe '#cache_last_push_event' do + it "caches the event for the event's project and current user" do + expect(service).to receive(:set_key) + .ordered + .with('last-push-event/1/2', 3) + + expect(service).to receive(:set_key) + .ordered + .with('last-push-event/1', 3) + + service.cache_last_push_event(event) + end + + it 'caches the event for the origin project when pushing to a fork' do + source = build(:project, id: 5) + + allow(project).to receive(:forked?).and_return(true) + allow(project).to receive(:forked_from_project).and_return(source) + + expect(service).to receive(:set_key) + .ordered + .with('last-push-event/1/2', 3) + + expect(service).to receive(:set_key) + .ordered + .with('last-push-event/1', 3) + + expect(service).to receive(:set_key) + .ordered + .with('last-push-event/1/5', 3) + + service.cache_last_push_event(event) + end + end + + describe '#last_event_for_user' do + it 'returns the last push event for the current user' do + expect(service).to receive(:find_cached_event) + .with('last-push-event/1') + .and_return(event) + + expect(service.last_event_for_user).to eq(event) + end + + it 'returns nil when no push event could be found' do + expect(service).to receive(:find_cached_event) + .with('last-push-event/1') + .and_return(nil) + + expect(service.last_event_for_user).to be_nil + end + end + + describe '#last_event_for_project' do + it 'returns the last push event for the given project' do + expect(service).to receive(:find_cached_event) + .with('last-push-event/1/2') + .and_return(event) + + expect(service.last_event_for_project(project)).to eq(event) + end + + it 'returns nil when no push event could be found' do + expect(service).to receive(:find_cached_event) + .with('last-push-event/1/2') + .and_return(nil) + + expect(service.last_event_for_project(project)).to be_nil + end + end + + describe '#find_cached_event', :use_clean_rails_memory_store_caching do + context 'with a non-existing cache key' do + it 'returns nil' do + expect(service.find_cached_event('bla')).to be_nil + end + end + + context 'with an existing cache key' do + before do + service.cache_last_push_event(event) + end + + it 'returns a PushEvent when no merge requests exist for the event' do + allow(service).to receive(:find_event_in_database) + .with(event.id) + .and_return(event) + + expect(service.find_cached_event('last-push-event/1')).to eq(event) + end + + it 'removes the cache key when no event could be found and returns nil' do + allow(PushEvent).to receive(:without_existing_merge_requests) + .and_return(PushEvent.none) + + expect(Rails.cache).to receive(:delete) + .with('last-push-event/1') + .and_call_original + + expect(service.find_cached_event('last-push-event/1')).to be_nil + end + end + end +end |
