Skip to content

Commit

Permalink
remove explicit project column from notifications
Browse files Browse the repository at this point in the history
  • Loading branch information
ulferts committed Aug 22, 2024
1 parent 1c6a142 commit 70407f5
Show file tree
Hide file tree
Showing 36 changed files with 230 additions and 132 deletions.
1 change: 0 additions & 1 deletion app/contracts/notifications/create_contract.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ class CreateContract < ::ModelContract
attribute :recipient
attribute :subject
attribute :reason
attribute :project
attribute :actor
attribute :resource
attribute :journal
Expand Down
3 changes: 1 addition & 2 deletions app/mailers/digest_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,9 @@ def work_packages(recipient_id, notification_ids)
def load_notifications(notification_ids)
Notification
.where(id: notification_ids)
.includes(:project, :resource)
.includes(:resource)
.reject do |notification|
notification.resource.nil? ||
notification.project.nil? ||
(notification.journal.nil? && !notification.date_alert?)
end
end
Expand Down
1 change: 0 additions & 1 deletion app/models/notification.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ class Notification < ApplicationRecord

belongs_to :recipient, class_name: "User"
belongs_to :actor, class_name: "User"
belongs_to :project
belongs_to :journal
belongs_to :resource, polymorphic: true

Expand Down
11 changes: 11 additions & 0 deletions app/models/queries/notifications/filters/project_filter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,15 @@

class Queries::Notifications::Filters::ProjectFilter < Queries::Notifications::Filters::NotificationFilter
include Queries::Filters::Shared::ProjectFilter::Optional

# This is currently work package specific same as all the other parts of the NotificationQuery
self.model = WorkPackage

def joins
<<~SQL.squish
JOIN #{WorkPackage.table_name}
ON #{WorkPackage.table_name}.id = #{Notification.table_name}.resource_id
AND #{Notification.table_name}.resource_type = 'WorkPackage'
SQL
end
end
10 changes: 10 additions & 0 deletions app/models/queries/notifications/group_bys/group_by_project.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,16 @@ def self.key
:project
end

def joins
# Only Notifications for work_packages are currently supported via the query.
# E.g. the visible statement used in the query is WorkPackage specific.
<<~SQL.squish
JOIN work_packages
ON notifications.resource_id = work_packages.id
AND notifications.resource_type = 'WorkPackage'
SQL
end

def name
:project_id
end
Expand Down
16 changes: 12 additions & 4 deletions app/models/queries/notifications/orders/project_order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,23 @@ def self.key
end

def joins
:project
<<~SQL.squish
JOIN #{WorkPackage.table_name} work_packages_order
ON work_packages_order.id = #{Notification.table_name}.resource_id
AND #{Notification.table_name}.resource_type = 'WorkPackage'
JOIN #{Project.table_name}
ON #{Project.table_name}.id = work_packages_order.project_id
SQL
end

protected

def order(scope)
order_string = "projects.name"
order_string += " DESC" if direction == :desc
with_raise_on_invalid do
order_string = "#{Project.table_name}.name"
order_string += " DESC" if direction == :desc

