Add held changes

This feature detects when Gertty is about to upload a review with
a positive vote after someone else has left a negative vote while
Gertty was offline.  This prevents a situation where it appears
a user is ignoring negative feedback from others.  The local user
is alerted and has the option to re-evaluate their review before
proceeding.

Change-Id: I838acaae6d12a2f8557bfd5a16837784c97c031a

README.rst

@@ -131,6 +131,16 @@ If Gertty is offline, it will so indicate in the status bar.  It will
 retry requests if needed, and will switch between offline and online
 mode automatically.
 
+If you review a change while offline with a positive vote, and someone
+else leaves a negative vote on that change in the same category before
+Gertty is able to upload your review, Gertty will detect the situation
+and mark the change as "held" so that you may re-inspect the change
+and any new comments before uploading the review.  The status bar will
+alert you to any held changes and direct you to a list of them (the
+`F12` key by default).  When viewing a change, the "held" flag may be
+toggeled with the exclamation key (`!`).  Once held, a change must be
+explicitly un-held in this manner for your review to be uploaded.
+
 If Gertty encounters an error, this will also be indicated in the
 status bar.  You may wish to examine ~/.gertty.log to see what the
 error was.  In many cases, Gertty can continue after encountering an

gertty/alembic/versions/3cc7e3753dc3_add_hold.py

