[PATCH 3 of 7] comment: add draft flag for comments
Jan Heylen
heyleke at gmail.com
Fri Apr 24 17:47:43 EDT 2015
On Fri, Apr 24, 2015 at 7:20 PM, Mads Kiilerich <mads at kiilerich.com> wrote:
> On 04/14/2015 06:03 PM, Jan Heylen wrote:
>>
>> # HG changeset patch
>> # User Jan Heylen <heyleke at gmail.com>
>> # Date 1427735588 -7200
>> # Mon Mar 30 19:13:08 2015 +0200
>> # Node ID f48bc03627761dd62f7e7094bb0d322834da336c
>> # Parent 55ba8e68269da8fb0c33c65711a57b4fd9e1b675
>> comment: add draft flag for comments
>>
>> diff -r 55ba8e68269d -r f48bc0362776 kallithea/model/comment.py
>> --- a/kallithea/model/comment.py Fri Mar 27 16:13:46 2015 +0100
>> +++ b/kallithea/model/comment.py Mon Mar 30 19:13:08 2015 +0200
>> @@ -165,7 +165,7 @@
>> def create(self, text, repo, user, revision=None,
>> pull_request=None,
>> f_path=None, line_no=None, status_change=None,
>> closing_pr=False,
>> - send_email=True):
>> + send_email=True, draft=False):
>> """
>> Creates new comment for changeset or pull request.
>> If status_change is not None this comment is associated with a
>> @@ -194,7 +194,7 @@
>> comment.text = text
>> comment.f_path = f_path
>> comment.line_no = line_no
>> - comment.draft = False
>> + comment.draft = draft
>> if revision:
>> comment.revision = revision
>> @@ -207,7 +207,7 @@
>> Session().add(comment)
>> Session().flush()
>> - if send_email:
>> + if (not draft) and send_email:
>> (subj, body, recipients, notification_type,
>> email_kwargs) = self._get_notification_data(
>> repo, comment, user,
>> @@ -250,7 +250,64 @@
>> return comment
>> - def get_comments(self, repo_id, revision=None, pull_request=None):
>> + def draft_to_comment(self, repo, draft, user, status_change,
>> closing_pr):
>> + draft = self.__get_changeset_comment(draft)
>> + draft.draft = False
>> + (subj, body, recipients, notification_type,
>> + email_kwargs) = self._get_notification_data(
>> + repo, draft, user,
>> + comment_text=draft.text,
>> + line_no=draft.line_no,
>> + revision=draft.revision,
>> + pull_request=draft.pull_request,
>> + status_change=status_change,
>> + closing_pr=closing_pr)
>> +
>> + mentions =
>> set(self._extract_mentions(body)).difference(recipients)
>> +
>> + return subj, body, recipients, notification_type, email_kwargs,
>> mentions
>> +
>> + def commit_drafts(self, repo_id, user_id, revision=None,
>> pull_request=None, status_change=None):
>> + repo = self._get_repo(repo_id)
>> + user = self._get_user(user_id)
>> + bodies = []
>> + mention_recipients = set()
>> +
>> + drafts = self.get_inline_drafts(repo_id, revision=revision,
>> pull_request=pull_request, user_id=user_id)
>> + for __,lines in drafts:
>> + for draftlist in lines.values():
>> + for draft in draftlist:
>> + (subj, body, recipients, notification_type,
>> + email_kwargs, mentions) =
>> self.draft_to_comment(repo, draft, user, None, False)
>> + bodies.append(body)
>> + mention_recipients.update(mentions)
>> + gen_drafts = self.get_comments(repo_id, revision=revision,
>> pull_request=pull_request, user_id=user_id, draft=True)
>> + for draft in gen_drafts:
>> + (subj, body, recipients, notification_type,
>> + email_kwargs, mentions) = self.draft_to_comment(repo, draft,
>> user, status_change, False)
>> + bodies.append(body)
>> + mention_recipients.update(mentions)
>> +
>> + if email_kwargs:
>> + email_kwargs['is_mention'] = False
>> + # create notification objects, and emails
>> + NotificationModel().create(
>> + created_by=user_id, subject=subj, body=None,
>> bodies=bodies,
>> + recipients=recipients, type_=notification_type,
>> + email_kwargs=email_kwargs)
>> +
>> + if mention_recipients:
>> + email_kwargs['is_mention'] = True
>> + subj = _('[Mention]') + ' ' + subj
>> + NotificationModel().create(
>> + created_by=user_id, subject=subj, body=None,
>> bodies=bodies,
>> + recipients=mention_recipients,
>> + type_=notification_type,
>> + email_kwargs=email_kwargs
>> + )
>> + return drafts
>
>
> There seems to be a lot of duplication of existing code here. Could we start
> with a refactoring that extract methods for writing/updating a comment and
> for sending mails? At first without considering draftness and only used with
> lists of one.
Yes, true, my approach was the other way around, first add feature,
see how the code works with that, and then consider refactor. But
you're right, in this module, there is alot of unneeded duplication
and different interfaces to accomplish the same thing. It's like they
had an interface, and needed something extra, didn't change the
original interface, but just created another one.
Maybe my approach must be: first add feature, see how the code works,
remove feature, do refactor, add feature in the clean code,
I'll consider pre-patching this patch with a refactor of this code.
(Depending on how far you want to take the model/code redesign thing
you mentioned earlier)
>
>> +
>> + def get_comments(self, repo_id, revision=None, pull_request=None,
>> user_id=None, draft=False):
>> """
>> Gets main comments based on revision or pull_request_id
>> @@ -270,6 +327,12 @@
>> q = q.filter(ChangesetComment.pull_request == pull_request)
>> else:
>> raise Exception('Please specify revision or pull_request')
>> +
>> + q = q.filter(ChangesetComment.draft == draft)
>> +
>> + if user_id:
>> + q = q.filter(ChangesetComment.user_id == user_id)
>
>
> I guess this touches upon the question: how private are draft comments? Does
> it make sense to ask for draft comments without specifying a user_id?
Open question, e.g. admins should be able to see all drafts too? I
didn't wanted to exclude that choice at this level in the code, this
module should be able to do both.
>
>> +
>> q = q.order_by(ChangesetComment.created_on)
>> return q.all()
>> @@ -278,6 +341,7 @@
>> .filter(ChangesetComment.repo_id == repo_id)\
>> .filter(ChangesetComment.line_no != None)\
>> .filter(ChangesetComment.f_path != None)\
>> + .filter(ChangesetComment.draft == False)\
>> .order_by(ChangesetComment.comment_id.asc())\
>> if revision:
>> @@ -295,3 +359,31 @@
>> for co in comments:
>> paths[co.f_path][co.line_no].append(co)
>> return paths.items()
>> +
>> +
>> + def get_inline_drafts(self, repo_id, revision=None,
>> pull_request=None, user_id=None):
>
>
> Docstring, please. What is the intention with this function?
Duplication of get_inline_comments, but I'll (after refactor) add docs
>
>
>> + q = Session().query(ChangesetComment)\
>> + .filter(ChangesetComment.repo_id == repo_id)\
>> + .filter(ChangesetComment.line_no != None)\
>> + .filter(ChangesetComment.f_path != None)\
>> + .filter(ChangesetComment.draft == True)\
>> + .order_by(ChangesetComment.comment_id.asc())\
>> +
>> + if revision:
>> + q = q.filter(ChangesetComment.revision == revision)
>> + elif pull_request:
>> + pull_request = self.__get_pull_request(pull_request)
>> + q = q.filter(ChangesetComment.pull_request == pull_request)
>> + else:
>> + raise Exception('Please specify revision or pull_request_id')
>> +
>> + if user_id:
>> + q = q.filter(ChangesetComment.user_id == user_id)
>> +
>> + comments = q.all()
>> +
>> + paths = defaultdict(lambda: defaultdict(list))
>> +
>> + for co in comments:
>> + paths[co.f_path][co.line_no].append(co)
>> + return paths.items()
>
>
More information about the kallithea-general
mailing list