From 72361738d0cf05b542c3814668618d585400f402 Mon Sep 17 00:00:00 2001
From: Joe <johani@discourse.org>
Date: Tue, 29 Sep 2020 00:12:16 +0800
Subject: [PATCH] DEV: don't add username to share links when badges are
 disabled (#10730)

Context: https://meta.discourse.org/t/stop-appending-username-to-url-when-badge-system-disabled/164904

Share links for topics and posts have the current username appended to them when the user is logged in.

If badges are disabled, those added usernames are not beneficial since they're only used to track progress towards certain badges.

This PR makes two change.

1. it moves the logic for the share link url to a centralized helper because it's shared in both topic and post models.
2. it stops username params from being added to share links when badges are disabled.
---
 .../discourse/app/helpers/share-url.js        |  8 +++++++
 .../javascripts/discourse/app/models/post.js  |  9 ++------
 .../javascripts/discourse/app/models/topic.js |  4 ++--
 .../share-and-invite-desktop-test.js          | 22 ++++++++++++++++++
 .../share-and-invite-mobile-test.js           | 23 +++++++++++++++++++
 5 files changed, 57 insertions(+), 9 deletions(-)
 create mode 100644 app/assets/javascripts/discourse/app/helpers/share-url.js

diff --git a/app/assets/javascripts/discourse/app/helpers/share-url.js b/app/assets/javascripts/discourse/app/helpers/share-url.js
new file mode 100644
index 00000000000..afa0f75c44e
--- /dev/null
+++ b/app/assets/javascripts/discourse/app/helpers/share-url.js
@@ -0,0 +1,8 @@
+import { helperContext } from "discourse-common/lib/helpers";
+
+export function resolveShareUrl(url, user) {
+  const badgesEnabled = helperContext().siteSettings.enable_badges;
+  const userSuffix = user && badgesEnabled ? `?u=${user.username_lower}` : "";
+
+  return url + userSuffix;
+}
diff --git a/app/assets/javascripts/discourse/app/models/post.js b/app/assets/javascripts/discourse/app/models/post.js
index 512cf9ea8ca..8da2e306279 100644
--- a/app/assets/javascripts/discourse/app/models/post.js
+++ b/app/assets/javascripts/discourse/app/models/post.js
@@ -17,18 +17,13 @@ import Site from "discourse/models/site";
 import User from "discourse/models/user";
 import showModal from "discourse/lib/show-modal";
 import { fancyTitle } from "discourse/lib/topic-fancy-title";
+import { resolveShareUrl } from "discourse/helpers/share-url";
 
 const Post = RestModel.extend({
   @discourseComputed("url")
   shareUrl(url) {
     const user = User.current();
-    const userSuffix = user ? `?u=${user.username_lower}` : "";
-
-    if (this.firstPost) {
-      return this.get("topic.url") + userSuffix;
-    } else {
-      return url + userSuffix;
-    }
+    return resolveShareUrl(url, user);
   },
 
   new_user: equal("trust_level", 0),
diff --git a/app/assets/javascripts/discourse/app/models/topic.js b/app/assets/javascripts/discourse/app/models/topic.js
index ed9f513d249..13ab5fae0b0 100644
--- a/app/assets/javascripts/discourse/app/models/topic.js
+++ b/app/assets/javascripts/discourse/app/models/topic.js
@@ -24,6 +24,7 @@ import Site from "discourse/models/site";
 import User from "discourse/models/user";
 import bootbox from "bootbox";
 import { deepMerge } from "discourse-common/lib/object";
+import { resolveShareUrl } from "discourse/helpers/share-url";
 
 export function loadTopicView(topic, args) {
   const data = deepMerge({}, args);
@@ -242,8 +243,7 @@ const Topic = RestModel.extend({
   @discourseComputed("url")
   shareUrl(url) {
     const user = User.current();
-    const userQueryString = user ? `?u=${user.get("username_lower")}` : "";
-    return `${url}${userQueryString}`;
+    return resolveShareUrl(url, user);
   },
 
   printUrl: fmt("url", "%@/print"),
diff --git a/test/javascripts/acceptance/share-and-invite-desktop-test.js b/test/javascripts/acceptance/share-and-invite-desktop-test.js
index 445f17701e9..0b6358e0872 100644
--- a/test/javascripts/acceptance/share-and-invite-desktop-test.js
+++ b/test/javascripts/acceptance/share-and-invite-desktop-test.js
@@ -70,3 +70,25 @@ QUnit.test("Post date link", async (assert) => {
 
   assert.ok(exists("#share-link"), "it shows the share modal");
 });
+
+acceptance("Share url with badges disabled - desktop", {
+  loggedIn: true,
+  settings: {
+    enable_badges: false,
+  },
+});
+
+QUnit.test(
+  "topic footer button - badges disabled - desktop",
+  async (assert) => {
+    await visit("/t/internationalization-localization/280");
+    await click("#topic-footer-button-share-and-invite");
+
+    assert.notOk(
+      find(".share-and-invite.modal .modal-panel.share .topic-share-url")
+        .val()
+        .includes("?u=eviltrout"),
+      "it doesn't add the username param when badges are disabled"
+    );
+  }
+);
diff --git a/test/javascripts/acceptance/share-and-invite-mobile-test.js b/test/javascripts/acceptance/share-and-invite-mobile-test.js
index c21ae36b00a..b5b088e87e1 100644
--- a/test/javascripts/acceptance/share-and-invite-mobile-test.js
+++ b/test/javascripts/acceptance/share-and-invite-mobile-test.js
@@ -60,3 +60,26 @@ QUnit.test("Post date link", async (assert) => {
 
   assert.ok(exists("#share-link"), "it shows the share modal");
 });
+
+acceptance("Share url with badges disabled - mobile", {
+  loggedIn: true,
+  mobileView: true,
+  settings: {
+    enable_badges: false,
+  },
+});
+
+QUnit.test("topic footer button - badges disabled - mobile", async (assert) => {
+  await visit("/t/internationalization-localization/280");
+
+  const subject = selectKit(".topic-footer-mobile-dropdown");
+  await subject.expand();
+  await subject.selectRowByValue("share-and-invite");
+
+  assert.notOk(
+    find(".share-and-invite.modal .modal-panel.share .topic-share-url")
+      .val()
+      .includes("?u=eviltrout"),
+    "it doesn't add the username param when badges are disabled"
+  );
+});