Merge pull request #212 from paramsingh/import-musicbrainz-row-ids

Import MusicBrainz row IDs into CB

admin/schema_changes/12.sql

@@ -0,0 +1,7 @@
+-- Add a MusicBrainz Row ID column to the user table
+BEGIN;
+
+ALTER TABLE "user" ADD COLUMN musicbrainz_row_id INTEGER;
+ALTER TABLE "user" ADD CONSTRAINT user_musicbrainz_row_id_key UNIQUE (musicbrainz_row_id);
+
+COMMIT;

admin/sql/create_tables.sql

@@ -108,16 +108,18 @@ CREATE TABLE spam_report (
 );
 
 CREATE TABLE "user" (
-    id             UUID        NOT NULL DEFAULT uuid_generate_v4(),
-    display_name   VARCHAR     NOT NULL,
-    email          VARCHAR,
-    created        TIMESTAMP   NOT NULL,
-    musicbrainz_id VARCHAR,
-    show_gravatar  BOOLEAN     NOT NULL DEFAULT False,
-    is_blocked     BOOLEAN     NOT NULL DEFAULT False,
-    license_choice VARCHAR
+    id                  UUID        NOT NULL DEFAULT uuid_generate_v4(),
+    display_name        VARCHAR     NOT NULL,
+    email               VARCHAR,
+    created             TIMESTAMP   NOT NULL,
+    musicbrainz_id      VARCHAR,
+    musicbrainz_row_id  INTEGER,
+    show_gravatar       BOOLEAN     NOT NULL DEFAULT False,
+    is_blocked          BOOLEAN     NOT NULL DEFAULT False,
+    license_choice      VARCHAR
 );
 ALTER TABLE "user" ADD CONSTRAINT user_musicbrainz_id_key UNIQUE (musicbrainz_id);
