Merge lp:~kolmis/wxbanker/transaction-tagging into lp:wxbanker

[Michael Rooney]

[Michael Rooney (mrooney)]

I'll propose this for a merge since it would be nice to integrate, and then I can potentially merge the categories branch as well.

One issue I noticed off-hand is that using a list as a default argument probably isn't what you want (tags=[]) as default arguments are only evaluated once thus it will be the same list:

>>> def foo(x=[]):
... x.append(1)
... print x
...
>>> foo()
[1]
>>> foo()
[1, 1]
>>> foo()
[1, 1, 1]

So typically in this case what you do is set tags=None and at the top of the function say if tags is None: tags = []

What are your thoughts on merging this branch? I don't want an extra tags column by default as I want wxBanker to be as simple and clean as possible as surely some users won't need tags, but I'd like it to be as discoverable as possible. Perhaps have some exposed way to tag a transaction, and then only show the column for accounts which have a tagged transaction.

[Michael Rooney (mrooney)]

review: Needs Information

[Karel Kolman (kolmis)]

Michael, there are a few issues:
- i need access to bankModel in persistenStore; i had reverted the code that cached the bankModel in a persistentStore member variable, however i didn't realize the bankModel was used to access cached Tag objects. could you spare a simple explanation on the Model - Store coupling, or explain the way multiple models/stores are used in the unit tests (that failed by caching the bankModel)

- as for the tag column hiding, the visibility toggling you're suggesting seems like a good idea to me

- i was not aware of python's default argument once-only-evaluation, thanks for pointing that out

- i promised writing a few tests for the tags, i didn't get around to it yet (i admit there are times when i don't enjoy writing tests)

[Michael Rooney (mrooney)]

The tests rely on getting a fresh model from the db in a few cases, to make sure something changed on the model actually ended up in the db and is reflected in a new model. See the test*IsStored tests in modeltests.

I've push up a new branch (~mrooney/wxbanker/transaction-tagging) and r248 should address this for both of us. GetModel now uses a cached model by default if one exists, but the tests pass in useCached=False to make sure they get a fresh one.

I also fixed the default argument issue in r249, so I'd recommend merging my branch and continuing from there.

Any ideas on how to expose tagging a transaction in a discoverable way? Or, is there already such a way in your branch?

=== modified file 'bankobjects.py'
--- bankobjects.py	2009-04-08 02:30:18 +0000
+++ bankobjects.py	2009-05-15 12:07:21 +0000
@@ -24,14 +24,30 @@
 
 
 class BankModel(object):
-    def __init__(self, store, accountList):
+    def __init__(self, store):
         self.Store = store
-        self.Accounts = accountList
+        self.Accounts = AccountList(store, store.getAccounts())
+        self.Tags = store.getTags()
         
         Publisher().subscribe(self.onCurrencyChanged, "user.currency_changed")
         
     def GetBalance(self):
         return self.Accounts.Balance
+        
+    def GetTagById(self, id):
+        for tag in self.Tags:
+            if tag.ID == id:
+                return tag
+        return None
+        
+    def GetTagByName(self, name):
+        ''' returns a tag by name, creates a new one if none by that name exists '''
+        for tag in self.Tags:
+            if tag.Name == name:
+                return tag
+        newTag = self.Store.CreateTag(name)
+        self.Tags.append(newTag)
+        return newTag
     
     def GetTransactions(self):
         transactions = []
@@ -56,7 +72,7 @@
     
     def Search(self, searchString, account=None, matchIndex=1, matchCase=False):
         """
-        matchIndex: 0: Amount, 1: Description, 2: Date
+        matchIndex: 0: Amount, 1: Description, 2: Date, 3: Tags
         I originally used strings here but passing around and then validating on translated
         strings seems like a bad and fragile idea.
         """
@@ -73,9 +89,13 @@
         matches = []
         for trans in potentials:
             #print unicode(trans.Description), searchString
