refactor: Reduce code duplication

`Activities::DaysComponent` factorizes the code to generate the HTML for
the activities per days.

By moving `journal` into `Activities::Event`, `journals_by_id` does not
need to be passed around anymore. Eager loading of journals is moved to
`Activities::Fetcher` instead of being handled by the controller. It
also means that the atom feed will be slower as it does not need the
journals information.

app/components/activities/days_component.html.erb

@@ -0,0 +1,7 @@
+<div class="op-activity-days">
+  <% events_by_day.each do |day, events| %>
+    <%= content_tag(@header_tag, helpers.format_activity_day(day), class: 'op-activity-days--header') %>
+
+    <%= render(Activities::ListComponent.new(events:, display_user: @display_user)) %>
+  <% end -%>
+</div>

app/components/activities/days_component.rb

@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+#-- copyright
+# OpenProject is an open source project management software.
+# Copyright (C) 2012-2022 the OpenProject GmbH
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See COPYRIGHT and LICENSE files for more details.
+#++
+
+class Activities::DaysComponent < ViewComponent::Base
+  def initialize(events:, display_user: true, header_tag: 'h3')
+    super()
+    @events = events
+    @display_user = display_user
+    @header_tag = header_tag
+  end
+
+  def events_by_day
+    @events_by_day ||= @events
+      .group_by { |e| e.event_datetime.in_time_zone(User.current.time_zone).to_date }
+      .sort_by { |day, _events| day }
+      .reverse
+  end
+end

app/components/activities/item_component.html.erb

@@ -8,7 +8,7 @@
   <%=
     render(Activities::ItemSubtitleComponent.new(user: display_user? && @event.event_author,
                                              datetime: @event.event_datetime,
-                                             is_creation: @journal.initial?))
+                                             is_creation: @event.journal.initial?))
   %>
   <% if @event.event_description.present? -%>
     <div class="op-activity-list--item-description">

app/components/activities/item_component.rb

@@ -29,15 +29,14 @@
 #++
 
 class Activities::ItemComponent < ViewComponent::Base
-  def initialize(event:, journal:, display_user: true)
+  def initialize(event:, display_user: true)
     super()
     @event = event
-    @journal = journal
     @display_user = display_user
   end
 
   def display_belonging_project?
-    @journal.journable_type != 'Project'
+    @event.journal.journable_type != 'Project'
   end
 
   def display_user?
@@ -45,16 +44,16 @@ class Activities::ItemComponent < ViewComponent::Base
   end
 
   def display_details?
-    return false if @journal.initial?
+    return false if @event.journal.initial?
 
     rendered_details.present?
   end
 
   def rendered_details
     @rendered_details ||=
-      @journal.details
-      .map { |detail| @journal.render_detail(detail) }
-      .compact
+      @event.journal
+        .details
+        .flat_map { |detail| @event.journal.render_detail(detail) }
   end
 
   def format_activity_title(text)

app/components/activities/list_component.html.erb

@@ -1,6 +1,5 @@
 <ul class="op-activity-list">
   <% @events.each do |event| -%>
-    <% journal = @journals_by_id[event.event_id] -%>
-    <%= render(Activities::ItemComponent.new(event:, journal:, display_user: @display_user)) %>
+    <%= render(Activities::ItemComponent.new(event:, display_user: @display_user)) %>
   <% end -%>
 </ul>

app/components/activities/list_component.rb

@@ -29,10 +29,9 @@
 #++
 
 class Activities::ListComponent < ViewComponent::Base
-  def initialize(events:, journals_by_id:, display_user: true)
+  def initialize(events:, display_user: true)
     super()
     @events = events.sort { |x, y| y.event_datetime <=> x.event_datetime }
-    @journals_by_id = journals_by_id
     @display_user = display_user
   end
 end

app/controllers/activities_controller.rb

@@ -45,14 +45,14 @@ class ActivitiesController < ApplicationController
                                         author: @author,
                                         scope: activity_scope)
 
-    events = @activity.events(@date_from.to_datetime, @date_to.to_datetime)
+    @events = @activity.events(from: @date_from.to_datetime, to: @date_to.to_datetime)
 
     respond_to do |format|
       format.html do
