FIX: Consistently show history modal when clicking edit notifications (#13912)

FIX: Consistently show history modal when clicking edit notifications (#13912)

Currently when a user clicks on an edit notification, we use appEvents to notify the topics controller that it should open up the history modal for the edited post and the appEvents callback opens up the history modal in the next Ember runloop (by scheduling an afterRender callback).

There are 2 problems with this implementation:

  1. the callbacks are fired/executed too early and if the post has never been loaded from the server (i.e. not in cache), we will not get a modal history because the method that shows the modal returns if it can’t find the post:

https://github.com/discourse/discourse/blob/016efeadf6f242e04daf5ef8e18c2ca708a1392d/app/assets/javascripts/discourse/app/controllers/topic.js#L145-L152

  1. when clicking an edit notification from a non-topic page, you’re redirected to the topic page that contains the edited post and you’ll see the history modal briefly and it’ll be closed immediately. The reason for this is because we attempt to show the history modal before the route transition finishes completely, and we have cleanup code in initializers/page-tracking.js that’s called after every transition and it does several things one of which is closing any open modals.

The fix in this commit defers showing the history modal until posts are loaded (whether fresh or cached). It works by storing some bits of information (topic id, post number, revision number) whenever the user clicks on an edit notification, and when the user is redirected to the topic (or scrolled to the edited post if they’re already in the topic), the post stream model checks if we have stored information of an edit notification and requests the history modal to be shown by the topics controller.

diff --git a/app/assets/javascripts/discourse/app/initializers/edit-notification-clicks-tracker.js b/app/assets/javascripts/discourse/app/initializers/edit-notification-clicks-tracker.js
new file mode 100644
index 0000000..c9e2914
--- /dev/null
+++ b/app/assets/javascripts/discourse/app/initializers/edit-notification-clicks-tracker.js
@@ -0,0 +1,16 @@
+import { setLastEditNotificationClick } from "discourse/models/post-stream";
+
+export default {
+  name: "edit-notification-clicks-tracker",
+
+  initialize(container) {
+    container
+      .lookup("service:app-events")
+      .on(
+        "edit-notification:clicked",
+        ({ topicId, postNumber, revisionNumber }) => {
+          setLastEditNotificationClick(topicId, postNumber, revisionNumber);
+        }
+      );
+  },
+};
diff --git a/app/assets/javascripts/discourse/app/models/post-stream.js b/app/assets/javascripts/discourse/app/models/post-stream.js
index 2c07775..a28a483 100644
--- a/app/assets/javascripts/discourse/app/models/post-stream.js
+++ b/app/assets/javascripts/discourse/app/models/post-stream.js
@@ -15,6 +15,23 @@ import { isEmpty } from "@ember/utils";
 import { loadTopicView } from "discourse/models/topic";
 import { schedule } from "@ember/runloop";
 
+let _lastEditNotificationClick = null;
+export function setLastEditNotificationClick(
+  topicId,
+  postNumber,
+  revisionNumber
+) {
+  _lastEditNotificationClick = {
+    topicId,
+    postNumber,
+    revisionNumber,
+  };
+}
+
+export function resetLastEditNotificationClick() {
+  _lastEditNotificationClick = null;
+}
+
 export default RestModel.extend({
   _identityMap: null,
   posts: null,
@@ -324,7 +341,7 @@ export default RestModel.extend({
     } else {
       const postWeWant = this.posts.findBy("post_number", opts.nearPost);
       if (postWeWant) {
-        return Promise.resolve();
+        return Promise.resolve().then(() => this._checkIfShouldShowRevisions());
       }
     }
 
@@ -345,6 +362,7 @@ export default RestModel.extend({
           timelineLookup: json.timeline_lookup,
           loaded: true,
         });
+        this._checkIfShouldShowRevisions();
       })
       .catch((result) => {
         this.errorLoading(result);
@@ -1207,4 +1225,24 @@ export default RestModel.extend({
       topic.set("noRetry", result.jqXHR.status === 403);
     }
   },
+
+  _checkIfShouldShowRevisions() {
+    if (_lastEditNotificationClick) {
+      const copy = _lastEditNotificationClick;
+      resetLastEditNotificationClick();
+      const postsNumbers = this.posts.mapBy("post_number");
+      if (
+        copy.topicId === this.topic.id &&
+        postsNumbers.includes(copy.postNumber)
+      ) {
+        schedule("afterRender", () => {
+          this.appEvents.trigger(
+            "post:show-revision",
+            copy.postNumber,
+            copy.revisionNumber
+          );
+        });
+      }
+    }
+  },
 });
diff --git a/app/assets/javascripts/discourse/app/widgets/default-notification-item.js b/app/assets/javascripts/discourse/app/widgets/default-notification-item.js
index 8a183f4..ff9d09f 100644
--- a/app/assets/javascripts/discourse/app/widgets/default-notification-item.js
+++ b/app/assets/javascripts/discourse/app/widgets/default-notification-item.js
@@ -29,6 +29,12 @@ export const DefaultNotificationItem = createWidget(
       if (attrs.is_warning) {
         classNames.push("is-warning");
       }
+      const notificationType = attrs.notification_type;
+      const lookup = this.site.get("notificationLookup");
+      const notificationName = lookup[notificationType];
+      if (notificationName) {
+        classNames.push(notificationName.replaceAll("_", "-"));
+      }
       return classNames;
     },
 
@@ -156,19 +162,14 @@ export const DefaultNotificationItem = createWidget(
       e.preventDefault();
 
       this.sendWidgetEvent("linkClicked");
-      DiscourseURL.routeTo(this.url(this.attrs.data), {
-        afterRouteComplete: () => {
-          if (!this.attrs.data.revision_number) {
-            return;
-          }
-
-          this.appEvents.trigger(
-            "post:show-revision",
-            this.attrs.post_number,
-            this.attrs.data.revision_number
-          );
-        },
-      });
+      if (this.attrs.data.revision_number) {
+        this.appEvents.trigger("edit-notification:clicked", {
+          topicId: this.attrs.topic_id,
+          postNumber: this.attrs.post_number,
+          revisionNumber: this.attrs.data.revision_number,
+        });
+      }
+      DiscourseURL.routeTo(this.url(this.attrs.data));
     },
 
     mouseUp(event) {
diff --git a/app/assets/javascripts/discourse/tests/acceptance/edit-notification-click-test.js b/app/assets/javascripts/discourse/tests/acceptance/edit-notification-click-test.js
new file mode 100644
index 0000000..00f5fa6
--- /dev/null
+++ b/app/assets/javascripts/discourse/tests/acceptance/edit-notification-click-test.js
@@ -0,0 +1,57 @@
+import { acceptance, queryAll } from "discourse/tests/helpers/qunit-helpers";
+import { click, visit } from "@ember/test-helpers";
+import { test } from "qunit";
+
+acceptance("Edit Notification Click", function (needs) {
+  needs.user();
+  needs.pretender((server, helper) => {
+    server.get("/posts/133/revisions/1.json", () => {
+      return helper.response({
+        created_at: "2021-07-30T11:19:59.549Z",
+        post_id: 133,
+        previous_hidden: false,
+        current_hidden: false,
+        first_revision: 2,
+        previous_revision: null,
+        current_revision: 2,
+        next_revision: null,
+        last_revision: 2,
+        current_version: 2,
+        version_count: 2,
+        username: "velesin",
+        display_username: "velesin",
+        avatar_template: "/letter_avatar_proxy/v4/letter/j/13edae/{size}.png",
+        edit_reason: null,
+        body_changes: {
+          inline:
+            '<div class="inline-diff"><p>Hello world this is a test</p><p class="diff-ins">another edit!</p></div>',
+          side_by_side:
+            '<div class="revision-content"><p>Hello world this is a test</p></div><div class="revision-content"><p>Hello world this is a test</p><p class="diff-ins">This is an edit!</p></div>',
+          side_by_side_markdown:
+            '<table class="markdown"><tr><td class="diff-del">Hello world this is a test</td><td class="diff-ins">Hello world this is a test<ins>\n\nThis is an edit!</ins></td></tr></table>',
+        },
+        title_changes: null,
+        user_changes: null,
+        wiki: false,
+        can_edit: true,
+      });
+    });
+  });
+
+  test("history modal is shown when navigating from a non-topic page", async function (assert) {
+    await visit("/");
+    await click(".d-header-icons #current-user");
+    await click("#quick-access-notifications .edited");
+    const [v1, v2] = queryAll(".history-modal .revision-content");
+    assert.equal(
+      v1.textContent.trim(),
+      "Hello world this is a test",
+      "history modal for the edited post is shown"
+    );
+    assert.equal(
+      v2.textContent.trim(),
+      "Hello world this is a testThis is an edit!",
+      "history modal for the edited post is shown"
+    );
+  });
+});
diff --git a/app/assets/javascripts/discourse/tests/acceptance/notifications-test.js b/app/assets/javascripts/discourse/tests/acceptance/notifications-test.js
index 674f532..42946e0 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/notifications-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/notifications-test.js
@@ -35,7 +35,7 @@ acceptance("User Notifications", function (needs) {
 
     await visit("/"); // wait for re-render
 
-    assert.equal(count("#quick-access-notifications li"), 5);
+    assert.equal(count("#quick-access-notifications li"), 6);
 
     // high priority, unread notification - should be first
 

[... diff too long, it was truncated ...]

GitHub sha: e67670c1e4b0a9de886a532a25ba7b87feb2b2d7

This commit appears in #13912 which was approved by eviltrout. It was merged by OsamaSayegh.