-            potentialStr = unicode((trans.Amount, trans.Description, trans.Date)[matchIndex])
-            if re.findall(searchString, potentialStr, flags=reFlag):
-                matches.append(trans)
+            potentialStrings = (
+                (trans.Amount, ), (trans.Description, ), (trans.Date, ), trans.Tags or ('', )
+            )[matchIndex]
+            for potentialStr in potentialStrings:
+                if re.search(searchString, unicode(potentialStr), flags=reFlag):
+                    matches.append(trans)
+                    break
         return matches
     
     def Save(self):
@@ -316,6 +336,18 @@
         self.RemoveTransactions(transactions)
         destAccount.AddTransactions(transactions)
         Publisher.sendMessage("batch.end")
+    
+    def TagTransactions(self, transactions, tag):
+        Publisher.sendMessage("batch.start")
+        for t in transactions:
+            t.AddTag(tag)
+        Publisher.sendMessage("batch.end")
+        
+    def UntagTransactions(self, transactions, tag):
+        Publisher.sendMessage("batch.start")
+        for t in transactions:
+            t.RemoveTag(tag)
+        Publisher.sendMessage("batch.end")
         
     def onTransactionAmountChanged(self, message):
         transaction, difference = message.data
@@ -371,7 +403,7 @@
     Changes to this object get sent out via pubsub,
     typically causing the model to make the change.
     """
-    def __init__(self, tID, parent, amount, description, date):
+    def __init__(self, tID, parent, amount, description, date, tags=[]):
         self.IsFrozen = True
         
         self.ID = tID
@@ -379,9 +411,29 @@
         self.Date = date
         self.Description = description
         self.Amount = amount
+        self.Tags = tags
         
         self.IsFrozen = False
         
+    def GetTags(self):
+        return self._Tags
+    
+    def SetTags(self, tags):
+        self._Tags = tags
+        
+        if not self.IsFrozen:
+            Publisher.sendMessage("transaction.updated.tags", (self, None))
+    
+    def AddTag(self, tag):
+        if not tag in self.Tags:
+            self.Tags.append(tag)
+            Publisher.sendMessage("transaction.updated.tags", (self, None))
+            
+    def RemoveTag(self, tag):
+        if tag in self.Tags:
+            self.Tags.remove(tag)
+            Publisher.sendMessage("transaction.updated.tags", (self, None))
+        
     def GetDate(self):
         return self._Date
         
@@ -480,8 +532,21 @@
             
     Date = property(GetDate, SetDate)
     Description = property(GetDescription, SetDescription)
+    Tags = property(GetTags, SetTags)
     Amount = property(GetAmount, SetAmount)
                
+
+class Tag(object):
+    def __init__(self, aID, name):
+        self.ID = aID
+        self.Name = name
+    
+    def __str__(self):
+        return self.Name
+        
+    def __cmp__(self, other):
+        return cmp(self.ID, other.ID)
+
         
 if __name__ == "__main__":
     import doctest
 
=== modified file 'persistentstore.py'
--- persistentstore.py	2009-04-08 02:30:18 +0000
+++ persistentstore.py	2009-05-15 13:15:36 +0000
@@ -44,7 +44,7 @@
     back the changes.
     """
     def __init__(self, path, autoSave=True):
-        self.Version = 3
+        self.Version = 4
         self.Path = path
         self.AutoSave = autoSave
         self.Dirty = False
@@ -70,6 +70,8 @@
             debug.debug(self.Meta)
             
         self.commitIfAppropriate()