-        respond_html(events)
+        respond_html
       end
       format.atom do
-        respond_atom(events)
+        respond_atom
       end
     end
   rescue ActiveRecord::RecordNotFound => e
@@ -89,25 +89,18 @@ class ActivitiesController < ApplicationController
     @author = params[:user_id].blank? ? nil : User.active.find(params[:user_id])
   end
 
-  def respond_html(events)
-    @events_by_day = events.group_by { |e| e.event_datetime.in_time_zone(User.current.time_zone).to_date }
-    @journals_by_id = Journal
-      .includes(:data, :customizable_journals, :attachable_journals, :bcf_comment)
-      .find(events.pluck(:event_id))
-      .then { |journals| ::API::V3::Activities::ActivityEagerLoadingWrapper.wrap(journals) }
-      .index_by(&:id)
-
+  def respond_html
     render layout: !request.xhr?
   end
 
-  def respond_atom(events)
+  def respond_atom
     title = t(:label_activity)
     if @author
       title = @author.name
     elsif @activity.scope.size == 1
       title = t("label_#{@activity.scope.first.singularize}_plural")
     end
-    render_feed(events, title: "#{@project || Setting.app_title}: #{title}")
+    render_feed(@events, title: "#{@project || Setting.app_title}: #{title}")
   end
 
   def activity_scope

app/controllers/users_controller.rb

@@ -69,26 +69,14 @@ class UsersController < ApplicationController
                         .where.not(project_id: nil)
                         .visible(current_user)
 
-    events = Activities::Fetcher.new(User.current, author: @user).events(nil, nil, limit: 10)
-
-    if can_show_user?(events)
-      render_show_user(events)
+    if can_show_user?
+      @events = events
+      render layout: 'no_menu'
     else
       render_404
     end
   end
 
-  def render_show_user(events)
-    @events_by_day = events.group_by { |e| e.event_datetime.to_date }
-    @journals_by_id = Journal
-      .includes(:data, :customizable_journals, :attachable_journals, :bcf_comment)
-      .find(events.pluck(:event_id))
-      .then { |journals| ::API::V3::Activities::ActivityEagerLoadingWrapper.wrap(journals) }
-      .index_by(&:id)
-
-    render layout: 'no_menu'
-  end
-
   def new
     @user = User.new(language: Setting.default_language)
   end
@@ -242,7 +230,7 @@ class UsersController < ApplicationController
 
   private
 
-  def can_show_user?(events)
+  def can_show_user?
     return true if current_user.allowed_to_globally?(:manage_user)
     return true if @user == User.current
 
@@ -250,6 +238,10 @@ class UsersController < ApplicationController
     && (@memberships.present? || events.present?)
   end
 
+  def events
+    @events ||= Activities::Fetcher.new(User.current, author: @user).events(limit: 10)
+  end
+
   def find_user
     if params[:id] == 'current' || params['id'].nil?
       require_login || return

app/models/activities/base_activity_provider.rb

@@ -175,7 +175,7 @@ class Activities::BaseActivityProvider
     params = { provider: self,
                event_id: event_data['event_id'],
                event_description: event_data['event_description'],
-               author_id: event_data['event_author'].to_i,
+               author_id: event_data['author_id'].to_i,
                journable_id: event_data['journable_id'],
                project_id: event_data['project_id'].to_i }
 
@@ -191,7 +191,7 @@ class Activities::BaseActivityProvider
   def event_projection
     [[:id, 'event_id'],
      [:created_at, 'event_datetime'],
-     [:user_id, 'event_author'],
+     [:user_id, 'author_id'],
      [:notes, 'event_description'],
      [:version, 'version'],
      [:journable_id, 'journable_id']].map do |column, alias_name|

app/models/activities/event.rb

@@ -5,13 +5,14 @@ module Activities
                      :event_title,
                      :event_description,
                      :author_id,
-                     :event_author,
                      :event_datetime,
                      :journable_id,
                      :project_id,
-                     :project,
                      :event_type,
                      :event_path,
                      :event_url,
-                     keyword_init: true)
+                     # attributes below are eager loaded by Activities::Fetcher
+                     :event_author,
+                     :journal,
+                     :project)
 end

