Launchpad itself

Merge lp://staging/~gary/launchpad/direct-personal-subscription-actions into lp://staging/launchpad

  1. direct-personal-subscription-actions
  2. Merge into devel
Proposed by Gary Poster
Status: Merged
Approved by: Gary Poster
Approved revision: no longer in the source branch.
Merged at revision: 12884
Proposed branch: lp://staging/~gary/launchpad/direct-personal-subscription-actions
Merge into: lp://staging/launchpad
Prerequisite: lp://staging/~gary/launchpad/direct-personal-subscription-actions-scaffolding
Diff against target: 0 lines
To merge this branch: bzr merge lp://staging/~gary/launchpad/direct-personal-subscription-actions
Reviewer Review Type Date Requested Status
Benji York (community) code Approve
Review via email: mp+58216@code.staging.launchpad.net

Commit message

[bug=728370] [r=benji][incr] another incremental step towards bug 728370. Tweaks the direct-subscription presentation code benji has added to make it possible to divide up the actions for unmuting, subscribing, and changing a description into smaller parts.

Description of the change

This branch is another incremental step towards bug 728370. It tweaks the direct-subscription presentation code benji has added to make it possible to divide up the actions for unmuting, subscribing, and changing a description into smaller parts, that are correctly identified as decreasing or increasing the number of potential emails that might be sent.

I did not tweak the look of the code; further branches will provide the actions, make it look pretty, and maybe describe the current level of subscription for direct subscriptions.

This branch works from ~gary/launchpad/direct-personal-subscription-actions-scaffolding, which is my conflict resolution of a merge from devel into ~benji/launchpad/direct-personal-subscription-actions-scaffolding. I'm hoping Benji (hi, Benji :-D ) will be willing to use my conflict resolution branch to merge his own work--or we can just land this one to get all of the pertinent code in the tree.

Thank you

Gary

To post a comment you must log in.
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)
Revision history for this message
Gary Poster (gary) wrote :

Thank you, and thank you for the patch! I applied and will submit.

I think you are right that we should formalize on one of the phrasings. I suspect the other phrasing is better, but I'm not going to investigate that now.

Preview Diff

Empty