Code review comment for lp://staging/~gary/launchpad/direct-personal-subscription-actions

Revision history for this message
Benji York (benji) wrote :

This branch looks good.

I like the intent behind prefixing the errors that shouldn't happen with
"Programmer error". I wonder if we use that phrasing other places. It
seems to me that something like "Internal Launchpad error" might
communicate better in the rare chance that one of those are generated.

I searched the code and found that we currently use "Programmer error"
in one place and "Launchpad [internal] error" in five places. I don't
feel strongly about it but thought I would bring it up.

The code to make the border box is a little redundant. Here's a patch
that will DRY it up for this branch: http://pastebin.ubuntu.com/596022/
We should consider moving this to a place the structural subscription
code can use it to build its border boxes.

unsubscribe_action() and unsubscribe_with_warning_action() need the
constantification treatment. (I've included this in my patch.)

The patch also fixes this lint:

./lib/lp/bugs/javascript/subscription.js
     700: Expected ';' and instead saw 'function'.
     980: 'i' is already defined.
     896: Line exceeds 78 characters.
     911: Line exceeds 78 characters.

The lib/lp/bugs/javascript/tests/test_subscription.html tests pass with
the patch applied.

review: Approve (code)

« Back to merge proposal