Code review comment for lp://staging/~adeuring/launchpad/api-query-permissions-on-object

Revision history for this message
Abel Deuring (adeuring) wrote :

On 19.04.2011 00:48, Robert Collins wrote:
[...]
>>From this I suggest the following options:
> - either returning the needed information from the set packaging call
> (perhaps by using a custom call to do what you need). More dev time
> needed, but no additional round trips and no overhead on API GET calls
> for other code paths.

That would indeed be the most efficient option. But it feels wrong to me
to return such data from Sourcepackage.setPackaging(): The corresponding
method ProductSeries.setPackaging() returns the packaging itself. While
we already have an inconsistency (Sourcepackage.setPackaging() returns
None at present), I think we should not let the method return something
that is tailored to be useful as the result of a webservice request for
a special web page. This return value would not make any sense if for
example the "+edit-packaging" view for source package would be AJAXified.

> - look into being able to get a fresh jsoncache from an API call
> (variant of the first option) - specific to a view, will include all
> the right data, easy to keep consistent with the scripts on a page

We could add something like
SourcePackage.setPackagingInTranslationSettingPage(), which just calls
setPackaging() and returns the permission information needed on the
"+sharing-details" page. This looks still a bit odd in model code, but
we don't export anything else than model classes in the webservice.

> - have a dedicated api call to check what you need - one extra
> roundtrip, but nicely isolated and reusable

Considering that the view we are talking about is not intended to be
called very frequently (once you got the translation settings right you
should no longer change them, otherwise your translators might go
ballistic...), this seems the most reasonable suggestion to me.

So, what about a mixin class with just one method, like so:

class PermissionCheck:

    def userHasPermissionsFor(
        self, access_attributes, write_attributes):

access_attributes and write_attributes would be sequences of attribute
names; the method returns True if the current user has access/write
permissions for all given attributes.

In our case, this will still require _two_ request, because we need to
check permissions for one Product attribute and for two ProductSeries
Attributes, but as written above, we are not talking about a page that
is supposed to be used 100 times per day by a single user.

« Back to merge proposal