Launchpad itself

Code review comment for lp:~wgrant/launchpad/export-structural-subscriptions-bug-392515

  1. export-structural-subscriptions-bug-392515
  2. Merge into devel
Revision history for this message
William Grant (wgrant) wrote :

Hi Gavin,

On Wed, 2009-09-09 at 13:35 +0000, Gavin Panella wrote:
> Review: Approve code
> Hi William,
>
> This is a very nice patch. I have a few questions/comments, but they're small.

Thanks for the review.

As discussed on IRC, I've adjusted the parameter descriptions to
indicate the behaviour of omitted arguments.

At intellectronica's suggestion, I've also unexported
ISST.parent_subscription_target for now (it is considered an
implementation detail, and we can do something much nicer later).

Diff from 9230 to 9233 attached.

9230..9233.diff
1=== modified file 'lib/canonical/launchpad/interfaces/structuralsubscription.py'
2--- lib/canonical/launchpad/interfaces/structuralsubscription.py 2009-08-25 11:26:58 +0000
3+++ lib/canonical/launchpad/interfaces/structuralsubscription.py 2009-09-09 14:25:50 +0000
4@@ -160,10 +160,8 @@
5 :return: A sequence of `IStructuralSubscription`.
6 """
7
8- parent_subscription_target = exported(Reference(
9- schema=Interface, # IStructuralSubscriptionTarget
10- required=True, readonly=True,
11- title=_("The target's parent, or None if one doesn't exist.")))
12+ parent_subscription_target = Attribute(
13+ "The target's parent, or None if one doesn't exist.")
14
15 def addSubscription(subscriber, subscribed_by):
16 """Add a subscription for this structure.
17@@ -171,14 +169,19 @@
18 This method is used to create a new `IStructuralSubscription`
19 for the target, with no levels set.
20
21- :subscriber: The IPerson who will be subscribed.
22+ :subscriber: The IPerson who will be subscribed. If omitted,
23+ subscribed_by will be used.
24 :subscribed_by: The IPerson creating the subscription.
25 :return: The new subscription.
26 """
27
28 @operation_parameters(
29 subscriber=Reference(
30- schema=IPerson, title=_('Person'), required=False))
31+ schema=IPerson,
32+ title=_(
33+ 'Person to subscribe. If omitted, the requesting user will be'
34+ ' subscribed.'),
35+ required=False))
36 @call_with(subscribed_by=REQUEST_USER)
37 @export_factory_operation(IStructuralSubscription, [])
38 def addBugSubscription(subscriber, subscribed_by):
39@@ -188,14 +191,19 @@
40 for the target with the bug notification level set to
41 COMMENTS, the only level currently in use.
42
43- :subscriber: The IPerson who will be subscribed.
44+ :subscriber: The IPerson who will be subscribed. If omitted,
45+ subscribed_by will be used.
46 :subscribed_by: The IPerson creating the subscription.
47 :return: The new bug subscription.
48 """
49
50 @operation_parameters(
51 subscriber=Reference(
52- schema=IPerson, title=_('Person'), required=False))
53+ schema=IPerson,
54+ title=_(
55+ 'Person to unsubscribe. If omitted, the requesting user will '
56+ 'be unsubscribed.'),
57+ required=False))
58 @call_with(unsubscribed_by=REQUEST_USER)
59 @export_write_operation()
60 def removeBugSubscription(subscriber, unsubscribed_by):
61@@ -205,7 +213,8 @@
62 set the subscription's `bug_notification_level` to
63 `NOTHING`, otherwise, destroy the subscription.
64
65- :subscriber: The IPerson who will be subscribed.
66+ :subscriber: The IPerson who will be unsubscribed. If omitted,
67+ unsubscribed_by will be used.
68 :unsubscribed_by: The IPerson removing the subscription.
69 """
70

« Back to merge proposal