@@ -0,0 +1,37 @@
+"""add held
+
+Revision ID: 3cc7e3753dc3
+Revises: 1cdd4e2e74c
+Create Date: 2015-03-22 08:48:15.516289
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '3cc7e3753dc3'
+down_revision = '1cdd4e2e74c'
+
+import warnings
+
+from alembic import op
+import sqlalchemy as sa
+
+from gertty.dbsupport import sqlite_alter_columns
+
+
+def upgrade():
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore")
+        op.add_column('change', sa.Column('held', sa.Boolean()))
+
+    connection = op.get_bind()
+    change = sa.sql.table('change',
+                          sa.sql.column('held', sa.Boolean()))
+    connection.execute(change.update().values({'held':False}))
+
+    sqlite_alter_columns('change', [
+        sa.Column('held', sa.Boolean(), index=True, nullable=False),
+        ])
+
+
+def downgrade():
+    pass

gertty/app.py

@@ -64,8 +64,10 @@ class StatusHeader(urwid.WidgetWrap):
         self.error_widget = urwid.Text('')
         self.offline_widget = urwid.Text('')
         self.sync_widget = urwid.Text(u'Sync: 0')
+        self.held_widget = urwid.Text(u'')
         self._w.contents.append((self.title_widget, ('pack', None, False)))
         self._w.contents.append((urwid.Text(u''), ('weight', 1, False)))
+        self._w.contents.append((self.held_widget, ('pack', None, False)))
         self._w.contents.append((self.error_widget, ('pack', None, False)))
         self._w.contents.append((self.offline_widget, ('pack', None, False)))
         self._w.contents.append((self.sync_widget, ('pack', None, False)))
@@ -73,18 +75,23 @@ class StatusHeader(urwid.WidgetWrap):
         self.offline = None
         self.title = None
         self.sync = None
+        self.held = None
         self._error = False
         self._offline = False
         self._title = ''
         self._sync = 0
+        self._held = 0
+        self.held_key = self.app.config.keymap.formatKeys(keymap.LIST_HELD)
 
-    def update(self, title=None, error=None, offline=None, refresh=True):
+    def update(self, title=None, error=None, offline=None, refresh=True, held=None):
         if title is not None:
             self.title = title
         if error is not None:
             self.error = error
         if offline is not None:
             self.offline = offline
+        if held is not None:
+            self.held = held
         self.sync = self.app.sync.queue.qsize()
         if refresh:
             self.refresh()
@@ -93,6 +100,12 @@ class StatusHeader(urwid.WidgetWrap):
         if self._title != self.title:
             self._title = self.title
             self.title_widget.set_text(self._title)
+        if self._held != self.held:
+            self._held = self.held
+            if self._held:
+                self.held_widget.set_text(('error', u'Held: %s (%s)' % (self._held, self.held_key)))
+            else:
+                self.held_widget.set_text(u'')
         if self._error != self.error:
             self._error = self.error
             if self._error:
@@ -202,6 +215,7 @@ class App(object):
         self.header = urwid.AttrMap(self.status, 'header')
         screen = view_project_list.ProjectListView(self)
         self.status.update(title=screen.title)
+        self.updateStatusQueries()
         self.loop = urwid.MainLoop(screen, palette=self.config.palette.getPalette(),
                                    unhandled_input=self.unhandledInput)
 
@@ -280,20 +294,30 @@ class App(object):
             self.loop.widget = widget
 
     def refresh(self, data=None, force=False):
-        self.status.refresh()
         widget = self.loop.widget
         while isinstance(widget, urwid.Overlay):
             widget = widget.contents[0][0]
         interested = force
+        invalidate = False
         try:
             while True:
                 event = self.sync.result_queue.get(0)
                 if widget.interested(event):
                     interested = True
+                if hasattr(event, 'held_changed') and event.held_changed:
+                    invalidate = True
         except Queue.Empty:
             pass
         if interested:
             widget.refresh()
+        if invalidate:
+            self.updateStatusQueries()
+        self.status.refresh()
+
+    def updateStatusQueries(self):
+        with self.db.getSession() as session:
+            held = len(session.getHeld())
+            self.status.update(held=held)
 
     def popup(self, widget,
               relative_width=50, relative_height=25,
@@ -441,6 +465,8 @@ class App(object):
             self.quit()
         elif keymap.CHANGE_SEARCH in commands:
             self.searchDialog()
+        elif keymap.LIST_HELD in commands:
+            self.doSearch("status:open is:held")
         elif key in self.config.dashboards:
             d = self.config.dashboards[key]
             self.clearHistory()
@@ -488,6 +514,21 @@ class App(object):
         self.error_queue.put(('Warning', m))
         os.write(self.error_pipe, 'error\n')
 
+    def toggleHeldChange(self, change_key):
+        with self.db.getSession() as session:
+            change = session.getChange(change_key)
+            change.held = not change.held
+            ret = change.held
+            if not change.held:
+                for r in change.revisions:
+                    for m in change.messages:
+                        if m.pending:
+                            self.sync.submitTask(
+                                sync.UploadReviewTask(m.key, sync.HIGH_PRIORITY))
+        self.updateStatusQueries()
+        return ret
+
+
 def version():
     return "Gertty version: %s" % gertty.version.version_info.version_string()
 

gertty/db.py

@@ -59,6 +59,7 @@ change_table = Table(
     Column('hidden', Boolean, index=True, nullable=False),
     Column('reviewed', Boolean, index=True, nullable=False),
     Column('starred', Boolean, index=True, nullable=False),
+    Column('held', Boolean, index=True, nullable=False),
     Column('pending_rebase', Boolean, index=True, nullable=False),
     Column('pending_topic', Boolean, index=True, nullable=False),
     Column('pending_starred', Boolean, index=True, nullable=False),
@@ -184,7 +185,7 @@ class Branch(object):
 class Change(object):
     def __init__(self, project, id, owner, number, branch, change_id,
                  subject, created, updated, status, topic=None,
-                 hidden=False, reviewed=False, starred=False,
+                 hidden=False, reviewed=False, starred=False, held=False,
                  pending_rebase=False, pending_topic=False,
                  pending_starred=False, pending_status=False,
                  pending_status_message=None):
@@ -202,6 +203,7 @@ class Change(object):
         self.hidden = hidden
         self.reviewed = reviewed
         self.starred = starred
+        self.held = held
         self.pending_rebase = pending_rebase
         self.pending_topic = pending_topic
         self.pending_starred = pending_starred
@@ -679,6 +681,9 @@ class DatabaseSession(object):
         except sqlalchemy.orm.exc.NoResultFound:
             return None
 
+    def getHeld(self):
+        return self.session().query(Change).filter_by(held=True).all()
+
     def getPendingMessages(self):
         return self.session().query(Message).filter_by(pending=True).all()
 

gertty/keymap.py

@@ -34,10 +34,12 @@ PREV_SCREEN = 'previous screen'
 HELP = 'help'
 QUIT = 'quit'
 CHANGE_SEARCH = 'change search'
+LIST_HELD = 'list held changes'
 # Change screen:
 TOGGLE_REVIEWED = 'toggle reviewed'
 TOGGLE_HIDDEN = 'toggle hidden'
 TOGGLE_STARRED = 'toggle starred'
+TOGGLE_HELD = 'toggle held'
 REVIEW = 'review'
 DIFF = 'diff'
 LOCAL_CHECKOUT = 'local checkout'
@@ -82,10 +84,12 @@ DEFAULT_KEYMAP = {
     HELP: ['f1', '?'],
     QUIT: 'ctrl q',
     CHANGE_SEARCH: 'ctrl o',
+    LIST_HELD: 'f12',
 
     TOGGLE_REVIEWED: 'v',
     TOGGLE_HIDDEN: 'k',
     TOGGLE_STARRED: '*',
+    TOGGLE_HELD: '!',
     REVIEW: 'r',
     DIFF: 'd',
     LOCAL_CHECKOUT: 'c',

gertty/mywid.py

@@ -25,6 +25,8 @@ GLOBAL_HELP = (
      "Quit Gertty"),
     (keymap.CHANGE_SEARCH,
      "Search for changes"),
+    (keymap.LIST_HELD,
+     "List held changes"),
     )
 
 class TextButton(urwid.Button):

gertty/palette.py

@@ -81,6 +81,8 @@ DEFAULT_PALETTE={
     'focused-reviewed-change': ['dark gray,standout', ''],
     'starred-change': ['light cyan', ''],
     'focused-starred-change': ['light cyan,standout', ''],
+    'held-change': ['light red', ''],
+    'focused-held-change': ['light red,standout', ''],
     }
 
 # A delta from the default palette

gertty/search/parser.py

@@ -271,6 +271,9 @@ def SearchParser():
             p[0] = gertty.db.account_table.c.username == username
         elif p[2] == 'starred':
             p[0] = gertty.db.change_table.c.starred == True
+        elif p[2] == 'held':
+            # A gertty extension
+            p[0] = gertty.db.change_table.c.held == True
         elif p[2] == 'reviewer':
             filters = []
             filters.append(gertty.db.approval_table.c.change_key == gertty.db.change_table.c.key)

gertty/sync.py

@@ -111,6 +111,7 @@ class ChangeAddedEvent(UpdateEvent):
         self.related_change_keys = set()
         self.review_flag_changed = True
         self.status_changed = True
+        self.held_changed = False
 
 class ChangeUpdatedEvent(UpdateEvent):
     def __repr__(self):
@@ -123,6 +124,7 @@ class ChangeUpdatedEvent(UpdateEvent):
         self.related_change_keys = set()
         self.review_flag_changed = False
         self.status_changed = False
+        self.held_changed = False
 
 class Task(object):
     def __init__(self, priority=NORMAL_PRIORITY):
@@ -420,7 +422,8 @@ class SyncChangeTask(Task):
                                                      remote_revision['commit']['message'], remote_commit,
                                                      remote_revision['commit']['parents'][0]['commit'],
                                                      auth, ref)
-                    self.log.info("Created new revision %s for change %s revision %s in local DB.", revision.key, self.change_id, remote_revision['_number'])
+                    self.log.info("Created new revision %s for change %s revision %s in local DB.",
+                                  revision.key, self.change_id, remote_revision['_number'])
                     new_revision = True
                 revision.message = remote_revision['commit']['message']
                 actions = remote_revision.get('actions', {})
@@ -454,7 +457,8 @@ class SyncChangeTask(Task):
                                                              created,
                                                              remote_file, parent, remote_comment.get('line'),
                                                              remote_comment['message'])
-                            self.log.info("Created new comment %s for revision %s in local DB.", comment.key, revision.key)
+                            self.log.info("Created new comment %s for revision %s in local DB.",
+                                          comment.key, revision.key)
                         else:
                             if comment.author != account:
                                 comment.author = account
@@ -506,9 +510,26 @@ class SyncChangeTask(Task):
             remote_label_keys = set(remote_label_entries.keys())
             local_approvals = {}
             local_labels = {}
+            user_votes = {}
             for approval in change.approvals:
+                if approval.draft and not new_revision:
+                    # If we have a new revision, we need to delete
+                    # draft local approvals because they can no longer
+                    # be uploaded.  Otherwise, keep them because we
+                    # may be about to upload a review.  Ignoring an
+                    # approval here means it will not be deleted.
+                    # Also keep track of these approvals so we can
+                    # determine whether we should hold the change
+                    # later.
+                    user_votes[approval.category] = approval.value
+                    # Count draft votes as having voted for the
+                    # purposes of deciding whether to clear the
+                    # reviewed flag later.
+                    user_voted = True
+                    continue
                 key = '%s~%s' % (approval.category, approval.reviewer.id)
                 if key in local_approvals:
+                    # Delete duplicate approvals.
                     session.delete(approval)
                 else:
                     local_approvals[key] = approval
@@ -534,6 +555,16 @@ class SyncChangeTask(Task):
                                       remote_approval['category'],
                                       remote_approval['value'])
                 self.log.info("Created approval for change %s in local DB.", change.id)
+                user_value = user_votes.get(remote_approval['category'], 0)
+                if user_value > 0 and remote_approval['value'] < 0:
+                    # Someone left a negative vote after the local
+                    # user created a draft positive vote.  Hold the
+                    # change so that it doesn't look like the local
+                    # user is ignoring negative feedback.
+                    if not change.held:
+                        change.held = True
+                        result.held_changed = True
+                        self.log.info("Setting change %s to held due to negative review after positive", change.id)
 
             for key in remote_label_keys-local_label_keys:
                 remote_label = remote_label_entries[key]
@@ -831,12 +862,25 @@ class UploadReviewTask(Task):
 
     def run(self, sync):
         app = sync.app
+
+        with app.db.getSession() as session:
+            message = session.getMessage(self.message_key)
+            change = message.revision.change
+        if not change.held:
+            self.log.debug("Syncing %s to find out if it should be held" % (change.id,))
+            t = SyncChangeTask(change.id)
+            t.run(sync)
+            self.results += t.results
         submit = False
         change_id = None
         with app.db.getSession() as session:
             message = session.getMessage(self.message_key)
             revision = message.revision
             change = message.revision.change
+            if change.held:
+                self.log.debug("Not uploading review to %s because it is held" %
+                               (change.id,))
+                return
             change_id = change.id
             current_revision = change.revisions[-1]
             if change.pending_status and change.status == 'SUBMITTED':

gertty/view/change.py

@@ -407,6 +407,8 @@ class ChangeView(urwid.WidgetWrap):
              "Go to the previous change in the list"),
             (key(keymap.REVIEW),
              "Leave a review for the most recent revision"),
+            (key(keymap.TOGGLE_HELD),
+             "Toggle the held flag for the current change"),
             (key(keymap.TOGGLE_HIDDEN_COMMENTS),
              "Toggle display of hidden comments"),
             (key(keymap.SEARCH_RESULTS),
@@ -546,20 +548,17 @@ class ChangeView(urwid.WidgetWrap):
             change = session.getChange(self.change_key)
             self.topic = change.topic or ''
             self.pending_status_message = change.pending_status_message or ''
+            reviewed = hidden = starred = held = ''
             if change.reviewed:
                 reviewed = ' (reviewed)'
-            else:
-                reviewed = ''
             if change.hidden:
                 hidden = ' (hidden)'
-            else:
-                hidden = ''
             if change.starred:
                 starred = '* '
-            else:
+            if change.held:
-                starred = ''
+                held = ' (held)'
-            self.title = '%sChange %s%s%s' % (starred, change.number,
+            self.title = '%sChange %s%s%s%s' % (starred, change.number, reviewed,
-                                              reviewed, hidden)
+                                                hidden, held)
             self.app.status.update(title=self.title)
             self.project_key = change.project.key
             self.change_rest_id = change.id
@@ -777,6 +776,9 @@ class ChangeView(urwid.WidgetWrap):
         self.app.sync.submitTask(
             sync.ChangeStarredTask(self.change_key, sync.HIGH_PRIORITY))
 
+    def toggleHeld(self):
+        return self.app.toggleHeldChange(self.change_key)
+
     def keypress(self, size, key):
         r = super(ChangeView, self).keypress(size, key)
         commands = self.app.config.keymap.getCommands(r)
@@ -792,6 +794,10 @@ class ChangeView(urwid.WidgetWrap):
             self.toggleStarred()
             self.refresh()
             return None
+        if keymap.TOGGLE_HELD in commands:
+            self.toggleHeld()
+            self.refresh()
+            return None
         if keymap.REVIEW in commands:
             row = self.revision_rows[self.last_revision_key]
             row.review_button.openReview()

gertty/view/change_list.py

@@ -52,6 +52,7 @@ class ChangeRow(urwid.Button):
                         'unreviewed-change': 'focused-unreviewed-change',
                         'reviewed-change': 'focused-reviewed-change',
                         'starred-change': 'focused-starred-change',
+                        'held-change': 'focused-held-change',
                         'positive-label': 'focused-positive-label',
                         'negative-label': 'focused-negative-label',
                         'min-label': 'focused-min-label',
@@ -98,6 +99,9 @@ class ChangeRow(urwid.Button):
         if change.starred:
             flag = '*'
             style = 'starred-change'
+        if change.held:
+            flag = '!'
+            style = 'held-change'
         subject = flag + subject
         self.row_style.set_attr_map({None: style})
         self.subject.set_text(subject)
@@ -152,6 +156,8 @@ class ChangeListView(urwid.WidgetWrap):
     def help(self):
         key = self.app.config.keymap.formatKeys
         return [
+            (key(keymap.TOGGLE_HELD),
+             "Toggle the held flag for the currently selected change"),
             (key(keymap.TOGGLE_HIDDEN),
              "Toggle the hidden flag for the currently selected change"),
             (key(keymap.TOGGLE_LIST_REVIEWED),
@@ -368,6 +374,9 @@ class ChangeListView(urwid.WidgetWrap):
             sync.ChangeStarredTask(change_key, sync.HIGH_PRIORITY))
         return ret
 
+    def toggleHeld(self, change_key):
+        return self.app.toggleHeldChange(change_key)
+
     def toggleHidden(self, change_key):
         with self.app.db.getSession() as session:
             change = session.getChange(change_key)
@@ -420,6 +429,17 @@ class ChangeListView(urwid.WidgetWrap):
                 # where we're not just popping a row from the list of changes.
                 self.refresh()
             return None
+        if keymap.TOGGLE_HELD in commands:
+            if not len(self.listbox.body):
+                return None
+            pos = self.listbox.focus_position
+            change_key = self.listbox.body[pos].change_key
+            held = self.toggleHeld(change_key)
+            row = self.change_rows[change_key]
+            with self.app.db.getSession() as session:
+                change = session.getChange(change_key)
+                row.update(change, self.categories)
+            return None
         if keymap.TOGGLE_STARRED in commands:
             if not len(self.listbox.body):
                 return None