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 `return`s if it can't find the post: 016efeadf6/app/assets/javascripts/discourse/app/controllers/topic.js (L145-L152) 2) 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.
2021-08-03 19:06:23 +03:00 · 2021-08-03 19:06:23 +03:00 · e67670c1e4
parent 52520638ca
commit e67670c1e4
9 changed files with 149 additions and 26 deletions
--- a/app/assets/javascripts/discourse/app/initializers/edit-notification-clicks-tracker.js
+++ 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);
+        }
+      );
+  },
+};
--- 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
+          );
+        });
+      }
+    }
+  },
 });
--- 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) {
--- a/app/assets/javascripts/discourse/tests/acceptance/edit-notification-click-test.js
+++ 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"
+    );
+  });
+});
--- 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

--- a/app/assets/javascripts/discourse/tests/acceptance/user-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/user-test.js
@ -45,7 +45,7 @@ acceptance("User Routes", function (needs) {
    const $links = queryAll(".item.notification a");

    assert.ok(
-      $links[1].href.includes(
+      $links[2].href.includes(
        "/u/eviltrout/notifications/likes-received?acting_username=aquaman"
      )
    );
--- a/app/assets/javascripts/discourse/tests/fixtures/notification-fixtures.js
+++ b/app/assets/javascripts/discourse/tests/fixtures/notification-fixtures.js
@ -4,6 +4,15 @@ import { NOTIFICATION_TYPES } from "./concerns/notification-types";
 export default {
  "/notifications": {
    notifications: [
+      {
+        id: 5340,
+        notification_type: NOTIFICATION_TYPES.edited,
+        read: false,
+        post_number: 1,
+        topic_id: 130,
+        slug: "lorem-ipsum-dolor-sit-amet",
+        data: { topic_title: "edited topic 443", display_username: "velesin", revision_number: 1, original_post_id: 133, original_post_type: 1, original_username: "velesin" },
+      },
      {
        id: 123,
        notification_type: NOTIFICATION_TYPES.replied,
--- a/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js
+++ b/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js
@ -46,6 +46,7 @@ import {
  cleanUpComposerUploadMarkdownResolver,
  cleanUpComposerUploadProcessor,
 } from "discourse/components/composer-editor";
+import { resetLastEditNotificationClick } from "discourse/models/post-stream";

 const LEGACY_ENV = !setupApplicationTest;

@ -282,6 +283,7 @@ export function acceptance(name, optionsOrCallback) {
      cleanUpComposerUploadHandler();
      cleanUpComposerUploadProcessor();
      cleanUpComposerUploadMarkdownResolver();
+      resetLastEditNotificationClick();
      app._runInitializer("instanceInitializers", (initName, initializer) => {
        if (initializer && initializer.teardown) {
          initializer.teardown(this.container);
--- a/app/assets/javascripts/discourse/tests/integration/widgets/user-menu-test.js
+++ b/app/assets/javascripts/discourse/tests/integration/widgets/user-menu-test.js
@ -37,25 +37,25 @@ discourseModule(
      async test(assert) {
        const $links = queryAll(".quick-access-panel li a");

-        assert.equal($links.length, 5);
-        assert.ok($links[0].href.includes("/t/a-slug/123"));
+        assert.equal($links.length, 6);
+        assert.ok($links[1].href.includes("/t/a-slug/123"));

        assert.ok(
-          $links[1].href.includes(
+          $links[2].href.includes(
            "/u/eviltrout/notifications/likes-received?acting_username=aquaman"
          )
        );

        assert.equal(
-          $links[1].text,
+          $links[2].text,
          `aquaman ${I18n.t("notifications.liked_consolidated_description", {
            count: 5,
          })}`
        );

-        assert.ok($links[2].href.includes("/u/test2/messages/group/test"));
+        assert.ok($links[3].href.includes("/u/test2/messages/group/test"));
        assert.ok(
-          $links[2].innerHTML.includes(
+          $links[3].innerHTML.includes(
            I18n.t("notifications.group_message_summary", {
              count: 5,
              group_name: "test",
@ -63,16 +63,16 @@ discourseModule(
          )
        );

-        assert.ok($links[3].href.includes("/u/test1"));
+        assert.ok($links[4].href.includes("/u/test1"));
        assert.ok(
-          $links[3].innerHTML.includes(
+          $links[4].innerHTML.includes(
            I18n.t("notifications.invitee_accepted", { username: "test1" })
          )
        );

-        assert.ok($links[4].href.includes("/g/test"));
+        assert.ok($links[5].href.includes("/g/test"));
        assert.ok(
-          $links[4].innerHTML.includes(
+          $links[5].innerHTML.includes(
            I18n.t("notifications.membership_request_accepted", {
              group_name: "test",
            })