357773: Add Retrieve label events by project via the API
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.
-
I have evaluated the MR acceptance checklist for this MR.
Related to #357773
Merge request reports
Activity
added typefeature label
Hey @lenikadali!
Thank you for your contribution to GitLab. Please refer to the contribution flow documentation for a quick overview of the process, and the merge request (MR) guidelines for the detailed process.
When you're ready for a first review, post
@gitlab-bot ready
. If you know a relevant reviewer(s) (for example, someone that was involved in a related issue), you can also assign them directly with@gitlab-bot ready @user1 @user2
.At any time, if you need help, feel free to post
@gitlab-bot help
or initiate a mentor session on Discord. Read more on how to get help.To enable automated checks on your MR, please configure Danger for your fork.
You can comment
@gitlab-bot label <label1> <label2>
to add labels to your MR. Please see the list of allowed labels in thelabel
command documentation.This message was generated automatically. You're welcome to improve it.
added Community contribution workflowin dev labels
assigned to @lenikadali
added groupproject management label
mentioned in issue #357773
Two questions:
- How would I go about implement the filter that was being proposed here?
- When I ran RuboCop on the proposed implementation, apparently we're in violation of the
CodeReuse/ActiveRecord
with this issue mentioned. Please let me know how we can fix it. For now I've disabled the cop and added a Todo
Edit: Forgot to link to the comment where mentioned filters.
Edited by Leni Kadali@lenikadali I have a couple suggestions on this:
- We would want to make sure we have paginated results. Depending on the project it can be very expensive to fetch all events at once and it may actually just timeout.
- I think we may also want some sort of time based filtering as well, e.g.
events where created_at > X
orevents where created_at < Y
etc, so that one does not have to fetch all events over and over again. - To fix the rubocop we suggest moving the where clauses to model scopes and also would be nice to move the searching functionality to a Finder.
@acroitor for no. 1 wouldn't this snippet:
params do use :pagination end
do that?
Otherwise will look into doing 2 and 3. Thank you
Edited by Leni KadaliI tried to make the changes as suggested:
- Not sure what the domain of
ResourceLabelEvents
is so for disabled that cop: https://gitlab.com/lenikadali/gitlab/-/blob/357773-api-retrieve-label-events-by-project/app/finders/resource_label_events/resource_label_event_finder.rb#L3 - Not sure what the
Finder
was meant to contain so have left it as a skeleton for now. - Made both the
where
andfilter
clauses model scopes: https://gitlab.com/lenikadali/gitlab/-/blob/357773-api-retrieve-label-events-by-project/app/models/resource_label_event.rb#L12-17 - Not sure where how to call the scopes from within the API call.
Your feedback is appreciated
- Not sure what the domain of
Not sure what the domain of
ResourceLabelEvents
is so for disabled that cop: https://gitlab.com/lenikadali/gitlab/-/blob/357773-api-retrieve-label-events-by-project/app/finders/resource_label_events/resource_label_event_finder.rb#L3ResourceEvents/EventsFinder
kinda makes sense to me, WDYT @cablett ?Not sure what the
Finder
was meant to contain so have left it as a skeleton for now.Finder is supposed to hold all the searching logic, so that instead of having the logic within the REST API endpoint, we have it in a service, in this case a finder. You can check out
app/finders
for other finder examples.Made both the
where
andfilter
clauses model scopesAwesome, I think we already have a
created_after
scope in base classResourceEvent
so perhaps we can add acreated_before
next to that one. Having those defined in base class means we do not need to define it for each type of the event.Perhaps we can also move the filtering by project and group there as well, though it is a bit trickier as group is only available for some of the events.
Not sure where how to call the scopes from within the API call.
Scopes would be called from within Finder class and you can read a bit on rails scopes here https://www.rubyguides.com/2019/10/scopes-in-ruby-on-rails/ or other google resources, but mainly scopes would apply to the model to search by that specific scope/attribute.
Hope this helps, sorry for the delayed response, and feel free to ping me back if you need any further clarifications.
Hi @acroitor
Thanks so much for the feedback; it cleared things up for me
In the new changes I've added the
created_after
scope in theResourceEvent
base class: https://gitlab.com/lenikadali/gitlab/-/blob/357773-api-retrieve-label-events-by-project/app/models/resource_event.rb#L16and used the domain
ResourceLabelEvents/EventsFinder
: https://gitlab.com/lenikadali/gitlab/-/blob/357773-api-retrieve-label-events-by-project/app/finders/resource_label_events/events_finder.rb#L3-4have made a basic method for finding project events here https://gitlab.com/lenikadali/gitlab/-/blob/357773-api-retrieve-label-events-by-project/app/finders/resource_label_events/events_finder.rb#L12-14
I think you can review it again now. Thank you
Setting label(s) devopsplan sectiondev based on groupproject management.
added devopsplan sectiondev labels
added 792 commits
-
99db8dd5...63476ae2 - 784 commits from branch
gitlab-org:master
- 9add327b - Merge branch 'master' into 357773-api-retrieve-label-events-by-project
- ef97976a - Merge branch 'master' into 357773-api-retrieve-label-events-by-project
- a4dcda4d - Merge branch 'master' into 357773-api-retrieve-label-events-by-project
- 60329384 - Merge branch 'master' into 357773-api-retrieve-label-events-by-project
- ccb98d01 - Merge branch 'master' into 357773-api-retrieve-label-events-by-project
- b7f1f57c - Add scope for time based fitering and finder
- c0e9d747 - Make finder skeleton, move searching into model scope
- 5ee87e40 - RuboCop fixes
Toggle commit list-
99db8dd5...63476ae2 - 784 commits from branch
added 2067 commits
-
5ee87e40...e8d63823 - 2065 commits from branch
gitlab-org:master
- 7bf968ea - Merge branch 'master' into 357773-api-retrieve-label-events-by-project
- 3b4d24f9 - Rename Finder, scopes, add created_after scope
-
5ee87e40...e8d63823 - 2065 commits from branch
- Resolved by Leni Kadali
- Resolved by Leni Kadali
- Resolved by Leni Kadali
- Resolved by Leni Kadali
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
Toggle commit list-
3567b2dc...5e8e5e0b - 383 commits from branch
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 - Comment on lines +40 to +43
@acroitor I am a bit unsure about the filtering here. I've defaulted to using
Date
as the type but let me know if I should includeDate
toTime
conversion in thefind_project_eventable
method: https://gitlab.com/lenikadali/gitlab/-/blob/357773-api-retrieve-label-events-by-project/app/finders/resource_label_events/resource_label_events_finder.rb#L12given that
ResourceEvent
seems to expecttime
(meaning aTime
object?): https://gitlab.com/lenikadali/gitlab/-/blob/357773-api-retrieve-label-events-by-project/app/models/resource_event.rb#L15-16Edited by Leni Kadali I've defaulted to using
Date
as the type but let me know if I should includeDate
toTime
We can use DateTime as that would cover both Date and Time, so if one needs just date that can be provided as "2023-01-01 00:00:00 UTC". BTW we should not that we will take UTC times in that case.
Please consider checking an existing finder, e.g. IssuableFinder
We usually use an
execute
method to run the finder also note that filtering is cumulative, and that would be smth that we would want here as well. So if user can provides justcreated_before
or justcreated_after
or both and these filters would cumulate on the query that is going to be ran.Please consider checking an existing finder, e.g. IssuableFinder
I wasn't sure about basing the implementation on some of the other
Finder
s because they had disabled some Cops in regards toActiveRecord
. That made me assume they were implemented during a time when the current style guide wasn't in place.
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
Toggle commit list-
165ddada...da6b6936 - 418 commits from branch
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 - Comment on lines +7 to +32
@acroitor I've tried to re-do the
Finder
based on theIssuableFinder
. Let me know if this lines up with what you were suggesting
Hello @acroitor may you please review? Thank you
@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 aproject_id
column, so you would probably have to join it withissues
ormerge_requests
table in order to be able to filter events for a givenproject
or we'd need to backfill theproject_id
info into theresource_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
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 Kadalior 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 }) }
requested review from @acroitor
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
Toggle commit list-
66c8290e...0f40eaf3 - 4422 commits from branch
@lenikadali, it seems we're waiting on an action from you for approximately two weeks.
- Do you still have capacity to work on this? If not, you might want to close this MR and/or ask someone to take over.
- Do you need help in getting it ready? At any time, you can:
- If you're actually ready for a review, you can post
@gitlab-bot ready
.
This message was generated automatically. You're welcome to improve it.
added automation:author-reminded label
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
Toggle commit list-
6d615169...b4be1179 - 3667 commits from branch
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
@lenikadali, how was your code review experience with this merge request? Please tell us how we can continue to iterate and improve:
- React with a
or a on this comment to describe your experience. - 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!
This message was generated automatically. You're welcome to improve it.
- React with a
removed review request for @acroitor
Setting label(s) Category:Team Planning based on groupproject management.
added Category:Team Planning label
added idle label
added linked-issue label
Hi @donaldcook!
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
- Work on the provided feedback. Add
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!
added automation:stale-reminded label
removed stale label