From 35f5af52e7adb909aeab4b2f04281d66f71e6bf1 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Wed, 25 Mar 2015 16:22:33 -0700 Subject: [PATCH] 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 | 10 ++++ .../alembic/versions/3cc7e3753dc3_add_hold.py | 37 ++++++++++++++ gertty/app.py | 45 ++++++++++++++++- gertty/db.py | 7 ++- gertty/keymap.py | 4 ++ gertty/mywid.py | 2 + gertty/palette.py | 2 + gertty/search/parser.py | 3 ++ gertty/sync.py | 48 ++++++++++++++++++- gertty/view/change.py | 22 +++++---- gertty/view/change_list.py | 20 ++++++++ 11 files changed, 187 insertions(+), 13 deletions(-) create mode 100644 gertty/alembic/versions/3cc7e3753dc3_add_hold.py diff --git a/README.rst b/README.rst index a4e44e0..f160b97 100644 --- a/README.rst +++ b/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 diff --git a/gertty/alembic/versions/3cc7e3753dc3_add_hold.py b/gertty/alembic/versions/3cc7e3753dc3_add_hold.py new file mode 100644 index 0000000..370072c --- /dev/null +++ b/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 diff --git a/gertty/app.py b/gertty/app.py index 99f0122..ea743f6 100644 --- a/gertty/app.py +++ b/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() diff --git a/gertty/db.py b/gertty/db.py index 4948177..716cc77 100644 --- a/gertty/db.py +++ b/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() diff --git a/gertty/keymap.py b/gertty/keymap.py index 45ed9bc..b30241a 100644 --- a/gertty/keymap.py +++ b/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', diff --git a/gertty/mywid.py b/gertty/mywid.py index 340bdef..945f9b7 100644 --- a/gertty/mywid.py +++ b/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): diff --git a/gertty/palette.py b/gertty/palette.py index cbca463..286ae39 100644 --- a/gertty/palette.py +++ b/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 diff --git a/gertty/search/parser.py b/gertty/search/parser.py index 0840305..83090a4 100644 --- a/gertty/search/parser.py +++ b/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) diff --git a/gertty/sync.py b/gertty/sync.py index 919311e..da0ab88 100644 --- a/gertty/sync.py +++ b/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': diff --git a/gertty/view/change.py b/gertty/view/change.py index 5beae5b..ba4ddd2 100644 --- a/gertty/view/change.py +++ b/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: - starred = '' - self.title = '%sChange %s%s%s' % (starred, change.number, - reviewed, hidden) + if change.held: + held = ' (held)' + self.title = '%sChange %s%s%s%s' % (starred, change.number, reviewed, + 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() diff --git a/gertty/view/change_list.py b/gertty/view/change_list.py index d7b17ce..cd50f9c 100644 --- a/gertty/view/change_list.py +++ b/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