Re-do that. Turn the detail_values into their own table and drop the …

…comma-separated nonsense.

fmn/lib/models.py

@@ -141,16 +141,22 @@ def create(cls, session, name, description,
 
     def _recipients(self, session, config, valid_paths, message):
         """ Returns the list of recipients for a message. """
+
         for user in User.all(session):
             pref = Preference.load(session, user, self)
-            if pref and pref.detail_value:
-                filter = pref.prefers(session, config, valid_paths, message)
-                if filter:
-                    yield {
-                        'user': user.openid,
-                        pref.context.detail_name: pref.detail_value.split(','),
-                        'filter': filter.name,
-                    }
+            if not pref or not pref.detail_values:
+                continue
+
+            flter = pref.prefers(session, config, valid_paths, message)
+            if not flter:
+                continue
+
+            for value in pref.detail_values:
+                yield {
+                    'user': user.openid,
+                    pref.context.detail_name: value.value,
+                    'filter': flter.name,
+                }
 
     def recipients(self, session, config, valid_paths, message):
         return list(self._recipients(session, config, valid_paths, message))
@@ -189,7 +195,11 @@ def get_or_create(cls, session, openid, openid_url, create_defaults=True):
 
     @classmethod
     def create(cls, session, openid, openid_url, create_defaults):
-        user = cls(openid=openid, openid_url=openid_url, api_key=str(uuid.uuid4()))
+        user = cls(
+            openid=openid,
+            openid_url=openid_url,
+            api_key=str(uuid.uuid4()),
+        )
         session.add(user)
         session.flush()
 
@@ -348,13 +358,43 @@ def matches(self, session, config, paths, message):
         return True
 
 
+class DetailValue(BASE):
+    __tablename__ = 'detail_values'
+    id = sa.Column(sa.Integer, primary_key=True)
+    created_on = sa.Column(sa.DateTime, default=datetime.datetime.utcnow)
+    value = sa.Column(sa.String(1024), unique=True)
+    preference_id = sa.Column(
+        sa.Integer,
+        sa.ForeignKey('preferences.id'))
+    preference = relation('Preference', backref=backref('detail_values'))
+
+    @classmethod
+    def get(cls, session, value):
+        return session.query(cls).filter(cls.value==value).first()
+
+    @classmethod
+    def create(cls, session, value):
+        obj = cls()
+        obj.value = value
+        session.add(obj)
+        session.commit()
+        return obj
+
+    @classmethod
+    def exists(cls, session, value):
+        return (
+            session.query(cls).filter(
+                cls.value == value).count() > 0 or
+            session.query(Confirmation).filter(
+                Confirmation.detail_value == value).count() > 0
+        )
+
+
 class Preference(BASE):
     __tablename__ = 'preferences'
     id = sa.Column(sa.Integer, primary_key=True)
     created_on = sa.Column(sa.DateTime, default=datetime.datetime.utcnow)
 
-    detail_value = sa.Column(sa.String(1024), unique=True)
-
     # Number of seconds that have elapsed since the earliest queued message
     # before we send a digest over whatever medium.
     batch_delta = sa.Column(sa.Integer, nullable=True)
@@ -403,7 +443,7 @@ def set_batch_values(self, session, delta, count):
         session.commit()
 
     @classmethod
-    def by_user(cls, session, openid, allow_none=False):
+    def by_user(cls, session, openid):
         query = session.query(
             cls
         ).filter(
@@ -412,18 +452,12 @@ def by_user(cls, session, openid, allow_none=False):
             cls.context_name
         )
 
-        if not allow_none:
-            query = query.filter(sa.not_(cls.detail_value == None))
-
         return query.all()
 
     @classmethod
     def by_detail(cls, session, detail_value):
-        return session.query(
-            cls
-        ).filter(
-            cls.detail_value == detail_value
-        ).first()
+        value = DetailValue.get(session, detail_value)
+        return value.preference
 
     @classmethod
     def create(cls, session, user, context, detail_value=None):
@@ -435,7 +469,11 @@ def create(cls, session, user, context, detail_value=None):
         pref.user = user
         pref.context = context
 
-        pref.detail_value = detail_value
+        if detail_value:
+            value = DetailValue.create(session, detail_value)
+            pref.detail_values.append(value)
+
+            session.add(value)
 
         session.add(pref)
         session.flush()
@@ -471,14 +509,10 @@ def load(cls, session, user, context):
             .filter_by(context_name=context)\
             .first()
 
-    def update_details(self, session, value):
-        value = value.strip()
-        if self.detail_value:
-            tokens = self.detail_value.split(',')
-            if value not in tokens:
-                self.detail_value = ','.join(tokens + [value])
-        else:
-            self.detail_value = value
+    def update_details(self, session, detail_value):
+        detail_value = detail_value.strip()
+        value = DetailValue.create(session, detail_value)
+        self.detail_values.append(value)
         session.flush()
         session.commit()
 

fmn/lib/tests/test_confirmations.py

@@ -35,11 +35,20 @@ def test_updating_details(self):
         context = fmn.lib.models.Context.get(self.sess, name="irc")
         preference = fmn.lib.models.Preference.load(self.sess, user, context)
         preference.update_details(self.sess, 'wat')
-        eq_(preference.detail_value, 'wat')
+        eq_(preference.detail_values[0].value, 'wat')
         preference.update_details(self.sess, 'wat2')
-        eq_(preference.detail_value, 'wat,wat2')
-        preference.update_details(self.sess, 'wat')
-        eq_(preference.detail_value, 'wat,wat2')
+        eq_(preference.detail_values[0].value, 'wat')
+        eq_(preference.detail_values[1].value, 'wat2')
+
+        try:
+            preference.update_details(self.sess, 'wat')
+            assert(False)
+        except Exception:
+            self.sess.rollback()
+
+        eq_(len(preference.detail_values), 2)
+        eq_(preference.detail_values[0].value, 'wat')
+        eq_(preference.detail_values[1].value, 'wat2')
 
     def test_confirmation(self):
         self.create_user_and_context_data()
@@ -54,6 +63,6 @@ def test_confirmation(self):
             context,
             detail_value='awesome',
         )
