From e8b0648ee782bda67ac179a1d5594d00fed87a2d Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 15 Jan 2025 09:34:38 +0300 Subject: [PATCH 1/3] Reminders generate two or more lines for a work package in notification center, clicking one selects all https://community.openproject.org/work_packages/60449 Aggregate the Reminder notifications with other notifications (regular aggregation) so that only one item per work package is visible in the notification center. This reverts: https://github.com/opf/openproject/pull/17426 where we previously defined reminder notifications as standalone. --- .../center/state/ian-center.service.ts | 23 +------------------ .../notification_center_reminder_spec.rb | 12 ++-------- 2 files changed, 3 insertions(+), 32 deletions(-) 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 9c41f1a46587..0979826f6503 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 @@ -111,28 +111,7 @@ export class IanCenterService extends UntilDestroyedMixin { notifications$ = this .aggregatedCenterNotifications$ .pipe( - map((items) => { - return Object.values(items).reduce((acc, workPackageNotificationGroup) => { - const { reminders, others } = workPackageNotificationGroup.reduce((result, notification) => { - if (notification.reason === 'reminder') { - result.reminders.push(notification); - } else { - result.others.push(notification); - } - return result; - }, { reminders: [] as INotification[], others: [] as INotification[] }); - - // Extract reminders into standalone groups so they can be displayed individually - if (reminders.length > 0) { - reminders.forEach((reminder) => acc.push([reminder])); - } - if (others.length > 0) { - acc.push(others); - } - - return acc; - }, [] as INotification[][]); - }), + map((items) => Object.values(items)), distinctUntilChanged(), ); diff --git a/spec/features/notifications/notification_center/notification_center_reminder_spec.rb b/spec/features/notifications/notification_center/notification_center_reminder_spec.rb index 015aa20a36f1..af66d5907162 100644 --- a/spec/features/notifications/notification_center/notification_center_reminder_spec.rb +++ b/spec/features/notifications/notification_center/notification_center_reminder_spec.rb @@ -47,18 +47,10 @@ wait_for_reload end - it "shows the reminder alert in own entry" do + it "shows the reminder alert within aggregation with reminder note" do center.within_item(notification_reminder) do - expect(page).to have_text("##{work_package.id}\n- #{project.name} -\nReminder") - expect(page).to have_no_text("Actor user") + expect(page).to have_text("Date alert, Mentioned, Reminder") expect(page).to have_text("a few seconds ago.\nNote: “This is an important reminder”") end end - - it "shows other notification reasons aggregated" do - center.within_item(notification_date_alert) do - expect(page).to have_text("##{work_package.id}\n- #{project.name} -\nDate alert, Mentioned") - expect(page).to have_no_text("Actor user") - end - end end From 75180a5172aa3e0df5c36d6d5bf7643a3c753573 Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 15 Jan 2025 16:01:09 +0300 Subject: [PATCH 2/3] Add missing full stops in date alerts bottom texts --- config/locales/js-en.yml | 10 ++++---- .../notification_center_date_alerts_spec.rb | 24 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/config/locales/js-en.yml b/config/locales/js-en.yml index bffc4ecc4b6f..fd80166a7665 100644 --- a/config/locales/js-en.yml +++ b/config/locales/js-en.yml @@ -681,11 +681,11 @@ en: date_alerts: milestone_date: "Milestone date" overdue: "Overdue" - overdue_since: "since %{difference_in_days}" - property_today: "is today" - property_is: "is in %{difference_in_days}" - property_was: "was %{difference_in_days} ago" - property_is_deleted: "is deleted" + overdue_since: "since %{difference_in_days}." + property_today: "is today." + property_is: "is in %{difference_in_days}." + property_was: "was %{difference_in_days} ago." + property_is_deleted: "is deleted." upsale: title: "Date alerts" description: "With date alerts, you will be notified of upcoming start or finish dates so that you never miss or forget an important deadline." diff --git a/spec/features/notifications/notification_center/notification_center_date_alerts_spec.rb b/spec/features/notifications/notification_center/notification_center_date_alerts_spec.rb index 92a8a53f3e1a..88b514c928da 100644 --- a/spec/features/notifications/notification_center/notification_center_date_alerts_spec.rb +++ b/spec/features/notifications/notification_center/notification_center_date_alerts_spec.rb @@ -195,7 +195,7 @@ def run_create_date_alerts_notifications_job it "shows the upsale page" do side_menu.click_item "Date alert" - expect(page).to have_current_path /notifications\/date_alerts/ + expect(page).to have_current_path(/notifications\/date_alerts/) expect(page).to have_text "Date alerts is an Enterprise" expect(page).to have_text "Please upgrade to a paid plan " @@ -207,18 +207,18 @@ def run_create_date_alerts_notifications_job context "with date alerts ee", with_ee: %i[date_alerts] do it "shows the date alerts according to specification" do - center.expect_item(notification_wp_start_past, "Start date was 1 day ago") - center.expect_item(notification_wp_start_future, "Start date is in 7 days") + center.expect_item(notification_wp_start_past, "Start date was 1 day ago.") + center.expect_item(notification_wp_start_future, "Start date is in 7 days.") - center.expect_item(notification_wp_due_past, "Overdue since 3 days") - center.expect_item(notification_wp_due_future, "Finish date is in 3 days") + center.expect_item(notification_wp_due_past, "Overdue since 3 days.") + center.expect_item(notification_wp_due_future, "Finish date is in 3 days.") - center.expect_item(notification_milestone_past, "Overdue since 2 days") - center.expect_item(notification_milestone_future, "Milestone date is in 1 day") + center.expect_item(notification_milestone_past, "Overdue since 2 days.") + center.expect_item(notification_milestone_future, "Milestone date is in 1 day.") - center.expect_item(notification_wp_unset_date, "Finish date is deleted") + center.expect_item(notification_wp_unset_date, "Finish date is deleted.") - center.expect_item(notification_wp_due_today, "Finish date is today") + center.expect_item(notification_wp_due_today, "Finish date is today.") # Doesn't show the date alert for the mention, not the alert center.expect_item(notification_wp_double_mention, /(seconds|minutes) ago by Anonymous/) @@ -227,7 +227,7 @@ def run_create_date_alerts_notifications_job # When switch to date alerts, it shows the alert, no longer the mention side_menu.click_item "Date alert" wait_for_network_idle - center.expect_item(notification_wp_double_date_alert, "Finish date is in 1 day") + center.expect_item(notification_wp_double_date_alert, "Finish date is in 1 day.") center.expect_no_item(notification_wp_double_mention) # Ensure that start is created later than due for implicit ID sorting @@ -236,7 +236,7 @@ def run_create_date_alerts_notifications_job # We see that start is actually the newest ID, hence shown as the primary notification # but the date alert still shows the finish date - center.expect_item(double_alert_start, "Finish date is in 1 day") + center.expect_item(double_alert_start, "Finish date is in 1 day.") center.expect_no_item(double_alert_due) # Opening a date alert opens in overview @@ -262,7 +262,7 @@ def run_create_date_alerts_notifications_job page.driver.refresh wait_for_reload - center.expect_item(notification_wp_double_date_alert, "Finish date is in 5 days") + center.expect_item(notification_wp_double_date_alert, "Finish date is in 5 days.") center.expect_no_item(notification_wp_double_mention) end end From a0dcb97598761c9ae84ddccf60134a35af11dfde Mon Sep 17 00:00:00 2001 From: Kabiru Mwenja Date: Wed, 15 Jan 2025 16:02:35 +0300 Subject: [PATCH 3/3] Show date alert + reminder note When there exists a reminder + date alert for a given WP, show the date alert text + the reminder note --- ...notification-reminder-alert.component.html | 10 +++- ...p-notification-reminder-alert.component.ts | 4 ++ .../notification_center_reminder_spec.rb | 59 +++++++++++++------ 3 files changed, 54 insertions(+), 19 deletions(-) diff --git a/frontend/src/app/features/in-app-notifications/entry/reminder-alert/in-app-notification-reminder-alert.component.html b/frontend/src/app/features/in-app-notifications/entry/reminder-alert/in-app-notification-reminder-alert.component.html index 1956ae5dfa8f..14ad39483e6a 100644 --- a/frontend/src/app/features/in-app-notifications/entry/reminder-alert/in-app-notification-reminder-alert.component.html +++ b/frontend/src/app/features/in-app-notifications/entry/reminder-alert/in-app-notification-reminder-alert.component.html @@ -1,7 +1,13 @@ + + + [hasActorByLine]="false"> + + notification.reason === 'dateAlert'); + this.hasDateAlert = this.dateAlerts.length > 0; } private deriveMostRecentReminder(aggregatedNotifications:INotification[]):INotification { diff --git a/spec/features/notifications/notification_center/notification_center_reminder_spec.rb b/spec/features/notifications/notification_center/notification_center_reminder_spec.rb index af66d5907162..ca000c92f740 100644 --- a/spec/features/notifications/notification_center/notification_center_reminder_spec.rb +++ b/spec/features/notifications/notification_center/notification_center_reminder_spec.rb @@ -13,13 +13,19 @@ member_with_permissions: { project => %w[view_work_packages] }) end shared_let(:work_package) { create(:work_package, project:, due_date: 1.day.ago) } + shared_let(:work_package2) { create(:work_package, project:) } - shared_let(:notification_mention) do - create(:notification, - reason: :mentioned, - recipient: user, - resource: work_package, - actor:) + shared_let(:notification_mentions) do + [create(:notification, + reason: :mentioned, + recipient: user, + resource: work_package, + actor:), + create(:notification, + reason: :mentioned, + recipient: user, + resource: work_package2, + actor: actor)] end shared_let(:notification_date_alert) do @@ -30,13 +36,11 @@ end shared_let(:notification_reminder) do - reminder = create(:reminder, remindable: work_package, creator: user, note: "This is an important reminder") - notification = create(:notification, - reason: :reminder, - recipient: user, - resource: work_package) - create(:reminder_notification, reminder:, notification:) - notification + create_reminder_notification_for(work_package: work_package, user: user) + end + + shared_let(:notification_reminder2) do + create_reminder_notification_for(work_package: work_package2, user: user) end let(:center) { Pages::Notifications::Center.new } @@ -47,10 +51,31 @@ wait_for_reload end - it "shows the reminder alert within aggregation with reminder note" do - center.within_item(notification_reminder) do - expect(page).to have_text("Date alert, Mentioned, Reminder") - expect(page).to have_text("a few seconds ago.\nNote: “This is an important reminder”") + context "with a reminder, mention and date alert" do + it "shows the reminder alert within aggregation with date alert + reminder note" do + center.within_item(notification_reminder) do + expect(page).to have_text("##{work_package.id}\n- #{project.name} -\nDate alert, Mentioned, Reminder") + expect(page).to have_text("Overdue since 1 day.\nNote: “This is an important reminder”") + end end end + + context "with a reminder and mention, no date alert" do + it "shows the reminder alert within aggregation with mention" do + center.within_item(notification_reminder2) do + expect(page).to have_text("##{work_package2.id}\n- #{project.name} -\nMentioned, Reminder") + expect(page).to have_text("a few seconds ago.\nNote: “This is an important reminder”") + end + end + end + + def create_reminder_notification_for(work_package:, user:) + reminder = create(:reminder, remindable: work_package, creator: user, note: "This is an important reminder") + notification = create(:notification, + reason: :reminder, + recipient: user, + resource: work_package) + create(:reminder_notification, reminder:, notification:) + notification + end end