Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove explicit project column from notifications #16492

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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