Add data problems list (#84)

- when rendering check for some simple problems and show them  at `/tourist/problems`
- abstract out tstore.Place.is_world

Next up is working out how to fix the issues this finds.

tourist/models/render.py

@@ -138,5 +138,17 @@ class Pool:
         name: str
 
 
+@attrs.frozen()
+class Problem:
+    path: str
+    name: str
+    description: str
+
+
+@attrs.frozen()
+class Problems:
+    problems: List[Problem]
+
+
 cattrs.register_structure_hook(datetime.datetime, lambda d, t: datetime.datetime.fromisoformat(d))
 cattrs.register_unstructure_hook(datetime.datetime, lambda d: d.isoformat())

tourist/models/tstore.py

@@ -114,7 +114,7 @@ def __str__(self):
 
     def validate(self):
         _validate_short_name(self.short_name)
-        if self.parent_id is None and self.parent is None and self.short_name != 'world':
+        if self.parent_id is None and self.parent is None and not self.is_world:
             raise ValueError("parent unset. Every place must have a parent.")
         if self.markdown and re.search(WIKI_LINK_RE, self.markdown) is not None:
             raise ValueError("Place markdown must not contain [[Wiki Links]]")
@@ -188,6 +188,10 @@ def as_attrib_entity(self):
         parent_short_name = self.parent and self.parent.short_name or ''
         return place_as_attrib_entity(self, parent_short_name)
 
+    @property
+    def is_world(self) -> bool:
+        return self.short_name == 'world'
+
 
 def place_as_attrib_entity(place, parent_short_name: str):
     return attrib.Entity(
@@ -218,11 +222,16 @@ class User(db.Model, flask_login.UserMixin):
     def can_view_comments(self) -> bool:
         return self.edit_granted
 
+    @property
+    def can_view_problems(self) -> bool:
+        return self.edit_granted
+
 
 # Anonymous user with same attributes as a logged in `User` for consistency in templates.
 class AnonymousUser(flask_login.AnonymousUserMixin):
     edit_granted = False
     can_view_comments = False
+    can_view_problems = False
 
 
 class OAuth(OAuthConsumerMixin, db.Model):

tourist/render_factory.py

@@ -1,4 +1,5 @@
 import csv
+import datetime
 import enum
 import io
 from typing import List, Mapping
@@ -20,6 +21,7 @@ class RenderName(enum.Enum):
     PLACE_NAMES_WORLD = "/place_names_world"
     CSV_ALL = "/csv"
     POOLS_GEOJSON = "/pools.geojson"
+    PROBLEMS = "/problems_list"
 
 
 def _build_render_club_source(orm_source: tstore.Source) -> render.ClubSource:
@@ -88,7 +90,7 @@ def _build_render_place(orm_place: tstore.Place, source_by_short_name: Mapping[s
             east=maxx
         )
 
-    if orm_place.short_name == 'world':
+    if orm_place.is_world:
         club: tstore.Club
         recently_updated = []
         for club in tstore.db.session.query(tstore.Club).filter(tstore.Club.status_date.like('____-__-__')).order_by(tstore.Club.status_date.desc()).limit(5):
@@ -155,6 +157,51 @@ def _build_geojson_feature_collection(all_places, all_pools):
     return geojson_feature_collection
 
 
+@attrs.frozen(order=True)
+class StatusDateClub:
+    status_date: datetime.date = attrs.field(order=True)
+    club: tstore.Club = attrs.field(order=False)
+
+
+def _build_problems(all_places: List[tstore.Place], all_clubs: List[tstore.Club]) -> List[
+    render.Problem]:
+    """Returns a list of data quality problems found in the places and clubs."""
+    problems = []
+    for place in all_places:
+        if place.area == 0 and not place.is_world:
+            problems.append(render.Problem(place.path,
+                                           place.name,
+                                           "Add place location as a polygon on the map"))
+    status_date_clubs = []
+    for club in all_clubs:
+        if club.source_short_name:
+            continue
+        try:
+            parsed = datetime.date.fromisoformat(club.status_date)
+        except (ValueError, TypeError):
+            parsed = None
+        if parsed:
+            status_date_clubs.append(StatusDateClub(parsed, club))
+        else:
+            problems.append(
+                render.Problem(club.path,
+                               club.parent.name,
+                               f"Add a status_date as a valid YYYY-MM-DD to {club.name}"))
+    status_date_clubs.sort()
+    for sdc in status_date_clubs[0:5]:
+        problems.append(render.Problem(
+            sdc.club.path,
+            sdc.club.parent.name,
+            f"Track down what's happening with {sdc.club.name}, status_date is {sdc.status_date}"))
+    return problems
+
+
+
+
+
+
+
+
 def yield_cache():
     def get_all(cls):
         all_objects = IdentitySet(cls.query.all()) | tstore.db.session.dirty | tstore.db.session.new
@@ -171,12 +218,16 @@ def get_all(cls):
         render_place = _build_render_place(place, source_by_short_name)
         yield tstore.RenderCache(name=RenderName.PLACE_PREFIX.value + place.short_name,
                                      value_dict=cattrs.unstructure(render_place))
-        if place.short_name == 'world':
+        if place.is_world:
             render_names_world = _build_place_recursive_names(place)
             yield tstore.RenderCache(name=RenderName.PLACE_NAMES_WORLD.value,
                                          value_dict=attrs.asdict(
                                              render_names_world))
 
+    yield tstore.RenderCache(name=RenderName.PROBLEMS.value,
+                             value_dict=cattrs.unstructure(render.Problems(_build_problems(
+                                 all_places, all_clubs))))
+
     geojson_feature_collection = _build_geojson_feature_collection(all_places, all_pools)
 
     yield tstore.RenderCache(name=RenderName.POOLS_GEOJSON.value,
@@ -213,3 +264,7 @@ def get_place_names_world() -> render.PlaceRecursiveNames:
 def get_string(name: RenderName) -> str:
     return tstore.RenderCache.query.get(name.value).value_str
 
+
+def get_problems() -> render.Problems:
+    problems_dict = tstore.RenderCache.query.get_or_404(RenderName.PROBLEMS.value ).value_dict
+    return cattrs.structure(problems_dict, render.Problems)

tourist/routes.py

@@ -65,6 +65,15 @@ def list_view_func():
     return render_template("list.html", world=render_world)
 
 
+@tourist_bp.route("/problems")
+def problems_view_func():
+    if not flask_login.current_user.can_view_problems:
+        return tourist.inaccessible_response()
+
+    problems = render_factory.get_problems()
+    return render_template("problems.html", problems=problems.problems)
+
+
 # Static routes
 #
 # These routes don't load any data from dynamic storage, but some templates have conditional

tourist/templates/layout.html

@@ -327,7 +327,10 @@
     {% for href, name in navigation_bar %}
     {% if menu_active != name %}<li><a class="mui-btn" href="{{ href|e }}">{{ name|e }}</a></li>{% endif -%}
     {% endfor -%}
-    {% if menu_active != 'Comments' and current_user.can_view_comments -%}<li><a class="mui-btn" href="/tourist/comments">Comments</a></li>{% endif -%}
+    {% if menu_active != 'Comments' and current_user.can_view_comments -%}
+      <li><a class="mui-btn" href="/tourist/comments">Comments</a></li>{% endif -%}
+    {% if menu_active != 'Problems' and current_user.can_view_problems -%}
+      <li><a class="mui-btn" href="/tourist/problems">Problems</a></li>{% endif -%}
     {% if current_user.is_authenticated -%}
     <li><a class="mui-btn" href="{{ url_for('github.logout', next=request.url) }}">Sign out</a></li>
     {%  else %}

tourist/templates/problems.html

@@ -0,0 +1,14 @@
+{% extends "layout.html" %}
+
+{% block htmltitle %}UWHT: Problems{% endblock %}
+
+{% block headertitle %}Problems{% endblock %}
+
+{% block content %}
+
+<ul>
+{%- for p in problems %}
+<li>In <a href="{{ p.path }}">{{p.name}}</a>: {{ p.description }}</li>
+{%- endfor %}
+</ul>
+{% endblock %}

tourist/tests/test_basic.py

@@ -229,6 +229,7 @@ def add_and_return_edit_granted_user(test_app):
 
 def test_club_without_status_date(test_app):
     add_some_entities(test_app)
+    edit_user = add_and_return_edit_granted_user(test_app)
 
     with test_app.app_context():
         assert not tstore.Club.query.filter_by(short_name='shortie').one().status_date
@@ -241,6 +242,11 @@ def test_club_without_status_date(test_app):
         # Check link from pool back to club
         assert 'Foo Club</a> practices here' in response.get_data(as_text=True)
 
+    with test_app.app_context(), test_app.test_client(user=edit_user) as c:
+        response = c.get('/tourist/problems')
+        assert response.status_code == 200
+        assert 'Add a status_date as a valid YYYY-MM-DD to Foo' in response.get_data(as_text=True)
+
 
 def test_list(test_app):
     add_some_entities(test_app)

tourist/tests/test_login.py

@@ -29,6 +29,9 @@ def test_heavy(test_app):
         response = c.get('/tourist/comments')
         assert response.status_code == 302  # Without login, redirects
 
+        response = c.get('/tourist/problems')
+        assert response.status_code == 302  # Without login, redirects
+
     # Login. This user isn't authorized to /admin
     with test_app.test_client(user=user_plain) as c:
         response = c.get('/tourist/')
@@ -54,6 +57,9 @@ def test_heavy(test_app):
         response = c.get('/tourist/comments')
         assert response.status_code == 403
 
+        response = c.get('/tourist/problems')
+        assert response.status_code == 403
+
     with test_app.app_context():
         new_au = tstore.Place.query.filter_by(short_name='au').first()
         assert new_au.name == 'Australia'
@@ -84,6 +90,10 @@ def test_heavy(test_app):
         response = c.get('/tourist/comments')
         assert response.status_code == 200
 
+        response = c.get('/tourist/problems')
+        assert response.status_code == 200
+
+
     with test_app.app_context():
         new_au = tstore.Place.query.filter_by(short_name='au').one()
         assert new_au.name == 'Australia Changed'