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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
