Remove needs_sync in favor of just deleting things.

migrations/versions/527436ba4453_.py → migrations/versions/e7f8a917aa8e_add_user_details_json_column.py

@@ -1,23 +1,23 @@
-"""Add details JSONB column to user table.
+"""Add user details JSON column.
 
-Revision ID: 527436ba4453
+Revision ID: e7f8a917aa8e
 Revises: 71477dadd6ef
-Create Date: 2018-10-23 18:44:35.622627
+Create Date: 2018-11-08 16:12:17.023569
 
 """
 from alembic import op
 import sqlalchemy as sa
 from sqlalchemy.dialects import postgresql
 
 # revision identifiers, used by Alembic.
-revision = '527436ba4453'
+revision = 'e7f8a917aa8e'
 down_revision = '71477dadd6ef'
 branch_labels = None
 depends_on = None
 
 
 def upgrade():
-    op.add_column('users', sa.Column('details', postgresql.JSONB(astext_type=sa.Text()), server_default='{"active_at": null}', nullable=True))
+    op.add_column('users', sa.Column('details', postgresql.JSON(astext_type=sa.Text()), server_default='{"active_at": null}', nullable=True))
 
 
 def downgrade():

redash/models.py

@@ -5,6 +5,7 @@
 import hashlib
 import itertools
 import logging
+import operator
 import pytz
 import time
 from functools import reduce
@@ -128,7 +129,6 @@ class UserDetail(walrus.Model):
 
     user_id = walrus.IntegerField(index=True)
     updated_at = UTCDateTimeField(index=True, default=utcnow)
-    needs_sync = walrus.BooleanField(index=True, default=True)
 
     @classmethod
     def update(cls, user_id):
@@ -144,38 +144,25 @@ def update(cls, user_id):
         # or create one if it doesn't exist yet (e.g. when key was purged)
         try:
             user_detail = cls.get(cls.user_id == user_id)
+            # update the timestamp with the current time
+            user_detail.updated_at = utcnow()
+            # save to Redis
+            user_detail.save()
         except ValueError:
-            user_detail = cls.create(user_id=user_id)
-        # update the timestamp with the current time
-        user_detail.updated_at = utcnow()
-        # mark the user detail ready for syncing
-        user_detail.needs_sync = True
-        # save to Redis
-        user_detail.save()
+            user_detail = cls.create(
+                user_id=user_id,
+                updated_at=utcnow(),
+            )
         return user_detail
 
-    @classmethod
-    def stop_sync(cls, user_id):
-        """
-        Mark the user detail to not need syncing with Postgres anymore,
-        e.g. when it has just been synced.
-        """
-        try:
-            user_detail = cls.get(cls.user_id == user_id)
-        except ValueError:
-            pass
-        else:
-            user_detail.needs_sync = False
-            user_detail.save()
-
     @classmethod
     def sync(cls, chunksize=1000):
         """
