From 7613928e8a0ec6c69801b84e56ead7fe74b035ca Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Fri, 16 Dec 2022 09:17:00 -0500 Subject: [PATCH] apps: Fix redirect from /apps -> https://zulip.com/apps/. When this code was moved from being in zerver in 21a2fd482eae, it kept the `if ZILENCER_ENABLED` blocks. Since ZILENCER and CORPORATE are generally either both on or both off, the if statement became mostly-unnecessary. However, because tests cannot easily remove elements from INSTALLED_APPS and re-determine URL resolution, we switch to checking `if CORPORATE_ENABLED` as a guard, and leave these in-place. The other side effect of this is that with e54ded49c44c, most Zulip deployments started to 404 requests for `/apps` instead of redirecting them to `https://zulip.com/apps/` since they no longer had any path configured for `/apps`. Unfortunately, this URL is in widespread use in the app (e.g. in links from the Welcome Bot), so we should ensure that it does successfully redirect. Add the `/apps` path to `zerver`, but only if not CORPORATE_ENABLED, so the URLs do not overlap. --- corporate/views/portico.py | 20 ++++++++++++----- zerver/context_processors.py | 2 +- zerver/tests/test_docs.py | 43 +++++++++++++++++++++--------------- zproject/urls.py | 8 +++++++ 4 files changed, 48 insertions(+), 25 deletions(-) diff --git a/corporate/views/portico.py b/corporate/views/portico.py index 7d38ef80ea..cca62bf4b6 100644 --- a/corporate/views/portico.py +++ b/corporate/views/portico.py @@ -23,12 +23,20 @@ from zerver.models import Realm @add_google_analytics def apps_view(request: HttpRequest, platform: Optional[str] = None) -> HttpResponse: - if settings.ZILENCER_ENABLED: - return TemplateResponse( - request, - "corporate/apps.html", - ) - return HttpResponseRedirect("https://zulip.com/apps/", status=301) + if not settings.CORPORATE_ENABLED: + # This seems impossible (CORPORATE_ENABLED set to false when + # rendering a "corporate" view) -- but we add it to make + # testing possible. Tests default to running with the + # "corporate" app installed, and unsetting that is difficult, + # as one cannot easily reload the URL resolution -- so we add + # a redirect here, equivalent to the one zerver would have + # installed when "corporate" is not enabled, to make the + # behaviour testable with CORPORATE_ENABLED set to false. + return HttpResponseRedirect("https://zulip.com/apps/", status=301) + return TemplateResponse( + request, + "corporate/apps.html", + ) def app_download_link_redirect(request: HttpRequest, platform: str) -> HttpResponse: diff --git a/zerver/context_processors.py b/zerver/context_processors.py index 328aea1b31..a822ec1d05 100644 --- a/zerver/context_processors.py +++ b/zerver/context_processors.py @@ -76,7 +76,7 @@ def get_valid_realm_from_request(request: HttpRequest) -> Realm: def get_apps_page_url() -> str: - if settings.ZILENCER_ENABLED: + if settings.CORPORATE_ENABLED: return "/apps/" return "https://zulip.com/apps/" diff --git a/zerver/tests/test_docs.py b/zerver/tests/test_docs.py index 47149cebdd..f38dd7f0c3 100644 --- a/zerver/tests/test_docs.py +++ b/zerver/tests/test_docs.py @@ -176,9 +176,8 @@ class DocPageTest(ZulipTestCase): self._test("/errors/5xx/", "Internal server error", landing_page=False) def test_corporate_portico_endpoints(self) -> None: - if settings.ZILENCER_ENABLED: - self._test("/team/", "industry veterans") - self._test("/apps/", "Apps for every platform.") + self._test("/team/", "industry veterans") + self._test("/apps/", "Apps for every platform.") self._test("/history/", "Zulip released as open source!") # Test the i18n version of one of these pages. @@ -524,34 +523,42 @@ class PlansPageTest(ZulipTestCase): class AppsPageTest(ZulipTestCase): def test_get_apps_page_url(self) -> None: - with self.settings(ZILENCER_ENABLED=False): + with self.settings(CORPORATE_ENABLED=False): apps_page_url = get_apps_page_url() self.assertEqual(apps_page_url, "https://zulip.com/apps/") - with self.settings(ZILENCER_ENABLED=True): + with self.settings(CORPORATE_ENABLED=True): apps_page_url = get_apps_page_url() self.assertEqual(apps_page_url, "/apps/") def test_apps_view(self) -> None: - result = self.client_get("/apps") - self.assertEqual(result.status_code, 301) - self.assertTrue(result["Location"].endswith("/apps/")) + with self.settings(CORPORATE_ENABLED=False): + # Note that because this cannot actually uninstall the + # "corporate" app and trigger updates to URL resolution, + # this does not test the "apps/" path installed in + # zproject.urls, but rather the special-case for testing + # in corporate.views.portico + result = self.client_get("/apps") + self.assertEqual(result.status_code, 301) + self.assertTrue(result["Location"].endswith("/apps/")) - with self.settings(ZILENCER_ENABLED=False): result = self.client_get("/apps/") - self.assertEqual(result.status_code, 301) - self.assertTrue(result["Location"] == "https://zulip.com/apps/") + self.assertEqual(result.status_code, 301) + self.assertTrue(result["Location"] == "https://zulip.com/apps/") - with self.settings(ZILENCER_ENABLED=False): result = self.client_get("/apps/linux") - self.assertEqual(result.status_code, 301) - self.assertTrue(result["Location"] == "https://zulip.com/apps/") + self.assertEqual(result.status_code, 301) + self.assertTrue(result["Location"] == "https://zulip.com/apps/") + + with self.settings(CORPORATE_ENABLED=True): + result = self.client_get("/apps") + self.assertEqual(result.status_code, 301) + self.assertTrue(result["Location"].endswith("/apps/")) - with self.settings(ZILENCER_ENABLED=True): result = self.client_get("/apps/") - self.assertEqual(result.status_code, 200) - html = result.content.decode() - self.assertIn("Apps for every platform.", html) + self.assertEqual(result.status_code, 200) + html = result.content.decode() + self.assertIn("Apps for every platform.", html) def test_app_download_link_view(self) -> None: return_value = "https://desktop-download.zulip.com/v5.4.3/Zulip-Web-Setup-5.4.3.exe" diff --git a/zproject/urls.py b/zproject/urls.py index 27973467f0..ece1c834e2 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -798,6 +798,14 @@ urls += [ path("policies/", policy_documentation_view), ] +if not settings.CORPORATE_ENABLED: + # This conditional behavior cannot be tested directly, since + # urls.py is not readily reloaded in Django tests. See the block + # comment inside apps_view for details. + urls += [ + path("apps/", RedirectView.as_view(url="https://zulip.com/apps/", permanent=True)), + ] + # Two-factor URLs if settings.TWO_FACTOR_AUTHENTICATION_ENABLED: urls += [path("", include(tf_urls)), path("", include(tf_twilio_urls))]