Merge lp:~wallyworld/launchpad/improved-broken-link-handling into lp:launchpad

Just a note, not a review: why so much 2009 and so little 2010 in the copyright notices? :-)

There's only one real instance, due to a cut'n'paste :-) The other one
was a file that was deleted and then reverted and bzr has treated it as
a delete and then an add, so it is a false positive.

On 21/10/10 19:47, Jeroen T. Vermeulen wrote:
> Just a note, not a review: why so much 2009 and so little 2010 in the copyright notices? :-)

This is a nice piece of functionality. I have a lot of comments but
most if not all of them are trivial or minor. I'm going to ask someone
else who has more up-to-date experience with Javascript in Launchpad
to give this a look too.

[1]

+ <browser:page
+ for="*"
+ name="+check-links"
+ class="lp.app.browser.linkchecker.LinkCheckerAPI"
+ permission="zope.Public"
+ />

Although the indentation is inconsistent in that file, and although it
doesn't matter very much, can you make this consistent with what's
already there?

[2]

+from zope.component._api import getUtility

getUtility is normally imported directly from zope.component.

[3]

+from zope.component._api import getUtility
+
+from lp.code.interfaces.branchlookup import IBranchLookup
+
+__metaclass__ = type
+
+__all__ = [
+ 'LinkCheckerAPI',
+ ]
+
+import simplejson

I think __metaclass__ and __all__ normally appear first in Launchpad
source, followed by imports. I think utilities/format-imports expects
that too (and you should also run this on your changed files, or use
utilities/format-new-and-modified-imports -r submit: as a convenience).

[4]

