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#error-handling 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
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Fedora-infrastructure-list mailing list Fedora-infrastructure-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/fedora-infrastructure-list
