show place page history to editors (#85)

* when logged in to an editor account the changes for a place and its direct children appears at the bottom of the place page
* Update sqlalchemy-continuum to 1.4.0
* update render cache with explicit call after commit instead of in a sqlalchemy event handler. Moving logic out of sqlalchemy is hopefully a step towards clearer data flows

requirements.txt

@@ -306,7 +306,7 @@ sqlalchemy[asyncio]==1.4.31
     #   prefect
     #   sqlalchemy-continuum
     #   sqlalchemy-utils
-sqlalchemy-continuum==1.3.14
+sqlalchemy-continuum==1.4.0
     # via -r requirements.in
 sqlalchemy-utils==0.38.2
     # via sqlalchemy-continuum

tourist/__init__.py

@@ -141,45 +141,25 @@ def uwht_redirect():
 
     @event.listens_for(db.session, "before_flush")
     def before_flush(session, flush_context, instances):
-        update_render_after_flush = False
         for instance in session.new | session.dirty:
             if not session.is_modified(instance):
                 continue
-            if isinstance(instance, (tstore.Entity, tstore.EntityChild)):
-                update_render_after_flush = True
             if isinstance(instance, tstore.Entity):
                 instance.validate()
 
-        for instance in session.deleted:
-            if isinstance(instance, (tstore.Entity, tstore.EntityChild)):
-                update_render_after_flush = True
-
-        if update_render_after_flush:
-            session.info[UPDATE_RENDER_AFTER_FLUSH] = True
-
-    @event.listens_for(db.session, "after_flush_postexec")
-    def after_flush_postexec(session, flush_context):
-        if session.info.get(UPDATE_RENDER_AFTER_FLUSH, False):
-            # ORM model objects that haven't been loaded from the database are slightly different
-            # from those populated from a form. In particular `region` is a str instead of
-            # geometry type. Instead of changing the render_factory to handle both types force
-            # objects used for the render to be refreshed from the database.
-            # expire_all() breaks some login tests so expire only pool/place/club objects.
-            for instance in session.identity_map.values():
-                if isinstance(instance, tstore.Entity):
-                    session.expire(instance)
-            new_cache_ids = []
-            for new_cache in render_factory.yield_cache():
-                session.add(new_cache)
-                new_cache_ids.append(new_cache.name)
-            # Remove rows in RenderCache not in new_cache_ids. This should be removed places.
-            session.query(tstore.RenderCache).filter(tstore.RenderCache.name.notin_(
-                new_cache_ids)).delete()
-            del session.info[UPDATE_RENDER_AFTER_FLUSH]
-
     return app
 
 
+def update_render_cache(session):
+    new_cache_ids = []
+    for new_cache in render_factory.yield_cache():
+        session.add(new_cache)
+        new_cache_ids.append(new_cache.name)
+    # Remove rows in RenderCache not in new_cache_ids. This should be removed places.
+    session.query(tstore.RenderCache).filter(tstore.RenderCache.name.notin_(new_cache_ids)).delete()
+    session.commit()
+
+
 def initialise_logger(app):
     """ Read environment config then initialise a 100MB rotating log """
     log_dir = app.config['LOG_DIR']

tourist/models/render.py

@@ -101,6 +101,22 @@ class RecentlyUpdated:
     source_name: Optional[str] = None
 
 
+@attrs.frozen()
+class PlaceEntityChanges:
+    """Changes for an entity (place, club, pool) in a very crude format, but good enough for
+    debugging."""
+
+    @attrs.frozen()
+    class Change:
+        """A single version of an entity"""
+        timestamp: datetime.datetime
+        user: str
+        change: str
+
+    entity_name: str
+    changes: List[Change] = attrs.field(factory=list)
+
+
 @attrs.frozen()
 class Place:
     id: int
@@ -114,7 +130,10 @@ class Place:
     child_places: List[ChildPlace]
     parents: List[ChildPlace]
     comments: List[PlaceComment] = attrs.field(factory=list)
+    # recently_updated, only set for 'world'
     recently_updated: Optional[List[RecentlyUpdated]] = None
+    # changes for this place and all direct children. not set for 'world'
+    changes: Optional[List[PlaceEntityChanges]] = None
 
 
 @attrs.frozen()

tourist/render_factory.py

@@ -2,7 +2,9 @@
 import datetime
 import enum
 import io
+import itertools
 from typing import List, Mapping
+from typing import Union
 
 from sqlalchemy.util import IdentitySet
 
@@ -59,6 +61,20 @@ def _build_render_pool(orm_pool: tstore.Pool) -> render.Pool:
     )
 
 
