diff --git a/app/models/notifications/scopes/visible.rb b/app/models/notifications/scopes/visible.rb index 733ba28e39be..9ab437346f8e 100644 --- a/app/models/notifications/scopes/visible.rb +++ b/app/models/notifications/scopes/visible.rb @@ -36,10 +36,31 @@ module Visible # * the user is the recipient # * the user has the permission to see the associated resource # - # As currently only notifications for work packages exist, the implementation is work package specific. + # Notifications exist for work packages and documents. Each resource type is + # checked against the visibility scope of its own model. def visible(user) recipient(user) - .where(resource_type: "WorkPackage", resource_id: WorkPackage.visible(user).select(:id)) + .merge(visible_resources(user)) + end + + # The resource types notifications can be visible for, mapped to the scope + # of resources of that type visible to the user. + def visible_resource_scopes(user) + { + "WorkPackage" => WorkPackage.visible(user), + "Document" => Document.visible(user) + } + end + + private + + # One `resource_type = ... AND resource_id IN (subquery)` condition per resource type, + # combined by OR. Built on an unscoped relation so that the conditions of the scope + # `visible` is called on are not repeated within each OR branch. + def visible_resources(user) + visible_resource_scopes(user) + .map { |resource_type, scope| unscoped.where(resource_type:, resource_id: scope.select(:id)) } + .reduce(:or) end end end diff --git a/app/models/queries/notifications/filters/project_filter.rb b/app/models/queries/notifications/filters/project_filter.rb index 56ce09253d52..9080e9d6eba3 100644 --- a/app/models/queries/notifications/filters/project_filter.rb +++ b/app/models/queries/notifications/filters/project_filter.rb @@ -30,14 +30,22 @@ 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 + # Notifications are matched if their resource (e.g. a work package or a document) + # matches the filter on its project. The condition is evaluated per resource type + # within a subquery so that each operator (including the negating ones) is applied + # only to the resource the notification is about. + def where + conditions = Queries::Notifications::ResourceProject.resource_classes.map do |klass| + <<~SQL.squish + (#{Notification.table_name}.resource_type = '#{klass.name}' + AND #{Notification.table_name}.resource_id IN ( + SELECT #{klass.table_name}.id + FROM #{klass.table_name} + WHERE #{operator_strategy.sql_for_field(values, klass.table_name, :project_id)} + )) + SQL + end - 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 + "(#{conditions.join(' OR ')})" end end diff --git a/app/models/queries/notifications/filters/resource_id_filter.rb b/app/models/queries/notifications/filters/resource_id_filter.rb index 010e52253942..c722fae81b5f 100644 --- a/app/models/queries/notifications/filters/resource_id_filter.rb +++ b/app/models/queries/notifications/filters/resource_id_filter.rb @@ -29,9 +29,11 @@ class Queries::Notifications::Filters::ResourceIdFilter < Queries::Notifications::Filters::NotificationFilter def allowed_values - WorkPackage - .visible(User.current) - .pluck(:id, :id) + Notification + .visible_resource_scopes(User.current) + .values + .flat_map { |scope| scope.pluck(:id, :id) } + .uniq end def type diff --git a/app/models/queries/notifications/filters/resource_type_filter.rb b/app/models/queries/notifications/filters/resource_type_filter.rb index 89daecf16b45..d3a0c6f19144 100644 --- a/app/models/queries/notifications/filters/resource_type_filter.rb +++ b/app/models/queries/notifications/filters/resource_type_filter.rb @@ -29,7 +29,7 @@ class Queries::Notifications::Filters::ResourceTypeFilter < Queries::Notifications::Filters::NotificationFilter def allowed_values - [[WorkPackage.name, WorkPackage.name]] + Notification.visible_resource_scopes(User.current).keys.map { |resource_type| [resource_type, resource_type] } end def type diff --git a/app/models/queries/notifications/group_bys/group_by_project.rb b/app/models/queries/notifications/group_bys/group_by_project.rb index 8c5da2a993c1..c5b93b20bf07 100644 --- a/app/models/queries/notifications/group_bys/group_by_project.rb +++ b/app/models/queries/notifications/group_bys/group_by_project.rb @@ -34,18 +34,10 @@ 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 + delegate :joins, to: Queries::Notifications::ResourceProject def name - :project_id + Arel.sql(Queries::Notifications::ResourceProject.project_id_sql) end def association_class diff --git a/app/models/queries/notifications/orders/project_order.rb b/app/models/queries/notifications/orders/project_order.rb index 28fae28ae556..a89890cf26a2 100644 --- a/app/models/queries/notifications/orders/project_order.rb +++ b/app/models/queries/notifications/orders/project_order.rb @@ -30,25 +30,27 @@ class Queries::Notifications::Orders::ProjectOrder < Queries::Orders::Base self.model = Notification + PROJECTS_ALIAS = "notification_projects" + def self.key :project end def joins - <<~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 + [ + Queries::Notifications::ResourceProject.joins, + <<~SQL.squish + LEFT OUTER JOIN #{Project.table_name} #{PROJECTS_ALIAS} + ON #{PROJECTS_ALIAS}.id = #{Queries::Notifications::ResourceProject.project_id_sql} + SQL + ] end protected def order(scope) with_raise_on_invalid do - order_string = "#{Project.table_name}.name" + order_string = "#{PROJECTS_ALIAS}.name" order_string += " DESC" if direction == :desc scope.order(order_string) diff --git a/app/models/queries/notifications/resource_project.rb b/app/models/queries/notifications/resource_project.rb new file mode 100644 index 000000000000..bc379b7f7b23 --- /dev/null +++ b/app/models/queries/notifications/resource_project.rb @@ -0,0 +1,69 @@ +# frozen_string_literal: true + +#-- 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. +#++ + +# The project of a notification is the project of its resource. Notifications are +# created for resources of different types (work packages and documents), all of which +# store their project in a `project_id` column. +module Queries::Notifications::ResourceProject + module_function + + def resource_classes + [WorkPackage, Document] + end + + # LEFT OUTER JOINs every resource table under an alias of its own. Only the table + # matching the notification's resource_type will have a matching row. + # Identical in every query part using it so that Rails joins the tables only once + # when a query is filtered, grouped and ordered by project at the same time. + def joins + resource_classes + .map do |klass| + table_alias = table_alias_for(klass) + + <<~SQL.squish + LEFT OUTER JOIN #{klass.table_name} #{table_alias} + ON #{table_alias}.id = #{Notification.table_name}.resource_id + AND #{Notification.table_name}.resource_type = '#{klass.name}' + SQL + end + .join(" ") + end + + # The project id of the notification's resource. Requires the `joins`. + def project_id_sql + columns = resource_classes.map { |klass| "#{table_alias_for(klass)}.project_id" } + + "COALESCE(#{columns.join(', ')})" + end + + def table_alias_for(klass) + "notification_#{klass.table_name}" + end +end diff --git a/config/locales/js-en.yml b/config/locales/js-en.yml index a031761e4357..95df150c9c62 100644 --- a/config/locales/js-en.yml +++ b/config/locales/js-en.yml @@ -504,6 +504,7 @@ en: label_description: "Description" label_details: "Details" label_display: "Display" + label_document: "Document" label_drop_files: "Drop files here to attach files." label_drop_folders_hint: You cannot upload folders as an attachment. Please select single files. label_drop_or_click_files: "Drop files here or click to attach files." diff --git a/docs/api/apiv3/components/schemas/notification_model.yml b/docs/api/apiv3/components/schemas/notification_model.yml index b010c89069de..a69617ff584f 100644 --- a/docs/api/apiv3/components/schemas/notification_model.yml +++ b/docs/api/apiv3/components/schemas/notification_model.yml @@ -46,7 +46,6 @@ properties: type: object required: - project - - resource properties: actor: $ref: './user_model.yml' @@ -58,6 +57,9 @@ properties: activity: $ref: './activity_model.yml' resource: + description: |- + The resource of the notification. Only embedded for work packages. + Documents are only linked via `_links.resource`. oneOf: - $ref: './work_package_model.yml' _links: @@ -66,7 +68,6 @@ properties: - self - project - actor - - activity - resource properties: self: @@ -105,14 +106,15 @@ properties: allOf: - "$ref": "./link.yml" - description: |- - The linked resource of the notification, if any. + The resource the notification is about. This can be either a work package or a document. + The link's title is the work package's subject or the document's title respectively. - **Resource**: Polymorphic + **Resource**: WorkPackage or Document activity: allOf: - "$ref": "./link.yml" - description: |- - The journal activity, if the notification originated from a journal entry. This might be null in - case no activity triggered the notification. + The journal activity, if the notification originated from a journal entry. Only present + if the notification was triggered by an activity, e.g. not for date alerts or mentions in documents. **Resource**: Activity diff --git a/docs/api/apiv3/paths/notifications.yml b/docs/api/apiv3/paths/notifications.yml index 05002c138c5e..cfd5689a8942 100644 --- a/docs/api/apiv3/paths/notifications.yml +++ b/docs/api/apiv3/paths/notifications.yml @@ -72,7 +72,7 @@ get: + resourceId: Filter by the id of the resource the notification was created for. Ideally used together with the `resourceType` filter. - + resourceType: Filter by the type of the resource the notification was created for. Ideally used together with the `resourceId` filter. + + resourceType: Filter by the type of the resource the notification was created for, i.e. `WorkPackage` or `Document`. Ideally used together with the `resourceId` filter. example: '[{ "readIAN": { "operator": "=", "values": ["t"] } }]' required: false schema: diff --git a/docs/api/apiv3/tags/notifications.yml b/docs/api/apiv3/tags/notifications.yml index 15fb39cce7a1..9415630210f1 100644 --- a/docs/api/apiv3/tags/notifications.yml +++ b/docs/api/apiv3/tags/notifications.yml @@ -4,6 +4,9 @@ description: |- Notifications are triggered by actions carried out in the system by users, e.g. editing a work package, but can also be send out because of time passing e.g. when a user is notified of a work package that is overdue. + Besides work packages, notifications can also concern documents (currently when a user is mentioned in a document). + In that case, the document is only linked as the notification's resource, not embedded. + Listing notifications only returns in-app notifications, not notifications that are only send via email. ## Actions @@ -21,7 +24,7 @@ description: |- | project | The project containing the resource | Project | not null | READ | | | actor | The user that caused the notification | User | | READ | optional | | resource | The resource the notification belongs to | Polymorphic | not null | READ | | - | activity | The journal the notification belongs to | Polymorphic | | READ | optional | + | activity | The journal the notification belongs to | Polymorphic | | READ | only if triggered by an activity | | details | A list of objects including detailed information | Polymorphic | | READ | optional | ## Local Properties diff --git a/frontend/src/app/core/path-helper/path-helper.service.ts b/frontend/src/app/core/path-helper/path-helper.service.ts index cb32f17975e1..2c1d70b280bc 100644 --- a/frontend/src/app/core/path-helper/path-helper.service.ts +++ b/frontend/src/app/core/path-helper/path-helper.service.ts @@ -57,6 +57,10 @@ export class PathHelperService { return `${this.staticBase}/attribute_help_texts/${id.toString()}/show_dialog`; } + public documentPath(documentId:string|number):string { + return `${this.staticBase}/documents/${documentId.toString()}`; + } + public fileLinksPath():string { return `${this.api.v3.apiV3Base}/file_links`; } diff --git a/frontend/src/app/features/in-app-notifications/center/in-app-notification-center.component.html b/frontend/src/app/features/in-app-notifications/center/in-app-notification-center.component.html index 9e961ed162e3..3d10bea38ec1 100644 --- a/frontend/src/app/features/in-app-notifications/center/in-app-notification-center.component.html +++ b/frontend/src/app/features/in-app-notifications/center/in-app-notification-center.component.html @@ -19,7 +19,7 @@ [aggregatedNotifications]="records" attr.data-test-selector="op-ian-notification-item-{{records[0].id}}" [attr.data-qa-ian-read]="records[0].readIAN === true || undefined" - [attr.data-qa-ian-selected]="notificationMatchesSelectedWorkPackage(records[0], selectedWorkPackage$ | async)" + [attr.data-qa-ian-selected]="notificationMatchesSelectedWorkPackage(records[0], selectedWorkPackage$ | async) || undefined" /> } @else { diff --git a/frontend/src/app/features/in-app-notifications/center/in-app-notification-center.component.ts b/frontend/src/app/features/in-app-notifications/center/in-app-notification-center.component.ts index 6cb4f8b1e797..109ca11f289e 100644 --- a/frontend/src/app/features/in-app-notifications/center/in-app-notification-center.component.ts +++ b/frontend/src/app/features/in-app-notifications/center/in-app-notification-center.component.ts @@ -40,6 +40,7 @@ import { IanBellService } from 'core-app/features/in-app-notifications/bell/stat import { imagePath } from 'core-app/shared/helpers/images/path-helper'; import { UrlParamsService } from 'core-app/core/navigation/url-params.service'; import idFromLink from 'core-app/features/hal/helpers/id-from-link'; +import { notificationWorkPackageId } from 'core-app/features/in-app-notifications/notification-resource-id'; import { ColorsService, } from 'core-app/shared/components/colors/colors.service'; @@ -182,9 +183,12 @@ export class InAppNotificationCenterComponent implements OnInit { } notificationMatchesSelectedWorkPackage(notification:INotification, selected:string|null):boolean { - const href = notification._links.resource?.href; - const workPackageId = href ? idFromLink(href) : null; - const workPackage = workPackageId ? this.apiV3.work_packages.cache.current(workPackageId) : null; + const workPackageId = notificationWorkPackageId(notification); + if (!workPackageId) { + return false; + } + + const workPackage = this.apiV3.work_packages.cache.current(workPackageId); return selected === workPackageId || selected === workPackage?.displayId; } diff --git a/frontend/src/app/features/in-app-notifications/center/state/ian-center.service.spec.ts b/frontend/src/app/features/in-app-notifications/center/state/ian-center.service.spec.ts new file mode 100644 index 000000000000..5b9de45f9098 --- /dev/null +++ b/frontend/src/app/features/in-app-notifications/center/state/ian-center.service.spec.ts @@ -0,0 +1,130 @@ +//-- 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, see . +// +// See COPYRIGHT and LICENSE files for more details. +//++ + +import { TestBed } from '@angular/core/testing'; +import { of, Subject } from 'rxjs'; +import { type MockInstance } from 'vitest'; +import { I18nService } from 'core-app/core/i18n/i18n.service'; +import { ToastService } from 'core-app/shared/components/toaster/toast.service'; +import { ApiV3Service } from 'core-app/core/apiv3/api-v3.service'; +import { DeviceService } from 'core-app/core/browser/device.service'; +import { UrlParamsService } from 'core-app/core/navigation/url-params.service'; +import { INotification } from 'core-app/core/state/in-app-notifications/in-app-notification.model'; +import { + InAppNotificationsResourceService, +} from 'core-app/core/state/in-app-notifications/in-app-notifications.service'; +import { IanBellService } from 'core-app/features/in-app-notifications/bell/state/ian-bell.service'; +import { IanCenterService } from './ian-center.service'; + +describe('IanCenterService', () => { + let service:IanCenterService; + let selectedWorkPackage$:Subject; + let openSplitScreen:MockInstance; + let closeSplitScreen:MockInstance; + + function notificationFor(id:number, resourceHref:string):INotification[] { + return [{ id, _links: { resource: { href: resourceHref } } } as unknown as INotification]; + } + + const workPackage3 = notificationFor(1, '/api/v3/work_packages/3'); + const document5 = notificationFor(2, '/api/v3/documents/5'); + const workPackage5 = notificationFor(3, '/api/v3/work_packages/5'); + const document7 = notificationFor(4, '/api/v3/documents/7'); + + beforeEach(() => { + selectedWorkPackage$ = new Subject(); + + TestBed.configureTestingModule({ + providers: [ + IanCenterService, + { provide: I18nService, useValue: { t: (key:string) => key } }, + { provide: InAppNotificationsResourceService, useValue: {} }, + { provide: ApiV3Service, useValue: {} }, + { provide: ToastService, useValue: {} }, + { provide: UrlParamsService, useValue: { pathMatching$: () => selectedWorkPackage$ } }, + { provide: DeviceService, useValue: { isMobile: false } }, + { provide: IanBellService, useValue: {} }, + ], + }); + + service = TestBed.inject(IanCenterService); + openSplitScreen = vi.spyOn(service, 'openSplitScreen').mockImplementation(() => undefined); + closeSplitScreen = vi.spyOn(service, 'closeSplitScreen').mockImplementation(() => undefined); + }); + + describe('showNextNotification', () => { + it('skips documents and opens the next work package', () => { + service.notifications$ = of([document5, document7, workPackage5]); + service.selectedNotificationIndex = 0; + + service.showNextNotification(); + + expect(openSplitScreen).toHaveBeenCalledWith('5'); + expect(closeSplitScreen).not.toHaveBeenCalled(); + }); + + it('falls back to a preceding work package when only documents follow', () => { + service.notifications$ = of([workPackage3, document5, document7]); + service.selectedNotificationIndex = 1; + + service.showNextNotification(); + + expect(openSplitScreen).toHaveBeenCalledWith('3'); + }); + + it('closes the split screen when only documents are left', () => { + service.notifications$ = of([document5, document7]); + service.selectedNotificationIndex = 0; + + service.showNextNotification(); + + expect(openSplitScreen).not.toHaveBeenCalled(); + expect(closeSplitScreen).toHaveBeenCalledOnce(); + }); + + it('closes the split screen when no notification is left', () => { + service.notifications$ = of([]); + service.selectedNotificationIndex = 0; + + service.showNextNotification(); + + expect(openSplitScreen).not.toHaveBeenCalled(); + expect(closeSplitScreen).toHaveBeenCalledOnce(); + }); + }); + + describe('selecting the notification of the opened work package', () => { + it('ignores a document with the same id as the selected work package', () => { + service.notifications$ = of([workPackage3, document5, workPackage5]); + + selectedWorkPackage$.next('5'); + + expect(service.selectedNotificationIndex).toEqual(2); + expect(service.selectedNotification).toBe(workPackage5[0]); + }); + }); +}); diff --git a/frontend/src/app/features/in-app-notifications/center/state/ian-center.service.ts b/frontend/src/app/features/in-app-notifications/center/state/ian-center.service.ts index ed459514bd73..371a085e5e84 100644 --- a/frontend/src/app/features/in-app-notifications/center/state/ian-center.service.ts +++ b/frontend/src/app/features/in-app-notifications/center/state/ian-center.service.ts @@ -42,7 +42,6 @@ import { import { INotification } from 'core-app/core/state/in-app-notifications/in-app-notification.model'; import { EffectCallback, EffectHandler } from 'core-app/core/state/effects/effect-handler.decorator'; import { ActionsService } from 'core-app/core/state/actions/actions.service'; -import { HalResource } from 'core-app/features/hal/resources/hal-resource'; import { ApiV3Service } from 'core-app/core/apiv3/api-v3.service'; import { InAppNotificationsResourceService, @@ -54,7 +53,6 @@ import { InAppNotificationFacet, } from 'core-app/features/in-app-notifications/center/state/ian-center.store'; import { UntilDestroyedMixin } from 'core-app/shared/helpers/angular/until-destroyed.mixin'; -import idFromLink from 'core-app/features/hal/helpers/id-from-link'; import { DeviceService } from 'core-app/core/browser/device.service'; import { ApiV3ListFilter, ApiV3ListParameters } from 'core-app/core/apiv3/paths/apiv3-list-resource.interface'; import { FrameElement } from '@hotwired/turbo'; @@ -62,6 +60,7 @@ import { PathHelperService } from 'core-app/core/path-helper/path-helper.service import { UrlParamsService } from 'core-app/core/navigation/url-params.service'; import { IanBellService } from 'core-app/features/in-app-notifications/bell/state/ian-bell.service'; import { WP_ID_URL_PATTERN } from 'core-app/shared/helpers/work-package-id-pattern'; +import { notificationWorkPackageId } from 'core-app/features/in-app-notifications/notification-resource-id'; const DETAILS_URL_PATTERN = new RegExp(`/details/(${WP_ID_URL_PATTERN})(?:/|$)`); @@ -235,25 +234,30 @@ export class IanCenterService extends UntilDestroyedMixin { .notifications$ .pipe(take(1)) .subscribe((notifications:INotification[][]) => { - if (notifications.length <= 0) { - window.location.href = this.pathHelper.notificationsPath(); - return; + let index:number; + if (this.selectedNotificationIndex === notifications.length) { + // If the last notification is marked as read, we do not jump to the top, but rather show the new last notification + index = notifications.length - 1; + } else { + index = this.selectedNotificationIndex > notifications.length ? 0 : this.selectedNotificationIndex; } - if (notifications[0][0]._links.resource || notifications[this.selectedNotificationIndex][0]._links.resource) { - let index:number; - if (this.selectedNotificationIndex === notifications.length) { - // If the last notification is marked as read, we do not jump to the top, but rather show the new last notification - index = notifications.length - 1; - } else { - index = this.selectedNotificationIndex > notifications.length ? 0 : this.selectedNotificationIndex; - } - const wpId = idFromLink(notifications[index][0]._links.resource.href); - this.openSplitScreen(wpId); + const workPackageIds = notifications.map((group) => notificationWorkPackageId(group[0])); + const nextWorkPackageId = workPackageIds.slice(index).find(Boolean) + ?? workPackageIds.slice(0, index).reverse().find(Boolean); + + if (nextWorkPackageId) { + this.openSplitScreen(nextWorkPackageId); + } else { + this.closeSplitScreen(); } }); } + closeSplitScreen():void { + window.location.href = this.pathHelper.notificationsPath(); + } + /** * Pull latest notifications from API directly and trigger all related UI updates */ @@ -310,10 +314,7 @@ export class IanCenterService extends UntilDestroyedMixin { private sideLoadInvolvedWorkPackages(elements:INotification[]):Promise { const { cache } = this.apiV3Service.work_packages; const wpIds = elements - .map((element) => { - const href = element._links.resource?.href; - return href && HalResource.matchFromLink(href, 'work_packages'); - }) + .map((element) => notificationWorkPackageId(element)) .filter((id) => id && cache.stale(id)) as string[]; const promise = this @@ -340,7 +341,7 @@ export class IanCenterService extends UntilDestroyedMixin { .subscribe( (notifications:INotification[][]) => { for (let i = 0; i < notifications.length; ++i) { - if (notifications[i][0]._links.resource && idFromLink(notifications[i][0]._links.resource.href) === selected) { + if (notificationWorkPackageId(notifications[i][0]) === selected) { this.selectedNotificationIndex = i; [this.selectedNotification] = notifications[i]; return; diff --git a/frontend/src/app/features/in-app-notifications/entry/document-content/in-app-notification-document-content.component.html b/frontend/src/app/features/in-app-notifications/entry/document-content/in-app-notification-document-content.component.html new file mode 100644 index 000000000000..397c03889665 --- /dev/null +++ b/frontend/src/app/features/in-app-notifications/entry/document-content/in-app-notification-document-content.component.html @@ -0,0 +1,31 @@ +
+ + + +
+
+ + +
+
+ +
diff --git a/frontend/src/app/features/in-app-notifications/entry/document-content/in-app-notification-document-content.component.sass b/frontend/src/app/features/in-app-notifications/entry/document-content/in-app-notification-document-content.component.sass new file mode 100644 index 000000000000..5b9bf75e625c --- /dev/null +++ b/frontend/src/app/features/in-app-notifications/entry/document-content/in-app-notification-document-content.component.sass @@ -0,0 +1,18 @@ +.op-ian-document-content + display: contents + +.op-ian-item + &--top-line_without-status + grid-template-columns: max-content minmax(0, max-content) minmax(0, max-content) auto minmax(20px, max-content) + grid-template-areas: "resourceLabel project reason count buttons" + + &--resource-label + grid-area: resourceLabel + + &--document-title + grid-column: 1 / -1 + color: inherit + &:hover, + &:focus-visible + color: inherit + text-decoration: none diff --git a/frontend/src/app/features/in-app-notifications/entry/document-content/in-app-notification-document-content.component.ts b/frontend/src/app/features/in-app-notifications/entry/document-content/in-app-notification-document-content.component.ts new file mode 100644 index 000000000000..88256df012a9 --- /dev/null +++ b/frontend/src/app/features/in-app-notifications/entry/document-content/in-app-notification-document-content.component.ts @@ -0,0 +1,65 @@ +//-- 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, see . +// +// See COPYRIGHT and LICENSE files for more details. +//++ + +import { ChangeDetectionStrategy, Component, HostBinding, Input, OnInit, ViewEncapsulation, inject } from '@angular/core'; +import { I18nService } from 'core-app/core/i18n/i18n.service'; +import { PathHelperService } from 'core-app/core/path-helper/path-helper.service'; +import { INotification } from 'core-app/core/state/in-app-notifications/in-app-notification.model'; +import { notificationResourceId } from 'core-app/features/in-app-notifications/notification-resource-id'; + +@Component({ + selector: 'op-in-app-notification-document-content', + templateUrl: './in-app-notification-document-content.component.html', + styleUrls: ['./in-app-notification-document-content.component.sass'], + changeDetection: ChangeDetectionStrategy.OnPush, + encapsulation: ViewEncapsulation.None, + standalone: false, +}) +export class InAppNotificationDocumentContentComponent implements OnInit { + private I18n = inject(I18nService); + private pathHelper = inject(PathHelperService); + + @HostBinding('class.op-ian-document-content') className = true; + + @Input() notification:INotification; + + @Input() aggregatedNotifications:INotification[]; + + documentPath:string; + + text = { + document: this.I18n.t('js.label_document'), + }; + + ngOnInit():void { + this.documentPath = this.pathHelper.documentPath(notificationResourceId(this.notification, 'documents')!); + } + + onLinkClick(event:Event):void { + event.stopPropagation(); + } +} diff --git a/frontend/src/app/features/in-app-notifications/entry/has-notification-reason.ts b/frontend/src/app/features/in-app-notifications/entry/has-notification-reason.ts new file mode 100644 index 000000000000..990325e68b09 --- /dev/null +++ b/frontend/src/app/features/in-app-notifications/entry/has-notification-reason.ts @@ -0,0 +1,32 @@ +//-- 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, see . +// +// See COPYRIGHT and LICENSE files for more details. +//++ + +import { INotification } from 'core-app/core/state/in-app-notifications/in-app-notification.model'; + +export function hasNotificationReason(notifications:INotification[], reason:string):boolean { + return notifications.some((notification) => notification.reason === reason); +} diff --git a/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.html b/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.html index 3fa18ead2f9d..a10682d52e7b 100644 --- a/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.html +++ b/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.html @@ -1,49 +1,76 @@
@if (workPackage$ | async; as workPackage) { -
-
- -
- - - @if (project) { -
+ + } @else if (documentId) { + + + + } @else { +
+ @if (workPackageId) { + - - - -
- } -
- @for (item of translatedReasons | keyvalue; track item.key; let last = $last) { - - {{ item.key }}@if (!last) {, } - + + + + } + + + +
+ + +
+
+ } +
+ + + @if (project) { +
+ - -
+ } +
+ @for (item of translatedReasons | keyvalue; track item.key; let last = $last) { + + {{ item.key }}@if (!last) {, } + + } +
@if (!notification.readIAN) { } - - -
-
-
- - - - -
-
- @if (hasReminderAlert) { - - } - @if (!hasReminderAlert) { - @if (showDateAlert) { - - } - @if (!showDateAlert) { - - } - } -
-} @else { -
@if (workPackageId) { - - - - - - } - - - -
- -
+ }
-} - + diff --git a/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.sass b/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.sass index 833ed3eb8d82..894b0367c4e4 100644 --- a/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.sass +++ b/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.sass @@ -63,11 +63,12 @@ $subject-font-size: 14px align-items: center cursor: pointer + &_static + cursor: auto + &--top-line grid-area: header display: grid - grid-template-columns: max-content max-content minmax(0, max-content) minmax(0, max-content) auto minmax(20px, max-content) - grid-template-areas: "status wpIdLink project reason count buttons" grid-column-gap: 5px font-size: var(--body-font-size) align-items: baseline @@ -88,16 +89,14 @@ $subject-font-size: 14px &.-read font-weight: normal - &--work-package-id-link - grid-area: wpIdLink - &--project grid-area: project color: var(--fgColor-muted) @include text-shortener &--project-link, - &--work-package-id-link + &--work-package-id-link, + &--resource-label color: var(--fgColor-muted) font-weight: var(--base-text-weight-bold) @@ -111,10 +110,6 @@ $subject-font-size: 14px @include text-shortener color: var(--fgColor-muted) - &--status - grid-area: status - margin-right: 5px - &--buttons grid-area: buttons font-size: 16px diff --git a/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.spec.ts b/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.spec.ts new file mode 100644 index 000000000000..736c71ea2100 --- /dev/null +++ b/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.spec.ts @@ -0,0 +1,241 @@ +//-- 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, see . +// +// See COPYRIGHT and LICENSE files for more details. +//++ + +import { CUSTOM_ELEMENTS_SCHEMA } from '@angular/core'; +import { CommonModule } from '@angular/common'; +import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { NEVER, of } from 'rxjs'; +import { type Mock } from 'vitest'; +import { ApiV3Service } from 'core-app/core/apiv3/api-v3.service'; +import { I18nService } from 'core-app/core/i18n/i18n.service'; +import { TimezoneService } from 'core-app/core/datetime/timezone.service'; +import { DeviceService } from 'core-app/core/browser/device.service'; +import { UrlParamsService } from 'core-app/core/navigation/url-params.service'; +import { INotification } from 'core-app/core/state/in-app-notifications/in-app-notification.model'; +import { IanCenterService } from 'core-app/features/in-app-notifications/center/state/ian-center.service'; +import { InAppNotificationEntryComponent } from './in-app-notification-entry.component'; +import { + InAppNotificationDocumentContentComponent, +} from './document-content/in-app-notification-document-content.component'; +import { + InAppNotificationWorkPackageContentComponent, +} from './work-package-content/in-app-notification-work-package-content.component'; + +describe('InAppNotificationEntryComponent', () => { + const I18nStub = { + t(key:string, options?:{ defaultValue?:string }) { + return { + 'js.label_document': 'Document', + 'js.notifications.center.mark_as_read': 'Mark as read', + 'js.notifications.reasons.mentioned': 'Mentioned', + }[key] ?? options?.defaultValue ?? key; + }, + }; + + let fixture:ComponentFixture; + let element:HTMLElement; + let storeService:{ openSplitScreen:Mock, markAsRead:Mock, query:{ selectLoading:() => unknown } }; + let requestWorkPackage:Mock; + let turboVisit:Mock; + + function buildNotification(resourceHref:string, resourceTitle:string):INotification { + return { + id: 17, + subject: '', + createdAt: '2026-10-15T10:00:00Z', + updatedAt: '2026-10-15T10:00:00Z', + reason: 'mentioned', + readIAN: false, + readEmail: null, + expanded: false, + _links: { + resource: { href: resourceHref, title: resourceTitle }, + project: { href: '/api/v3/projects/3', title: 'Flight school' }, + actor: { href: '/api/v3/users/9', title: 'Jean Cérien' }, + activity: { href: null }, + }, + } as unknown as INotification; + } + + const documentNotification = buildNotification('/api/v3/documents/5', 'Take-off checklist'); + const workPackageNotification = buildNotification('/api/v3/work_packages/42', 'Fix the landing gear'); + + function render(notification:INotification):void { + fixture = TestBed.createComponent(InAppNotificationEntryComponent); + fixture.componentInstance.notification = notification; + fixture.componentInstance.aggregatedNotifications = [notification]; + fixture.detectChanges(); + element = fixture.nativeElement as HTMLElement; + } + + function row():HTMLElement { + return element.querySelector('.op-ian-item--row')!; + } + + function pressKey(target:HTMLElement, key:string):KeyboardEvent { + const event = new KeyboardEvent('keydown', { key, bubbles: true, cancelable: true }); + target.dispatchEvent(event); + return event; + } + + beforeEach(async () => { + storeService = { + openSplitScreen: vi.fn(), + markAsRead: vi.fn(), + query: { selectLoading: () => of(false) }, + }; + requestWorkPackage = vi.fn(() => ({ requireAndStream: () => NEVER })); + turboVisit = vi.fn(); + vi.stubGlobal('Turbo', { visit: turboVisit }); + + await TestBed.configureTestingModule({ + imports: [CommonModule], + declarations: [ + InAppNotificationEntryComponent, + InAppNotificationWorkPackageContentComponent, + InAppNotificationDocumentContentComponent, + ], + providers: [ + { provide: I18nService, useValue: I18nStub }, + { provide: IanCenterService, useValue: storeService }, + { provide: ApiV3Service, useValue: { work_packages: { id: requestWorkPackage } } }, + { provide: TimezoneService, useValue: {} }, + { provide: DeviceService, useValue: { isMobile: false } }, + { provide: UrlParamsService, useValue: {} }, + ], + schemas: [CUSTOM_ELEMENTS_SCHEMA], + }).compileComponents(); + }); + + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + describe('for a document notification', () => { + beforeEach(() => render(documentNotification)); + + it('renders the document label, project and reason as plain text and the project link in a new tab', () => { + const label = element.querySelector('.op-ian-item--resource-label')!; + expect(label.textContent).toEqual('Document'); + expect(label.closest('a')).toBeNull(); + expect(element.querySelector('.op-ian-item--work-package-id-link')).toBeNull(); + + const projectLink = element.querySelector('.op-ian-item--project-link')!; + expect(projectLink.textContent).toEqual('Flight school'); + expect(projectLink.getAttribute('href')).toEqual('/projects/3'); + expect(projectLink.target).toEqual('_blank'); + + expect(element.querySelector('.op-ian-item--reason-wrapper')!.textContent.trim()).toEqual('Mentioned'); + }); + + it('links only the title to the document in a new tab', () => { + const title = element.querySelector('.op-ian-item--middle-line a')!; + expect(title.textContent).toEqual('Take-off checklist'); + expect(title.getAttribute('href')).toEqual('/documents/5'); + expect(title.target).toEqual('_blank'); + expect(title.rel).toEqual('noopener'); + }); + + it('renders no status, type, info button or loading placeholder and requests no work package', () => { + expect(element.querySelector('op-in-app-notification-status')).toBeNull(); + expect(element.querySelector('.op-ian-item--work-package-type')).toBeNull(); + expect(element.querySelector('.icon-info1')).toBeNull(); + expect(element.querySelector('.op-ian-item--loading')).toBeNull(); + expect(element.querySelector('op-in-app-notification-actors-line')).not.toBeNull(); + expect(requestWorkPackage).not.toHaveBeenCalled(); + }); + + it('renders a static row without button role or tab stop', () => { + expect(row().hasAttribute('role')).toBe(false); + expect(row().hasAttribute('tabindex')).toBe(false); + expect(row().classList).toContain('op-ian-item--row_static'); + }); + + it('does nothing on row click, double click, Enter or Space', () => { + vi.useFakeTimers(); + + row().click(); + row().dispatchEvent(new MouseEvent('dblclick', { bubbles: true })); + const enter = pressKey(row(), 'Enter'); + const space = pressKey(row(), ' '); + vi.advanceTimersByTime(300); + + expect(storeService.openSplitScreen).not.toHaveBeenCalled(); + expect(turboVisit).not.toHaveBeenCalled(); + expect(enter.defaultPrevented).toBe(false); + expect(space.defaultPrevented).toBe(false); + }); + + it('lets Enter on the title link through to the link', () => { + const title = element.querySelector('.op-ian-item--middle-line a')!; + + expect(pressKey(title, 'Enter').defaultPrevented).toBe(false); + }); + + it('marks the notification as read', () => { + element.querySelector('[data-test-selector="mark-as-read-button"]')!.click(); + + expect(storeService.markAsRead).toHaveBeenCalledWith([17]); + }); + }); + + describe('for a work package notification', () => { + beforeEach(() => render(workPackageNotification)); + + it('requests the work package and shows the loading placeholder until it arrives', () => { + expect(requestWorkPackage).toHaveBeenCalledWith('42'); + expect(element.querySelector('.op-ian-item--loading-indicator')).not.toBeNull(); + expect(element.querySelector('.op-ian-item--resource-label')).toBeNull(); + }); + + it('renders an interactive row', () => { + expect(row().getAttribute('role')).toEqual('button'); + expect(row().getAttribute('tabindex')).toEqual('0'); + expect(row().classList).not.toContain('op-ian-item--row_static'); + }); + + it('opens the split screen on row click and Enter', () => { + vi.useFakeTimers(); + + row().click(); + vi.advanceTimersByTime(300); + expect(storeService.openSplitScreen).toHaveBeenCalledWith('42', 'activity'); + + const enter = pressKey(row(), 'Enter'); + vi.advanceTimersByTime(300); + expect(enter.defaultPrevented).toBe(true); + expect(storeService.openSplitScreen).toHaveBeenCalledTimes(2); + }); + + it('opens the full view on double click', () => { + row().dispatchEvent(new MouseEvent('dblclick', { bubbles: true })); + + expect(turboVisit).toHaveBeenCalledWith(expect.stringMatching(/^\/work_packages\/42(\?|$)/), { action: 'advance' }); + }); + }); +}); diff --git a/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.ts b/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.ts index 3041fd388923..7984ad078f26 100644 --- a/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.ts +++ b/frontend/src/app/features/in-app-notifications/entry/in-app-notification-entry.component.ts @@ -34,13 +34,16 @@ import idFromLink from 'core-app/features/hal/helpers/id-from-link'; import { I18nService } from 'core-app/core/i18n/i18n.service'; import { TimezoneService } from 'core-app/core/datetime/timezone.service'; import { PathHelperService } from 'core-app/core/path-helper/path-helper.service'; -import { HalResource } from 'core-app/features/hal/resources/hal-resource'; import { INotification } from 'core-app/core/state/in-app-notifications/in-app-notification.model'; import { IanCenterService } from 'core-app/features/in-app-notifications/center/state/ian-center.service'; import { DeviceService } from 'core-app/core/browser/device.service'; import { UrlParamsService } from 'core-app/core/navigation/url-params.service'; import { UntilDestroyedMixin } from 'core-app/shared/helpers/angular/until-destroyed.mixin'; -import { Highlighting } from 'core-app/features/work-packages/components/wp-fast-table/builders/highlighting/highlighting.functions'; +import { + notificationResourceId, + notificationWorkPackageId, +} from 'core-app/features/in-app-notifications/notification-resource-id'; +import { hasNotificationReason } from 'core-app/features/in-app-notifications/entry/has-notification-reason'; @Component({ selector: 'op-in-app-notification-entry', @@ -72,7 +75,6 @@ export class InAppNotificationEntryComponent extends UntilDestroyedMixin impleme private latestWorkPackage:WorkPackageResource|null = null; showDateAlert = false; - hasReminderAlert = false; loading$ = this.storeService.query.selectLoading(); @@ -91,21 +93,18 @@ export class InAppNotificationEntryComponent extends UntilDestroyedMixin impleme workPackageId:string|null; + documentId:string|null; + ngOnInit():void { - const href = this.notification._links.resource?.href; - this.workPackageId = href && HalResource.matchFromLink(href, 'work_packages'); + this.workPackageId = notificationWorkPackageId(this.notification); + this.documentId = notificationResourceId(this.notification, 'documents'); - this.hasReminderAlert = this.hasNotificationReason('reminder'); - this.showDateAlert = this.hasNotificationReason('dateAlert'); + this.showDateAlert = hasNotificationReason(this.aggregatedNotifications, 'dateAlert'); this.buildTranslatedReason(); this.buildProject(); this.loadWorkPackage(); } - private hasNotificationReason(reason:string):boolean { - return this.aggregatedNotifications.some((notification) => notification.reason === reason); - } - private loadWorkPackage() { // not a work package reference if (!this.workPackageId) { @@ -132,6 +131,15 @@ export class InAppNotificationEntryComponent extends UntilDestroyedMixin impleme }, 250); } + onRowKeydown(event:Event):void { + if (!this.workPackageId) { + return; + } + + event.preventDefault(); + this.onClick(); + } + showDetails():void { if (!this.workPackageId) { return; @@ -157,10 +165,6 @@ export class InAppNotificationEntryComponent extends UntilDestroyedMixin impleme Turbo.visit(link, { action: 'advance' }); } - onLinkClick(e:Event):void { - e.stopPropagation(); - } - projectClicked(event:MouseEvent):void { event.stopPropagation(); } @@ -174,10 +178,6 @@ export class InAppNotificationEntryComponent extends UntilDestroyedMixin impleme return this.deviceService.isMobile; } - typeHighlightingClass(workPackage:WorkPackageResource):string { - return Highlighting.typeClass(workPackage.type.id!); - } - private buildTranslatedReason() { const reasons:Record = {}; diff --git a/frontend/src/app/features/in-app-notifications/entry/work-package-content/in-app-notification-work-package-content.component.html b/frontend/src/app/features/in-app-notifications/entry/work-package-content/in-app-notification-work-package-content.component.html new file mode 100644 index 000000000000..0df8dcb839ec --- /dev/null +++ b/frontend/src/app/features/in-app-notifications/entry/work-package-content/in-app-notification-work-package-content.component.html @@ -0,0 +1,51 @@ +
+
+ +
+ + + +
+
+ + + + +
+
+ @if (hasReminderAlert) { + + } + @if (!hasReminderAlert) { + @if (showDateAlert) { + + } + @if (!showDateAlert) { + + } + } +
diff --git a/frontend/src/app/features/in-app-notifications/entry/work-package-content/in-app-notification-work-package-content.component.sass b/frontend/src/app/features/in-app-notifications/entry/work-package-content/in-app-notification-work-package-content.component.sass new file mode 100644 index 000000000000..3059fe045c2b --- /dev/null +++ b/frontend/src/app/features/in-app-notifications/entry/work-package-content/in-app-notification-work-package-content.component.sass @@ -0,0 +1,14 @@ +.op-ian-work-package-content + display: contents + + .op-ian-item--top-line + grid-template-columns: max-content max-content minmax(0, max-content) minmax(0, max-content) auto minmax(20px, max-content) + grid-template-areas: "status wpIdLink project reason count buttons" + +.op-ian-item + &--work-package-id-link + grid-area: wpIdLink + + &--status + grid-area: status + margin-right: 5px diff --git a/frontend/src/app/features/in-app-notifications/entry/work-package-content/in-app-notification-work-package-content.component.spec.ts b/frontend/src/app/features/in-app-notifications/entry/work-package-content/in-app-notification-work-package-content.component.spec.ts new file mode 100644 index 000000000000..01e2e1f7b22d --- /dev/null +++ b/frontend/src/app/features/in-app-notifications/entry/work-package-content/in-app-notification-work-package-content.component.spec.ts @@ -0,0 +1,111 @@ +//-- 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, see . +// +// See COPYRIGHT and LICENSE files for more details. +//++ + +import { CUSTOM_ELEMENTS_SCHEMA } from '@angular/core'; +import { CommonModule } from '@angular/common'; +import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { DeviceService } from 'core-app/core/browser/device.service'; +import { INotification } from 'core-app/core/state/in-app-notifications/in-app-notification.model'; +import { WorkPackageResource } from 'core-app/features/hal/resources/work-package-resource'; +import { InAppNotificationWorkPackageContentComponent } from './in-app-notification-work-package-content.component'; + +describe('InAppNotificationWorkPackageContentComponent', () => { + let fixture:ComponentFixture; + let element:HTMLElement; + + const workPackage = { + id: '42', + displayId: '42', + formattedId: '#42', + subject: 'Fix the landing gear', + status: { id: '1', name: 'New' }, + type: { id: '3', name: 'Task' }, + } as unknown as WorkPackageResource; + + function notificationWithReason(reason:string):INotification { + return { + id: 17, + reason, + readIAN: false, + _links: { resource: { href: '/api/v3/work_packages/42', title: workPackage.subject } }, + } as unknown as INotification; + } + + function render(...reasons:string[]):void { + const notifications = reasons.map((reason) => notificationWithReason(reason)); + fixture = TestBed.createComponent(InAppNotificationWorkPackageContentComponent); + fixture.componentRef.setInput('workPackage', workPackage); + fixture.componentRef.setInput('notification', notifications[0]); + fixture.componentRef.setInput('aggregatedNotifications', notifications); + fixture.detectChanges(); + element = fixture.nativeElement as HTMLElement; + } + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [CommonModule], + declarations: [InAppNotificationWorkPackageContentComponent], + providers: [{ provide: DeviceService, useValue: { isMobile: false } }], + schemas: [CUSTOM_ELEMENTS_SCHEMA], + }).compileComponents(); + }); + + it('renders the ID link, type and subject of the work package', () => { + render('mentioned'); + + const idLink = element.querySelector('.op-ian-item--work-package-id-link')!; + expect(idLink.textContent).toEqual('#42'); + expect(idLink.getAttribute('href')).toEqual('/work_packages/42'); + expect(element.querySelector('op-in-app-notification-status')).not.toBeNull(); + expect(element.querySelector('.op-ian-item--work-package-type')!.textContent).toEqual('Task'); + expect(element.querySelector('.op-ian-item--work-package-subject')!.textContent).toEqual('Fix the landing gear'); + }); + + it('keeps clicks on the ID link from reaching the row', () => { + render('mentioned'); + const rowClick = vi.fn(); + element.addEventListener('click', (event) => event.preventDefault(), { capture: true }); + element.addEventListener('click', rowClick); + + element.querySelector('.op-ian-item--work-package-id-link')! + .dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true })); + + expect(rowClick).not.toHaveBeenCalled(); + }); + + it.each([ + [['mentioned'], 'op-in-app-notification-actors-line'], + [['dateAlert'], 'op-in-app-notification-date-alert'], + [['dateAlert', 'reminder'], 'op-in-app-notification-reminder-alert'], + ])('renders the bottom line for %j with %s', (reasons, selector) => { + render(...reasons); + + const bottomLine = element.querySelector('.op-ian-item--bottom-line')!; + expect(bottomLine.children).toHaveLength(1); + expect(bottomLine.firstElementChild!.tagName.toLowerCase()).toEqual(selector); + }); +}); diff --git a/frontend/src/app/features/in-app-notifications/entry/work-package-content/in-app-notification-work-package-content.component.ts b/frontend/src/app/features/in-app-notifications/entry/work-package-content/in-app-notification-work-package-content.component.ts new file mode 100644 index 000000000000..a08de2cdfc8f --- /dev/null +++ b/frontend/src/app/features/in-app-notifications/entry/work-package-content/in-app-notification-work-package-content.component.ts @@ -0,0 +1,76 @@ +//-- 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, see . +// +// See COPYRIGHT and LICENSE files for more details. +//++ + +import { ChangeDetectionStrategy, Component, HostBinding, Input, OnInit, ViewEncapsulation, inject } from '@angular/core'; +import { DeviceService } from 'core-app/core/browser/device.service'; +import { PathHelperService } from 'core-app/core/path-helper/path-helper.service'; +import { INotification } from 'core-app/core/state/in-app-notifications/in-app-notification.model'; +import { WorkPackageResource } from 'core-app/features/hal/resources/work-package-resource'; +import { Highlighting } from 'core-app/features/work-packages/components/wp-fast-table/builders/highlighting/highlighting.functions'; +import { hasNotificationReason } from 'core-app/features/in-app-notifications/entry/has-notification-reason'; + +@Component({ + selector: 'op-in-app-notification-work-package-content', + templateUrl: './in-app-notification-work-package-content.component.html', + styleUrls: ['./in-app-notification-work-package-content.component.sass'], + changeDetection: ChangeDetectionStrategy.OnPush, + encapsulation: ViewEncapsulation.None, + standalone: false, +}) +export class InAppNotificationWorkPackageContentComponent implements OnInit { + readonly pathHelper = inject(PathHelperService); + readonly deviceService = inject(DeviceService); + + @HostBinding('class.op-ian-work-package-content') className = true; + + @Input() workPackage:WorkPackageResource; + + @Input() notification:INotification; + + @Input() aggregatedNotifications:INotification[]; + + hasReminderAlert = false; + + showDateAlert = false; + + ngOnInit():void { + this.hasReminderAlert = hasNotificationReason(this.aggregatedNotifications, 'reminder'); + this.showDateAlert = hasNotificationReason(this.aggregatedNotifications, 'dateAlert'); + } + + isMobile():boolean { + return this.deviceService.isMobile; + } + + onLinkClick(event:Event):void { + event.stopPropagation(); + } + + typeHighlightingClass(workPackage:WorkPackageResource):string { + return Highlighting.typeClass(workPackage.type.id!); + } +} diff --git a/frontend/src/app/features/in-app-notifications/in-app-notifications.module.ts b/frontend/src/app/features/in-app-notifications/in-app-notifications.module.ts index b5bcf4b0d449..60997f44dbfe 100644 --- a/frontend/src/app/features/in-app-notifications/in-app-notifications.module.ts +++ b/frontend/src/app/features/in-app-notifications/in-app-notifications.module.ts @@ -52,6 +52,12 @@ import { InAppNotificationDateAlertComponent } from './entry/date-alert/in-app-n import { InAppNotificationRelativeTimeComponent } from './entry/relative-time/in-app-notification-relative-time.component'; import { InAppNotificationReminderAlertComponent } from './entry/reminder-alert/in-app-notification-reminder-alert.component'; import { InAppNotificationStatusComponent } from './entry/status/in-app-notification-status.component'; +import { + InAppNotificationWorkPackageContentComponent, +} from './entry/work-package-content/in-app-notification-work-package-content.component'; +import { + InAppNotificationDocumentContentComponent, +} from './entry/document-content/in-app-notification-document-content.component'; @NgModule({ declarations: [ @@ -63,6 +69,8 @@ import { InAppNotificationStatusComponent } from './entry/status/in-app-notifica InAppNotificationDateAlertComponent, InAppNotificationRelativeTimeComponent, InAppNotificationReminderAlertComponent, + InAppNotificationWorkPackageContentComponent, + InAppNotificationDocumentContentComponent, ], imports: [ OpSharedModule, diff --git a/frontend/src/app/features/in-app-notifications/notification-resource-id.ts b/frontend/src/app/features/in-app-notifications/notification-resource-id.ts new file mode 100644 index 000000000000..145176002ded --- /dev/null +++ b/frontend/src/app/features/in-app-notifications/notification-resource-id.ts @@ -0,0 +1,38 @@ +//-- 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, see . +// +// See COPYRIGHT and LICENSE files for more details. +//++ + +import { HalResource } from 'core-app/features/hal/resources/hal-resource'; +import { INotification } from 'core-app/core/state/in-app-notifications/in-app-notification.model'; + +export function notificationResourceId(notification:INotification, resourcePath:'work_packages'|'documents'):string|null { + const href = notification._links.resource?.href; + return href ? HalResource.matchFromLink(href, resourcePath) : null; +} + +export function notificationWorkPackageId(notification:INotification):string|null { + return notificationResourceId(notification, 'work_packages'); +} diff --git a/frontend/src/app/shared/components/grids/widgets/documents/documents.component.ts b/frontend/src/app/shared/components/grids/widgets/documents/documents.component.ts index f361a99feca6..d96acede7279 100644 --- a/frontend/src/app/shared/components/grids/widgets/documents/documents.component.ts +++ b/frontend/src/app/shared/components/grids/widgets/documents/documents.component.ts @@ -71,12 +71,12 @@ export class WidgetDocumentsComponent extends AbstractWidgetComponent implements }); } - public get isEditable() { + public override get isEditable():boolean { return false; } public documentPath(document:DocumentResource) { - return `${this.pathHelper.appBasePath}/documents/${document.id}`; + return this.pathHelper.documentPath(document.id!); } public documentCreated(document:DocumentResource) { diff --git a/lib/api/decorators/polymorphic_resource.rb b/lib/api/decorators/polymorphic_resource.rb index 04d8f5935f54..a89dac51fbc8 100644 --- a/lib/api/decorators/polymorphic_resource.rb +++ b/lib/api/decorators/polymorphic_resource.rb @@ -28,7 +28,10 @@ module API module Decorators module PolymorphicResource - # Dynamically derive a linked resource from the given polymorphic resource + # Dynamically derive a linked resource from the given polymorphic resource. + # + # The `link_title_attribute` is either the name of the attribute providing the link's title + # or a callable returning that name for the resource at hand. def polymorphic_resource(name, as: nil, skip_render: ->(*) { false }, @@ -87,7 +90,7 @@ def polymorphic_link(name, title_attribute, skip_link) .new(represented, path: path_name, property_name: name, - title_attribute:, + title_attribute: title_attribute.respond_to?(:call) ? title_attribute.call(resource) : title_attribute, getter: :"#{name}_id") .to_hash end diff --git a/lib/api/v3/notifications/notification_representer.rb b/lib/api/v3/notifications/notification_representer.rb index 0e98d7a82e9b..5fdf4b6fe981 100644 --- a/lib/api/v3/notifications/notification_representer.rb +++ b/lib/api/v3/notifications/notification_representer.rb @@ -82,7 +82,12 @@ class NotificationRepresenter < ::API::Decorators::Single v3_path: :activity, skip_render: ->(*) { represented.journal_id.nil? } - polymorphic_resource :resource + # Documents are only linked, never embedded, as their representation contains the + # large content_binary. + polymorphic_resource :resource, + skip_render: ->(*) { represented.resource.is_a?(Document) }, + skip_link: ->(*) { false }, + link_title_attribute: ->(resource) { resource.is_a?(Document) ? :title : :name } resource :project, getter: ->(*) { diff --git a/lib/api/v3/notifications/notifications_api.rb b/lib/api/v3/notifications/notifications_api.rb index bf97ccfa8844..e93237dd1b95 100644 --- a/lib/api/v3/notifications/notifications_api.rb +++ b/lib/api/v3/notifications/notifications_api.rb @@ -85,7 +85,12 @@ def mention_recipient_id def bulk_update_status(attributes) if notification_query.valid? - notification_query.results.update_all({ updated_at: Time.zone.now }.merge(attributes)) + # Notifications not supporting in app notifications (read_ian being nil) must not + # be turned into in app notifications by marking them as (un)read. + notification_query + .results + .where.not(read_ian: nil) + .update_all({ updated_at: Time.zone.now }.merge(attributes)) status 204 else raise_query_errors(notification_query) diff --git a/modules/documents/app/services/notifications/create_from_model_service/document_strategy.rb b/modules/documents/app/services/notifications/create_from_model_service/document_strategy.rb index ae719790e5de..f94267bc6a51 100644 --- a/modules/documents/app/services/notifications/create_from_model_service/document_strategy.rb +++ b/modules/documents/app/services/notifications/create_from_model_service/document_strategy.rb @@ -45,11 +45,9 @@ def self.api_mention_permission :manage_documents end - # The notification center does not support documents yet. Hence, notifications - # for documents (both added documents and mentions created via the API) are only - # sent out by mail. - def self.supports_ian?(_reason) - false + # Added documents are only sent out by mail. The API passes the reason as a string. + def self.supports_ian?(reason) + reason.to_s == "mentioned" end def self.supports_mail_digest?(_reason) diff --git a/modules/documents/spec/features/notifications/document_mention_notification_spec.rb b/modules/documents/spec/features/notifications/document_mention_notification_spec.rb new file mode 100644 index 000000000000..4455d5768fd2 --- /dev/null +++ b/modules/documents/spec/features/notifications/document_mention_notification_spec.rb @@ -0,0 +1,201 @@ +# frozen_string_literal: true + +#-- 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. +#++ + +require "spec_helper" + +RSpec.describe "Document mention notifications in the notification center", :js do + include API::V3::Utilities::PathHelper + + shared_let(:project) { create(:project, enabled_module_names: %w[documents work_package_tracking]) } + shared_let(:mentioner) do + create(:user, + firstname: "Jean", + lastname: "Cérien", + member_with_permissions: { project => %i[view_documents manage_documents] }) + end + shared_let(:recipient) do + create(:user, + member_with_permissions: { project => %i[view_documents view_work_packages] }) + end + shared_let(:work_package) { create(:work_package, project:) } + shared_let(:document) { create(:document, id: work_package.id, project:, title: "Take-off checklist (Robin DR-400)") } + + let!(:document_notification) do + create(:notification, + recipient:, + resource: document, + journal: nil, + actor: mentioner, + reason: :mentioned, + read_ian: false) + end + + let(:center) { Pages::Notifications::Center.new } + let(:side_menu) { Components::Submenu.new } + + current_user { recipient } + + def visit_center + center.visit! + wait_for_reload + end + + it "shows the document row with only the title linked and leaves the row inert" do + visit_center + + center.expect_bell_count 1 + center.expect_document_item document_notification + center.within_item(document_notification) do + expect(page).to have_text("by Jean Cérien", normalize_ws: true) + expect(page).to have_css("a[href='#{project_path(project.id)}'][target='_blank']", exact_text: project.name) + expect(page).to have_no_css("[role='button'].op-ian-item--row") + end + + center.click_document_label document_notification + wait_for_network_idle + + expect(page).to have_current_path(notifications_center_path, ignore_query: true) + center.expect_item_not_read document_notification + center.expect_bell_count 1 + end + + it "marks the document notification as read" do + visit_center + + center.mark_notification_as_read document_notification + + center.expect_bell_count 0 + center.expect_no_item document_notification + expect(document_notification.reload.read_ian).to be(true) + end + + context "with a work package notification next to it" do + let!(:work_package_notification) do + create(:notification, + recipient:, + resource: work_package, + reason: :assigned) + end + + it "marks both as read with mark all as read" do + visit_center + center.expect_bell_count 2 + + center.mark_all_read + + center.expect_bell_count 0 + center.expect_no_item document_notification, work_package_notification + end + + it "does not select the document whose id equals the opened work package's id" do + expect(document.id).to eq(work_package.id) + + visit_center + center.expect_number_of_notifications 2 + center.click_item work_package_notification + + expect(page).to have_current_path(%r{/notifications/details/#{work_package.id}}) + center.expect_item_selected work_package_notification + expect(page).to have_no_css( + "[data-test-selector='op-ian-notification-item-#{document_notification.id}'][data-qa-ian-selected]" + ) + end + + it "closes the split screen instead of opening the document's id when only the document is left" do + visit_center + center.expect_number_of_notifications 2 + center.click_item work_package_notification + Pages::PrimerizedSplitWorkPackage.new(work_package).expect_open + Components::WorkPackages::Activities.new(work_package).expect_wp_has_been_created_activity(work_package) + + center.mark_notification_as_read work_package_notification + + expect(page).to have_current_path(notifications_center_path, ignore_query: true) + center.expect_number_of_notifications 1 + center.expect_document_item document_notification + end + end + + context "with document notifications from two projects" do + shared_let(:other_project) { create(:project, enabled_module_names: %w[documents]) } + shared_let(:other_document) { create(:document, project: other_project, title: "Pre-landing checklist") } + + let!(:other_document_notification) do + create(:member, + project: other_project, + principal: recipient, + roles: [create(:project_role, permissions: %i[view_documents])]) + create(:notification, + recipient:, + resource: other_document, + journal: nil, + actor: mentioner, + reason: :mentioned, + read_ian: false) + end + + it "lists the document projects in the project filter" do + visit_center + center.expect_bell_count 2 + + side_menu.expect_item_with_count other_project.name, 1 + side_menu.click_item other_project.name + side_menu.finished_loading + + center.expect_document_item other_document_notification + center.expect_no_item document_notification + end + end + + context "when the mention is created through the API" do + let!(:document_notification) { nil } + + it "shows the notification in the recipient's notification center" do + login_as mentioner + api = Rack::Test::Session.new(Rails.application) + api.post(api_v3_paths.notifications, + { + reason: "mentioned", + _links: { + resource: { href: api_v3_paths.document(document.id) }, + recipient: { href: api_v3_paths.user(recipient.id) } + } + }.to_json, + "CONTENT_TYPE" => "application/json") + expect(api.last_response).to have_http_status(:created) + + login_as recipient + visit_center + + center.expect_bell_count 1 + center.expect_document_item Notification.find_by!(resource: document, recipient:) + end + end +end diff --git a/modules/documents/spec/services/notifications/create_from_model_service/document_strategy_spec.rb b/modules/documents/spec/services/notifications/create_from_model_service/document_strategy_spec.rb index 6f7bfc3780e9..1c8d0adabf91 100644 --- a/modules/documents/spec/services/notifications/create_from_model_service/document_strategy_spec.rb +++ b/modules/documents/spec/services/notifications/create_from_model_service/document_strategy_spec.rb @@ -36,7 +36,8 @@ end describe ".supports_ian?" do - it { expect(described_class.supports_ian?(:mentioned)).to be(false) } + it { expect(described_class.supports_ian?(:mentioned)).to be(true) } + it { expect(described_class.supports_ian?("mentioned")).to be(true) } it { expect(described_class.supports_ian?(:subscribed)).to be(false) } end diff --git a/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb b/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb index 1acd4b82ad5e..b771520ee46b 100644 --- a/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb +++ b/spec/lib/api/v3/notifications/notification_representer_rendering_spec.rb @@ -176,6 +176,47 @@ .at_path("_embedded/resource/_type") end end + + context "for a document" do + let(:resource) { build_stubbed(:document, project:) } + + it_behaves_like "has a titled link" do + let(:link) { "resource" } + let(:title) { resource.title } + let(:href) { api_v3_paths.document resource.id } + end + + it_behaves_like "has a titled link" do + let(:link) { "project" } + let(:href) { api_v3_paths.project project.id } + let(:title) { project.name } + end + + it_behaves_like "has no link" do + let(:link) { "activity" } + end + + context "when embedding is true" do + let(:embed_links) { true } + + it_behaves_like "has a titled link" do + let(:link) { "resource" } + let(:title) { resource.title } + let(:href) { api_v3_paths.document resource.id } + end + + it "does not embed the resource" do + expect(generated) + .not_to have_json_path("_embedded/resource") + end + + it "embeds the project" do + expect(generated) + .to be_json_eql(project.name.to_json) + .at_path("_embedded/project/name") + end + end + end end describe "actor" do diff --git a/spec/models/notifications/scopes/visible_spec.rb b/spec/models/notifications/scopes/visible_spec.rb index c6b59ddcc0a5..cca5196652f8 100644 --- a/spec/models/notifications/scopes/visible_spec.rb +++ b/spec/models/notifications/scopes/visible_spec.rb @@ -75,5 +75,90 @@ it_behaves_like "is empty" end + + context "for a document notification" do + let(:project) { create(:project, enabled_module_names: %w[work_package_tracking documents]) } + let(:document) { create(:document, project:) } + let(:notification) do + create(:notification, + resource: document, + journal: nil, + recipient: notification_recipient) + end + let(:permissions) { %i[view_documents] } + + context "with the user being recipient and being allowed to see the document" do + it "returns the notification" do + expect(scope) + .to contain_exactly(notification) + end + end + + context "with the user being recipient and not being allowed to see the document" do + let(:permissions) { %i[view_work_packages] } + + it_behaves_like "is empty" + end + + context "with the user being recipient and the documents module being disabled" do + let(:project) { create(:project, enabled_module_names: %w[work_package_tracking]) } + + it_behaves_like "is empty" + end + + context "with the user not being recipient but being allowed to see the document" do + let(:notification_recipient) { create(:user, member_with_permissions: { project => permissions }) } + + it_behaves_like "is empty" + end + end + + context "with work package and document notifications" do + let(:project) { create(:project, enabled_module_names: %w[work_package_tracking documents]) } + let(:permissions) { %i[view_work_packages view_documents] } + let(:document) { create(:document, project:) } + let(:document_notification) do + create(:notification, + resource: document, + journal: nil, + recipient: user) + end + let(:other_project) { create(:project, enabled_module_names: %w[work_package_tracking documents]) } + let(:invisible_work_package_notification) do + create(:notification, + resource: create(:work_package, project: other_project), + recipient: user) + end + let(:invisible_document_notification) do + create(:notification, + resource: create(:document, project: other_project), + journal: nil, + recipient: user) + end + let(:other_recipient_document_notification) do + create(:notification, + resource: document, + journal: nil, + recipient: create(:user, member_with_permissions: { project => permissions })) + end + + let!(:notifications) do + [notification, + document_notification, + invisible_work_package_notification, + invisible_document_notification, + other_recipient_document_notification] + end + + it "returns the notifications of the recipient for visible resources only" do + expect(scope) + .to contain_exactly(notification, document_notification) + end + + it "returns the same when chained onto another scope" do + expect(Notification.where(read_ian: false).visible(user)) + .to contain_exactly(notification, document_notification) + end + end end end diff --git a/spec/models/queries/base_query_group_bys_spec.rb b/spec/models/queries/base_query_group_bys_spec.rb index dfd9a1f05812..92b7dd5302f8 100644 --- a/spec/models/queries/base_query_group_bys_spec.rb +++ b/spec/models/queries/base_query_group_bys_spec.rb @@ -51,13 +51,13 @@ it "sets several group bys, keeping their order" do instance.group(:reason, :project) - expect(instance.group_bys.map(&:name)).to eq(%i[reason project_id]) + expect(instance.group_bys.map { |group_by| group_by.class.key }).to eq(%i[reason project]) end it "replaces previously set group bys rather than appending" do instance.group(:reason).group(:project) - expect(instance.group_bys.map(&:name)).to eq([:project_id]) + expect(instance.group_bys.map { |group_by| group_by.class.key }).to eq([:project]) end it "returns the query to allow chaining" do @@ -105,7 +105,8 @@ it "selects every group by and a count for several group bys" do instance.group(:reason, :project) - expect(instance.groups.to_sql).to include('"notifications"."reason", "project_id", COUNT(*)') + expect(instance.groups.to_sql) + .to include(%("notifications"."reason", #{Queries::Notifications::ResourceProject.project_id_sql}, COUNT(*))) end it "applies the joins of every group by" do diff --git a/spec/models/queries/notifications/notification_query_spec.rb b/spec/models/queries/notifications/notification_query_spec.rb index 3bc6e3f49106..d6ab01e84715 100644 --- a/spec/models/queries/notifications/notification_query_spec.rb +++ b/spec/models/queries/notifications/notification_query_spec.rb @@ -188,13 +188,17 @@ describe "#results" do it "is the same as handwriting the query" do + project_id = Arel.sql("COALESCE(notification_work_packages.project_id, notification_documents.project_id)") expected = base_scope - .joins("JOIN work_packages " \ - "ON notifications.resource_id = work_packages.id " \ - "AND notifications.resource_type = 'WorkPackage'") - .group(:project_id) - .order(project_id: :asc) - .select(:project_id, Arel.sql("COUNT(*)")) + .joins("LEFT OUTER JOIN work_packages notification_work_packages " \ + "ON notification_work_packages.id = notifications.resource_id " \ + "AND notifications.resource_type = 'WorkPackage' " \ + "LEFT OUTER JOIN documents notification_documents " \ + "ON notification_documents.id = notifications.resource_id " \ + "AND notifications.resource_type = 'Document'") + .group(project_id) + .order(project_id) + .select(project_id, Arel.sql("COUNT(*)")) .to_sql expect(instance.groups.to_sql).to eql expected @@ -202,6 +206,126 @@ end end + context "with work package and document notifications in different projects" do + shared_let(:document_project) do + create(:project, + name: "A document project", + enabled_module_names: %w[work_package_tracking documents]) + end + shared_let(:other_project) do + create(:project, + name: "Z other project", + enabled_module_names: %w[work_package_tracking documents]) + end + shared_let(:document_recipient) do + create(:user, + member_with_permissions: { + project => %i[view_work_packages], + document_project => %i[view_documents], + other_project => %i[view_work_packages view_documents] + }) + end + shared_let(:work_package_notification) do + create(:notification, recipient: document_recipient, resource: work_package) + end + shared_let(:document_notification) do + create(:notification, + recipient: document_recipient, + resource: create(:document, project: document_project), + journal: nil) + end + shared_let(:other_work_package_notification) do + create(:notification, + recipient: document_recipient, + resource: create(:work_package, project: other_project)) + end + shared_let(:other_document_notification) do + create(:notification, + recipient: document_recipient, + resource: create(:document, project: other_project), + journal: nil) + end + + let(:instance) { described_class.new(user: document_recipient) } + + current_user { document_recipient } + + context "with a project filter" do + it "returns the notifications of work packages and documents in the project" do + instance.where(:project_id, "=", [other_project.id]) + + expect(instance.results) + .to contain_exactly(other_work_package_notification, other_document_notification) + end + + it "returns document notifications of a project with only documents visible" do + instance.where(:project_id, "=", [document_project.id]) + + expect(instance.results) + .to contain_exactly(document_notification) + end + + it "returns the notifications of resources outside of the project for the negated operator" do + instance.where(:project_id, "!", [other_project.id]) + + expect(instance.results) + .to contain_exactly(work_package_notification, document_notification) + end + end + + context "with a resource type filter" do + it "returns only the document notifications" do + instance.where(:resource_type, "=", [Document.name]) + + expect(instance.results) + .to contain_exactly(document_notification, other_document_notification) + end + end + + context "with a resource id filter for a document" do + it "returns the document notification" do + instance.where(:resource_id, "=", [document_notification.resource_id]) + instance.where(:resource_type, "=", [Document.name]) + + expect(instance).to be_valid + expect(instance.results) + .to contain_exactly(document_notification) + end + end + + context "with a project group_by" do + it "counts work package and document notifications per project" do + instance.group(:project) + + expect(instance.group_values) + .to eq(project => 1, document_project => 1, other_project => 2) + end + end + + context "with a project order" do + it "orders work package and document notifications by the project name" do + instance.order(project: :asc) + + expect(instance.results.map { |notification| notification.resource.project }) + .to eq [document_project, project, other_project, other_project] + end + end + + context "with a project filter, group_by and order combined" do + it "returns the notifications of the project" do + instance.where(:project_id, "=", [other_project.id, document_project.id]) + instance.group(:project) + instance.order(project: :desc) + + # Within the same project, notifications are ordered by id descending. + expect(instance.results) + .to eq [other_document_notification, other_work_package_notification, document_notification] + expect(instance.group_values) + .to eq(other_project => 2, document_project => 1) + end + end + end + context "with a non existing group_by" do before do instance.group(:does_not_exist) diff --git a/spec/requests/api/v3/notifications/bulk_unread_ian_resource_spec.rb b/spec/requests/api/v3/notifications/bulk_unread_ian_resource_spec.rb index c8c1448c1c83..40acfe22e9fb 100644 --- a/spec/requests/api/v3/notifications/bulk_unread_ian_resource_spec.rb +++ b/spec/requests/api/v3/notifications/bulk_unread_ian_resource_spec.rb @@ -85,6 +85,29 @@ .to all(be_truthy) end + context "with a document notification not supporting in app notifications" do + shared_let(:document_project) do + create(:project, + enabled_module_names: %w[documents], + members: { recipient => create(:project_role, permissions: %i[view_documents]) }) + end + shared_let(:non_ian_document_notification) do + create(:notification, + recipient:, + resource: create(:document, project: document_project), + journal: nil, + read_ian: nil) + end + + it "does not turn it into an in app notification" do + expect(non_ian_document_notification.reload.read_ian) + .to be_nil + + expect(Notification.where(id: [notification1.id, notification2.id, notification3.id]).pluck(:read_ian)) + .to all(be_falsey) + end + end + context "with a filter for id" do let(:filters) do [ diff --git a/spec/requests/api/v3/notifications/index_resource_spec.rb b/spec/requests/api/v3/notifications/index_resource_spec.rb index 6703a384647b..efa2dd3e694f 100644 --- a/spec/requests/api/v3/notifications/index_resource_spec.rb +++ b/spec/requests/api/v3/notifications/index_resource_spec.rb @@ -303,6 +303,120 @@ end end + context "with a document notification" do + shared_let(:document_project) do + create(:project, + enabled_module_names: %w[documents], + members: { recipient => create(:project_role, permissions: %i[view_documents]) }) + end + shared_let(:document) { create(:document, project: document_project) } + # Created with read_ian set explicitly as documents do not yet support + # in app notifications when created via the services. + shared_let(:document_notification) do + create(:notification, + recipient:, + reason: :mentioned, + resource: document, + journal: nil, + read_ian: false) + end + + it_behaves_like "API V3 collection response", 3, 3, "Notification" do + let(:elements) { [document_notification, date_alert_notification, mentioned_notification] } + end + + it "links but does not embed the document" do + expect(last_response.body) + .to be_json_eql(api_v3_paths.document(document.id).to_json) + .at_path("_embedded/elements/0/_links/resource/href") + expect(last_response.body) + .to be_json_eql(document.title.to_json) + .at_path("_embedded/elements/0/_links/resource/title") + expect(last_response.body) + .not_to have_json_path("_embedded/elements/0/_embedded/resource") + expect(last_response.body) + .to be_json_eql(api_v3_paths.project(document_project.id).to_json) + .at_path("_embedded/elements/0/_links/project/href") + end + + it "still links the work package titled by its subject" do + expect(last_response.body) + .to be_json_eql(api_v3_paths.work_package(work_package.id).to_json) + .at_path("_embedded/elements/1/_links/resource/href") + expect(last_response.body) + .to be_json_eql(work_package.subject.to_json) + .at_path("_embedded/elements/1/_links/resource/title") + end + + context "with a project filter" do + let(:filters) do + [{ "project" => { "operator" => "=", "values" => [document_project.id.to_s] } }] + end + + it_behaves_like "API V3 collection response", 1, 1, "Notification" do + let(:elements) { [document_notification] } + end + end + + context "with a resourceType filter" do + let(:filters) do + [{ "resourceType" => { "operator" => "=", "values" => [Document.name] } }] + end + + it_behaves_like "API V3 collection response", 1, 1, "Notification" do + let(:elements) { [document_notification] } + end + end + + context "with a project groupBy" do + let(:send_request) do + get api_v3_paths.path_for :notifications, group_by: :project + end + + let(:groups) { parsed_response["groups"] } + + it_behaves_like "API V3 collection response", 3, 3, "Notification" + + it "contains the project groups", :aggregate_failures do + keyed = groups.index_by { |el| el["value"] } + expect(keyed.keys).to contain_exactly document_project.name, work_package.project.name + expect(keyed[work_package.project.name]["count"]).to eq 2 + expect(keyed[document_project.name]["count"]).to eq 1 + + expect(keyed.dig(document_project.name, "_links", "valueLink")[0]["href"]) + .to eq api_v3_paths.project(document_project.id) + end + end + + context "with a project filter, groupBy and sortBy combined" do + let(:send_request) do + get api_v3_paths.path_for :notifications, + filters: [{ "project" => { "operator" => "=", + "values" => [document_project.id.to_s, + work_package.project_id.to_s] } }], + group_by: :project, + sort_by: [%w[project desc]] + end + + it_behaves_like "API V3 collection response", 3, 3, "Notification" + + it "contains the project groups" do + expect(parsed_response["groups"].to_h { |group| [group["value"], group["count"]] }) + .to eq(document_project.name => 1, work_package.project.name => 2) + end + end + + context "when having lost the permission to see the document" do + let(:additional_setup) do + Member.where(principal: recipient, project: document_project).destroy_all + end + + it_behaves_like "API V3 collection response", 2, 2, "Notification" do + let(:elements) { [date_alert_notification, mentioned_notification] } + end + end + end + context "when having lost the permission to see the work package" do let(:additional_setup) do Member.where(principal: recipient).destroy_all diff --git a/spec/requests/api/v3/notifications/notifications_api_create_resource_spec.rb b/spec/requests/api/v3/notifications/notifications_api_create_resource_spec.rb index 91e7254f2e03..0560312842e9 100644 --- a/spec/requests/api/v3/notifications/notifications_api_create_resource_spec.rb +++ b/spec/requests/api/v3/notifications/notifications_api_create_resource_spec.rb @@ -112,7 +112,7 @@ actor: caller_user, reason: "mentioned", journal: nil, - read_ian: nil, + read_ian: false, mail_alert_sent: true, mail_reminder_sent: nil) end diff --git a/spec/requests/api/v3/notifications/show_resource_spec.rb b/spec/requests/api/v3/notifications/show_resource_spec.rb index f44140c0fba3..d980c51a7fc3 100644 --- a/spec/requests/api/v3/notifications/show_resource_spec.rb +++ b/spec/requests/api/v3/notifications/show_resource_spec.rb @@ -63,6 +63,54 @@ end it_behaves_like "represents the notification" + + it "embeds the work package" do + expect(last_response.body) + .to be_json_eql("WorkPackage".to_json) + .at_path("_embedded/resource/_type") + end + end + + describe "recipient user for a document notification" do + shared_let(:document_project) do + create(:project, + enabled_module_names: %w[documents], + members: { recipient => create(:project_role, permissions: %i(view_documents)) }) + end + shared_let(:document) { create(:document, project: document_project) } + # Created with read_ian set explicitly as documents do not yet support + # in app notifications when created via the services. + shared_let(:notification) do + create(:notification, + recipient:, + resource: document, + journal: nil, + read_ian: false) + end + + current_user { recipient } + + before do + send_request + end + + it_behaves_like "represents the notification" + + it "links but does not embed the document", :aggregate_failures do + expect(last_response.body) + .to be_json_eql(api_v3_paths.document(document.id).to_json) + .at_path("_links/resource/href") + expect(last_response.body) + .to be_json_eql(document.title.to_json) + .at_path("_links/resource/title") + expect(last_response.body) + .not_to have_json_path("_embedded/resource") + expect(last_response.body) + .to be_json_eql(api_v3_paths.project(document_project.id).to_json) + .at_path("_links/project/href") + expect(last_response.body) + .not_to have_json_path("_links/activity") + end end describe "recipient user for a dateAlert notification" do diff --git a/spec/services/notifications/create_from_api_service_spec.rb b/spec/services/notifications/create_from_api_service_spec.rb index f43e92ceec2c..3647766988a3 100644 --- a/spec/services/notifications/create_from_api_service_spec.rb +++ b/spec/services/notifications/create_from_api_service_spec.rb @@ -49,7 +49,7 @@ instance.call(reason:, resource_type:, resource_id: document.id, recipient_id:) end - it "creates a mention notification without journal, only to be sent out by mail" do + it "creates an unread in-app mention notification without journal" do expect(service_call).to be_success expect(service_call.result) .to be_persisted @@ -58,7 +58,7 @@ actor: user, reason: "mentioned", journal: nil, - read_ian: nil, + read_ian: false, mail_reminder_sent: nil) end @@ -83,7 +83,7 @@ expect(service_call.result) .to be_persisted - .and have_attributes(recipient: recipient_without_mention_mails, read_ian: nil) + .and have_attributes(recipient: recipient_without_mention_mails, read_ian: false) expect(service_call.result.reload.mail_alert_sent).to be_nil end end diff --git a/spec/support/pages/notifications/center.rb b/spec/support/pages/notifications/center.rb index e44de22a8925..ce3586c011b8 100644 --- a/spec/support/pages/notifications/center.rb +++ b/spec/support/pages/notifications/center.rb @@ -119,6 +119,24 @@ def expect_work_package_item(*notifications) end end + def expect_document_item(notification, reasons: "Mentioned") + document = notification.resource + raise(ArgumentError, "Expected document") unless document.is_a?(Document) + + within_item(notification) do + expect(page).to have_text("Document\n- #{document.project.name} -\n#{reasons}") + expect(page).to have_css("a[href='/documents/#{document.id}'][target='_blank'][rel='noopener']", + exact_text: document.title) + expect(page).to have_no_link("Document") + end + end + + def click_document_label(notification) + within_item(notification) do + page.find(".op-ian-item--resource-label", text: "Document").click + end + end + def expect_closed expect(page).to have_no_css("opce-notification-center") end