app/models/activities/fetcher.rb

@@ -61,12 +61,12 @@ module Activities
 
     # Returns an array of events for the given date range
     # sorted in reverse chronological order
-    def events(from = nil, to = nil, limit: nil)
+    def events(from: nil, to: nil, limit: nil)
       events = events_from_providers(from, to, limit)
 
       eager_load_associations(events)
 
-      sort_by_date(events)
+      sort_by_most_recent_first(events)
     end
 
     protected
@@ -104,30 +104,38 @@ module Activities
     def eager_load_associations(events)
       projects = projects_of_event_set(events)
       users = users_of_event_set(events)
+      journals = journals_of_event_set(events)
 
       events.each do |e|
-        e.event_author = users[e.author_id]&.first
-        e.project = projects[e.project_id]&.first
+        e.event_author = users[e.author_id]
+        e.project = projects[e.project_id]
+        e.journal = journals[e.event_id]
       end
     end
 
     def projects_of_event_set(events)
-      project_ids = events.map(&:project_id).compact.uniq
+      project_ids = events.filter_map(&:project_id).uniq
 
-      if project_ids.any?
-        Project.find(project_ids).group_by(&:id)
-      else
-        {}
-      end
+      Project.find(project_ids).index_by(&:id)
     end
 
     def users_of_event_set(events)
-      user_ids = events.map(&:author_id).compact.uniq
+      user_ids = events.filter_map(&:author_id).uniq
 
-      User.where(id: user_ids).group_by(&:id)
+      User.where(id: user_ids).index_by(&:id)
     end
 
-    def sort_by_date(events)
+    def journals_of_event_set(events)
+      journal_ids = events.map(&:event_id)
+
+      Journal
+        .includes(:data, :customizable_journals, :attachable_journals, :bcf_comment)
+        .find(journal_ids)
+        .then { |journals| ::API::V3::Activities::ActivityEagerLoadingWrapper.wrap(journals) }
+        .index_by(&:id)
+    end
+
+    def sort_by_most_recent_first(events)
       events.sort { |a, b| b.event_datetime <=> a.event_datetime }
     end
 

app/views/activities/index.html.erb

@@ -33,15 +33,9 @@ See COPYRIGHT and LICENSE files for more details.
             subtitle: t(:label_date_from_to, start: format_date(@date_to - @days), end: format_date(@date_to-1))
 %>
 
-<div class="op-activity-days">
-  <% @events_by_day.sort_by { |day, _events| day }.reverse_each do |day, events| %>
-    <h3 class="op-activity-days--header"><%= format_activity_day(day) %></h3>
+<%= render(Activities::DaysComponent.new(events: @events))  %>
 
-    <%= render(Activities::ListComponent.new(events:, journals_by_id: @journals_by_id)) %>
-  <% end -%>
-</div>
-
-<% if @events_by_day.empty? %>
+<% if @events.empty? %>
   <%= no_results_box %>
   <br>
 <% end %>

app/views/users/show.html.erb

@@ -73,20 +73,14 @@ See COPYRIGHT and LICENSE files for more details.
   </div>
 
   <div class="grid-content">
-    <% unless @events_by_day.empty? %>
+    <% unless @events.empty? %>
       <h3>
-        <%= link_to t(:label_activity), controller: '/activities', action: 'index', id: nil, user_id: @user, from: @events_by_day.keys.first %>
+        <%= link_to t(:label_activity), controller: '/activities', action: 'index', id: nil, user_id: @user, from: @events.first.event_datetime %>
       </h3>
       <p>
         <%=t(:label_reported_work_packages)%>: <%= @user.reported_work_package_count %>
       </p>
-      <div id="activity" class="op-activity-days">
-        <% @events_by_day.sort_by { |day, _events| day }.reverse_each do |day, events| -%>
-          <h4 class="op-activity-days--header"><%= format_activity_day(day) %></h3>
-
-          <%= render(Activities::ListComponent.new(events:, journals_by_id: @journals_by_id, display_user: false)) %>
-        <% end -%>
-      </div>
+      <%= render(Activities::DaysComponent.new(events: @events, display_user: false, header_tag: 'h4'))  %>
 
       <%= other_formats_links do |f| %>
         <%= f.link_to 'Atom', url: {controller: '/activities', action: 'index', id: nil, user_id: @user, key: User.current.rss_key} %>