+        # better delete tags on exit ?
+        #self.DeleteUnusedTags()
         
         self.Subscriptions = (
             (self.onTransactionUpdated, "transaction.updated"),
@@ -88,9 +90,7 @@
         if "--sync-balances" in sys.argv:
             self.syncBalances()
             
-        accounts = self.getAccounts()
-        accountList = bankobjects.AccountList(self, accounts)
-        bankmodel = bankobjects.BankModel(self, accountList)
+        bankmodel = bankobjects.BankModel(self)
         
         return bankmodel
     
@@ -134,6 +134,19 @@
         # everything is fine. So just return True, as there we no errors that we are aware of.
         return True
     
+    def CreateTag(self, name):
+        cursor = self.dbconn.cursor()
+        cursor.execute('INSERT INTO tags (name) VALUES (?)', [name])
+        ID = cursor.lastrowid
+        self.commitIfAppropriate()
+        
+        return bankobjects.Tag(ID, name)
+    
+    def DeleteUnusedTags(self):
+        cursor = self.dbconn.cursor()
+        cursor.execute('DELETE FROM tags WHERE NOT EXISTS (SELECT 1 FROM transactions_tags_link l WHERE tagId = tags.id)')
+        self.commitIfAppropriate()
+    
     def Save(self):
         import time; t = time.time()
         self.dbconn.commit()
@@ -173,9 +186,15 @@
 
         cursor.execute('CREATE TABLE accounts (id INTEGER PRIMARY KEY, name VARCHAR(255), currency INTEGER, balance FLOAT)')
         cursor.execute('CREATE TABLE transactions (id INTEGER PRIMARY KEY, accountId INTEGER, amount FLOAT, description VARCHAR(255), date CHAR(10))')
+        cursor.execute('CREATE INDEX transactions_accountId_idx ON transactions(accountId)')
+        
+        cursor.execute('CREATE TABLE tags (id INTEGER PRIMARY KEY, name VARCHAR(255))')
+        cursor.execute('CREATE TABLE transactions_tags_link (id INTEGER PRIMARY KEY, transactionId INTEGER, tagId INTEGER)')
+        cursor.execute('CREATE INDEX transactions_tags_transactionId_idx ON transactions_tags_link(transactionId)')
+        cursor.execute('CREATE INDEX transactions_tags_tagId_idx ON transactions_tags_link(tagId)')
         
         cursor.execute('CREATE TABLE meta (id INTEGER PRIMARY KEY, name VARCHAR(255), value VARCHAR(255))')
-        cursor.execute('INSERT INTO meta VALUES (null, ?, ?)', ('VERSION', '3'))
+        cursor.execute('INSERT INTO meta VALUES (null, ?, ?)', ('VERSION', self.Version))
 
         return connection
     
@@ -219,12 +238,24 @@
             # Add `total` column to the accounts table.
             cursor.execute('ALTER TABLE accounts ADD balance FLOAT not null DEFAULT 0.0')
             self.syncBalances()
-            # Update the meta version number.
-            cursor.execute('UPDATE meta SET value=? WHERE name=?', (3, "VERSION"))
+            self.__updateDbVersion(3)
+        elif fromVer == 3:
+            # index for the accountId column in transactions
+            cursor.execute('CREATE INDEX transactions_accountId_idx ON transactions(accountId)')
+            # tags table; transactions - tags link table
+            cursor.execute('CREATE TABLE tags (id INTEGER PRIMARY KEY, name VARCHAR(255))')
+            cursor.execute('CREATE TABLE transactions_tags_link (id INTEGER PRIMARY KEY, transactionId INTEGER, tagId INTEGER)')
+            cursor.execute('CREATE INDEX transactions_tags_transactionId_idx ON transactions_tags_link(transactionId)')
+            cursor.execute('CREATE INDEX transactions_tags_tagId_idx ON transactions_tags_link(tagId)')
+            self.__updateDbVersion(4)
         else:
             raise Exception("Cannot upgrade database from version %i"%fromVer)
         
         self.commitIfAppropriate()
+    
+    def __updateDbVersion(self, version):
+        # Update the meta version number.
+        self.dbconn.cursor().execute('UPDATE meta SET value=? WHERE name=?', (version, "VERSION"))
         
     def syncBalances(self):
         debug.debug("Syncing balances...")
@@ -252,17 +283,32 @@
         self.dbconn.cursor().execute('DELETE FROM transactions WHERE accountId=?', (account.ID,))
         self.commitIfAppropriate()
         
+    def getTags(self):
+        return [self.result2tag(result) for result in self.dbconn.cursor().execute("SELECT * FROM tags").fetchall()]
+        
+    def result2tag(self, result):
+        ID, name = result
+        return bankobjects.Tag(ID, name)
+    
+    def getTagsForTransaction(self, transId):
+        result = self.dbconn.cursor().execute(
+            'SELECT tagId FROM transactions_tags_link WHERE transactionId=?', (transId, )).fetchall()
+        return [self.bankModel.GetTagById(row[0]) for row in result]
+        
     def getTransactionsFrom(self, account):
         transactions = bankobjects.TransactionList()
         for result in self.dbconn.cursor().execute('SELECT * FROM transactions WHERE accountId=?', (account.ID,)).fetchall():
             tid, pid, amount, description, date = result
-            transactions.append(bankobjects.Transaction(tid, account, amount, description, date))
+            transactions.append(bankobjects.Transaction(tid, account, amount, description, date, self.getTagsForTransaction(tid)))
         return transactions
     
     def updateTransaction(self, transObj):
         result = self.transaction2result(transObj)
         result.append( result.pop(0) ) # Move the uid to the back as it is last in the args below.
         self.dbconn.cursor().execute('UPDATE transactions SET amount=?, description=?, date=? WHERE id=?', result)
+        self.dbconn.cursor().execute('DELETE FROM transactions_tags_link WHERE transactionId=?', (transObj.ID, ))
+        for tag in transObj.Tags:
+            self.dbconn.cursor().execute('INSERT INTO transactions_tags_link (transactionId, tagId) VALUES (?, ?)', (transObj.ID, tag.ID))
         self.commitIfAppropriate()
         
     def renameAccount(self, oldName, account):
 
=== modified file 'searchctrl.py'
--- searchctrl.py	2009-04-13 03:09:07 +0000
+++ searchctrl.py	2009-05-14 08:11:28 +0000
@@ -37,7 +37,7 @@
         # The More/Less button.
         self.moreButton = bankcontrols.MultiStateButton(self, labelDict={True: _("More options"), False: _("Less options")}, state=True)
 
-        self.matchChoices = [_("Amount"), _("Description"), _("Date")]
+        self.matchChoices = [_("Amount"), _("Description"), _("Date"), _("Tags")]
         self.matchBox = bankcontrols.CompactableComboBox(self, value=self.matchChoices[1], choices=self.matchChoices, style=wx.CB_READONLY)
 
         self.caseCheck = wx.CheckBox(self, label=_("Case Sensitive"))
@@ -107,4 +107,4 @@
         # Give or take the appropriate amount of space.
         self.Parent.Layout()
         Publisher().sendMessage("SEARCH.MORETOGGLED")
-        
 \ No newline at end of file
+        
 
=== modified file 'transactionolv.py'
--- transactionolv.py	2009-05-06 22:24:20 +0000
+++ transactionolv.py	2009-05-12 21:24:35 +0000
@@ -46,6 +46,7 @@
         self.oddRowsBackColor = wx.WHITE
         self.cellEditMode = GroupListView.CELLEDIT_DOUBLECLICK
         self.SetEmptyListMsg(_("No transactions entered."))
+        self.TagSeparator = ','
         
         # Calculate the necessary width for the date column.
         dateStr = str(datetime.date.today())
@@ -61,6 +62,7 @@
             ColumnDefn(_("Description"), valueGetter="Description", isSpaceFilling=True),
             ColumnDefn(_("Amount"), "right", valueGetter="Amount", stringConverter=self.renderFloat),
             ColumnDefn(_("Total"), "right", valueGetter=self.getTotal, stringConverter=self.renderFloat, isEditable=False),
+            ColumnDefn(_("Tags"), valueGetter=self.getTagsString, valueSetter=self.setTagsFromString),
         ])
         # Our custom hack in OLV.py:2017 will render amount floats appropriately as %.2f when editing.
         
