-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pull content for moratorium banner from Contentful. #2125
Changes from all commits
d713931
a8a3005
0977a9f
82306c0
52c623f
d3b324d
1d70b2e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { Document } from "@contentful/rich-text-types"; | ||
import { ContentfulCommonStrings } from "@justfixnyc/contentful-common-strings"; | ||
import { useContext } from "react"; | ||
|
||
import { AppContext } from "./app-context"; | ||
import i18n from "./i18n"; | ||
|
||
/** | ||
* Retrieve a common string (one shared across multiple JustFix properties) | ||
* in the current locale from Contentful and return it. | ||
* | ||
* If the string doesn't exist or Contentful integration is disabled, | ||
* `null` will be returned. | ||
*/ | ||
export function useContentfulCommonString(key: string): Document | null { | ||
const ccs = new ContentfulCommonStrings( | ||
useContext(AppContext).server.contentfulCommonStrings || {} | ||
); | ||
|
||
return ccs.get(key, i18n.locale); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
import logging | ||
from django.conf import settings | ||
from django.core.cache import cache | ||
from typing import Any, Dict, Optional | ||
import requests | ||
|
||
|
||
logger = logging.getLogger(__name__) | ||
|
||
ORIGIN = "https://cdn.contentful.com" | ||
|
||
CommonStrings = Dict[str, Any] | ||
|
||
|
||
def _to_common_strings_map(raw: Any) -> CommonStrings: | ||
""" | ||
Converts the given raw Contentful API result and converts | ||
it into a Contentful common strings mapping. | ||
|
||
This is the Python version of the `toCommonStringsMap` | ||
function from the following TypeScript code: | ||
|
||
https://github.com/JustFixNYC/justfix-ts/blob/master/packages/contentful-common-strings/src/fetch-common-strings.ts | ||
""" | ||
|
||
result: CommonStrings = {} | ||
|
||
for item in raw["items"]: | ||
fields = item["fields"] | ||
key = fields.get("id", {}).get("en") | ||
value = fields.get("value") | ||
if key and value: | ||
result[key] = value | ||
|
||
return result | ||
|
||
|
||
def get_common_strings() -> Optional[CommonStrings]: | ||
""" | ||
Fetches Contentful common strings and returns them. | ||
|
||
Caching is used to ensure that we don't trigger Contentful's rate | ||
limiting or cause undue latency. | ||
|
||
If Contentful integration is disabled, or if a network error | ||
occurs and we don't have a cached value, returns `None`. | ||
""" | ||
|
||
if not (settings.CONTENTFUL_ACCESS_TOKEN and settings.CONTENTFUL_SPACE_ID): | ||
# Contentful integration is disabled. | ||
return None | ||
|
||
cache_key = f"contentful_common_strings.{settings.CONTENTFUL_SPACE_ID}" | ||
|
||
result = cache.get(cache_key) | ||
|
||
if result is None: | ||
try: | ||
response = requests.get( | ||
f"{ORIGIN}/spaces/{settings.CONTENTFUL_SPACE_ID}/entries", | ||
{ | ||
"access_token": settings.CONTENTFUL_ACCESS_TOKEN, | ||
"locale": "*", | ||
"metadata.tags.sys.id[in]": settings.CONTENTFUL_COMMON_STRING_TAG, | ||
}, | ||
timeout=settings.CONTENTFUL_TIMEOUT, | ||
) | ||
response.raise_for_status() | ||
result = _to_common_strings_map(response.json()) | ||
cache.set(cache_key, result, settings.CONTENTFUL_CACHE_TIMEOUT) | ||
except Exception: | ||
logger.exception(f"Error while retrieving data from {ORIGIN}") | ||
|
||
return result |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -396,6 +396,10 @@ class JustfixEnvironment(typed_environ.BaseEnvironment): | |
# The origin of the NYC GeoSearch API. | ||
NYC_GEOSEARCH_ORIGIN: str = "https://geosearch.planninglabs.nyc" | ||
|
||
CONTENTFUL_SPACE_ID: str = "" | ||
|
||
CONTENTFUL_ACCESS_TOKEN: str = "" | ||
Comment on lines
+399
to
+401
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should consider just using our actual space ID and access token as defaults here; because the access token is read-only and these credentials are intended for use in browser-side JS, they're effectively public (not secrets), and as such, just hard-coding them as defaults won't be a security hazard, and will also make it easier for developers to get up and running quickly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm into that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this is kind of frustrating, I ended up adding the defaults to the WoW version of this PR, in JustFixNYC/who-owns-what#482, and immediately got this email: ARGH. While I am pretty sure this isn't actually a security hole, I really, really wish it were possible to just tell Contentful to make a space publicly readable so we didn't have to go through this false alarm stuff. |
||
|
||
|
||
class JustfixBuildPipelineDefaults(JustfixEnvironment): | ||
""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
import pytest | ||
from django.core.cache import cache | ||
|
||
from project.contentful import get_common_strings | ||
|
||
|
||
ENTRIES_URL = "https://cdn.contentful.com/spaces/myspaceid/entries" | ||
|
||
CONTENTFUL_DOC = { | ||
"nodeType": "document", | ||
"data": {}, | ||
"content": [ | ||
{ | ||
"nodeType": "paragraph", | ||
"content": [ | ||
{ | ||
"nodeType": "text", | ||
"value": "Hello!", | ||
"marks": [], | ||
"data": {}, | ||
}, | ||
], | ||
}, | ||
], | ||
} | ||
|
||
RAW_ENTRIES_RESPONSE = { | ||
"sys": {"type": "Array"}, | ||
"total": 1, | ||
"skip": 0, | ||
"limit": 100, | ||
"items": [ | ||
{ | ||
"metadata": {"tags": [{"sys": {"type": "Link", "linkType": "Tag", "id": "common"}}]}, | ||
"sys": { | ||
"space": {"sys": {"type": "Link", "linkType": "Space", "id": "markmr2gi204"}}, | ||
"id": "6JHYqWl0h2QWvObWQfNH4m", | ||
"type": "Entry", | ||
"createdAt": "2021-06-16T11:01:57.811Z", | ||
"updatedAt": "2021-06-16T12:44:37.768Z", | ||
"environment": {"sys": {"id": "master", "type": "Link", "linkType": "Environment"}}, | ||
"revision": 6, | ||
"contentType": {"sys": {"type": "Link", "linkType": "ContentType", "id": "string"}}, | ||
}, | ||
"fields": { | ||
"id": {"en": "covidMoratoriumBanner"}, | ||
"value": { | ||
"en": CONTENTFUL_DOC, | ||
}, | ||
}, | ||
} | ||
], | ||
} | ||
|
||
|
||
@pytest.fixture | ||
def enabled(settings): | ||
settings.CONTENTFUL_ACCESS_TOKEN = "myaccesstoken" | ||
settings.CONTENTFUL_SPACE_ID = "myspaceid" | ||
|
||
|
||
class TestGetCommonStrings: | ||
def setup(self): | ||
cache.clear() | ||
|
||
def test_it_returns_none_when_disabled(self): | ||
assert get_common_strings() is None | ||
|
||
def test_it_returns_none_when_enabled_but_err_occurs(self, enabled, requests_mock): | ||
requests_mock.get(ENTRIES_URL, status_code=500) | ||
assert get_common_strings() is None | ||
|
||
def test_it_works(self, enabled, requests_mock): | ||
requests_mock.get(ENTRIES_URL, json=RAW_ENTRIES_RESPONSE) | ||
assert get_common_strings() == {"covidMoratoriumBanner": {"en": CONTENTFUL_DOC}} | ||
|
||
def test_it_caches_result(self, enabled, requests_mock): | ||
requests_mock.get(ENTRIES_URL, json=RAW_ENTRIES_RESPONSE) | ||
|
||
strings = get_common_strings() | ||
assert strings is not None | ||
|
||
requests_mock.get(ENTRIES_URL, status_code=500) | ||
|
||
assert get_common_strings() == strings | ||
|
||
def test_it_does_not_cache_errors(self, enabled, requests_mock): | ||
requests_mock.get(ENTRIES_URL, status_code=500) | ||
|
||
assert get_common_strings() is None | ||
|
||
requests_mock.get(ENTRIES_URL, json=RAW_ENTRIES_RESPONSE) | ||
|
||
assert get_common_strings() is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be bad, if contentful ever goes down. We don't cache anything when there's a network error, so if cdn.contentful.com happens to timeout, all requests to our server could take an unusually long time, effectively resulting in a denial of service.
That said, contentful does use a CDN so this request should be pretty darn fast and timeouts should be extremely rare. Lots of businesses depend on Contentful's CDN to be responsive so we could always just give this a shot and revisit if anything ever goes wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems reasonable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, though there are a few wrinkles--more details at JustFixNYC/who-owns-what#482!