[Place Page Revamp] Fix one chart and add API Tests (#4922)

Fixes one chart metadata to remove scaling on units that don't need it.

Adds significant test coverage across our utils for the new place page.
Creates helpers for mocking data. More testing to come.

server/config/chart_config/economics_new.json

@@ -1082,7 +1082,6 @@
     ],
     "denominator": [],
     "non_dividable": false,
-    "scaling": 100,
     "unit": "%",
     "blocks": [
       {

server/routes/dev_place/api.py

@@ -56,20 +56,9 @@ async def place_charts(place_dcid: str):
   - Charts specific to the place
   - Translated category strings for the charts
   """
-  # Validate the category parameter.
-  place_category = request.args.get("category", place_utils.OVERVIEW_CATEGORY)
-  if place_category not in place_utils.ALLOWED_CATEGORIES:
-    return error_response(
-        f"Argument 'category' {place_category} must be one of: {', '.join(place_utils.ALLOWED_CATEGORIES)}"
-    )
-
-  # Retrieve available place page charts
-  full_chart_config = place_utils.read_chart_configs()
 
-  # Blocking call to fetch the current place info
-  place = await asyncio.to_thread(place_utils.fetch_place, place_dcid, g.locale)
-
-  async def fetch_place_types() -> tuple[Place | None, str, str | None]:
+  async def fetch_place_types(
+      place: Place) -> tuple[Place | None, str, str | None]:
     """Asynchronously fetch the parent place override, the child place type to
     highlight, and the current place type to highlight.
     Returns a Tuple with the Place override, the child place type & place type.
@@ -87,13 +76,27 @@ async def fetch_place_types() -> tuple[Place | None, str, str | None]:
                                 child_place_type_to_highlight_task,
                                 place_type_task)
 
+  # Validate the category parameter.
+  place_category = request.args.get("category", place_utils.OVERVIEW_CATEGORY)
+  if place_category not in place_utils.ALLOWED_CATEGORIES:
+    return error_response(
+        f"Argument 'category' {place_category} must be one of: {', '.join(place_utils.ALLOWED_CATEGORIES)}"
+    )
+
+  # Retrieve available place page charts
+  full_chart_config = place_utils.read_chart_configs()
+
+  # Blocking call to fetch the current place info
+  place = await asyncio.to_thread(place_utils.fetch_place, place_dcid, g.locale)
+
   parent_place_override, child_place_type_to_highlight, place_type = await fetch_place_types(
-  )
+      place)
+
   parent_place_dcid = parent_place_override.dcid if parent_place_override else None
 
   # Filter out place page charts that don't have any data for the current place_dcid
   chart_config_existing_data = await asyncio.to_thread(
-      place_utils.filter_chart_config_for_data_existence,
+      place_utils.memoized_filter_chart_config_for_data_existence,
       chart_config=full_chart_config,
       place_dcid=place_dcid,
       place_type=place_type,

server/routes/dev_place/types.py

@@ -55,6 +55,12 @@ class Place:
   types: List[str]
   dissolved: bool = False
 
+  def __eq__(self, other):
+    if not isinstance(other, Place):
+      return False
+    return (self.dcid == other.dcid and self.name == other.name and
+            self.types == other.types and self.dissolved == other.dissolved)
+
 
 @dataclass
 class Category:
@@ -93,6 +99,11 @@ class ServerChartMetadata:
   type: str
   max_places: Optional[int] = None
 
+  def __eq__(self, other):
+    if not isinstance(other, ServerChartMetadata):
+      return False
+    return (self.type == other.type and self.max_places == other.max_places)
+
 
 @dataclass
 class ServerBlockMetadata:
@@ -103,6 +114,15 @@ class ServerBlockMetadata:
   non_dividable: bool = False  # After existence checks
   title: Optional[str] = None
 
+  def __eq__(self, other):
+    if not isinstance(other, ServerBlockMetadata):
+      return False
+    return (self.place_scope == other.place_scope and
+            self.is_overview == other.is_overview and
+            self.is_overview == other.is_overview and
+            self.non_dividable == other.non_dividable and
+            self.title == other.title and self.charts == other.charts)
+
 
 @dataclass
 class ServerChartConfiguration:
@@ -120,6 +140,19 @@ class ServerChartConfiguration:
   non_dividable: bool = False  # Read in from configs
   scale: bool = False
 
+  def __eq__(self, other):
+    if not isinstance(other, ServerChartConfiguration):
+      return False
+    return (self.category == other.category and
+            self.title_id == other.title_id and self.title == other.title and
+            self.description == other.description and
+            self.variables == other.variables and
+            self.denominator == other.denominator and
+            self.blocks == other.blocks and self.unit == other.unit and
+            self.scaling == other.scaling and
+            self.non_dividable == other.non_dividable and
+            self.scale == other.scale)
+
 
 @dataclass
 class OverviewTableDataRow:

server/routes/dev_place/utils.py

@@ -21,7 +21,6 @@
 
 import flask
 from flask import current_app
-from flask_babel import gettext
 
 from server.lib import fetch
 from server.lib.cache import cache
@@ -65,6 +64,11 @@
 ALLOWED_CATEGORIES = {OVERVIEW_CATEGORY}.union(TOPICS)
 
 
+def get_place_url(place_dcid: str) -> str:
+  """Returns the URL for the flask place endpoint."""
+  return flask.url_for('place.place', place_dcid=place_dcid)
+
+
 def get_place_html_link(place_dcid: str, place_name: str) -> str:
   """Get <a href-place page url> tag linking to the place page for a place
   
@@ -75,7 +79,7 @@ def get_place_html_link(place_dcid: str, place_name: str) -> str:
   Returns:
     An html anchor tag linking to a place page.
   """
-  url = flask.url_for('place.place', place_dcid=place_dcid)
+  url = get_place_url(place_dcid)
   return f'<a href="{url}">{place_name}</a>'
 
 
@@ -101,7 +105,6 @@ def get_parent_places(dcid: str) -> List[Place]:
   return all_parents
 
 
-# def get_ordered_parents_to_highlight(all_parents: List[Place]) -> List[Place]:
 def get_ordered_by_place_type_to_highlight(places: List[Place]) -> List[Place]:
   """Returns the ordered list of parents to highlight.
   We only consider the types to highlight we favor types that mean more to users (such as State, Country) over entities such as UNGeoRegions, ContinentalUnions etc.
@@ -172,9 +175,9 @@ def get_place_type_with_parent_places_links(dcid: str) -> str:
   ]
 
   if links:
-    return gettext('%(placeType)s in %(parentPlaces)s',
-                   placeType=place_type_display_name,
-                   parentPlaces=', '.join(links))
+    return place_api.translate('%(placeType)s in %(parentPlaces)s',
+                               placeType=place_type_display_name,
+                               parentPlaces=', '.join(links))
   return ''
 
 
@@ -257,7 +260,6 @@ def count_places_per_stat_var(
   return stat_var_to_places_with_data
 
 
-@cache.memoize(timeout=TIMEOUT)
 async def filter_chart_config_for_data_existence(
     chart_config: List[ServerChartConfiguration], place_dcid: str,
     place_type: str, child_place_type: str,
@@ -291,7 +293,6 @@ async def fetch_and_process_stats():
     current_place_obs_point_response, child_places_obs_point_within, peer_places_obs_point_within, fetch_peer_places = await asyncio.gather(
         current_place_obs_point_task, child_places_obs_point_within_task,
         peer_places_obs_point_within_task, fetch_peer_places_task)
-
     count_places_per_child_sv_task = asyncio.to_thread(
         count_places_per_stat_var, child_places_obs_point_within,
         child_places_stat_var_dcids, 2)
@@ -442,6 +443,18 @@ async def fetch_and_process_stats():
   return valid_chart_configs
 
 
+@cache.memoize(timeout=TIMEOUT)
+async def memoized_filter_chart_config_for_data_existence(
+    chart_config: List[ServerChartConfiguration], place_dcid: str,
+    place_type: str, child_place_type: str,
+    parent_place_dcid: str) -> List[ServerChartConfiguration]:
+  """Memoized version of filter_chart_config_for_data_existence"""
+  return await filter_chart_config_for_data_existence(chart_config, place_dcid,
+                                                      place_type,
+                                                      child_place_type,
+                                                      parent_place_dcid)
+
+
 def check_geo_data_exists(place_dcid: str, child_place_type: str) -> bool:
   """
   Check if geo data exists for a given place and child place type.
@@ -743,7 +756,7 @@ def translate_chart_config(
     parent_place_name: str | None) -> List[ServerChartConfiguration]:
   """
   Translates the 'titleId' field in each chart configuration item into a readable 'title'
-  using the gettext function.
+  using the place_api.translate function.
 
   Args:
       chart_config (List[Dict]): A list of dictionaries where each dictionary contains 
@@ -778,17 +791,17 @@ def translate_chart_config(
         title_sections.append(place_name)
       if translated_block.place_scope == "PEER_PLACES_WITHIN_PARENT":
         title_sections.append(
-            gettext(OTHER_PLACES_IN_PARENT_PLACE_STR,
-                    placeType=translated_place_type,
-                    parentPlace=parent_place_name))
+            place_api.translate(OTHER_PLACES_IN_PARENT_PLACE_STR,
+                                placeType=translated_place_type,
+                                parentPlace=parent_place_name))
       elif translated_block.place_scope == "CHILD_PLACES":
         title_sections.append(
-            gettext(PLACE_TYPE_IN_PARENT_PLACES_STR,
-                    placeType=translated_child_place_type,
-                    parentPlaces=place_name))
+            place_api.translate(PLACE_TYPE_IN_PARENT_PLACES_STR,
+                                placeType=translated_child_place_type,
+                                parentPlaces=place_name))
 
       if translated_item.title_id:
-        title_sections.append(gettext(translated_item.title_id))
+        title_sections.append(place_api.translate(translated_item.title_id))
       translated_block.title = ': '.join(title_sections)
 
     translated_chart_config.append(translated_item)
@@ -837,10 +850,10 @@ def get_categories_metadata(
     has_more_charts = block_count_category_charts.get(
         category, 0) < block_count_all_charts.get(category, 0)
 
-    category = Category(
-        name=category,
-        translatedName=gettext(f'CHART_TITLE-CHART_CATEGORY-{category}'),
-        hasMoreCharts=has_more_charts)
+    category = Category(name=category,
+                        translatedName=place_api.translate(
+                            f'CHART_TITLE-CHART_CATEGORY-{category}'),
+                        hasMoreCharts=has_more_charts)
     categories.append(category)
   return categories
 
@@ -1020,7 +1033,7 @@ def fetch_overview_table_data(place_dcid: str,
   # Iterate over each variable and extract the most recent observation
   for item in PLACE_OVERVIEW_TABLE_VARIABLES:
     variable_dcid = item["variable_dcid"]
-    name = gettext(item["i18n_message_id"])
+    name = place_api.translate(item["i18n_message_id"])
     ordered_facet_observations = resp.get("byVariable", {}).get(
         variable_dcid, {}).get("byEntity", {}).get(place_dcid,
                                                    {}).get("orderedFacets", [])

server/routes/shared_api/place.py

@@ -136,18 +136,24 @@ def get_place_type(place_dcids):
   return ret
 
 
+def translate(*args, **kwargs):
+  """Returns the gettextd string"""
+  return gettext(*args, **kwargs)
+
+
 def get_place_type_i18n_name(place_type: str, plural: bool = False) -> str:
   """For a given place type, get its localized name for display"""
   place_type_to_local_map = PLACE_TYPE_TO_LOCALE_MESSAGE_PLURAL if plural else PLACE_TYPE_TO_LOCALE_MESSAGE
   if place_type in place_type_to_local_map:
-    return gettext(place_type_to_local_map[place_type])
+    return translate(place_type_to_local_map[place_type])
   elif place_type.startswith('AdministrativeArea'):
     level = place_type[-1]
-    return gettext(place_type_to_local_map['AdministrativeArea<Level>'],
-                   level=level)
+    return translate(place_type_to_local_map['AdministrativeArea<Level>'],
+                     level=level)
   elif place_type.startswith('EurostatNUTS'):
     level = place_type[-1]
-    return gettext(place_type_to_local_map['EurostatNUTS<Level>'], level=level)
+    return translate(place_type_to_local_map['EurostatNUTS<Level>'],
+                     level=level)
   else:
     # Return place type un-camel-cased
     words = re.findall(r'[A-Z](?:[a-z]+|[A-Z]*(?=[A-Z]|$))', place_type)
@@ -501,21 +507,21 @@ def api_ranking(dcid):
   # Contains statistical variable and the display name used for place rankings.
   ranking_stats = {
       # TRANSLATORS: Label for rankings of places by size of population (sorted from highest to lowest).
-      'Count_Person': gettext('Largest Population'),
+      'Count_Person': translate('Largest Population'),
       # TRANSLATORS: Label for rankings of median individual income (sorted from highest to lowest).
-      'Median_Income_Person': gettext('Highest Median Income'),
+      'Median_Income_Person': translate('Highest Median Income'),
       # TRANSLATORS: Label for rankings of places by the median age of it's population (sorted from highest to lowest).
-      'Median_Age_Person': gettext('Highest Median Age'),
+      'Median_Age_Person': translate('Highest Median Age'),
       # TRANSLATORS: Label for rankings of places by the unemployment rate of it's population (sorted from highest to lowest).
-      'UnemploymentRate_Person': gettext('Highest Unemployment Rate'),
+      'UnemploymentRate_Person': translate('Highest Unemployment Rate'),
   }
   # Crime stats var is separted from RANKING_STATS as it uses perCapita
   # option.
   # TOOD(shifucun): merge this once https://github.com/datacommonsorg/mixer/issues/262 is fixed.
   crime_statsvar = {
       # TRANSLATORS: Label for rankings of places by the number of combined criminal activities, per capita (sorted from highest to lowest).
       'Count_CriminalActivities_CombinedCrime':
-          gettext('Highest Crime Per Capita')
+          translate('Highest Crime Per Capita')
   }
   for parent_dcid in selected_parents:
     response = dc.related_place(dcid,

server/tests/routes/api/dev_place_test.py → server/tests/routes/dev_place/api_test.py

@@ -26,7 +26,9 @@
 from web_app import app
 
 
+# TODO(gmechali): Break up these tests into the overview_table_test, place_charts_test and related_places_test.
 class TestPlaceAPI(unittest.TestCase):
+  """Tests for the Place API."""
 
   @patch('server.routes.shared_api.place.parent_places')
   @patch('server.lib.fetch.raw_property_values')
@@ -68,6 +70,7 @@ def test_dev_place_charts(self, mock_obs_point_within, mock_obs_point,
       response = app.test_client().get(f'/api/dev-place/charts/{place_dcid}')
 
       # Check if the response is successful
+      print(response)
       self.assertEqual(response.status_code, 200)
 
       # Check that the response data contains expected fields

server/tests/routes/dev_place/overview_table_test.py

@@ -0,0 +1 @@
+# TODO(gmechali): Implement tests.

server/tests/routes/dev_place/place_charts_test.py

@@ -0,0 +1 @@
+# TODO(gmechali): Implement tests.

server/tests/routes/dev_place/related_places_test.py

@@ -0,0 +1 @@
+# TODO(gmechali): Implement tests.