Migrate Place Page Functionality to Flask Service #4627

dwnoble · 2024-09-14T00:15:07Z

Migrate Place Page Functionality to Flask Service with `place_charts` and `related_places` Endpoints

This PR migrates place page functionality from the Mixer service's /v1/internal/page/place endpoint to the Flask service by introducing two new endpoints: place_charts and related_places. This change centralizes application-specific logic within the Flask service and optimizes payload sizes by adjusting how observational data is handled.

TODO / Next steps:

Add childPlaceType
Fetch geo overlapping places?
Create new place page layout based on updated mockups
Use these new endpoints to fetch data for the updated place page

New Endpoints

`/api/dev-place/charts/<place_dcid>`

Returns chart definitions for a specified place, translated into the user's locale.
Features:
- Filters charts based on data availability for the place.
- Excludes observational data from the payload; web components will fetch this data as needed.
- Supports localization for chart titles and category strings.

`/api/dev-place/related_places/<place_dcid>`

Provides related place data, including child, nearby, and similar places.
Features:
- Fetches child places contained within the specified place.
- Retrieves geographically nearby places.
- Identifies similar places based on predefined cohorts.
- Supports localization for place names.

Rationale

Application-Specific Logic: Moving the logic from Mixer to Flask keeps application-specific code within the application layer.
Optimized Payloads: Excludes inline observational data resulting in a smaller payloads. Observational data will be fetched separately by the client via web components.

Testing

Unit Tests: Added tests for both endpoints including basic localization tests

…to new-place-page-api

…d related places

beets

sorry for the late review!

we actually started with these as separate calls processed in flask, then we combined it in flask, then we moved it to mixer -- each move was driven by page latency improvements.

would we make both api calls concurrently from the client? the results from both calls would be needed to start rendering charts so i wonder what would improve page performance (since it's flask, maybe it's better to keep it split like this).

thanks also for adding types & tests!

beets · 2024-09-16T21:57:58Z

server/lib/util.py

+  return wrapper
+
+
+from flask import jsonify


super nit: this should be added to the list of includes above.

Whoops, fixed

beets · 2024-09-16T21:58:05Z

server/lib/util.py

@@ -922,3 +923,44 @@ def post_body_cache_key():
  else:
    cache_key = full_path
  return cache_key
+
+
+def log_execution_time(func):


this is nice!

beets · 2024-09-16T22:00:04Z

server/routes/dev_place/api.py

+}
+
+# Define blueprint
+bp = Blueprint("dev_place_api", __name__, url_prefix='/api/place')


any issues with shared_api/place.py also registering the same url_prefix?

Good callout- it looks like blueprint allows for this, but it's a little confusing. Changing to /api/dev-place temporarily, and ill migrate it to /api/place before this makes it to production

beets · 2024-09-17T18:42:46Z

server/routes/dev_place/types.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""
+Dev Place API dataclass types


nit: we can drop the "dev" prefix in the files so we don't forget to remove when these pages become the default place page.

beets · 2024-09-17T18:43:02Z

server/routes/dev_place/types.py

+
+
+@dataclass
+class Chart:


this is nice - thanks for adding types

beets · 2024-09-17T22:13:26Z

server/routes/dev_place/utils.py

+  """
+  charts = []
+  for page_config_item in chart_config:
+    denominator = next(iter(page_config_item.get("denominator", [])), None)


if i'm reading this right, this gets the first denominator from the list.

could you make sure it works in cases like this
the sv and denominator should come from the same index where the denominator.size() > 1

Good catch! fixed

beets · 2024-09-17T22:15:28Z

server/routes/dev_place/utils.py

+    Builds a dictionary of translated category strings for each unique category found in 
+    the chart configuration.


update the comment?

beets · 2024-09-17T22:16:44Z

server/routes/dev_place/api.py

+  place = place_utils.fetch_place(place_dcid, locale=g.locale)
+
+  # Retrieve available place page charts
+  chart_config = current_app.config['CHART_CONFIG']


don't forget to make a deep copy of the chart_config before passing it along to functions that alter the config. in prod, the config will get permanently updated across requests

Good call-done!

beets · 2024-09-17T22:22:18Z

server/tests/routes/api/dev_place_test.py

+        "scale":
+            True,
+        "denominator":
+            "Count_Person_25OrMoreYears",


consider adding another test with more denominators, and one without translations

beets · 2024-09-17T22:31:29Z

server/routes/dev_place/utils.py

@@ -13,10 +13,18 @@
 # limitations under the License.
 """Helper functions for development place page routes"""


we can drop "development" here too

dwnoble · 2024-09-26T18:40:33Z

sorry for the late review!

we actually started with these as separate calls processed in flask, then we combined it in flask, then we moved it to mixer -- each move was driven by page latency improvements.

would we make both api calls concurrently from the client? the results from both calls would be needed to start rendering charts so i wonder what would improve page performance (since it's flask, maybe it's better to keep it split like this).

thanks also for adding types & tests!

Good to know! I've been paying attention to the latency, and i'll put together some metrics about how this performs vs the mixer version.

Yes im thinking we can make both calls concurrently from the client, and we can have some logic to render the page after both calls return.

dwnoble added 2 commits September 12, 2024 11:22

Merge branch 'master' of https://github.com/datacommonsorg/website in…

5188067

…to new-place-page-api

Added dev place api endpoints for fetching place chart information an…

cbb3591

…d related places

dwnoble requested review from shifucun and beets September 14, 2024 00:15

beets approved these changes Sep 17, 2024

View reviewed changes

dwnoble added 3 commits September 26, 2024 14:32

pr feedback

7a694f1

pr feedback

8df9a13

Merge branch 'master' into new-place-page-api

ba7609a

dwnoble enabled auto-merge (squash) September 27, 2024 20:50

dwnoble merged commit b204f85 into datacommonsorg:master Sep 27, 2024
8 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Place Page Functionality to Flask Service #4627

Migrate Place Page Functionality to Flask Service #4627

dwnoble commented Sep 14, 2024 •

edited

Loading

beets left a comment

beets Sep 16, 2024

dwnoble Sep 26, 2024

beets Sep 16, 2024

beets Sep 16, 2024

dwnoble Sep 27, 2024

beets Sep 17, 2024

dwnoble Sep 26, 2024

beets Sep 17, 2024

beets Sep 17, 2024

dwnoble Sep 27, 2024

beets Sep 17, 2024

dwnoble Sep 26, 2024

beets Sep 17, 2024

dwnoble Sep 26, 2024

beets Sep 17, 2024

dwnoble Sep 27, 2024

beets Sep 17, 2024

dwnoble Sep 26, 2024

dwnoble commented Sep 26, 2024

		Builds a dictionary of translated category strings for each unique category found in
		the chart configuration.

		@@ -13,10 +13,18 @@
		# limitations under the License.
		"""Helper functions for development place page routes"""



		@dataclass
		class Chart:

Migrate Place Page Functionality to Flask Service #4627

Migrate Place Page Functionality to Flask Service #4627

Conversation

dwnoble commented Sep 14, 2024 • edited Loading

Migrate Place Page Functionality to Flask Service with place_charts and related_places Endpoints

New Endpoints

/api/dev-place/charts/<place_dcid>

/api/dev-place/related_places/<place_dcid>

Rationale

Testing

beets left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnoble commented Sep 26, 2024

dwnoble commented Sep 14, 2024 •

edited

Loading

Migrate Place Page Functionality to Flask Service with `place_charts` and `related_places` Endpoints

`/api/dev-place/charts/<place_dcid>`

`/api/dev-place/related_places/<place_dcid>`