launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Needs Fixing code

This branch seems to do a couple of different things: use PreviewDiff.id in place of PreviewDiff.date_created, and add a diff navigation UI. They could probably have been split into two branches for additional clarity.

Regarding the PreviewDiff.id change, would it be better to take a reference to an IPreviewDiff in most places? If you'd really prefer to take an ID, it probably should be called preview_diff_id so as to not be confused with Diff.id.

For the diff navigation, is the server-side ++diff-nav actually valuable? This feature is only going to be usable in the presence of JavaScript anyway, so I'd just render the whole thing client-side. See some of the sharing stuff (eg. granteetable.js) for an example of rendering a nice JS-only UI from the JSON cache. We'll need the client-side rendering later anyway for AJAX updates, so I'd go client-only from the start.


148	+ :param diff_id: the context diff creation ID which will be used to
149	+ retrive the actual `PreviewDiff` register.

"retrive", "register"? This whole sentence could probable do with a bit of rewording.

325	+ var config_published = {
326	+ on: {
327	+ success: function (comments) {
328	+ // Display published inline comments.
329	+ // [{'line_number': <lineno>, 'person': <IPerson>,
330	+ // 'text': <comment>, 'date': <timestamp>}, ...]
331	+ comments.forEach( function (comment) {
332	+ namespace.create_row(comment);
333	+ });
334	+ },
335	+ },
336	+ parameters: {
337	+ diff_id: namespace.current_diff_id,
338	+ }
339	+ };

We're not ES5-only yet, so the trailing commas in these dicts are illegal.

343	+ var config_draft = {
344	+ on: {
345	+ success: function (drafts) {
346	+ namespace.inlinecomments = {};
347	+ if (drafts === null) {
348	+ return;
349	+ }
350	+ // Display draft inline comments:
351	+ // {'<line_number>':''<comment>', ...})
352	+ Object.keys(drafts).forEach( function (line_number) {
353	+ var comment = {
354	+ 'line_number': line_number,
355	+ 'text': drafts[line_number],
356	+ };
357	+ namespace.create_row(comment);
358	+ });
359	+ },
360	+ },
361	+ parameters: {
362	+ diff_id: namespace.current_diff_id,
363	+ }
364	+ };

Likewise here.

430	+ navigator.on('change', function() {
431	+ var diff_id = this.get('value');
432	+ var container = self.get('srcNode').one('.diff-content');

Most of this is duplicated with branchmergeproposal.updater.js. We'll want to factor the common bits out before we go much further.

440	+ failure: function(ignore, response, args) {
441	+ container.set('innerHTML', response.statusText);
442	+ Y.lp.anim.red_flash({node: diff_node}).run();
443	+ },

statusText is text, not HTML. This is an XSS vulnerability if someone figures out a way to control the content of a failure. We probably want to display an error dialog overlay instead.

968	+ 'lp.code.branch.revisionexpander', 'lp.anin',

"anin" should be "anim"?
-- 
https://code.launchpad.net/~cprov/launchpad/diff-navigator/+merge/205689
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.