DEV: don't add username to share links when badges are disabled (PR #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 used to track progress towards certain badges.

This PR removes those added usernames when badges are disabled.

GitHub

One thing that you could do is define this as resolveShareUrl(user, url), and pass this.currentUser into the function when used in components. Then you can avoid importing User.

Also I would probably scratch line 5 and just make line 7

const badgesEnabled = helperContext.siteSettings.enable_badges;

Thanks Mark!

I wasn’t able to get this.currentUser working but I get your concern. Since the User is defined in both the Topic and Post models. I’ve made some changes to pass the current user from there instead of importing in the helper.

I’ve also made the other change your requested to remove the siteSettings const.

Looks good! Bummer that this.currentUser wasn’t available… Not I am not sure if my original suggestion was correct or not, but I think this looks good.

Feels like reaching out too far. Why don’t you pass the enable_badges site setting value to the function instead? It should be available from the callers.

Sure, I can do that, but I feel like if we do this then the helper is redundant because it would only return one value based on a three parameters that we pass to it from the callers.

const userSuffix = user && badgesEnabled ? `?u=${user.username_lower}` : "";

return url + userSuffix;

And we would need to duplicate some code in both callers (there’s only two for now)

Let me know what works for you.

My threshold for refactoring “duplicated” codes into a method is 3, so not quite 2 :wink:

Up to you really. I’m fine either way (especially since you’ve already done most of the work).