spec/components/activities/item_component_spec.rb

@@ -36,9 +36,10 @@ RSpec.describe Activities::ItemComponent, type: :component do
       event_title: "Event Title",
       event_description: "something",
       event_datetime: journal.created_at,
+      event_path: "/project/123",
       project_id: project.id,
       project:,
-      event_path: "/project/123"
+      journal:
     )
   end
   let(:project) { build_stubbed(:project) }
@@ -46,14 +47,14 @@ RSpec.describe Activities::ItemComponent, type: :component do
 
   it 'renders the title escaped' do
     event.event_title = 'Hello <b>World</b>!'
-    render_inline(described_class.new(event:, journal:))
+    render_inline(described_class.new(event:))
 
     expect(page).to have_css('.op-activity-list--item-title', text: 'Hello <b>World</b>!')
   end
 
   it 'renders the project name to which the event belongs, escaped' do
     event.project.name = 'Project <b>name</b> with HTML'
-    render_inline(described_class.new(event:, journal:))
+    render_inline(described_class.new(event:))
 
     expect(page).to have_css('.op-activity-list--item-title', text: '(Project: Project <b>name</b> with HTML)')
   end

spec/controllers/activities_controller_spec.rb

@@ -63,7 +63,7 @@ describe ActivitiesController do
 
       it_behaves_like 'valid index response'
 
-      it { expect(assigns(:events_by_day)).not_to be_empty }
+      it { expect(assigns(:events)).not_to be_empty }
 
       describe 'view' do
         render_views
@@ -86,7 +86,7 @@ describe ActivitiesController do
 
         it_behaves_like 'valid index response'
 
-        it { expect(assigns(:events_by_day)).to be_empty }
+        it { expect(assigns(:events)).to be_empty }
       end
     end
 

spec/controllers/users_controller_spec.rb

@@ -935,14 +935,6 @@ describe UsersController do
 
         expect(response.body).to have_selector('p', text: /#{label}.*42/)
       end
-
-      it 'has @events_by_day grouped by day' do
-        expect(assigns(:events_by_day).keys.first.class).to eq(Date)
-      end
-
-      it 'has more than one event for today' do
-        expect(assigns(:events_by_day).first.size).to be > 1
-      end
     end
   end
 

spec/models/activities/fetcher_integration_spec.rb

@@ -54,7 +54,7 @@ describe Activities::Fetcher, 'integration' do
       create(:wiki_page, wiki:, content:)
     end
 
-    subject { instance.events(30.days.ago, 1.day.from_now) }
+    subject { instance.events(from: 30.days.ago, to: 1.day.from_now) }
 
     context 'for global activities' do
       let!(:activities) { [project, work_package, message, news, time_entry, changeset, wiki_page.content] }

spec/models/repository/git_spec.rb

@@ -412,7 +412,7 @@ describe Repository::Git do
         def find_events(user, options = {})
           options[:scope] = ['changesets']
           fetcher = Activities::Fetcher.new(user, options)
-          fetcher.events(30.days.ago, 1.day.from_now)
+          fetcher.events(from: 30.days.ago, to: 1.day.from_now)
         end
 
         it 'activitieses' do

spec/models/repository/subversion_spec.rb

@@ -309,7 +309,7 @@ describe Repository::Subversion do
         def find_events(user, options = {})
           options[:scope] = ['changesets']
           fetcher = Activities::Fetcher.new(user, options)
-          fetcher.events(30.days.ago, 1.day.from_now)
+          fetcher.events(from: 30.days.ago, to: 1.day.from_now)
         end
 
         it 'finds events' do

spec/views/users/show.html.erb_spec.rb

@@ -45,7 +45,7 @@ describe 'users/show' do
 
     assign(:user, user)
     assign(:memberships, user.memberships)
-    assign(:events_by_day, [])
+    assign(:events, [])
   end
 
   it 'renders the visible custom values' do