launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code

10	+ } else if (Y.Lang.isObject(value)) {
11	+ elems.push(enc(key) + "=" + Y.JSON.stringify(value));
12	+ } else {

I don't think the output of Y.JSON.stringify is necessarily safe to include directly in a query string value. You probably want to enc() it; I'd try comments containing +, &, =, etc.

31	+ this.publish_inline_comments = Y.one(
32	+ '[id="field.publish_inline_comments"]');

What will happen to the bits that touch this node if the feature flag is disabled?

306	def createComment(owner, subject, content=None, vote=None,
307	- review_type=None, parent=None):
308	+ review_type=None, parent=None,
309	+ publish_inline_comments=False):

I'm not a huge fan of this publish_inline_comments flag; I'd prefer to have an inline_comments argument taking a dict directly. That would reduce race conditions, leave CRICD isolated to the frontend JS, and make API clients and tests cleaner (you could probably get rid of the two new LaunchpadObjectFactory methods, for example). An API client could manipulate drafts directly if it wanted draft functionality, but I don't think anyone would ever desire that.

Internally pulling the comments out of CRICD also raises a fairly serious issue: which preview diff is used by createComment? In the current implementation, the flag will be ignored if the diff updates while being reviewed, as there will be no draft to publish from. So I'd replace the publish_inline_comments flag with args for diff_timestamp and an inline_comments dict. diff_timestamp should always be set by the webapp because we can, but we can't make it mandatory for existing API clients, so I'd only require it when inline_comments are provided.

We also need to consider how longpoll MP diff updates interact with comments, but that's best left for a subsequent branch.

329	+ def getInlineComments(person):
330	+ """Return the set of inline comments related to this MP.
331	+
332	+ The return value is a list of 4-tuples representing published and
333	+ draft inline comments.

Similar to my previous CRICD suggestion, I don't think it's a good idea to conflate published comments and incomplete drafts here. I'd rather have two separate methods: one returning published comments like {line: [{person: person, comment: comment, date: date}]}, and another returning drafts like {line: comment}. This lets you drop the person arg from getInlineComments and findByPreviewDiff, and findByPreviewDiff becomes much simpler. The client code possibly becomes simpler too: it doesn't have to distinguish editability etc. between the two types of 4-tuples, as it can just make two passes instead.

510	- row.line, row.person, row.comment, row.date);
511	+ row[0], row[1], row[2], row[3]);

I feel ill! Another reason to have getInlineComments return {line: [{person: person, comment: comment, date: date}]} :)

679	+ comments = Unicode()

This should probably be JSON().

690	+ comments = Unicode()

Likewise.

731	+ cric.previewdiff = cricd.previewdiff
732	+ # XXX cprov 20140114: why the hell cricd.person is empty here ?
733	+ cric.person = person

This needs to be looked at again. Are you sure it's that cricd.person is empty, and not rather that accessing cricd.person after setting cric.previewdiff forces the cric to be flushed, violating the primary key constraint because cric.person isn't set yet?
-- 
https://code.launchpad.net/~cprov/launchpad/cric-api-take2/+merge/200563
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cprov/launchpad/cric-api-take2 into lp:launchpad.