8:59 p.m.
Hello.
It is channges for the dispatcher.py. I added the function remove_user().
=== modified file 'pkgdb/dispatcher.py'
--- pkgdb/dispatcher.py 2009-02-27 15:58:55 +0000
+++ pkgdb/dispatcher.py 2009-03-13 01:20:10 +0000
@@ -1397,3 +1397,95 @@
identity.current.user, [clone_branch])
return dict(pkglisting=clone_branch)
+
+ @expose(allow_json=True)
+ # Check that the requestor is in a group that could potentially set ACLs.
+ @identity.require(identity.not_anonymous())
+ def remove_user(self, pkg_name, username, collectn_list=None):
+ '''Remove users from a package.
+ :arg pkg_name: Name of the package
+ :arg username: Name of user to remove from the package
+ :arg collectn_list: list of collections like 'F-10', 'devel'
+ '''
+ person = fas.person_by_username(username)
+ if not person:
+ return dict(status=False,
+ message='Specified user name %s does not have a' \
+ ' Fedora Account' % username)
+ try:
+ # pylint: disable-msg=E1101
+ pkg = Package.query.filter_by(name=pkg_name).one()
+ except InvalidRequestError:
+ return dict(status=False, message='Package %s does not exist' %
pkg_name)
+
+ # Check that the current user is allowed to change acl statuses
+ approved = self._user_can_set_acls(identity, pkg)
+ if not ident.in_group('cvsadmin'):
+ return dict(status=False, message=
+ '%s is not allowed to remove user from the package' %
+ identity.current.user_name)
+
+ log_msgs = []
+
+ if collectn_list:
+ for simple_name in collectn_list:
+ try:
+ collectn = Collection.by_simple_name(simple_name)
+ except InvalidRequestError:
+ return dict(status=False, message='Collection %s does not exist' %
simple_name)
+
+ pkg_listing = PackageListing.query.filter_by(packageid=pkg.id,
+ collectionid=collectn.id).one()
+
+ acls = PersonPackageListingAcl.query.filter(and_(
+ PersonPackageListingAcl.c.personpackagelistingid
+ == PersonPackageListing.c.id,
+ PersonPackageListing.c.packagelistingid == pkg_listing.id,
+ PersonPackageListing.c.username ==
person['username'])).all()
+
+ for acl in acls:
+ person_acl = self._create_or_modify_acl(pkg_listing,
person['id'], acl, self.obsoleteStatus)
+
+ log_msg = u'%s has set the %s acl on %s (%s %s) to Obsolete for
%s' % (
+ identity.current.user_name, acl, pkg.name,
+ pkg_listing.collection.name,
pkg_listing.collection.version,
+ person['username'])
+ log = PersonPackageListingAclLog(identity.current.user.id,
+ self.obsoleteStatus.statuscodeid, log_msg)
+ log.acl = person_acl # pylint: disable-msg=W0201
+ log_msgs.append(log_msg)
+
+ else:
+ for pkg_listing in pkg.listings:
+ acls = PersonPackageListingAcl.query.filter(and_(
+ PersonPackageListingAcl.c.personpackagelistingid
+ == PersonPackageListing.c.id,
+ PersonPackageListing.c.packagelistingid == pkg_listing.id,
+ PersonPackageListing.c.username ==
person['username'])).all()
+
+ for acl in acls:
+ person_acl = self._create_or_modify_acl(pkg_listing,
person['id'], acl, self.obsoleteStatus)
+
+ log_msg = u'%s has set the %s acl on %s (%s %s) to Obsolete for
%s' % (
+ identity.current.user_name, acl, pkg.name,
+ pkg_listing.collection.name,
pkg_listing.collection.version,
+ person['username'])
+ log = PersonPackageListingAclLog(identity.current.user.id,
+ self.obsoleteStatus.statuscodeid, log_msg)
+ log.acl = person_acl # pylint: disable-msg=W0201
+ log_msgs.append(log_msg)
+
+ try:
+ session.flush()
+ except SQLError, e:
+ # An error was generated
+ return dict(status=False,
+ message='Not able to change acl %s on %s with status %s' \
+ % (acl, pkgid, self.obsoleteStatus.statusname))
+
+ # Send a log to people interested in this package as well
+ self._send_log_msg('\n'.join(log_msgs), '%s had acl change
status' % (
+ pkg.name), identity.current.user, pkg.listings,
+ other_email=(person['email'],))
+
+ return dict(status=True)
12:05 a.m.
New subject: tiket #72
Very nice work! I have a few comments; mostly based on new coding
styles that we're enforcing in new code but haven't made it into old
code yet.
Dmitry Kolesov wrote:
Hello.
It is channges for the dispatcher.py. I added the function remove_user().
=== modified file 'pkgdb/dispatcher.py'
--- pkgdb/dispatcher.py 2009-02-27 15:58:55 +0000
+++ pkgdb/dispatcher.py 2009-03-13 01:20:10 +0000
@@ -1397,3 +1397,95 @@
identity.current.user, [clone_branch])
return dict(pkglisting=clone_branch)
+
+ @expose(allow_json=True)
+ # Check that the requestor is in a group that could potentially set ACLs.
+ @identity.require(identity.not_anonymous())
+ def remove_user(self, pkg_name, username, collectn_list=None):
I'd reorder the arguments to be username, pkg_name, collectn_list.
This is because username is the subject of the action so it's more
important than the pkg_name and collectn_list. (Also, the pkg_name and
collectn_list work together to define the pkglistings that the user is
being removed from).
+ '''Remove users from a package.
+ :arg pkg_name: Name of the package
+ :arg username: Name of user to remove from the package
+ :arg collectn_list: list of collections like 'F-10', 'devel'
+ '''
I'd add the default value to the collectn_list documentation and what it
means.
+ person = fas.person_by_username(username)
+ if not person:
+ return dict(status=False,
+ message='Specified user name %s does not have a' \
+ ' Fedora Account' % username)
Since this is a removal, we don't need to retrieve the person
information from fas. The username should be sufficient.
+ try:
+ # pylint: disable-msg=E1101
+ pkg = Package.query.filter_by(name=pkg_name).one()
+ except InvalidRequestError:
+ return dict(status=False, message='Package %s does not exist' %
pkg_name)
We're trying to move to a new style of returning errors. It's
documented here:
https://fedorahosted.org/releases/p/y/python-fedora/doc/service.html#erro...
Basically, when returning an error you'll do something like this:
from turbogears import flash
[...]
flash('Package %s does not exist' % pkg_name)
return dict(exc='NoPackageError')
When the client receives this it will see that exc is set and create an
AppError exception with the name in exc and the message that you called
flash() with (it will end up in tg_flash).
+ # Check that the current user is allowed to change acl
statuses
+ approved = self._user_can_set_acls(identity, pkg)
+ if not ident.in_group('cvsadmin'):
I think we've abstracted 'cvsadmin' out to a config file option. You
should be able to do this:
from pkgdb.utils import admin_grp
[...]
if not identity.in_group(admin_grp):
Also, I think you want identity rather than ident
+ return dict(status=False, message=
+ '%s is not allowed to remove user from the package' %
+ identity.current.user_name)
+
Same thing about returning errors here.
+ log_msgs = []
+
+ if collectn_list:
+ for simple_name in collectn_list:
+ try:
+ collectn = Collection.by_simple_name(simple_name)
+ except InvalidRequestError:
+ return dict(status=False, message='Collection %s does not exist' %
simple_name)
+
Same thing about returning errors
+ pkg_listing =
PackageListing.query.filter_by(packageid=pkg.id,
+ collectionid=collectn.id).one()
+
+ acls = PersonPackageListingAcl.query.filter(and_(
+ PersonPackageListingAcl.c.personpackagelistingid
+ == PersonPackageListing.c.id,
+ PersonPackageListing.c.packagelistingid == pkg_listing.id
+ PersonPackageListing.c.username ==
person['username'])).all()
+
You can change this from person['username'] to username
+ for acl in acls:
+ person_acl = self._create_or_modify_acl(pkg_listing,
person['id'], acl, self.obsoleteStatus)
+
maploin has just committed a new db schema and code to the db that
changes things like this to use username instead of id. So you can just
pass username instead of person['id'].
+ log_msg = u'%s has set the %s acl on %s (%s
%s) to Obsolete for %s' % (
+ identity.current.user_name, acl, pkg.name,
+ pkg_listing.collection.name,
pkg_listing.collection.version,
+ person['username'])
+ log = PersonPackageListingAclLog(identity.current.user.id,
+ self.obsoleteStatus.statuscodeid, log_msg)
+ log.acl = person_acl # pylint: disable-msg=W0201
+ log_msgs.append(log_msg)
+
+ else:
+ for pkg_listing in pkg.listings:
There's common code with the above block. From here...
+ acls = PersonPackageListingAcl.query.filter(and_(
+ PersonPackageListingAcl.c.personpackagelistingid
+ == PersonPackageListing.c.id,
+ PersonPackageListing.c.packagelistingid == pkg_listing.id,
+ PersonPackageListing.c.username ==
person['username'])).all()
+
+ for acl in acls:
+ person_acl = self._create_or_modify_acl(pkg_listing,
person['id'], acl, self.obsoleteStatus)
+
+ log_msg = u'%s has set the %s acl on %s (%s %s) to Obsolete for
%s' % (
+ identity.current.user_name, acl, pkg.name,
+ pkg_listing.collection.name,
pkg_listing.collection.version,
+ person['username'])
+ log = PersonPackageListingAclLog(identity.current.user.id,
+ self.obsoleteStatus.statuscodeid, log_msg)
+ log.acl = person_acl # pylint: disable-msg=W0201
+ log_msgs.append(log_msg)
... to here. We can put them together by putting them in their own
function or by creating the list of collections inside the if-then and
then moving the loop out a level. Something like this:
package_listings = []
if collctn_list:
for simple_name in collectn_list:
collctn = Collection.by_simple_name(simple_name)
pkg_listing = PackageListing.query.filter_by(packageid=pkg.id,
collectionid=collectn.id).one()
package_listings.append(pkg_listing)
else:
package_listings = pkg.listings
for pkg_listing in package_listings:
[....]
+
+ try:
+ session.flush()
+ except SQLError, e:
+ # An error was generated
+ return dict(status=False,
+ message='Not able to change acl %s on %s with status %s' \
+ % (acl, pkgid, self.obsoleteStatus.statusname))
+
Same thing about returning errors.
+ # Send a log to people interested in this package as well
+ self._send_log_msg('\n'.join(log_msgs), '%s had acl change
status' % (
+ pkg.name), identity.current.user, pkg.listings,
+ other_email=(person['email'],))
+
+ return dict(status=True)
And this looks fine.
Cool. So if we can get you able to commit to a branch, you can commit
this, work on it a little bit, and then we can merge it into the trunk.
-Toshio
5516
days inactive
5516
days old
infrastructure@lists.fedoraproject.org
1 comments
2 participants
participants (2)
-
Dmitry Kolesov -
Toshio Kuratomi