mirror of https://github.com/zulip/zulip.git
apps: Fix redirect from /apps -> https://zulip.com/apps/.
When this code was moved from being in zerver in21a2fd482e
, 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 withe54ded49c4
, 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.
This commit is contained in:
parent
13ad9e8323
commit
7613928e8a
|
@ -23,12 +23,20 @@ from zerver.models import Realm
|
||||||
|
|
||||||
@add_google_analytics
|
@add_google_analytics
|
||||||
def apps_view(request: HttpRequest, platform: Optional[str] = None) -> HttpResponse:
|
def apps_view(request: HttpRequest, platform: Optional[str] = None) -> HttpResponse:
|
||||||
if settings.ZILENCER_ENABLED:
|
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(
|
return TemplateResponse(
|
||||||
request,
|
request,
|
||||||
"corporate/apps.html",
|
"corporate/apps.html",
|
||||||
)
|
)
|
||||||
return HttpResponseRedirect("https://zulip.com/apps/", status=301)
|
|
||||||
|
|
||||||
|
|
||||||
def app_download_link_redirect(request: HttpRequest, platform: str) -> HttpResponse:
|
def app_download_link_redirect(request: HttpRequest, platform: str) -> HttpResponse:
|
||||||
|
|
|
@ -76,7 +76,7 @@ def get_valid_realm_from_request(request: HttpRequest) -> Realm:
|
||||||
|
|
||||||
|
|
||||||
def get_apps_page_url() -> str:
|
def get_apps_page_url() -> str:
|
||||||
if settings.ZILENCER_ENABLED:
|
if settings.CORPORATE_ENABLED:
|
||||||
return "/apps/"
|
return "/apps/"
|
||||||
return "https://zulip.com/apps/"
|
return "https://zulip.com/apps/"
|
||||||
|
|
||||||
|
|
|
@ -176,7 +176,6 @@ class DocPageTest(ZulipTestCase):
|
||||||
self._test("/errors/5xx/", "Internal server error", landing_page=False)
|
self._test("/errors/5xx/", "Internal server error", landing_page=False)
|
||||||
|
|
||||||
def test_corporate_portico_endpoints(self) -> None:
|
def test_corporate_portico_endpoints(self) -> None:
|
||||||
if settings.ZILENCER_ENABLED:
|
|
||||||
self._test("/team/", "industry veterans")
|
self._test("/team/", "industry veterans")
|
||||||
self._test("/apps/", "Apps for every platform.")
|
self._test("/apps/", "Apps for every platform.")
|
||||||
|
|
||||||
|
@ -524,30 +523,38 @@ class PlansPageTest(ZulipTestCase):
|
||||||
|
|
||||||
class AppsPageTest(ZulipTestCase):
|
class AppsPageTest(ZulipTestCase):
|
||||||
def test_get_apps_page_url(self) -> None:
|
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()
|
apps_page_url = get_apps_page_url()
|
||||||
self.assertEqual(apps_page_url, "https://zulip.com/apps/")
|
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()
|
apps_page_url = get_apps_page_url()
|
||||||
self.assertEqual(apps_page_url, "/apps/")
|
self.assertEqual(apps_page_url, "/apps/")
|
||||||
|
|
||||||
def test_apps_view(self) -> None:
|
def test_apps_view(self) -> None:
|
||||||
|
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")
|
result = self.client_get("/apps")
|
||||||
self.assertEqual(result.status_code, 301)
|
self.assertEqual(result.status_code, 301)
|
||||||
self.assertTrue(result["Location"].endswith("/apps/"))
|
self.assertTrue(result["Location"].endswith("/apps/"))
|
||||||
|
|
||||||
with self.settings(ZILENCER_ENABLED=False):
|
|
||||||
result = self.client_get("/apps/")
|
result = self.client_get("/apps/")
|
||||||
self.assertEqual(result.status_code, 301)
|
self.assertEqual(result.status_code, 301)
|
||||||
self.assertTrue(result["Location"] == "https://zulip.com/apps/")
|
self.assertTrue(result["Location"] == "https://zulip.com/apps/")
|
||||||
|
|
||||||
with self.settings(ZILENCER_ENABLED=False):
|
|
||||||
result = self.client_get("/apps/linux")
|
result = self.client_get("/apps/linux")
|
||||||
self.assertEqual(result.status_code, 301)
|
self.assertEqual(result.status_code, 301)
|
||||||
self.assertTrue(result["Location"] == "https://zulip.com/apps/")
|
self.assertTrue(result["Location"] == "https://zulip.com/apps/")
|
||||||
|
|
||||||
with self.settings(ZILENCER_ENABLED=True):
|
with self.settings(CORPORATE_ENABLED=True):
|
||||||
|
result = self.client_get("/apps")
|
||||||
|
self.assertEqual(result.status_code, 301)
|
||||||
|
self.assertTrue(result["Location"].endswith("/apps/"))
|
||||||
|
|
||||||
result = self.client_get("/apps/")
|
result = self.client_get("/apps/")
|
||||||
self.assertEqual(result.status_code, 200)
|
self.assertEqual(result.status_code, 200)
|
||||||
html = result.content.decode()
|
html = result.content.decode()
|
||||||
|
|
|
@ -798,6 +798,14 @@ urls += [
|
||||||
path("policies/<slug:article>", policy_documentation_view),
|
path("policies/<slug:article>", 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
|
# Two-factor URLs
|
||||||
if settings.TWO_FACTOR_AUTHENTICATION_ENABLED:
|
if settings.TWO_FACTOR_AUTHENTICATION_ENABLED:
|
||||||
urls += [path("", include(tf_urls)), path("", include(tf_twilio_urls))]
|
urls += [path("", include(tf_urls)), path("", include(tf_twilio_urls))]
|
||||||
|
|
Loading…
Reference in New Issue