-        Syncs user details to Postges (to the JSONB field User.details).
+        Syncs user details to Postgres (to the JSON field User.details).
         """
         to_sync = {}
         try:
-            for user_detail in cls.all_to_sync():
+            for user_detail in cls.all():
                 to_sync[user_detail.user_id] = user_detail
 
             user_ids = list(to_sync.keys())
@@ -203,16 +190,17 @@ def sync(cls, chunksize=1000):
             # reset list of keys to stop sync
             pass
         finally:
-            for user_id in to_sync:
-                logger.info('Stop syncing user %s', user_id)
-                cls.stop_sync(user_id)
-
-    @classmethod
-    def all_to_sync(cls):
-        """
-        Return all the user details that need to be synced to Postgres.
-        """
-        return cls.query(UserDetail.needs_sync == True)  # noqa
+            user_ids = [str(user_id) for user_id in to_sync.keys()]
+            if user_ids:
+                logger.info(
+                    'Deleting temporary user details for users %s',
+                    ', '.join(user_ids)
+                )
+                delete_query = [
+                    UserDetail.user_id == str(user_id)
+                    for user_id in user_ids
+                ]
+                UserDetail.query_delete(reduce(or_, delete_query))
 
 
 class UserDetailsExtension(object):
@@ -593,7 +581,7 @@ class User(TimestampMixin, db.Model, BelongsToOrgMixin, UserMixin, PermissionsCh
 
     disabled_at = Column(db.DateTime(True), default=None, nullable=True)
     details_default = {u'active_at': None}
-    details = Column(MutableDict.as_mutable(postgresql.JSONB), nullable=True,
+    details = Column(MutableDict.as_mutable(postgresql.JSON), nullable=True,
                      server_default=json_dumps(details_default),
                      default=details_default)
     active_at = json_cast_property(db.DateTime(True), 'details', 'active_at', default=None)
@@ -1538,7 +1526,7 @@ def all(cls, org, group_ids, user_id):
                  (Dashboard.user_id == user_id) |
                  ((Widget.dashboard != None) & (Widget.visualization == None))),
                 Dashboard.org == org)
-            .distinct())
+            )
 
         query = query.filter(or_(Dashboard.user_id == user_id, Dashboard.is_draft == False))
 

tests/test_models.py

@@ -611,7 +611,6 @@ def test_userdetail_create(self):
             models.UserDetail.get(models.UserDetail.user_id == 1)._id,
             user_detail._id,
         )
-        self.assertTrue(user_detail.needs_sync)
 
     def test_userdetail_update(self):
         self.assertEqual(len(list(models.UserDetail.all())), 0)
@@ -621,7 +620,6 @@ def test_userdetail_update(self):
         all_user_details = list(models.UserDetail.all())
         self.assertEqual(len(all_user_details), 1)
         created_user_detail = all_user_details[0]
-        self.assertTrue(created_user_detail.needs_sync)
 
         # then see if we can update the same user detail again
         updated_user_detail = models.UserDetail.update(
@@ -631,38 +629,15 @@ def test_userdetail_update(self):
             updated_user_detail.updated_at,
             created_user_detail.updated_at
         )
-        self.assertTrue(updated_user_detail.needs_sync)
-
-    def test_userdetail_stop_sync(self):
-        self.assertEqual(len(list(models.UserDetail.all())), 0)
-        user_detail = models.UserDetail.create(user_id=1)
-        user_detail.save()
-        self.assertTrue(user_detail.needs_sync)
-        models.UserDetail.stop_sync(user_id=1)
-        self.assertFalse(
-            models.UserDetail.get(models.UserDetail.user_id == 1).needs_sync
-        )
-
-    def test_userdetail_all_to_sync(self):
-        self.assertEqual(len(list(models.UserDetail.all())), 0)
-        self.assertEqual(len(list(models.UserDetail.all_to_sync())), 0)
-        user_detail = models.UserDetail.create(user_id=1)
-        user_detail.save()
-        self.assertEqual(len(list(models.UserDetail.all())), 1)
-        self.assertEqual(len(list(models.UserDetail.all_to_sync())), 1)
-        user_detail2 = models.UserDetail.create(user_id=2)
-        user_detail2.save()
-        self.assertEqual(len(list(models.UserDetail.all())), 2)
-        self.assertEqual(len(list(models.UserDetail.all_to_sync())), 2)
-        models.UserDetail.stop_sync(user_id=user_detail2.user_id)
-        self.assertEqual(len(list(models.UserDetail.all())), 2)
-        self.assertEqual(len(list(models.UserDetail.all_to_sync())), 1)
 
     def test_sync(self):
         with authenticated_user(self.client) as user:
             user_detail = models.UserDetail.update(user_id=user.id)
             self.assertEqual(user.details, models.User.details_default)
+
+            self.assertEqual(len(list(models.UserDetail.all())), 1)
             models.UserDetail.sync()
+            self.assertEqual(len(list(models.UserDetail.all())), 0)
 
             user_reloaded = models.User.query.filter_by(id=user.id).first()
             self.assertIn('active_at', user_reloaded.details)