+ALTER TABLE "user" ADD CONSTRAINT user_musicbrainz_row_id_key UNIQUE (musicbrainz_row_id);
 
 CREATE TABLE vote (
     user_id     UUID        NOT NULL,

critiquebrainz/db/__init__.py

@@ -2,7 +2,7 @@
 from sqlalchemy.pool import NullPool
 
 # This value must be incremented after schema changes on exported tables!
-SCHEMA_VERSION = 11
+SCHEMA_VERSION = 12
 VALID_RATING_VALUES = [None, 1, 2, 3, 4, 5]
 REVIEW_RATING_MAX = 5
 REVIEW_RATING_MIN = 1

critiquebrainz/db/avg_rating_test.py

@@ -12,10 +12,10 @@ class AvgRatingTestCase(DataTestCase):
     def setUp(self):
         super(AvgRatingTestCase, self).setUp()
 
-        self.user = User(db_users.get_or_create("Tester", new_user_data={
+        self.user = User(db_users.get_or_create(1, "Tester", new_user_data={
             "display_name": "test user",
         }))
-        self.user_2 = User(db_users.get_or_create("Tester 2", new_user_data={
+        self.user_2 = User(db_users.get_or_create(2, "Tester 2", new_user_data={
             "display_name": "test user 2",
         }))
         self.license = db_license.create(

critiquebrainz/db/comment_test.py

@@ -35,10 +35,10 @@ def setUp(self):
         # review
 
         # create user
-        self.user = User(db_users.get_or_create("Tester", new_user_data={
+        self.user = User(db_users.get_or_create(1, "Tester", new_user_data={
             "display_name": "test user",
         }))
-        self.user_2 = User(db_users.get_or_create("Tester 2", new_user_data={
+        self.user_2 = User(db_users.get_or_create(2, "Tester 2", new_user_data={
             "display_name": "test user 2",
         }))
 

critiquebrainz/db/dump_manager_test.py

@@ -10,7 +10,7 @@
 from critiquebrainz.db.user import User
 
 utils.with_request_context = utils.with_test_request_context  # noqa
-from critiquebrainz.data import dump_manager # pylint:disable=wrong-import-position
+from critiquebrainz.data import dump_manager  # pylint:disable=wrong-import-position
 
 
 def get_archives(root_dir):
@@ -60,7 +60,7 @@ def test_public(self):
         self.assertIn(f'cbdump-reviews-{self.license["id"]}.tar.bz2', archives)
 
     def test_importer(self):
-        user = User(db_users.get_or_create("Tester", new_user_data={
+        user = User(db_users.get_or_create(1, "Tester", new_user_data={
             "display_name": "test user",
         }))
         review = db_review.create(

critiquebrainz/db/moderation_log_test.py

@@ -12,10 +12,10 @@ class ModerationLogCase(DataTestCase):
     def setUp(self):
         super(ModerationLogCase, self).setUp()
 
-        self.admin = User(db_users.get_or_create("Admin", new_user_data={
+        self.admin = User(db_users.get_or_create(1, "Admin", new_user_data={
             "display_name": "Admin",
         }))
-        self.user = User(db_users.get_or_create("Tester", new_user_data={
+        self.user = User(db_users.get_or_create(2, "Tester", new_user_data={
             "display_name": "Tester",
         }))
         self.reason = "Testing!"

critiquebrainz/db/oauth_client_test.py

@@ -8,7 +8,7 @@
 class OAuthClientTestCase(DataTestCase):
     def setUp(self):
         super(OAuthClientTestCase, self).setUp()
-        self.user = User(db_users.get_or_create("Author", new_user_data={
+        self.user = User(db_users.get_or_create(1, "Author", new_user_data={
             "display_name": "Author",
         }))
         self.application = dict(

critiquebrainz/db/oauth_grant_test.py

@@ -11,7 +11,7 @@ class OAuthGrantTestCase(DataTestCase):
 
     def setUp(self):
         super(OAuthGrantTestCase, self).setUp()
-        self.user = User(db_users.get_or_create('tester_1', new_user_data={
+        self.user = User(db_users.get_or_create(1, 'tester_1', new_user_data={
             "display_name": "test",
         }))
         db_oauth_client.create(

critiquebrainz/db/oauth_token_test.py

@@ -11,7 +11,7 @@ class OAuthTokenTestCase(DataTestCase):
 
     def setUp(self):
         super(OAuthTokenTestCase, self).setUp()
-        self.user = User(db_users.get_or_create('tester_1', new_user_data={
+        self.user = User(db_users.get_or_create(1, 'tester_1', new_user_data={
             "display_name": "test",
         }))
         db_oauth_client.create(

critiquebrainz/db/review_test.py

@@ -13,10 +13,10 @@ def setUp(self):
         super(ReviewTestCase, self).setUp()
 
         # Review needs user
-        self.user = User(db_users.get_or_create("Tester", new_user_data={
+        self.user = User(db_users.get_or_create(1, "Tester", new_user_data={
             "display_name": "test user",
         }))
-        self.user_2 = User(db_users.get_or_create("Tester 2", new_user_data={
+        self.user_2 = User(db_users.get_or_create(2, "Tester 2", new_user_data={
             "display_name": "test user 2",
         }))
 

critiquebrainz/db/revision_test.py

@@ -11,13 +11,13 @@ class RevisionTestCase(DataTestCase):
 
     def setUp(self):
         super(RevisionTestCase, self).setUp()
-        self.author = User(db_users.get_or_create('Author', new_user_data={
+        self.author = User(db_users.get_or_create(1, 'Author', new_user_data={
             "display_name": '0',
         }))
-        self.user_1 = User(db_users.get_or_create('Tester #1', new_user_data={
+        self.user_1 = User(db_users.get_or_create(2, 'Tester #1', new_user_data={
             "display_name": '1',
         }))
-        self.user_2 = User(db_users.get_or_create('Tester #2', new_user_data={
+        self.user_2 = User(db_users.get_or_create(3, 'Tester #2', new_user_data={
             "display_name": '2',
         }))
         self.license = db_license.create(

critiquebrainz/db/spam_report_test.py

@@ -10,13 +10,13 @@ class SpamReportTestCase(DataTestCase):
 
     def setUp(self):
         super(SpamReportTestCase, self).setUp()
-        author = User(db_users.get_or_create('0', new_user_data={
+        author = User(db_users.get_or_create(0, '0', new_user_data={
             "display_name": "Author",
         }))
-        self.user1 = User(db_users.get_or_create('1', new_user_data={
+        self.user1 = User(db_users.get_or_create(1, '1', new_user_data={
             "display_name": "Tester #1",
         }))
-        self.user2 = User(db_users.get_or_create('2', new_user_data={
+        self.user2 = User(db_users.get_or_create(2, '2', new_user_data={
             "display_name": "Tester #2",
         }))
         license = db_license.create(

critiquebrainz/db/user.py

@@ -18,6 +18,7 @@ def __init__(self, user):
         self.show_gravatar = user.get('show_gravatar', False)
         self.is_blocked = user.get('is_blocked', False)
         self.license_choice = user.get('license_choice', None)
+        self.musicbrainz_row_id = user.get('musicbrainz_row_id', None)
 
     @property
     def avatar(self):

critiquebrainz/db/users.py

@@ -37,6 +37,19 @@ def gravatar_url(source, default="identicon", rating="pg"):
     )
 
 
+USER_GET_COLUMNS = [
+    'id',
+    'display_name',
+    'email',
+    'created',
+    'musicbrainz_id',
+    'musicbrainz_row_id',
+    'show_gravatar',
+    'license_choice',
+    'is_blocked',
+]
+
+
 def get_many_by_mb_username(usernames):
     """Get information about users.
 
@@ -53,16 +66,10 @@ def get_many_by_mb_username(usernames):
 
     with db.engine.connect() as connection:
         result = connection.execute(sqlalchemy.text("""
-            SELECT id,
-                   display_name,
-                   email,
-                   created,
-                   musicbrainz_id,
-                   show_gravatar,
-                   license_choice
+            SELECT {columns}
               FROM "user"
              WHERE musicbrainz_id ILIKE ANY(:musicbrainz_usernames)
-        """), {
+        """.format(columns=','.join(USER_GET_COLUMNS))), {
             'musicbrainz_usernames': usernames,
         })
         row = result.fetchall()
@@ -98,17 +105,10 @@ def get_by_id(user_id):
     """
     with db.engine.connect() as connection:
         result = connection.execute(sqlalchemy.text("""
-            SELECT id,
-                   display_name,
-                   email,
-                   created,
-                   musicbrainz_id,
-                   show_gravatar,
-                   is_blocked,
-                   license_choice
+            SELECT {columns}
               FROM "user"
              WHERE id = :user_id
-        """), {
+        """.format(columns=','.join(USER_GET_COLUMNS))), {
             "user_id": user_id
         })
         row = result.fetchone()
@@ -125,6 +125,7 @@ def create(**user_data):
     Args:
         display_name(str): display_name of the user.
         musicbrainz_username(str, optional): musicbrainz username of user.
+        musicbrainz_row_id (int): the MusicBrainz row ID of the user,
         email(str, optional): email of the user.
         show_gravatar(bool, optional): whether to show gravatar(default: false).
         is_blocked(bool, optional): whether user is blocked(default: false).
@@ -149,13 +150,16 @@ def create(**user_data):
     show_gravatar = user_data.pop('show_gravatar', False)
     is_blocked = user_data.pop('is_blocked', False)
     license_choice = user_data.pop('license_choice', None)
+    musicbrainz_row_id = user_data.pop('musicbrainz_row_id', None)
     if user_data:
         raise TypeError('Unexpected **user_data: %r' % user_data)
 
     with db.engine.connect() as connection:
         result = connection.execute(sqlalchemy.text("""
-            INSERT INTO "user" (id, display_name, email, created, musicbrainz_id, show_gravatar, is_blocked, license_choice)
-                 VALUES (:id, :display_name, :email, :created, :musicbrainz_id, :show_gravatar, :is_blocked, :license_choice)
+            INSERT INTO "user" (id, display_name, email, created, musicbrainz_id, show_gravatar,
+                                is_blocked, license_choice, musicbrainz_row_id)
+                 VALUES (:id, :display_name, :email, :created, :musicbrainz_id, :show_gravatar,
+                        :is_blocked, :license_choice, :musicbrainz_row_id)
               RETURNING id
         """), {
             "id": str(uuid.uuid4()),
@@ -166,6 +170,7 @@ def create(**user_data):
             "show_gravatar": show_gravatar,
             "is_blocked": is_blocked,
             "license_choice": license_choice,
+            "musicbrainz_row_id": musicbrainz_row_id,
         })
         new_id = result.fetchone()[0]
     return get_by_id(new_id)
@@ -192,18 +197,11 @@ def get_by_mbid(musicbrainz_username):
     """
     with db.engine.connect() as connection:
         result = connection.execute(sqlalchemy.text("""
-            SELECT id,
-                   display_name,
-                   email,
-                   created,
-                   musicbrainz_id,
-                   show_gravatar,
-                   is_blocked,
-                   license_choice
-            FROM "user"
-            WHERE musicbrainz_id = :musicbrainz_username
-        """), {
-            "musicbrainz_username": musicbrainz_username
+            SELECT {columns}
+              FROM "user"
+             WHERE musicbrainz_id = :musicbrainz_username
+        """.format(columns=','.join(USER_GET_COLUMNS))), {
+            "musicbrainz_username": musicbrainz_username,
         })
         row = result.fetchone()
         if not row:
@@ -213,10 +211,11 @@ def get_by_mbid(musicbrainz_username):
     return row
 
 
-def get_or_create(musicbrainz_username, new_user_data):
+def get_or_create(musicbrainz_row_id, musicbrainz_username, new_user_data):
     """Get user from display_name and musicbrainz_username.
 
     Args:
+        musicbrainz_row_id (int): the musicbrainz row ID of the user
         musicbrainz_username(str): MusicBrainz username of the user.
         new_user_data(dict): Dictionary containing the user data which may
                              contain the following keys:
@@ -241,10 +240,15 @@ def get_or_create(musicbrainz_username, new_user_data):
             "license_choice": (str)
         }
     """
-    user = get_by_mbid(musicbrainz_username)
+    user = get_by_mb_row_id(musicbrainz_row_id, musicbrainz_username)
     if not user:
         display_name = new_user_data.pop("display_name")
-        user = create(display_name=display_name, musicbrainz_username=musicbrainz_username, **new_user_data)
+        user = create(
+            musicbrainz_row_id=musicbrainz_row_id,
+            display_name=display_name,
+            musicbrainz_username=musicbrainz_username,
+            **new_user_data
+        )
     return user
 
 
@@ -280,23 +284,17 @@ def list_users(limit=None, offset=0):
             "musicbrainz_username": (str),
             "show_gravatar": (bool),
             "is_blocked": (bool),
-            "license_choice": (str)
+            "license_choice": (str),
+            "musicbrainz_row_id": (int),
         }
     """
     with db.engine.connect() as connection:
         result = connection.execute(sqlalchemy.text("""
-            SELECT id,
-                   display_name,
-                   email,
-                   created,
-                   musicbrainz_id,
-                   show_gravatar,
-                   is_blocked,
-                   license_choice
+            SELECT {columns}
               FROM "user"
              LIMIT :limit
             OFFSET :offset
-        """), {
+        """.format(columns=','.join(USER_GET_COLUMNS))), {
             "limit": limit,
             "offset": offset
         })
@@ -651,3 +649,36 @@ def tokens(user_id):
 
         rows = result.fetchall()
     return [dict(row) for row in rows]
+
+
+def get_by_mb_row_id(musicbrainz_row_id, musicbrainz_id=None):
+    """ Get user with specified MusicBrainz row ID.
+
+    Note: this function optionally takes a MusicBrainz username to fall back on
+    if no user with specified MusicBrainz row ID is found.
+
+    Args:
+        musicbrainz_row_id (int): the MusicBrainz row ID of the user
+        musicbrainz_id (str): the MusicBrainz username of the user
+
+    Returns:
+        a dict representing the user if found, else None
+    """
+    filter_str = ""
+    filter_data = {}
+    if musicbrainz_id:
+        filter_str = "OR (LOWER(musicbrainz_id) = LOWER(:musicbrainz_id) AND musicbrainz_row_id IS NULL)"
+        filter_data["musicbrainz_id"] = musicbrainz_id
+
+    filter_data["musicbrainz_row_id"] = musicbrainz_row_id
+    with db.engine.connect() as connection:
+        result = connection.execute(sqlalchemy.text("""
+            SELECT {columns}
+              FROM "user"
+             WHERE musicbrainz_row_id = :musicbrainz_row_id
+             {optional_filter}
+        """.format(columns=','.join(USER_GET_COLUMNS), optional_filter=filter_str)), filter_data)
+
+        if result.rowcount:
+            return result.fetchone()
+        return None