From e67670c1e4b0a9de886a532a25ba7b87feb2b2d7 Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Tue, 3 Aug 2021 19:06:23 +0300 Subject: [PATCH] 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: https://github.com/discourse/discourse/blob/016efeadf6f242e04daf5ef8e18c2ca708a1392d/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. --- .../edit-notification-clicks-tracker.js | 16 ++++++ .../discourse/app/models/post-stream.js | 40 ++++++++++++- .../app/widgets/default-notification-item.js | 27 ++++----- .../edit-notification-click-test.js | 57 +++++++++++++++++++ .../tests/acceptance/notifications-test.js | 2 +- .../discourse/tests/acceptance/user-test.js | 2 +- .../tests/fixtures/notification-fixtures.js | 9 +++ .../discourse/tests/helpers/qunit-helpers.js | 2 + .../integration/widgets/user-menu-test.js | 20 +++---- 9 files changed, 149 insertions(+), 26 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/initializers/edit-notification-clicks-tracker.js create mode 100644 app/assets/javascripts/discourse/tests/acceptance/edit-notification-click-test.js 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 00000000000..c9e29142ac7 --- /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 2c0777538d8..a28a4833212 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 8a183f4e87e..ff9d09f2975 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 00000000000..00f5fa61c1b --- /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: + '

Hello world this is a test

another edit!

', + side_by_side: + '

Hello world this is a test

Hello world this is a test

This is an edit!

', + side_by_side_markdown: + '
Hello world this is a testHello world this is a test\n\nThis is an edit!
', + }, + 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 674f5327e92..42946e0786e 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 --git a/app/assets/javascripts/discourse/tests/acceptance/user-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-test.js index 6c8c2acfcbc..3bec0ce6d4c 100644 --- 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" ) ); diff --git a/app/assets/javascripts/discourse/tests/fixtures/notification-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/notification-fixtures.js index ac2fe07fe97..0dbc392ab2e 100644 --- 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, diff --git a/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js b/app/assets/javascripts/discourse/tests/helpers/qunit-helpers.js index 2d5a3fbf5ff..b92c4dd61bb 100644 --- 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); diff --git a/app/assets/javascripts/discourse/tests/integration/widgets/user-menu-test.js b/app/assets/javascripts/discourse/tests/integration/widgets/user-menu-test.js index 624c6837197..f41fd83fc34 100644 --- 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", })