Skip to content
Snippets Groups Projects

357773: Add Retrieve label events by project via the API

6 unresolved threads

What does this MR do and why?

This MR adds the ability to retrieve label events by project.

Added a spec and the implementation suggested here The initial proposed implementation has code that is suggested to be in the model.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Related to #357773

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Leni Kadali added 1 commit

    added 1 commit

    Compare with previous version

  • Leni Kadali added 387 commits

    added 387 commits

    • 3567b2dc...5e8e5e0b - 383 commits from branch gitlab-org:master
    • 0e12ef19 - Merge branch 'master' into 357773-api-retrieve-label-events-by-project
    • 44d5e003 - Merge branch 'master' into 357773-api-retrieve-label-events-by-project
    • d5536e2a - Add ability to fetch events before or after a given date
    • 165ddada - RuboCop fixes, general clean-up

    Compare with previous version

  • 34 34 present ResourceLabelEvent.visible_to_user?(current_user, paginate(events)), with: Entities::ResourceLabelEvent
    35 35 end
    36 36
    37 desc "Get a list of all resource label events for a project" do
    38 success Entities::ResourceLabelEvent
    39 end
    40 params do
    41 optional :created_before, type: Date, desc: 'Date before which the event occurred'
    42 optional :created_after, type: Date, desc: 'Date after which the event occurred'
    43 use :pagination
  • Leni Kadali added 421 commits

    added 421 commits

    • 165ddada...da6b6936 - 418 commits from branch gitlab-org:master
    • ba1d9eec - Merge branch 'master' into 357773-api-retrieve-label-events-by-project
    • b1f7dbb4 - Merge branch 'master' into 357773-api-retrieve-label-events-by-project
    • 66c8290e - Change Finder implementation, remove if statement

    Compare with previous version

  • 34 34 present ResourceLabelEvent.visible_to_user?(current_user, paginate(events)), with: Entities::ResourceLabelEvent
    35 35 end
    36 36
    37 desc "Get a list of all resource label events for a project" do
    38 success Entities::ResourceLabelEvent
    39 end
    40 params do
    41 optional :created_before, type: DateTime, desc: 'Date before which the event occurred not in UTC'
    42 optional :created_after, type: DateTime, desc: 'Date after which the event occurred not in UTC'
    43 use :pagination
    44 end
    45 get ":id/#{eventables_str}/resource_label_events", feature_category: feature_category, urgency: :low do
    46 events = ResourceEvents::ResourceLabelEventsFinder.new(current_user, params).execute
  • 17 by_project
    18 by_created_at(events)
    19 end
    20
    21 private
    22
    23 def by_project
    24 ResourceLabelEvent.find_project_events(params[:id])
    25 end
    26
    27 def by_created_at(events)
    28 events = ResourceLabelEvent.created_before(params[:created_before]) if params[:created_before].present?
    29 events = ResourceLabelEvent.created_after(params[:created_after]) if params[:created_after].present?
    30
    31 events
    32 end
    • Author Contributor

      Hello @acroitor may you please review? Thank you :pray_tone4:

    • @lenikadali would be helpful if you be able to test it out yourself before asking for a review. I don't think this code is working.

      Note also that resource_label_events table does not have a project_id column, so you would probably have to join it with issues or merge_requests table in order to be able to filter events for a given project or we'd need to backfill the project_id info into the resource_label_events table.

      here is a version of the code that would return resource label events by type. Note that this is not a finished version. I don't think this works for epics for instance. Also this needs to be tested for issues, merge_requests and epics. Also it is missing a spec for the finder and probably other things, but this should get you started.

      resource_labels_events_finder.patch

      diff --git a/app/finders/resource_events/resource_label_events_finder.rb b/app/finders/resource_events/resource_label_events_finder.rb
      index d3c758004cec..7961a4ca47e5 100644
      --- a/app/finders/resource_events/resource_label_events_finder.rb
      +++ b/app/finders/resource_events/resource_label_events_finder.rb
      @@ -4,24 +4,36 @@ module ResourceEvents
         class ResourceLabelEventsFinder
           include CreatedAtFilter
      
      -    def initialize(project, current_user, params)
      -      @project = project
      +    ACCEPTED_LABEL_RESOURCES = %w(issues merge_requests epics)
      +
      +    def initialize(container, current_user, params)
      +      @container = container
             @current_user = current_user
             @params = params
           end
      
           def execute
      -      events = ResourceLabelEvent.by_project(project)
      +      raise ArgumentError unless params[:issuable_type].present? && ACCEPTED_LABEL_RESOURCES.include?(params[:issuable_type])
      +
      +      events = initialize_events_list
             events = by_created_at(events)
             events.order(id: :asc)
           end
      
           private
      
      -    attr_reader :project, :params
      +    attr_reader :container, :params
      
           def initialize_events_list
      -      ResourceLabelEvent.by_project(project)
      +
      +      case params[:issuable_type]
      +      when 'issues'
      +        ResourceLabelEvent.for_issues_by_project(container)
      +      when 'merge_requests'
      +        ResourceLabelEvent.for_merge_requests_by_project(container)
      +      when 'epics'
      +        ResourceLabelEvent.for_epics_by_group(container)
      +      end
           end
         end
       end
      diff --git a/app/models/resource_event.rb b/app/models/resource_event.rb
      index c667e0b32379..c72a55dda6b9 100644
      --- a/app/models/resource_event.rb
      +++ b/app/models/resource_event.rb
      @@ -5,6 +5,7 @@ class ResourceEvent < ApplicationRecord
         include Importable
         include IssueResourceEvent
         include WorkItemResourceEvent
      +  include CreatedAtFilterable
      
         self.abstract_class = true
      
      @@ -12,9 +13,6 @@ class ResourceEvent < ApplicationRecord
      
         belongs_to :user
      
      -  scope :created_after, ->(time) { where('created_at > ?', time) }
      -  scope :created_before, ->(time) { where('created_at < ?', time) }
      -
         def discussion_id
           strong_memoize(:discussion_id) do
             Digest::SHA1.hexdigest(discussion_id_key.join("-"))
      diff --git a/app/models/resource_label_event.rb b/app/models/resource_label_event.rb
      index bc0f38b2b4e6..7e76a5a86a4a 100644
      --- a/app/models/resource_label_event.rb
      +++ b/app/models/resource_label_event.rb
      @@ -9,11 +9,8 @@ class ResourceLabelEvent < ResourceEvent
         belongs_to :label
      
         scope :inc_relations, -> { includes(:label, :user) }
      -  scope :find_project_events, -> {
      -    where.not('event.id' => :nil)
      -            .where('event.project_id == :id', id: project_id)
      -  }
      -  scope :by_project, ->(project) { where(project: project) }
      +  scope :for_issues_by_project, ->(project) { joins(:issue).where(issues: { project: project }) }
      +  scope :for_merge_requests_by_project, ->(project) { joins(:merge_requests).where(issues: { project: project }) }
      
         validates :label, presence: { unless: :importing? }, on: :create
         validate :exactly_one_issuable, unless: :importing?
      diff --git a/ee/app/models/ee/resource_label_event.rb b/ee/app/models/ee/resource_label_event.rb
      index 4aeec48e2413..4b127ff228f4 100644
      --- a/ee/app/models/ee/resource_label_event.rb
      +++ b/ee/app/models/ee/resource_label_event.rb
      @@ -7,6 +7,8 @@ module ResourceLabelEvent
      
           prepended do
             belongs_to :epic
      +
      +      scope :for_epics_by_group, ->(group) { joins(:epic).where(epics: { group: group }) }
           end
      
           class_methods do
      diff --git a/lib/api/resource_label_events.rb b/lib/api/resource_label_events.rb
      index 4cc86dc31fee..2cff3613ed84 100644
      --- a/lib/api/resource_label_events.rb
      +++ b/lib/api/resource_label_events.rb
      @@ -42,8 +42,10 @@ class ResourceLabelEvents < ::API::Base
                 optional :created_after, type: DateTime, desc: 'Date after which the event occurred not in UTC'
                 use :pagination
               end
      -        get ":id/#{eventables_str}/resource_label_events", feature_category: feature_category, urgency: :low do
      -          events = ResourceEvents::ResourceLabelEventsFinder.new(current_user, params).execute
      +        get ":id/resource_label_events/#{eventables_str}", feature_category: feature_category, urgency: :low do
      +          params[:issuable_type] = eventables_str
      +          events = ::ResourceEvents::ResourceLabelEventsFinder.new(user_project, current_user, params).execute
      +
                 present ResourceLabelEvent.visible_to_user?(current_user, paginate(events)), with: Entities::ResourceLabelEvent
               end
      
      diff --git a/spec/requests/api/resource_label_events_spec.rb b/spec/requests/api/resource_label_events_spec.rb
      index 7315faa42c39..b56015a99e77 100644
      --- a/spec/requests/api/resource_label_events_spec.rb
      +++ b/spec/requests/api/resource_label_events_spec.rb
      @@ -25,10 +25,24 @@
           end
         end
      
      -  context 'when eventable is an Project' do
      -    it_behaves_like 'resource_label_events API', 'projects', 'issues' do
      -      let(:parent) { project }
      -      let(:eventable) { create(:issue, project: project, author: user) }
      +  context 'when fetching all resource label events in a project' do
      +    let_it_be(:project2) { create(:project, :public, namespace: user.namespace) }
      +    let_it_be(:issue1) { create(:issue, project: project, author: user) }
      +    let_it_be(:issue2) { create(:issue, project: project, author: user) }
      +    let_it_be(:issue3) { create(:issue, project: project2, author: user) }
      +    let_it_be(:issue1_events) { create(:resource_label_event, issue: issue1, label: label) }
      +    let_it_be(:issue2_events) { create(:resource_label_event, issue: issue2, label: label) }
      +    let_it_be(:issue3_events) { create(:resource_label_event, issue: issue3, label: label) }
      +
      +    it 'fetches correct events for specific project' do
      +      get api("/projects/#{project.id}/resource_label_events/issues", user)
      +
      +      expect(response).to have_gitlab_http_status(:ok)
      +      expect(response).to include_pagination_headers
      +      expect(json_response).to be_an Array
      +      expect(json_response.size).to eq(2)
      +      expect(json_response.first['id']).to eq(issue1_events.id)
      +      expect(json_response.second['id']).to eq(issue2_events.id)
           end
         end
       end
    • Author Contributor

      Hi @acroitor thanks a lot for getting back to me.

      would be helpful if you be able to test it out yourself before asking for a review. I don't think this code is working.

      I apologize for this. I am not very familiar with this part of the codebase so was making my way via trial-and-error. How would I go about testing it apart from writing specs as per the example you've shared above?

      I will use the example you've given and try to get it working.

      Edited by Leni Kadali
    • Author Contributor

      Hi @acroitor I have a question about the epic part of the implementation. Should we fetch epic events via the group only or would we want the events of an epic whose project is under the given group e.g. here

    • or would we want the events of an epic whose project is under the given group

      @lenikadali epics are available only in groups, i.e. no epics in projects.

      So the events for epics are linked directly to epics so in order to get events for a given group we'd need to join the epics table similar to how we join the issues or merge requests tables above. So smth along the lines of:

      scope :for_epics_by_group, ->(group) { joins(:epic).where(epics: { group: group }) }
    • Please register or sign in to reply
  • Alexandru Croitor requested review from @acroitor

    requested review from @acroitor

  • Leni Kadali added 4433 commits

    added 4433 commits

    • 66c8290e...0f40eaf3 - 4422 commits from branch gitlab-org:master
    • cacdeccd - 1 earlier commit
    • 1c096be8 - Add scope for time based fitering and finder
    • caf5e313 - Make finder skeleton, move searching into model scope
    • 9c01151a - RuboCop fixes
    • 44c23fe6 - Rename Finder, scopes, add created_after scope
    • 66e08716 - Apply review feedback
    • 383b74b2 - Add ability to fetch events before or after a given date
    • e7a42d25 - RuboCop fixes, general clean-up
    • fa18eefe - Change Finder implementation, remove if statement
    • 4e0f1ad6 - Add initial re-working of implementation
    • 6d615169 - RuboCop fixes

    Compare with previous version

  • @lenikadali, it seems we're waiting on an action from you for approximately two weeks.

    This message was generated automatically. You're welcome to improve it.

  • Leni Kadali added 3679 commits

    added 3679 commits

    • 6d615169...b4be1179 - 3667 commits from branch gitlab-org:master
    • b4be1179...9c43e685 - 2 earlier commits
    • afee3cda - Make finder skeleton, move searching into model scope
    • 2fc9d49a - RuboCop fixes
    • 6b96add7 - Rename Finder, scopes, add created_after scope
    • 7df6865f - Apply review feedback
    • 9603210e - Add ability to fetch events before or after a given date
    • 564eca5a - RuboCop fixes, general clean-up
    • 4dd4bdad - Change Finder implementation, remove if statement
    • 0aaf4d6e - Add initial re-working of implementation
    • 2375eeca - RuboCop fixes
    • 8a3f069e - Add spec for retrieving epic resource label events

    Compare with previous version

  • Author Contributor

    Hello @acroitor

    Unfortunately I no longer have capacity to work on this. It seems I underestimated how much effort it would be for me.

    I apologize for the inconvenience caused :pray_tone4:

  • closed

  • @lenikadali, how was your code review experience with this merge request? Please tell us how we can continue to iterate and improve:

    1. React with a :thumbsup: or a :thumbsdown: on this comment to describe your experience.
    2. Create a new comment starting with @gitlab-bot feedback below, and leave any additional feedback you have for us in the comment.

    Interested in learning more tips and tricks to solve your next challenge faster? Subscribe to the GitLab Community Newsletter for contributor-focused content and opportunities to level up.

    Thanks for your help! :heart:

    This message was generated automatically. You're welcome to improve it.

  • reopened

  • Alexandru Croitor removed review request for @acroitor

    removed review request for @acroitor

  • added idle label

  • 🤖 GitLab Bot 🤖 added stale label and removed idle label

    added stale label and removed idle label

    • Hi @donaldcook! :wave:

      To provide a better contribution experience, we're identifying older merge requests with no human activity in the past 120 days. Our goal is to bring these merge requests to completion or close them, enabling us to spend more time on active merge requests.

      Review this merge request and determine if you should:

      • Work on the provided feedback. Add @gitlab-bot ready when you need a review or are looking for more feedback.
      • Don't know how to proceed? Ask for help from a merge request coach by adding @gitlab-bot help in a comment.
      • Close the merge request.

      Please see the handbook for additional details on this

      (Improve this message?)

    • Per !108974 (comment 1325894228), going to close this MR for now. @lenikadali if your capacity frees up some in the future, please feel free to reopen!

    • Please register or sign in to reply
  • closed

  • removed stale label

  • Please register or sign in to reply
    Loading