scope.order(order_string)
scope.order(order_string)
end
end
end
1 change: 0 additions & 1 deletion app/services/notifications/create_from_model_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ def update_or_create_notification(recipient_id, reason)
def create_notification(recipient_id, reason)
notification_attributes = {
recipient_id:,
project:,
resource:,
journal:,
actor: user_with_fallback,
Expand Down
17 changes: 1 addition & 16 deletions app/services/notifications/set_attributes_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,5 @@
#++

module Notifications
class SetAttributesService < ::BaseServices::SetAttributes
private

def set_default_attributes(params)
super

set_default_project unless model.project
end

##
# Try to determine the project context from the journal (if any)
# or the resource if it has a project set
def set_default_project
model.project = model.journal&.project || model.resource.try(:project)
end
end
class SetAttributesService < ::BaseServices::SetAttributes; end
end
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ def create_date_alert_notification(user, work_package, reason)
create_service = Notifications::CreateService.new(user:)
create_service.call(
recipient_id: user.id,
project_id: work_package.project_id,
resource: work_package,
reason:
)
Expand Down
83 changes: 83 additions & 0 deletions db/migrate/20240820123011_remove_project_from_notification.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
#-- copyright
# OpenProject is an open source project management software.
# Copyright (C) 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 RemoveProjectFromNotification < ActiveRecord::Migration[7.1]
def change
reversible do |direction|
direction.down do
execute <<~SQL.squish
UPDATE notifications
SET project_id = work_package_journals.project_id
FROM journals
JOIN work_package_journals
ON journals.data_id = work_package_journals.id AND journals.data_type = 'Journal::WorkPackageJournal'
WHERE notifications.journal_id = journals.id AND notifications.resource_type = 'WorkPackage'
SQL

execute <<~SQL.squish
UPDATE notifications
SET project_id = forums.project_id
FROM journals
JOIN message_journals
ON journals.data_id = message_journals.id AND journals.data_type = 'Journal::MessageJournal'
JOIN forums ON message_journals.forum_id = forums.id
WHERE notifications.journal_id = journals.id AND notifications.resource_type = 'Message'
SQL

execute <<~SQL.squish
UPDATE notifications
SET project_id = wikis.project_id
FROM wiki_pages
JOIN wikis
ON wiki_pages.wiki_id = wikis.id
WHERE notifications.resource_id = wiki_pages.id AND notifications.resource_type = 'WikiPage'
SQL

execute <<~SQL.squish
UPDATE notifications
SET project_id = news_journals.project_id
FROM journals
JOIN news_journals
ON journals.data_id = news_journals.id AND journals.data_type = 'Journal::NewsJournal'
WHERE notifications.journal_id = journals.id AND notifications.resource_type = 'News'
SQL

execute <<~SQL.squish
UPDATE notifications
SET project_id = news.project_id
FROM comments
JOIN news
ON comments.commented_id = news.id AND comments.commented_type = 'News'
WHERE notifications.resource_id = comments.id AND notifications.resource_type = 'Comment'
SQL
end
end

remove_reference :notifications, :project
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,10 @@ def wrap(notifications)
# because it is a polymorphic association. That being as it is, currently only
# work packages are assigned.
def set_resource(notifications)
work_packages_by_id = WorkPackage.where(id: notifications.pluck(:resource_id).uniq).index_by(&:id)
work_packages_by_id = WorkPackage
.includes(:project)
.where(id: notifications.pluck(:resource_id).uniq)
.index_by(&:id)

notifications.each do |notification|
notification.resource = work_packages_by_id[notification.resource_id]
Expand Down
19 changes: 16 additions & 3 deletions lib/api/v3/notifications/notification_representer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,6 @@ class NotificationRepresenter < ::API::Decorators::Single
skip_render: ->(*) { represented.actor.nil? },
v3_path: :user

associated_resource :project

associated_resource :journal,
as: :activity,
representer: ::API::V3::Activities::ActivityRepresenter,
Expand All @@ -87,11 +85,26 @@ class NotificationRepresenter < ::API::Decorators::Single

polymorphic_resource :resource

resource :project,
getter: ->(*) {
::API::V3::Projects::ProjectRepresenter
.create(represented.resource.project, current_user:, embed_links:)
},
link: ->(*) {
{
href: api_v3_paths.project(represented.resource.project.id),
title: represented.resource.project.name
}
},
setter: ->(*) {
raise NotImplementedError
}

def _type
"Notification"
end

self.to_eager_load = %i[project actor journal]
self.to_eager_load = %i[actor journal]
end
end
end
Expand Down
4 changes: 1 addition & 3 deletions spec/contracts/notifications/create_contract_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
end
end

let(:notification_context) { build_stubbed(:project) }
let(:notification_resource) { build_stubbed(:journal) }
let(:notification_recipient) { build_stubbed(:user) }
let(:notification_subject) { "Some text" }
Expand All @@ -46,8 +45,7 @@
let(:notification_mail_reminder_sent) { false }

let(:notification) do
Notification.new(project: notification_context,
recipient: notification_recipient,
Notification.new(recipient: notification_recipient,
subject: notification_subject,
reason: notification_reason,
resource: notification_resource,
Expand Down
5 changes: 2 additions & 3 deletions spec/factories/notification_factory.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,10 @@
mail_alert_sent { false }
reason { :mentioned }
recipient factory: :user
project
resource { association :work_package, project: }
resource { association :work_package }

trait :for_milestone do
resource { association :work_package, :is_milestone, project: }
resource { association :work_package, :is_milestone }
end
# journal and actor are not listed by intend.
# They will be set in the after_build callback.
Expand Down
2 changes: 0 additions & 2 deletions spec/features/notifications/navigation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,13 @@
shared_let(:notification) do
create(:notification,
recipient:,
project:,
resource: work_package,
journal: work_package.journals.last)
end

shared_let(:second_notification) do
create(:notification,
recipient:,
project:,
resource: second_work_package,
journal: second_work_package.journals.last)
end
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
require "spec_helper"
require "features/page_objects/notification"

RSpec.describe "Notification center date alert and mention",
:js,
:with_cuprite,
with_settings: { journal_aggregation_time_minutes: 0 } do
shared_let(:project) { create(:project) }
shared_let(:actor) { create(:user, firstname: "Actor", lastname: "User") }
shared_let(:user) do
create(:user,
member_with_permissions: { project => %w[view_work_packages] })
end
shared_let(:work_package) { create(:work_package, project:, due_date: 1.day.ago) }

shared_let(:notification_mention) do
create(:notification,
reason: :mentioned,
recipient: user,
resource: work_package,
actor:)
end

shared_let(:notification_date_alert) do
create(:notification,
reason: :date_alert_due_date,
recipient: user,
resource: work_package)
end

let(:center) { Pages::Notifications::Center.new }

before do
login_as user
visit notifications_center_path
wait_for_reload
end

context "with date alerts ee", with_ee: %i[date_alerts] do
it "shows only the date alert time, not the mentioned author" do
center.within_item(notification_date_alert) do
expect(page).to have_text("Date alert, Mentioned")
expect(page).to have_no_text("Actor user")
end
end
end
end
Loading

0 comments on commit 70407f5

Please sign in to comment.