From 26db3be4dd6efc3aba60cbea996fc95393507bc0 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 21 Mar 2024 17:53:52 +0000 Subject: [PATCH] DEV: Improve UX when user profiles are hidden from public (#26293) Previously, we had an instant redirect back to the homepage, and clicking avatars would do nothing. This made things feel 'broken' for anon when 'hide_user_profiles_from_public' was enabled. This commit does a few things to resolve this: 1. Improve our 'exception' system for routes so that developers can deliberately trigger it without an ajax error 2. Improve 'exception' system so that the browser URL bar is updated correctly, and the 'back' button works as expected 3. Replace the redirect-to-home with an 'access denied' error page, with specific copy for 'You must log in to view user profiles' 4. Update user-card logic to display this new page instead of doing nothing on click --- .../discourse/app/controllers/exception.js | 27 ++++++++++-- .../javascripts/discourse/app/lib/url.js | 5 +++ .../app/mixins/card-contents-base.js | 14 +++++-- .../discourse/app/routes/application.js | 41 ++++++++++++------- .../javascripts/discourse/app/routes/user.js | 7 +++- config/locales/client.en.yml | 1 + .../system/user_page/hide_from_public_spec.rb | 25 +++++++++++ 7 files changed, 97 insertions(+), 23 deletions(-) create mode 100644 spec/system/user_page/hide_from_public_spec.rb diff --git a/app/assets/javascripts/discourse/app/controllers/exception.js b/app/assets/javascripts/discourse/app/controllers/exception.js index 65af0845a14..83a28674410 100644 --- a/app/assets/javascripts/discourse/app/controllers/exception.js +++ b/app/assets/javascripts/discourse/app/controllers/exception.js @@ -7,6 +7,22 @@ import DiscourseURL from "discourse/lib/url"; import discourseComputed, { on } from "discourse-common/utils/decorators"; import I18n from "discourse-i18n"; +/** + * You can throw an instance of this error during a route's beforeModel/model/afterModel hooks. + * It will be caught by the top-level ApplicationController, and cause this Exception controller/template + * to be rendered without changing the URL. + */ +export class RouteException { + status; + reason; + + constructor({ status, reason, desc }) { + this.status = status; + this.reason = reason; + this.desc = desc; + } +} + // The controller for the nice error page export default Controller.extend({ thrown: null, @@ -48,7 +64,9 @@ export default Controller.extend({ @discourseComputed("isNetwork", "thrown.status", "thrown") reason(isNetwork, thrownStatus, thrown) { - if (isNetwork) { + if (thrown.reason) { + return thrown.reason; + } else if (isNetwork) { return I18n.t("errors.reasons.network"); } else if (thrownStatus >= 500) { return I18n.t("errors.reasons.server"); @@ -74,7 +92,9 @@ export default Controller.extend({ "thrown" ) desc(networkFixed, isNetwork, thrownStatus, thrownStatusText, thrown) { - if (networkFixed) { + if (thrown.desc) { + return thrown.desc; + } else if (networkFixed) { return I18n.t("errors.desc.network_fixed"); } else if (isNetwork) { return I18n.t("errors.desc.network"); @@ -138,8 +158,7 @@ export default Controller.extend({ back() { // Strip off subfolder const currentURL = DiscourseURL.router.location.getURL(); - if (this.lastTransition && currentURL !== "/exception") { - this.lastTransition.abort(); + if (this.lastTransition?.method === "replace") { this.setProperties({ lastTransition: null, thrown: null }); // Can't use routeTo because it handles navigation to the same page DiscourseURL.handleURL(currentURL); diff --git a/app/assets/javascripts/discourse/app/lib/url.js b/app/assets/javascripts/discourse/app/lib/url.js index a47edc3710b..f1d12162f10 100644 --- a/app/assets/javascripts/discourse/app/lib/url.js +++ b/app/assets/javascripts/discourse/app/lib/url.js @@ -177,6 +177,11 @@ const DiscourseURL = EmberObject.extend({ } }, + pushState(path) { + path = withoutPrefix(path); + this.router._routerMicrolib.updateURL(path); + }, + routeToTag(a) { // skip when we are provided nowhere to route to if (!a || !a.href) { diff --git a/app/assets/javascripts/discourse/app/mixins/card-contents-base.js b/app/assets/javascripts/discourse/app/mixins/card-contents-base.js index 5f6ac864f58..93d70a93753 100644 --- a/app/assets/javascripts/discourse/app/mixins/card-contents-base.js +++ b/app/assets/javascripts/discourse/app/mixins/card-contents-base.js @@ -45,7 +45,7 @@ export default Mixin.create({ _show(username, target, event) { // No user card for anon if (this.siteSettings.hide_user_profiles_from_public && !this.currentUser) { - return false; + return true; } username = escapeExpression(username.toString()); @@ -157,9 +157,15 @@ export default Mixin.create({ return true; } - event.preventDefault(); - event.stopPropagation(); - return this._show(transformText(matchingEl), matchingEl, event); + const shouldBubble = this._show( + transformText(matchingEl), + matchingEl, + event + ); + if (!shouldBubble) { + event.preventDefault(); + event.stopPropagation(); + } } return false; diff --git a/app/assets/javascripts/discourse/app/routes/application.js b/app/assets/javascripts/discourse/app/routes/application.js index d659d753be0..076c5979560 100644 --- a/app/assets/javascripts/discourse/app/routes/application.js +++ b/app/assets/javascripts/discourse/app/routes/application.js @@ -4,6 +4,7 @@ import CreateAccount from "discourse/components/modal/create-account"; import ForgotPassword from "discourse/components/modal/forgot-password"; import KeyboardShortcutsHelp from "discourse/components/modal/keyboard-shortcuts-help"; import LoginModal from "discourse/components/modal/login"; +import { RouteException } from "discourse/controllers/exception"; import { setting } from "discourse/lib/computed"; import cookie from "discourse/lib/cookie"; import logout from "discourse/lib/logout"; @@ -125,23 +126,27 @@ const ApplicationRoute = DiscourseRoute.extend({ error(err, transition) { const xhrOrErr = err.jqXHR ? err.jqXHR : err; const exceptionController = this.controllerFor("exception"); + let shouldBubble = false; const themeOrPluginSource = identifySource(err); - // eslint-disable-next-line no-console - console.error( - ...[consolePrefix(err, themeOrPluginSource), xhrOrErr].filter(Boolean) - ); - - if (xhrOrErr && xhrOrErr.status === 404) { - return this.router.transitionTo("exception-unknown"); - } - - if (themeOrPluginSource) { - this.clientErrorHandler.displayErrorNotice( - "Error loading route", - themeOrPluginSource + if (!(xhrOrErr instanceof RouteException)) { + shouldBubble = true; + // eslint-disable-next-line no-console + console.error( + ...[consolePrefix(err, themeOrPluginSource), xhrOrErr].filter(Boolean) ); + + if (xhrOrErr && xhrOrErr.status === 404) { + return this.router.transitionTo("exception-unknown"); + } + + if (themeOrPluginSource) { + this.clientErrorHandler.displayErrorNotice( + "Error loading route", + themeOrPluginSource + ); + } } exceptionController.setProperties({ @@ -149,8 +154,16 @@ const ApplicationRoute = DiscourseRoute.extend({ thrown: xhrOrErr, }); + if (transition.intent.url) { + if (transition.method === "replace") { + DiscourseURL.replaceState(transition.intent.url); + } else { + DiscourseURL.pushState(transition.intent.url); + } + } + this.intermediateTransitionTo("exception"); - return true; + return shouldBubble; }, showLogin: unlessStrictlyReadOnly( diff --git a/app/assets/javascripts/discourse/app/routes/user.js b/app/assets/javascripts/discourse/app/routes/user.js index 48b42f721c1..88372c9a0d1 100644 --- a/app/assets/javascripts/discourse/app/routes/user.js +++ b/app/assets/javascripts/discourse/app/routes/user.js @@ -1,8 +1,10 @@ import { action } from "@ember/object"; import { service } from "@ember/service"; +import { RouteException } from "discourse/controllers/exception"; import User from "discourse/models/user"; import DiscourseRoute from "discourse/routes/discourse"; import { bind } from "discourse-common/utils/decorators"; +import I18n from "discourse-i18n"; export default DiscourseRoute.extend({ router: service(), @@ -12,7 +14,10 @@ export default DiscourseRoute.extend({ beforeModel() { if (this.siteSettings.hide_user_profiles_from_public && !this.currentUser) { - this.router.replaceWith("discovery"); + throw new RouteException({ + status: 403, + desc: I18n.t("user.login_to_view_profile"), + }); } }, diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 159657b88eb..92a04329ff6 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -1164,6 +1164,7 @@ en: warning: "Are you sure you want to clear your featured topic?" use_current_timezone: "Use Current Timezone" profile_hidden: "This user's public profile is hidden." + login_to_view_profile: "You must log in to view user profiles" inactive_user: "This user is no longer active." expand_profile: "Expand" sr_expand_profile: "Expand profile details" diff --git a/spec/system/user_page/hide_from_public_spec.rb b/spec/system/user_page/hide_from_public_spec.rb new file mode 100644 index 00000000000..c3fb78ef7cb --- /dev/null +++ b/spec/system/user_page/hide_from_public_spec.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +describe "hide_user_profiles_from_public", type: :system do + let(:user) { Fabricate(:user) } + before { SiteSetting.hide_user_profiles_from_public = true } + + it "displays an error when navigating straight to a profile" do + visit("/u/#{user.username}") + expect(page).to have_css(".error-page .reason", text: I18n.t("js.errors.reasons.forbidden")) + expect(page).to have_css(".error-page .desc", text: I18n.t("js.user.login_to_view_profile")) + end + + it "displays an error when navigating from an internal link" do + Fabricate(:post, user: user) + visit("/latest") + find("[data-user-card='#{user.username}']").click + + expect(page).to have_css(".error-page .reason", text: I18n.t("js.errors.reasons.forbidden")) + expect(page).to have_css(".error-page .desc", text: I18n.t("js.user.login_to_view_profile")) + expect(page).to have_current_path("/u/#{user.username}") + + find(".error-page .buttons .btn-primary", text: "Back").click + expect(page).to have_current_path("/latest") + end +end