From d2405cc11c541cd897253563ef455c11fbb5cf50 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartosz=20Dziewo=C5=84ski?= <matma.rex@gmail.com>
Date: Fri, 21 Jan 2022 00:06:36 +0100
Subject: [PATCH] Simplify handling of sections in bundled notification links

This code previously ensured that the fragment identifier linking
to a section was only included if all events had the same section.
It doesn't actually seem worth the effort, since we handle scrolling
to the highlighted comments client-side anyway.

And the links were not quite correct, because we didn't parse and
strip the section title as expected by built-in Echo events. Just
use Echo's code for this.

Depends-On: Idb3a87fd18330f90a8cdc1276994d54288e17b28
Change-Id: Icae0d3654dd02109337ff8737b16f55bbd514f43
---
 .../Notifications/DiscussionToolsEventTrait.php | 17 ++++++++---------
 .../SubscribedNewCommentPresentationModel.php   |  2 +-
 2 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/includes/Notifications/DiscussionToolsEventTrait.php b/includes/Notifications/DiscussionToolsEventTrait.php
index d8ab0e86d..a4839581c 100644
--- a/includes/Notifications/DiscussionToolsEventTrait.php
+++ b/includes/Notifications/DiscussionToolsEventTrait.php
@@ -18,6 +18,7 @@ use WikiMap;
  *
  * @property \EchoEvent $event
  * @property \Language $language
+ * @property \EchoPresentationModelSection $section
  */
 trait DiscussionToolsEventTrait {
 
@@ -51,7 +52,6 @@ trait DiscussionToolsEventTrait {
 		if ( !$this->userCan( RevisionRecord::DELETED_TEXT ) ) {
 			return null;
 		}
-		$title = $this->event->getTitle();
 		if ( !$this->isBundled() ) {
 			// For a single-comment notification, make a pretty(ish) direct link to the comment.
 			// The browser scrolls and we highlight it client-side.
@@ -59,6 +59,7 @@ trait DiscussionToolsEventTrait {
 			if ( !$commentId ) {
 				return null;
 			}
+			$title = $this->event->getTitle();
 			return $title->createFragmentTarget( $commentId )->getFullURL();
 		} else {
 			// For a multi-comment notification, we can't make a direct link, because we don't know
@@ -72,23 +73,21 @@ trait DiscussionToolsEventTrait {
 			// * Mention notifications are *never* bundled.
 			// Code below tries to avoid assumptions in case this behavior changes.
 
-			$sectionTitles = [ $this->event->getExtraParam( 'section-title' ) ];
 			$commentIds = [ $this->event->getExtraParam( 'comment-id' ) ];
 			foreach ( $this->getBundledEvents() as $event ) {
-				$sectionTitles[] = $event->getExtraParam( 'section-title' );
 				$commentIds[] = $event->getExtraParam( 'comment-id' );
 			}
 			$commentIds = array_values( array_filter( $commentIds ) );
-			$sectionTitles = array_values( array_unique( array_filter( $sectionTitles ) ) );
 			if ( !$commentIds ) {
 				return null;
 			}
+
 			$params = [ 'dtnewcomments' => implode( '|', $commentIds ) ];
-			if ( count( $sectionTitles ) === 1 ) {
-				return $title->createFragmentTarget( $sectionTitles[0] )->getFullURL( $params );
-			} else {
-				return $title->getFullURL( $params );
-			}
+			// This may or may not have a fragment identifier, depending on whether it was recorded for
+			// the first one of the bundled events. It's usually not needed because we handle scrolling
+			// client-side, but we can keep it for no-JS users, and to reduce the jump when scrolling.
+			$titleWithOptionalSection = $this->section->getTitleWithSection();
+			return $titleWithOptionalSection->getFullURL( $params );
 		}
 	}
 
diff --git a/includes/Notifications/SubscribedNewCommentPresentationModel.php b/includes/Notifications/SubscribedNewCommentPresentationModel.php
index 505b957f5..5b52a3b99 100644
--- a/includes/Notifications/SubscribedNewCommentPresentationModel.php
+++ b/includes/Notifications/SubscribedNewCommentPresentationModel.php
@@ -25,7 +25,7 @@ class SubscribedNewCommentPresentationModel extends EchoEventPresentationModel {
 	/**
 	 * @var EchoPresentationModelSection
 	 */
-	private $section;
+	protected $section;
 
 	/**
 	 * @inheritDoc