Transitioning to a new status using the API is unintuitive

Bug #302836 reported by Martin Pitt on 2008-11-27
8
Affects Status Importance Assigned to Milestone
Launchpad itself
Undecided
Leonard Richardson

Bug Description

I tried to change a bug status with launchpadlib. My first, and IMHO intuitive approach:

>>> b = launchpad.bugs['89040']
>>> b.bug_tasks[0].status
u'Invalid'
>>> b.bug_tasks[0].status = 'Confirmed'
>>> b.lp_save()
>>>

However, that didn't help, bug 89040 stayed at "Invalid".

After consulting Gavin on IRC, he told me that this needs transitionToStatus():

>>> b.bug_tasks[0].transitionToStatus('Confirmed')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __call__() takes exactly 1 argument (2 given)

the first implicit argument is "self", and certainly it needs the new status? help() only brings some boilerplate, and https://help.launchpad.net/API/launchpadlib says the function takes the new status.

Since this isn't going to change, we need to:

 * Improve the documentation
 * Throw more informative errors

Gavin Panella (allenap) wrote :

I figured it out :)

It should be:

>>> b.bug_tasks[0].transitionToStatus(status='Confirmed')

Iirc, all the methods in the LP API like this one take keyword
arguments only. We definitely need to make that more obvious, and we
should try to give better errors in launchpadlib for this kinds of
things.

Fixing this bug therefore requires that the documentation and error
reporting is made clearer.

Changed in launchpadlib:
status: New → Confirmed
Markus Korn (thekorn) wrote :

This might be related to Bug 270096 "launchpadlib methods do not support positional parameters"

Francis J. Lacoste (flacoste) wrote :

The way status is exported (via a mutator) instead of an attribute is part of how the bugs api is done.

I'm reassigning this to the Bugs team. There is an infrastructure bug 210265 which tracks the underlying issue to make it easy to export mutators as attribute

Graham Binns (gmb) wrote :

Retargetting this to launchpadlib.

Changed in malone:
status: Confirmed → Triaged
Graham Binns (gmb) wrote :

And I didn't see Francis' last comment so I'm moving it back ;)

The way this is done is intentional. transitionToStatus does more than just set the status, and it's good to be explicit about it.

If we want to change this API, we need to discuss it more.

Changed in malone:
status: Triaged → Incomplete

Actually, I think Gavin's comment summarizes what is doable, so i'm setting it back to Triaged and change the description.

Changed in malone:
status: Incomplete → Triaged

b.t.w I wonder if we could somehow make launchpadlib indicate that this property is unsettable

description: updated

Tom Berger [2008-11-27 20:29 -0000]:
> b.t.w I wonder if we could somehow make launchpadlib indicate that this
> property is unsettable

I might be naive about it, but since this is a property anyway, why
can't the setter just be made to call transitionToStatus()?

> I might be naive about it, but since this is a property anyway, why
> can't the setter just be made to call transitionToStatus()?

There's no technical limitation. The API makes it explicit to the consumer.

Markus Korn (thekorn) wrote :

The attached branch has the easiest fix for launchpadlib I could find. I don't feel 100% positive about it, but it works. I personally like the workflow of doing the changes locally and submitting them at once by calling .lp_save() more, but I was not able to implement this in a sane way.

Example:
In [10]: b = launchpad.bugs[88102]

In [11]: t = b.bug_tasks[0]

In [12]: t.status
Out[12]: u'New'

In [13]: t.importance
Out[13]: u'Medium'

In [14]: t.assignee

In [15]: t.status = "Confirmed"

In [16]: t.status
Out[16]: u'Confirmed'

In [17]: t.importance = "High"

In [18]: t.importance
Out[18]: u'High'

In [19]: t.assignee = launchpad.me

In [20]: t.assignee
Out[20]: <person at https://api.staging.launchpad.net/beta/~thekorn>

Any comments, critique, ideas?

> The attached branch has the easiest fix for launchpadlib

Thanks! I do think, however, that if we want to change it, it should be done in the Launchpad API, not in launchpadlib.

Also, I'm still not convinced that this is such a bad API - we need to document it better, for sure, but I the transitionToSomething idiom is prevalent in the API, and it mirrors the way Launchpad works internally, which is something we want for the API very much. If we were to make the status property settable, the only opportunity to check for permissions and throw errors, for example, would be at save time, and that would result in an awkward interface.

Markus Korn [2008-11-27 23:13 -0000]:
> I personally like the
> workflow of doing the changes locally and submitting them at once by
> calling .lp_save() more

Right, me too. Please stay consistent about it. Consistency over
everything! The current way of some properties being writable, and
some being writable, but silently forgetting (like .status) is bad
enough. Now, having some changes which require lp_save() and some
which don't will turn an API user's life into hell, really. It will
cause delays or potential network errors where they aren't expected,
and don't allow for rolling back changes by not calling lp_save().

Leonard Richardson (leonardr) wrote :

Regardless of what happens behind the scenes when you change status, either the status changes or you get an error. Even if the side effects were important enough to mention, "transitionToStatus" doesn't mention them. So there's no reason why PATCHing a new value for status can't invoke transitionToStatus on the server side, except it's not possible in our framework.

I've implemented a branch that makes it possible, and makes the change for transitionToStatus and transitionToImportance.

Changed in malone:
assignee: nobody → leonardr
status: Triaged → In Progress
Leonard Richardson (leonardr) wrote :

I've made the framework change that makes it possible to modify a field directly even when its behind-the-scenes mutator is a custom method. I've made the change for bugtask status, importance, and assignee. Now it's up to the other teams to use this whenever it makes sense.

Changed in malone:
status: In Progress → Fix Committed
Changed in malone:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers