# HG changeset patch # User Steve Losh # Date 1277858972 14400 # Node ID 01a47fb22911848c5750c65b8c9021637195efed # Parent 0c22712a4c18fa52126acedc495663ee557e94c4 guts: refactor signoff editing to use the new renaming api diff -r 0c22712a4c18 -r 01a47fb22911 review/api.py --- a/review/api.py Tue Jun 29 20:00:12 2010 -0400 +++ b/review/api.py Tue Jun 29 20:49:32 2010 -0400 @@ -61,7 +61,7 @@ class SignoffExists(Exception): - """Raised when trying to signoff twice without forcing.""" + """Raised when trying to signoff twice.""" pass class FileNotInChangeset(Exception): @@ -325,6 +325,24 @@ old.style = style if style is not None else old.style old._rename(self.ui, self.repo, old.identifier) + def edit_signoff(self, identifier, message=None, opinion=None, style=None): + olds = self.get_items(identifier) + + if len(olds) == 0: + raise UnknownIdentifier + elif len(olds) > 1: + raise AmbiguousIdentifier + + old = olds[0] + if old.itemtype != 'signoff': + raise WrongEditItemType() + + old.hgdate = util.makedate() + old.opinion = opinion if opinion is not None else old.opinion + old.message = message if message is not None else old.message + old.style = style if style is not None else old.style + old._rename(self.ui, self.repo, old.identifier) + class ReviewChangeset(object): """The review data about one changeset in the target repository. @@ -408,24 +426,22 @@ return self.signoffs_for_user(self.ui.username()) - def add_signoff(self, message, opinion='', force=False, style=''): + def add_signoff(self, message, opinion='', style=''): """Add (and commit) a signoff for the given revision. The opinion argument should be 'yes', 'no', or ''. - If a signoff from the user already exists, a SignoffExists exception - will be raised unless the force argument is used. + If a signoff from the user already exists a SignoffExists exception + will be raised. """ existing = self.signoffs_for_current_user() if existing: - if not force: - raise SignoffExists - existing[0]._delete(self.ui, self.repo) + raise SignoffExists signoff = ReviewSignoff(self.ui.username(), util.makedate(), - self.node, opinion, message, style) + self.node, opinion, message, style) signoff._commit(self.ui, self.repo) def add_comment(self, message, filename='', lines=[], style=''): diff -r 0c22712a4c18 -r 01a47fb22911 review/cli.py --- a/review/cli.py Tue Jun 29 20:00:12 2010 -0400 +++ b/review/cli.py Tue Jun 29 20:49:32 2010 -0400 @@ -52,7 +52,20 @@ fnames[0], rd.target[comment.node].rev())) def _edit_signoff(rd, signoff, **opts): - raise util.Abort('unimplemented, sorry') + mdown = opts.pop('mdown') + message = opts.pop('message').strip() + + yes, no = opts.pop('yes'), opts.pop('no') + if yes and no: + raise util.Abort(messages.SIGNOFF_OPINION_CONFLICT) + opinion = 'yes' if yes else ('no' if no else signoff.opinion) + + if not message: + message = signoff.message + + style = 'markdown' if mdown or signoff.style == 'markdown' else '' + + rd.edit_signoff(signoff.identifier, message, opinion, style) def _web_command(ui, repo, **opts): @@ -120,7 +133,6 @@ def _signoff_command(ui, repo, **opts): message = opts.pop('message').strip() - force = opts.pop('force') mdown = opts.pop('mdown') rd = _get_datastore(ui, repo) rcset = rd[opts.pop('rev')] @@ -130,7 +142,7 @@ raise util.Abort(messages.SIGNOFF_OPINION_CONFLICT) opinion = 'yes' if yes else ('no' if no else '') - if rcset.signoffs_for_current_user() and not force: + if rcset.signoffs_for_current_user(): raise util.Abort(messages.SIGNOFF_EXISTS) if not message: @@ -141,7 +153,7 @@ style = mdown and 'markdown' or '' - rcset.add_signoff(message=message, opinion=opinion, force=force, style=style) + rcset.add_signoff(message=message, opinion=opinion, style=style) def _check_command(ui, repo, **opts): rd = _get_datastore(ui, repo) diff -r 0c22712a4c18 -r 01a47fb22911 review/helps.py --- a/review/helps.py Tue Jun 29 20:00:12 2010 -0400 +++ b/review/helps.py Tue Jun 29 20:49:32 2010 -0400 @@ -79,7 +79,7 @@ """ SIGNOFF = r""" -USAGE: hg review --signoff [-m MESSAGE] [--mdown] [--yes | --no] [-r REV] [--force] +USAGE: hg review --signoff [-m MESSAGE] [--mdown] [--yes | --no] [-r REV] If no revision is given the current parent of the working directory will be used. @@ -89,9 +89,6 @@ individual project to decide exactly what that means. If neither option is given the signoff will be marked as "neutral". -If you've already signed off on a changeset you can use ``--force`` to replace -your previous signoff with a new one. - If ``--mdown`` is used the signoff message text will be interpreted as Markdown. Examples:: @@ -99,7 +96,6 @@ hg review --signoff -m 'I do not work on this part of the code.' hg review --signoff --yes -m 'Thanks, this change looks good.' hg review --signoff --no -m 'This would break backwards compatibility!' - hg review --signoff --yes --force -m 'Nevermind, this is fine.' """ diff -r 0c22712a4c18 -r 01a47fb22911 review/messages.py --- a/review/messages.py Tue Jun 29 20:00:12 2010 -0400 +++ b/review/messages.py Tue Jun 29 20:49:32 2010 -0400 @@ -76,7 +76,7 @@ """ SIGNOFF_EXISTS = """\ -you have already signed off on this changeset (use -f to replace)! +you have already signed off on this changeset (use "hg review --edit" to modify)! """ SIGNOFF_EDITOR = """\ diff -r 0c22712a4c18 -r 01a47fb22911 review/templates/pieces/forms/signoff.html --- a/review/templates/pieces/forms/signoff.html Tue Jun 29 20:00:12 2010 -0400 +++ b/review/templates/pieces/forms/signoff.html Tue Jun 29 20:49:32 2010 -0400 @@ -26,6 +26,7 @@ {% if cu_signoff %} + Change Signoff {% else %} Add Signoff diff -r 0c22712a4c18 -r 01a47fb22911 review/tests/test_check.py --- a/review/tests/test_check.py Tue Jun 29 20:00:12 2010 -0400 +++ b/review/tests/test_check.py Tue Jun 29 20:49:32 2010 -0400 @@ -1,45 +1,32 @@ from nose import with_setup from util import setup_reviewed_sandbox, teardown_sandbox, review, should_fail_with +from util import get_identifiers from .. import messages @with_setup(setup_reviewed_sandbox, teardown_sandbox) def test_check_empty(): - review(check=True) - - output = review(check=True, verbose=True) - assert messages.CHECK_SUCCESS in output + def t(rev): + output = review(check=True, rev=rev) + assert not output - review(signoff=True, no=True, message='.') - output = review(check=True, verbose=True) - assert messages.CHECK_SUCCESS in output + output = review(check=True, verbose=True, rev=rev) + assert messages.CHECK_SUCCESS in output - review(signoff=True, yes=False, message='.', force=True) - output = review(check=True, verbose=True) - assert messages.CHECK_SUCCESS in output - - review(comment=True, message='.') - output = review(check=True, verbose=True) - assert messages.CHECK_SUCCESS in output + review(signoff=True, no=True, message='.', rev=rev) + output = review(check=True, verbose=True, rev=rev) + assert messages.CHECK_SUCCESS in output -@with_setup(setup_reviewed_sandbox, teardown_sandbox) -def test_check_empty_non_tip(): - review(rev='0', check=True) - - output = review(rev='0', check=True, verbose=True) - assert messages.CHECK_SUCCESS in output + i = get_identifiers(rev)[0] + review(edit=i, yes=True, rev=rev) + output = review(check=True, verbose=True, rev=rev) + assert messages.CHECK_SUCCESS in output - review(signoff=False, message='.') - output = review(rev='0', check=True, verbose=True) - assert messages.CHECK_SUCCESS in output - - review(signoff=True, message='.') - output = review(rev='0', check=True, verbose=True) - assert messages.CHECK_SUCCESS in output - - review(comment=True, message='.') - output = review(rev='0', check=True, verbose=True) - assert messages.CHECK_SUCCESS in output + review(comment=True, message='.', rev=rev) + output = review(check=True, verbose=True, rev=rev) + assert messages.CHECK_SUCCESS in output + t('.') + t('0') @with_setup(setup_reviewed_sandbox, teardown_sandbox) def test_check_no_nos(): @@ -49,7 +36,8 @@ review(signoff=True, no=True, message='.') should_fail_with(messages.CHECK_HAS_NOS, check=True, verbose=True, no_nos=True) - review(signoff=True, yes=True, message='.', force=True) + i = get_identifiers()[0] + review(edit=i, yes=True) output = review(check=True, verbose=True, no_nos=True) assert messages.CHECK_SUCCESS in output @@ -74,7 +62,8 @@ output = review(check=True, verbose=True, seen=True) assert messages.CHECK_SUCCESS in output - review(signoff=True, no=True, message='.', force=True) + i = get_identifiers()[0] + review(edit=i, no=True) output = review(check=True, verbose=True, seen=True) assert messages.CHECK_SUCCESS in output @@ -91,7 +80,8 @@ should_fail_with(messages.CHECK_HAS_NOS, check=True, verbose=True, no_nos=True, seen=True) should_fail_with(messages.CHECK_HAS_NOS, check=True, verbose=True, no_nos=True, seen=True, yeses='0') - review(signoff=True, yes=True, message='.', force=True) + i = get_identifiers()[0] + review(edit=i, yes=True) output = review(check=True, verbose=True, no_nos=True, seen=True, yeses='0') assert messages.CHECK_SUCCESS in output diff -r 0c22712a4c18 -r 01a47fb22911 review/tests/test_edit.py --- a/review/tests/test_edit.py Tue Jun 29 20:00:12 2010 -0400 +++ b/review/tests/test_edit.py Tue Jun 29 20:49:32 2010 -0400 @@ -26,6 +26,7 @@ should_fail_with(messages.AMBIGUOUS_ID % i, edit=i) + @with_setup(setup_reviewed_sandbox, teardown_sandbox) def test_touch_comment(): def t(rev): @@ -131,3 +132,70 @@ check_comment_exists_on_line(2, files=['always_changing'], rev=rev) t('1') t('0') + + +@with_setup(setup_reviewed_sandbox, teardown_sandbox) +def test_touch_signoff(): + def t(rev): + review(rev=rev, signoff=True, message='test', yes=True) + i = get_identifiers(rev)[0] + + # This sucks, but we need to do it to support testing "touch" edits. + time.sleep(1.1) + + output = review(edit=i) + assert not output + output = review(rev=rev, verbose=True) + assert '(%s)' % i not in output + assert len(get_identifiers(rev)) == 1 + assert messages.REVIEW_LOG_SIGNOFF_LINE % 'test' in output + assert 'yes' in output + t('1') + t('0') + +@with_setup(setup_reviewed_sandbox, teardown_sandbox) +def test_edit_signoff_message(): + def t(rev): + review(rev=rev, signoff=True, message='test') + i = get_identifiers(rev)[0] + + output = review(edit=i, message='edited') + assert not output + output = review(rev=rev, verbose=True) + assert '(%s)' % i not in output + assert len(get_identifiers(rev)) == 1 + assert messages.REVIEW_LOG_SIGNOFF_LINE % 'test' not in output + assert messages.REVIEW_LOG_SIGNOFF_LINE % 'edited' in output + t('.') + t('0') + +@with_setup(setup_reviewed_sandbox, teardown_sandbox) +def test_edit_signoff_opinion(): + def t(rev): + review(rev=rev, signoff=True, message='test') + + output = review(rev=rev, verbose=True) + assert 'as yes' not in output + assert 'as neutral' in output + assert 'as no' not in output + + i = get_identifiers(rev)[0] + output = review(edit=i, yes=True) + assert not output + + output = review(rev=rev, verbose=True) + assert 'as yes' in output + assert 'as neutral' not in output + assert 'as no' not in output + + i = get_identifiers(rev)[0] + output = review(edit=i, no=True) + assert not output + + output = review(rev=rev, verbose=True) + assert 'as yes' not in output + assert 'as neutral' not in output + assert 'as no' in output + t('.') + t('0') + diff -r 0c22712a4c18 -r 01a47fb22911 review/tests/test_signoff.py --- a/review/tests/test_signoff.py Tue Jun 29 20:00:12 2010 -0400 +++ b/review/tests/test_signoff.py Tue Jun 29 20:49:32 2010 -0400 @@ -71,8 +71,6 @@ should_fail_with(messages.SIGNOFF_EXISTS, signoff=True, message='Test signoff two.') - review(signoff=True, message='Test signoff two.', force=True) - output = review() assert messages.REVIEW_LOG_SIGNOFFS % (1, 0, 0, 1) in output diff -r 0c22712a4c18 -r 01a47fb22911 review/tests/util.py --- a/review/tests/util.py Tue Jun 29 20:00:12 2010 -0400 +++ b/review/tests/util.py Tue Jun 29 20:49:32 2010 -0400 @@ -132,12 +132,12 @@ a1, a2 = (messages.REVIEW_LOG_COMMENT_AUTHOR % '|').split('|') -s1, s2 = (messages.REVIEW_LOG_SIGNOFF_AUTHOR % ('|', 'neutral')).split('|') +s1, s2, s3 = (messages.REVIEW_LOG_SIGNOFF_AUTHOR % ('|', '|')).split('|') def get_identifiers(rev='.', files=[]): return [l.split(' ')[-1].strip('()\n') for l in review(rev=rev, verbose=True, args=files).splitlines() - if (a1 in l and a2 in l) or (s1 in l and s2 in l)] + if (a1 in l and a2 in l) or (s1 in l and s2 in l and s3 in l)] COMMENT_LINE_ERROR = '''\ diff -r 0c22712a4c18 -r 01a47fb22911 review/web.py --- a/review/web.py Tue Jun 29 20:00:12 2010 -0400 +++ b/review/web.py Tue Jun 29 20:49:32 2010 -0400 @@ -104,7 +104,7 @@ def _handle_signoff(revhash): - signoff = request.form.get('signoff', None) + signoff = request.form['signoff'] if signoff not in ['yes', 'no', 'neutral']: abort(400) @@ -115,8 +115,12 @@ body = request.form.get('new-signoff-body', '') style = 'markdown' if request.form.get('signoff-markdown') else '' - rcset = g.datastore[revhash] - rcset.add_signoff(body, signoff, force=True, style=style) + current = request.form.get('current') + if current: + g.datastore.edit_signoff(current, body, signoff, style=style) + else: + rcset = g.datastore[revhash] + rcset.add_signoff(body, signoff, style=style) return redirect("%s/changeset/%s/" % (app.site_root, revhash))