+ """ Validates Launchpad shortcut links.

s/" V/"V/

[5]

+ result['invalid_'+link_type]=invalid_links

s/[+=]/ \1 /

[6]

+ path = link.strip('/')[len('+branch/'):]

Is it possible that the branch link does not begin with +branch? I'm
not used to seeing branches with +branch in their URLs.

[7]

+ try:
+ branch_lookup.getByLPPath(path)
+ except:
+ invalid_links.append(link)

Don't use default except clauses like this. Catch Exception if
absolutely necessary, or, preferably, catch just those exceptions that
could be raised.

[8]

+ return '<a href="%s" class="%s">%s</a>%s' % (
                 cgi.escape(url, quote=True),
+ "branch-short-link",

Just chuck "branch-short-link" into the format string rather than
interpolate.

[9]

+ self.assertEqual(len(invalid_links), len(links_to_check))
+ self.assertEqual(set(invalid_links), set(links_to_check))

Only the second test is needed. In fact, if the first test fails the
error message will be less informative than if the second fails (which
it will if the first fails).

[10]

+ def invoke_branch_link_checker(
+ self, valid_branch_urls=dict(), invalid_branch_urls=dict()):

Default arguments must not be mutable. Use None as a default then
check for that in the method.

[11]

Is it likely that we hit any query length problems when requesting the
status of a page with many links?

[12]

+ // Get any links of the specified link_class and put store them as the
+ // specified link_type in the specified links_holder

s/put store/store/

[13]

+function harvest_links(Y, links_holder, link_class, link_type) {

I think this function would be cleaner if it returned the link_info
array instead of putting it into links_holder. That way it need only
accept two arguments, Y and link_class. Callers should check if the
returned array is empty.

[14]

Also, given [13] and the presence of the YUI collection module,
harvest_links() could be much shorter ...

Hi

Thanks for the most excellent review. It's the most thorough one I've
had. I've addressed most of the issues except for the one item - see my
comments. The import issues were due to the "curse" of having an IDE
that auto imports missing references and it guessed wrong. A small price
to pay given all the other great stuff it does.

>
> I think __metaclass__ and __all__ normally appear first in Launchpad
> source, followed by imports. I think utilities/format-imports expects
> that too (and you should also run this on your changed files, or use
> utilities/format-new-and-modified-imports -r submit: as a convenience).
>

Thanks for the tip - I had no idea those scripts existed.

>
> [6]
>
> + path = link.strip('/')[len('+branch/'):]
>
> Is it possible that the branch link does not begin with +branch? I'm
> not used to seeing branches with +branch in their URLs.
>

No, all branch links processed by this new functionality begin with
+branch. See line 276 of stringformatter.py:

    url = '/+branch/%s' % path

>
> [11]
>
> Is it likely that we hit any query length problems when requesting the
> status of a page with many links?
>

I don't think so? A post operation is used so I thought that would have
it covered?

>
> [13]
>
> +function harvest_links(Y, links_holder, link_class, link_type) {
>
> I think this function would be cleaner if it returned the link_info
> array instead of putting it into links_holder. That way it need only
> accept two arguments, Y and link_class. Callers should check if the
> returned array is empty.
>

The code has been written so support processing for than just one type
of link. At the moment, there's only branch links. But there will also
be bugs links etc. So the code will look like:

var links_to_check = {}

harvest_links(Y, links_to_check, 'branch-short-link', 'branch_links');
harvest_links(Y, links_to_check, 'bug-link', 'bugs_links');
etc

So we are building a dict mapping link_type_name->[hrefs]

The review comment would be valid if we only ever were going to process
the one type of link. I think the implementation as written is ok given
the above explanation?

>
> [14]
>
> Also, given [13] and the presence of the YUI collection module,
> harvest_links() could be much shorter and probably quicker:
>
> return Y.Array.unique(
> Y.all('.' + link_class).get('href'));
>
> To get the collection module you'll need to add the following in the
> right place (I added it at line 108) in base-layout-macros.pt:
>
> <script type="text/javascript"
> tal:attributes="src string:${yui}/collection/collection.js"></script>
>

I think Array.unique() is only available in YUI 3.2 and at the time of
writing we were not using that version yet. I originally tried to use
Array.unique() only to be disappointed :-(

>
> [15]
>
> + alert(msg);
>
> Really? Just checking :)

I wanted to give the user feedback why the "link" they just clicked on
didn't seem to work instead of just swallowing the onclick().

> [16]
>
> In TestBranchBugLinks (which should probably be renamed), instead of
> creating a new question via the browser, consider creating a new
> question...

> Thanks for the most excellent review. It's the most thorough one
> I've had.

I hope it was useful...

I often do quite thorough reviews and I worry that I'm being too
thorough. Let me know if you ever feel that my reviews are
demotivating or too picky. That would mean I'm doing it wrong :)

[14]

> I think Array.unique() is only available in YUI 3.2 and at the time
> of writing we were not using that version yet. I originally tried to
> use Array.unique() only to be disappointed :-(

Okay. Fwiw, this should work now in Launchpad, though it's confusing
because Array.unique() is a static function (or whatever YUI calls
it), and is not available on instances of Array.

[19]

> *news flash* - problem "solved" wtf. By changing the test to
> creating a bug instead of a question, it works in windmill. Go
> figure. But it works now \o/

Hurrah! And weird. But mostly hurrah :)

On 22/10/10 19:11, Gavin Panella wrote:
> Review: Approve
>> Thanks for the most excellent review. It's the most thorough one
>> I've had.
>
> I hope it was useful...
>

Extremely. Thanks. Especially since I'm a new to lp and have figured
stuff out mostly by trial and error or by looking at what others have
done or reverse engineering etc.

> I often do quite thorough reviews and I worry that I'm being too
> thorough. Let me know if you ever feel that my reviews are
> demotivating or too picky. That would mean I'm doing it wrong :)
>
>

On the contrary, I see code reviews as a great learning opportunity,
regardless of how experienced the code author may or may not be. A 2nd
set of eyes always picks up issues that the author is often too close to
the code to see. Plus they are a great way to help share knowledge
across team boundaries.