-        eq_(preference.detail_value, None)
+        eq_(preference.detail_values, [])
         confirmation.set_status(self.sess, 'accepted')
-        eq_(preference.detail_value, 'awesome')
+        eq_(preference.detail_values[0].value, 'awesome')

fmn/lib/tests/test_recipients.py

@@ -32,7 +32,7 @@ def create_preference_data_empty(self):
             self.sess,
             user=user,
             context=context,
-            detail_value="threebean,threebean2",
+            detail_value="threebean",
         )
 
     def create_preference_data_basic(self, code_path):
@@ -79,7 +79,7 @@ def test_basic_recipients_list(self):
         recipients = fmn.lib.recipients_for_context(
             self.sess, self.config, self.valid_paths, 'irc', msg)
         eq_(list(recipients), [{
-            'irc nick': ['threebean', 'threebean2'],
+            'irc nick': 'threebean',
             'user': 'ralph.id.fedoraproject.org',
             'filter': 'test filter',
         }])
@@ -139,7 +139,7 @@ def test_multiple_identical_filters_hit(self):
         recipients = fmn.lib.recipients_for_context(
             self.sess, self.config, self.valid_paths, 'irc', msg)
         eq_(list(recipients), [{
-            'irc nick': ['threebean', 'threebean2'],
+            'irc nick': 'threebean',
             'user': 'ralph.id.fedoraproject.org',
             'filter': 'test filter',
             }])
@@ -164,7 +164,7 @@ def test_multiple_different_filters_hit(self):
         recipients = fmn.lib.recipients_for_context(
             self.sess, self.config, self.valid_paths, 'irc', msg)
         eq_(list(recipients), [{
-            'irc nick': ['threebean', 'threebean2'],
+            'irc nick': 'threebean',
             'user': 'ralph.id.fedoraproject.org',
             'filter': 'test filter',
             }])