@@ -106,6 +108,13 @@
         self.Freeze()
         self.SortBy(self.SORT_COL)
         self.Thaw()
+
+    def getTagsString(self, transaction):
+        return (self.TagSeparator + ' ').join([tag.Name for tag in transaction.Tags])
+    
+    def setTagsFromString(self, transaction, tags):
+        tagNames = [tag.strip() for tag in tags.split(self.TagSeparator) if tag.strip()]
+        transaction.Tags = [self.BankController.Model.GetTagByName(name) for name in tagNames]
         
     def getTotal(self, transObj):
         """
@@ -192,9 +201,13 @@
         if len(transactions) == 1:
             removeStr = _("Remove this transaction")
             moveStr = _("Move this transaction to account")
+            addTagStr = _("Tag the transaction with")
+            removeTagStr = _("Untag the transaction with")
         else:
             removeStr = _("Remove these %i transactions") % len(transactions)
             moveStr = _("Move these %i transactions to account") % len(transactions)
+            addTagStr = _("Tag these %i transactions with") % len(transactions)
+            removeTagStr = _("Untag these %i transactions with") % len(transactions)
             
         removeItem = wx.MenuItem(menu, -1, removeStr)
         menu.Bind(wx.EVT_MENU, lambda e: self.onRemoveTransactions(transactions), source=removeItem)
@@ -211,6 +224,42 @@
                 accountsMenu.Bind(wx.EVT_MENU, lambda e, account=account: self.onMoveTransactions(transactions, account), source=accountItem)
             moveToAccountItem.SetSubMenu(accountsMenu)
             menu.AppendItem(moveToAccountItem)
+            
+            # Tagging
+            tags = sorted(self.BankController.Model.Tags, key=lambda x: x.Name)
+            
+            tagActionItem = wx.MenuItem(menu, -1, addTagStr)
+            tagsMenu = wx.Menu()
+            for tag in tags:
+                tagItem = wx.MenuItem(menu, -1, tag.Name)
+                tagsMenu.AppendItem(tagItem)
+                tagsMenu.Bind(wx.EVT_MENU, lambda e, tag=tag: self.onTagTransactions(transactions, tag), source=tagItem)
+
+            tagsMenu.AppendSeparator()
+
+            newTagItem = wx.MenuItem(menu,-1, _("New Tag"))
+            tagsMenu.AppendItem(newTagItem)
+            tagsMenu.Bind(wx.EVT_MENU, lambda e: self.onTagTransactions(transactions), source=newTagItem)
+            
+            tagActionItem.SetSubMenu(tagsMenu)
+            menu.AppendItem(tagActionItem)
+            
+            # get the tags currently applied to the selected transactions
+            currentTags = set()
+            for t in transactions:
+                currentTags.update(t.Tags)
+            currentTags = sorted(currentTags, key=lambda x: x.Name)
+            
+            if len(currentTags) > 0:
+                tagActionItem = wx.MenuItem(menu, -1, removeTagStr)
+                tagsMenu = wx.Menu()
+
+                for tag in currentTags:
+                    tagItem = wx.MenuItem(menu, -1, tag.Name)
+                    tagsMenu.AppendItem(tagItem)
+                    tagsMenu.Bind(wx.EVT_MENU, lambda e, tag=tag: self.onUntagTransactions(transactions, tag), source=tagItem)
+                tagActionItem.SetSubMenu(tagsMenu)
+                menu.AppendItem(tagActionItem)
 
         # Show the menu and then destroy it afterwards.
         self.PopupMenu(menu)
@@ -242,6 +291,20 @@
     def onMoveTransactions(self, transactions, targetAccount):
         """Move the transactions to the target account."""
         self.CurrentAccount.MoveTransactions(transactions, targetAccount)
+    
+    def onTagTransactions(self, transactions, tag=None):
+        if tag is None:
+            dialog = wx.TextEntryDialog(self, _("Enter new tag"), _("New Tag"))
+            dialog.ShowModal()
+            dialog.Destroy()
+            tagName = dialog.GetValue().strip()
+            if len(tagName) == 0:
+                return
+            tag = self.BankController.Model.GetTagByName(tagName)
+        self.CurrentAccount.TagTransactions(transactions, tag)
+        
+    def onUntagTransactions(self, transactions, tag):
+        self.CurrentAccount.UntagTransactions(transactions, tag)
         
     def frozenResize(self):
         self.Parent.Layout()