+def _build_changes(orm_entity: Union[tstore.Place, tstore.Club, tstore.Pool]) -> (
+        render.PlaceEntityChanges):
+    changes = render.PlaceEntityChanges(entity_name=orm_entity.name)
+
+    for v in orm_entity.versions:
+        user_email = None
+        if v.transaction.user:
+            user_email = v.transaction.user.email
+        changes.changes.append(render.PlaceEntityChanges.Change(
+            timestamp=v.transaction.issued_at, user=user_email,
+            change=str(v.changeset)))
+    return changes
+
+
 def _build_render_place(orm_place: tstore.Place, source_by_short_name: Mapping[str, render.ClubSource]) -> render.Place:
     children_geojson = orm_place.children_geojson_features
     if children_geojson:
@@ -106,8 +122,14 @@ def _build_render_place(orm_place: tstore.Place, source_by_short_name: Mapping[s
             recently_updated.append(render.RecentlyUpdated(
                 timestamp=source.sync_timestamp, path=place.path, place_name=place.name, source_name=source.name))
         recently_updated.sort(key=lambda ru: ru.timestamp, reverse=True)
+        entity_changes = None
     else:
         recently_updated = None
+        entity_changes = [_build_changes(orm_place)]
+        for child in itertools.chain(orm_place.child_places, orm_place.child_pools,
+                                           orm_place.child_clubs):
+            entity_changes.append(_build_changes(child))
+
 
     return render.Place(
         id=orm_place.id,
@@ -122,6 +144,7 @@ def _build_render_place(orm_place: tstore.Place, source_by_short_name: Mapping[s
         parents=parents,
         recently_updated=recently_updated,
         comments=comments,
+        changes=entity_changes,
     )
 
 
@@ -196,12 +219,6 @@ def _build_problems(all_places: List[tstore.Place], all_clubs: List[tstore.Club]
     return problems
 
 
-
-
-
-
-
-
 def yield_cache():
     def get_all(cls):
         all_objects = IdentitySet(cls.query.all()) | tstore.db.session.dirty | tstore.db.session.new

tourist/routes.py

@@ -126,6 +126,7 @@ def edit_club(club_id):
         form_delete_place_comments()
         flask.flash(f"Updated {club.name}")
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
         return redirect(club.path)
     return render_template("edit_club.html", form=form, club=club)
 
@@ -150,6 +151,7 @@ def edit_place(place_id):
         form_delete_place_comments()
         flask.flash(f"Updated {place.name}")
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
         return redirect(place.path)
     return render_template("edit_place.html", form=form, place=place)
 
@@ -179,6 +181,7 @@ def add_place_comment(place_id):
         place_comment.akismet_spam_status = tourist.get_comment_spam_status(place_comment)
         tstore.db.session.add(place_comment)
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
         flask.flash(f"Comment added to {place.name}")
     else:
         flask.flash(f"Ignored empty comment for {place.name}")
@@ -224,6 +227,7 @@ def delete_place(place_id):
         parent_path = place.parent.path
         delete_place_children_and_flash(place)
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
         return redirect(parent_path)
 
     return render_template('delete_place.html', form=form, place=place)
@@ -242,6 +246,7 @@ def delete_club(club_id):
         parent_path = club.parent.path
         delete_club_and_flash(club)
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
         return redirect(parent_path)
 
     return render_template('delete_club.html', form=form, club=club)
@@ -262,6 +267,7 @@ def delete_pool(pool_id):
         parent_path = pool.parent.path
         delete_pool_and_flash(pool)
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
         return redirect(parent_path)
 
     return render_template('delete_pool.html', form=form, pool=pool)

tourist/scripts/batchtool.py

@@ -94,6 +94,7 @@ def replace_club_pool_links(write):
     if write:
         click.echo('Committing changes')
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
     else:
         click.echo('Run with --write to commit changes')
 
@@ -161,6 +162,7 @@ def incr_column(cls, column_name: str):
     if write:
         click.echo('Committing changes')
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
     else:
         click.echo('Run with --write to commit changes')
 
@@ -404,6 +406,7 @@ def transactioninsert1(initial_snapshot: str, change_log: str, output_path: str,
     if commit:
         click.echo('Committing changes')
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
     else:
         click.echo('Run with --write to commit changes')
 
@@ -435,5 +438,6 @@ def _find_empty(place: tstore.Place) -> Tuple[bool, List]:
             tstore.db.session.delete(place)
         click.echo('Committing changes')
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
     else:
         click.echo('Run with --write to commit changes')

tourist/scripts/scrape.py

@@ -736,6 +736,7 @@ def extract_gbfeed(uk_place: tstore.Place, feed: GbUwhFeed, fetch_timestamp: dat
     tstore_source.place_id = uk_place.id
 
     tstore.db.session.commit()
+    tourist.update_render_cache(tstore.db.session)
 
     return []
 
@@ -874,6 +875,7 @@ def comment_command():
             print(f"Comment added to {place.short_name}:\n{comment}")
     tstore.db.session.add_all(comment_to_extract.keys())
     tstore.db.session.commit()
+    tourist.update_render_cache(tstore.db.session)
     for comment, extract in comment_to_extract.items():
         tstore.db.session.refresh(comment)
         assert comment.id

tourist/scripts/sync.py

@@ -9,6 +9,7 @@
 import click
 from prefect.deployments import Deployment
 
+import tourist
 from tourist.models import tstore, attrib
 from geoalchemy2.shape import to_shape
 import attr
@@ -226,6 +227,7 @@ def run(self, jsons_iterable: Iterable[str]):
         print('Updated fields ' + ','.join(self.updater.updated_fields))
         tstore.db.session.add_all(self.updater.to_add)
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
 
 
 @sync_cli.command('import_jsonl')

tourist/templates/place.html

@@ -143,11 +143,23 @@ <h3 id='{{ pool.short_name }}'>{{ pool.name }}
 
 {% if current_user.edit_granted -%}
 {{ place_comments(place.comments) }}
+
     <hr>
     <h3>Add in {{ place.name }}:</h3>
     <a class="mui-btn" href="{{ url_for('place.create_view', parent=place.short_name) }}">place <i class="material-icons">add</i></a>
     <a class="mui-btn" href="{{ url_for('club.create_view', parent=place.short_name) }}">club <i class="material-icons">add</i></a>
     <a class="mui-btn" href="{{ url_for('pool.create_view', parent=place.short_name) }}">pool <i class="material-icons">add</i></a>
+<hr>
+<h3>History</h3>
+<ul>
+    {% for e in place.changes -%}
+    <li><b>{{e.entity_name}}</b>
+        <ul>{% for cs in e.changes -%}
+            <li>{{cs.timestamp}} {{cs.user}} {{cs.change}}</li>{% endfor -%}
+        </ul>
+    </li>
+    {% endfor %}
+</ul>
 {% else %}
 {% if place.comments -%}<hr>There are comments about this place. Login to view and handle
 them.<hr>{% endif %}

tourist/tests/test_basic.py

@@ -215,6 +215,7 @@ def add_some_entities(test_app):
                                markdown='Some palace', id=1)
         tstore.db.session.add_all([world, country, metro, club, pool])
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
 
 
 def add_and_return_edit_granted_user(test_app):
@@ -269,6 +270,7 @@ def test_list(test_app):
         )
         tstore.db.session.add_all([metro])
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
 
     with test_app.test_client() as c:
         response = c.get('/tourist/list')
@@ -284,6 +286,7 @@ def test_club_with_bad_pool_link(test_app):
         club.markdown += " * [[badpoollink]]\n"
         tstore.db.session.add(club)
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
 
     with test_app.test_client() as c:
         response = c.get('/tourist/place/metro')
@@ -393,6 +396,7 @@ def test_delete_pool(test_app):
         club.markdown = 'Club Foo has no pool'
         tstore.db.session.add(club)
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
 
     with test_app.app_context(), test_app.test_client(user=user) as c:
         c.get('/tourist/delete/pool/1')  # GET to create the CSRF token
@@ -563,5 +567,6 @@ def test_add_delete_place_comment(test_app, mocker: MockerFixture):
         tstore.db.session.delete(place.child_clubs[0])
         tstore.db.session.delete(place)
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
         # Check that comment_id was implicitly deleted by cascade.
         assert tstore.PlaceComment.query.get(comment_id) is None

tourist/tests/test_login.py

@@ -1,3 +1,4 @@
+import tourist
 from tourist.models import tstore
 from tourist.tests.conftest import no_expire_on_commit
 
@@ -13,6 +14,7 @@ def test_heavy(test_app):
         with no_expire_on_commit():
             tstore.db.session.add_all([world, au, user_plain, user_edit_granted])
             tstore.db.session.commit()
+            tourist.update_render_cache(tstore.db.session)
 
     with test_app.test_client() as c:
         response = c.get('/tourist/')

tourist/tests/test_render_factory.py

@@ -1,5 +1,6 @@
 from geoalchemy2 import WKTElement
 
+import tourist
 from tourist import render_factory
 from tourist.models import tstore
 
@@ -34,6 +35,7 @@ def test_geojson(test_app):
                            markdown='', entrance=point1)
         tstore.db.session.add_all([world, country, metro_with_pool, metro_no_pool, pool, poolgeo])
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
 
     with test_app.app_context():
         collection = render_factory._build_geojson_feature_collection(tstore.Place.query.all(),

tourist/tests/test_scrape.py

@@ -149,6 +149,7 @@ def add_uk(test_app):
 
         tstore.db.session.add_all([world, uk, north, london, poolden, poolgeo2])
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
 
 
 def test_extract_gbuwh_short(test_app):
@@ -276,6 +277,7 @@ def add_canada(test_app):
                           region=some_region, markdown='')
         tstore.db.session.add_all([world, ca, cabc, caon])
         tstore.db.session.commit()
+        tourist.update_render_cache(tstore.db.session)
 
 
 def test_load_and_comment(test_app):