PR#1417: notification's optouts - koji

		`@@ -7306,7 +7306,7 @@`


		`def anon_handle_list_notifications(goptions, session, args):`
		`- "[monitor] List user's notifications"`
		`+ "[monitor] List user's notifications and blocks"`
		`usage = _("usage: %prog list-notifications [options]")`
		`usage += _("\n(Specify the --help global option for a list of other help options)")`
		`parser = OptionParser(usage=usage)`
		`@@ -7330,21 +7330,49 @@`
		`else:`
		`user_id = None`

		`- mask = "%(id)6s %(tag)-25s %(package)-25s %(email)-20s %(success)s"`
		`- head = mask % {'id': 'ID', 'tag': 'Tag', 'package': 'Package', 'email': 'E-mail', 'success': 'Success-only'}`
		`- print(head)`
		`- print('-' * len(head))`
		`- for notification in session.getBuildNotifications(user_id):`
		`- if notification['tag_id']:`
		`- notification['tag'] = session.getTag(notification['tag_id'])['name']`
		`- else:`
		`- notification['tag'] = '*'`
		`- if notification['package_id']:`
		`- notification['package'] = session.getPackage(notification['package_id'])['name']`
		`- else:`
		`- notification['package'] = '*'`
		`- notification['success'] = ['no', 'yes'][notification['success_only']]`
		`- print(mask % notification)`
		`+ mask = "%(id)6s %(tag)-25s %(package)-25s %(email)-20s %(success)-12s"`
		`+ headers = {'id': 'ID', 'tag': 'Tag', 'package': 'Package', 'email': 'E-mail', 'success': 'Success-only'}`
		`+ head = mask % headers`
		`+ notifications = session.getBuildNotifications(user_id)`
		`+ if notifications:`
		`+ print('Notifications')`
		`+ print(head)`
		`+ print('-' * len(head))`
		`+ for notification in notifications:`
		`+ if notification['tag_id']:`
		`+ notification['tag'] = session.getTag(notification['tag_id'])['name']`
		`+ else:`
		`+ notification['tag'] = '*'`
		`+ if notification['package_id']:`
		`+ notification['package'] = session.getPackage(notification['package_id'])['name']`
		`+ else:`
		`+ notification['package'] = '*'`
		`+ notification['success'] = ['no', 'yes'][notification['success_only']]`
		`+ print(mask % notification)`
		`+ else:`
		`+ print('No notifications')`
		`+`
		`+ print('')`
		`+`
		`+ mask = "%(id)6s %(tag)-25s %(package)-25s"`
		`+ head = mask % headers`
		`+ blocks = session.getBuildNotificationBlocks(user_id)`
		`+ if blocks:`
		`+ print('Notification blocks')`
		`+ print(head)`
		`+ print('-' * len(head))`
		`+ for notification in blocks:`
		`+ if notification['tag_id']:`
		`+ notification['tag'] = session.getTag(notification['tag_id'])['name']`
		`+ else:`
		`+ notification['tag'] = '*'`
		`+ if notification['package_id']:`
		`+ notification['package'] = session.getPackage(notification['package_id'])['name']`
		`+ else:`
		`+ notification['package'] = '*'`
		`+ print(mask % notification)`
		`+ else:`
		`+ print('No notification blocks')`


		`def handle_add_notification(goptions, session, args):`
		`@@ -7412,7 +7440,7 @@`
		`for n_id in n_ids:`
		`session.deleteNotification(n_id)`
		`if not goptions.quiet:`
		`- print(_("Notification %s successfully removed.") % n_id)`
		`+ print(_("Notification %d successfully removed.") % n_id)`


		`def handle_edit_notification(goptions, session, args):`
		`@@ -7470,3 +7498,75 @@`
		`success_only = old['success_only']`

		`session.updateNotification(n_id, package_id, tag_id, success_only)`
		`+`
		`+`
		`+ def handle_block_notification(goptions, session, args):`
		`+ "[monitor] Block user's notifications"`
		`+ usage = _("usage: %prog block-notification [options]")`
		`+ usage += _("\n(Specify the --help global option for a list of other help options)")`
		`+ parser = OptionParser(usage=usage)`
		`+ parser.add_option("--user", help=_("Block notifications for this user (admin-only)"))`
		`+ parser.add_option("--package", help=_("Block notifications for this package"))`
		`+ parser.add_option("--tag", help=_("Block notifications for this tag"))`
		`+ parser.add_option("--all", action="store_true", help=_("Block all notification for this user"))`
		`+ (options, args) = parser.parse_args(args)`
		`+`
		`+ if len(args) != 0:`
		`+ parser.error(_("This command takes no arguments"))`
		`+`
		`+ if not options.package and not options.tag and not options.all:`
		`+ parser.error(_("One of --tag, --package or --all must be specified."))`
		`+`
		`+ activate_session(session, goptions)`
		`+`
		`+ if options.user and not session.hasPerm('admin'):`
		`+ parser.error("--user requires admin permission")`
		`+`
		`+ if options.user:`
		`+ user_id = session.getUser(options.user)['id']`
		`+ else:`
		`+ user_id = session.getLoggedInUser()['id']`
		`+`
		`+ if options.package:`
		`+ package_id = session.getPackageID(options.package)`
		`+ if package_id is None:`
		`+ parser.error("Unknown package: %s" % options.package)`
		`+ else:`
		`+ package_id = None`
		`+`
		`+ if options.tag:`
		`+ try:`
		`+ tag_id = session.getTagID(options.tag, strict=True)`
		`+ except koji.GenericError:`
		`+ parser.error("Unknown tag: %s" % options.tag)`
		`+ else:`
		`+ tag_id = None`
		`+`
		`+ for block in session.getBuildNotificationBlocks(user_id):`
		`+ if (block['package_id'] == package_id and block['tag_id'] == tag_id):`
		`+ parser.error('Notification already exists.')`
		`+`
		`+ session.createNotificationBlock(user_id, package_id, tag_id)`
		`+`
		`+`
		`+ def handle_unblock_notification(goptions, session, args):`
		`+ "[monitor] Unblock user's notification"`
		`+ usage = _("usage: %prog unblock-notification [options] ID [ID2, ...]")`
		`+ usage += _("\n(Specify the --help global option for a list of other help options)")`
		`+ parser = OptionParser(usage=usage)`
		`+ (options, args) = parser.parse_args(args)`
		`+`
		`+ activate_session(session, goptions)`
		`+`
		`+ if len(args) < 1:`
		`+ parser.error(_("At least one notification block id has to be specified"))`
		`+`
		`+ try:`
		`+ n_ids = [int(x) for x in args]`
		`+ except ValueError:`
		`+ parser.error(_("All notification block ids has to be integers"))`
		`+`
		`+ for n_id in n_ids:`
		`+ session.deleteNotificationBlock(n_id)`
		`+ if not goptions.quiet:`
		`+ print(_("Notification block %d successfully removed.") % n_id)`

docs/schema-upgrade-1.17-1.18.sql

file modified

		`@@ -4,6 +4,14 @@`

		`BEGIN;`

		`+ -- new table for notifications' optouts`
		`+ CREATE TABLE build_notifications_block (`
		`+ id SERIAL NOT NULL PRIMARY KEY,`
		`+ user_id INTEGER NOT NULL REFERENCES users (id),`
		`+ package_id INTEGER REFERENCES package (id),`
		`+ tag_id INTEGER REFERENCES tag (id)`
		`+ ) WITHOUT OIDS;`
		`+`
		`-- add tgz to list of tar's extensions`
		`UPDATE archivetypes SET extensions = 'tar tar.gz tar.bz2 tar.xz tgz' WHERE name = 'tar';`
		`INSERT INTO archivetypes (name, description, extensions) VALUES ('vhdx', 'Hyper-V Virtual Hard Disk v2 image', 'vhdx');`

docs/schema.sql

file modified

		`@@ -721,6 +721,13 @@`
		`email TEXT NOT NULL`
		`) WITHOUT OIDS;`

		`+ CREATE TABLE build_notifications_block (`
		`+ id SERIAL NOT NULL PRIMARY KEY,`
		`+ user_id INTEGER NOT NULL REFERENCES users (id),`
		`+ package_id INTEGER REFERENCES package (id),`
		`+ tag_id INTEGER REFERENCES tag (id)`
		`+ ) WITHOUT OIDS;`
		`+`
		`GRANT SELECT ON build, package, task, tag,`
		`tag_listing, tag_config, tag_inheritance, tag_packages,`
		`rpminfo TO PUBLIC;`

hub/kojihub.py

file modified

+131 -26

		`@@ -7558,17 +7558,19 @@`
		`if state != koji.BUILD_STATES['COMPLETE']:`
		`clauses.append('success_only = FALSE')`

		`- query = QueryProcessor(columns=('email',), tables=['build_notifications'],`
		`- joins=joins, clauses=clauses, values=locals(),`
		`- opts={'asList':True})`
		`- emails = [result[0] for result in query.execute()]`
		`+ query = QueryProcessor(columns=('user_id', 'email'), tables=['build_notifications'],`
		`+ joins=joins, clauses=clauses, values=locals())`
		`+ recipients = query.execute()`

		`email_domain = context.opts['EmailDomain']`
		`notify_on_success = context.opts['NotifyOnSuccess']`

		`if build and (notify_on_success is True or state != koji.BUILD_STATES['COMPLETE']):`
		`# user who submitted the build`
		`- emails.append('%s@%s' % (build['owner_name'], email_domain))`
		`+ recipients.append({`
		`+ 'user_id': build['owner_id'],`
		`+ 'email': '%s@%s' % (build['owner_name'], email_domain)`
		`+ })`

		`if tag_id:`
		`packages = readPackageList(pkgID=package_id, tagID=tag_id, inherit=True)`
		`@@ -7580,12 +7582,40 @@`
		`owner = get_user(pkgdata['owner_id'], strict=True)`
		`if owner['status'] == koji.USER_STATUS['NORMAL'] and \`
		`owner['usertype'] == koji.USERTYPES['NORMAL']:`
		`- emails.append('%s@%s' % (owner['name'], email_domain))`
		`+ recipients.append({`
		`+ 'user_id': owner['id'],`
		`+ 'email': '%s@%s' % (owner['name'], email_domain)`
		`+ })`
		`#FIXME - if tag_id is None, we don't have a good way to get the package owner.`
		`# using all package owners from all tags would be way overkill.`

		`+ if not recipients:`
		`+ return None`
		`+`
		`+ # apply the out outs`
		`+ user_ids = list(set([r['user_id'] for r in recipients]))`
		`+ if user_ids:`
		`+ clauses = ['user_id IN %(user_ids)s']`
		`+ if build:`
		`+ package_id = build['package_id']`
		`+ clauses.append('package_id = %(package_id)i OR package_id IS NULL')`
		`+ else:`
		`+ clauses.append('package_id IS NULL')`
		`+ if tag_id:`
		`+ clauses.append('tag_id = %(tag_id)i OR tag_id IS NULL')`
		`+ else:`
		`+ clauses.append('tag_id IS NULL')`
		`+ query = QueryProcessor(columns=['user_id'], clauses=clauses,`
		`+ tables=['build_notifications_block'], values=locals())`
		`+ optouts = [r['user_id'] for r in query.execute()]`
		`+ optouts = set(optouts)`
		`+ else:`
		`+ optouts = set()`
		`+`
		`+ emails = [r['email'] for r in recipients if r['user_id'] not in optouts]`
		`return list(set(emails))`

		`+`
		`def tag_notification(is_successful, tag_id, from_id, build_id, user_id, ignore_success=False, failure_msg=''):`
		`if context.opts.get('DisableNotifications'):`
		`return`
		`@@ -7632,12 +7662,20 @@`
		`make_task('buildNotification', [recipients, build, target, web_url])`

		`def get_build_notifications(user_id):`
		`- fields = ('id', 'user_id', 'package_id', 'tag_id', 'success_only', 'email')`
		`- query = """SELECT %s`
		`- FROM build_notifications`
		`- WHERE user_id = %%(user_id)i`
		`- """ % ', '.join(fields)`
		`- return _multiRow(query, locals(), fields)`
		`+ query = QueryProcessor(tables=['build_notifications'],`
		`+ columns=('id', 'user_id', 'package_id', 'tag_id',`
		`+ 'success_only', 'email'),`
		`+ clauses=['user_id = %(user_id)i'],`
		`+ values=locals())`
		`+ return query.execute()`
		`+`
		`+ def get_build_notification_blocks(user_id):`
		`+ query = QueryProcessor(tables=['build_notifications_block'],`
		`+ columns=['id', 'user_id', 'package_id', 'tag_id'],`
		`+ clauses=['user_id = %(user_id)i'],`
		`+ values=locals())`
		`+ return query.execute()`
		`+`

		`def new_group(name):`
		`"""Add a user group to the database"""`
		`@@ -11329,12 +11367,37 @@`
		`If there is no notification with the given ID, when strict is True,`
		`raise GenericError, else return None.`
		`"""`
		`- fields = ('id', 'user_id', 'package_id', 'tag_id', 'success_only', 'email')`
		`- query = """SELECT %s`
		`- FROM build_notifications`
		`- WHERE id = %%(id)i`
		`- """ % ', '.join(fields)`
		`- return _singleRow(query, locals(), fields, strict=strict)`
		`+ query = QueryProcessor(tables=['build_notifications'],`
		`+ columns = ('id', 'user_id', 'package_id', 'tag_id',`
		`+ 'success_only', 'email'),`
		`+ clauses = ['id = %(id)i'],`
		`+ values = locals())`
		`+ result = query.executeOne()`
		`+ if strict and not result:`
		`+ raise koji.GenericError("No notification with ID %i found" % id)`
		`+ return result`
		`+`
		`+ def getBuildNotificationBlocks(self, userID=None):`
		`+ """Get build notifications for the user with the given ID, name or`
		`+ Kerberos principal. If no user is specified, get the notifications for`
		`+ the currently logged-in user. If there is no currently logged-in user,`
		`+ raise a GenericError."""`
		`+ userID = get_user(userID, strict=True)['id']`
		`+ return get_build_notification_blocks(userID)`
		`+`
		`+ def getBuildNotificationBlock(self, id, strict=False):`
		`+ """Get the build notification with the given ID.`
		`+ If there is no notification with the given ID, when strict is True,`
		`+ raise GenericError, else return None.`
		`+ """`
		`+ query = QueryProcessor(tables=['build_notifications_block'],`
		`+ columns = ('id', 'user_id', 'package_id', 'tag_id'),`
		`+ clauses = ['id = %(id)i'],`
		`+ values = locals())`
		`+ result = query.executeOne()`
		`+ if strict and not result:`
		`+ raise koji.GenericError("No notification block with ID %i found" % id)`
		`+ return result`

		`def updateNotification(self, id, package_id, tag_id, success_only):`
		`"""Update an existing build notification with new data. If the notification`
		`@@ -11344,11 +11407,8 @@`
		`if not currentUser:`
		`raise koji.GenericError('not logged-in')`

		`- orig_notif = self.getBuildNotification(id)`
		`- if not orig_notif:`
		`- raise koji.GenericError('no notification with ID: %i' % id)`
		`- elif not (orig_notif['user_id'] == currentUser['id'] or`
		`- self.hasPerm('admin')):`
		`+ orig_notif = self.getBuildNotification(id, strict=True)`
		`+ if not (orig_notif['user_id'] == currentUser['id'] or self.hasPerm('admin')):`
		`raise koji.GenericError('user %i cannot update notifications for user %i' % \`
		`(currentUser['id'], orig_notif['user_id']))`

		`@@ -11411,9 +11471,7 @@`
		`def deleteNotification(self, id):`
		`"""Delete the notification with the given ID. If the currently logged-in`
		`user is not the owner of the notification or an admin, raise a GenericError."""`
		`- notification = self.getBuildNotification(id)`
		`- if not notification:`
		`- raise koji.GenericError('no notification with ID: %i' % id)`
		`+ notification = self.getBuildNotification(id, strict=True)`
		`currentUser = self.getLoggedInUser()`
		`if not currentUser:`
		`raise koji.GenericError('not logged-in')`
		`@@ -11425,6 +11483,53 @@`
		`delete = """DELETE FROM build_notifications WHERE id = %(id)i"""`
		`_dml(delete, locals())`

		`+ def createNotificationBlock(self, user_id, package_id=None, tag_id=None):`
		`+ """Create notification block. If the user_id does not match the`
		`+ currently logged-in user and the currently logged-in user is not an`
		`+ admin, raise a GenericError."""`
		`+ currentUser = self.getLoggedInUser()`
		`+ if not currentUser:`
		`+ raise koji.GenericError('not logged in')`
		`+`
		`+ notificationUser = self.getUser(user_id)`
		`+ if not notificationUser:`
		`+ raise koji.GenericError('invalid user ID: %s' % user_id)`
		`+`
		`+ if not (notificationUser['id'] == currentUser['id'] or self.hasPerm('admin')):`
		`+ raise koji.GenericError('user %s cannot create notification blocks for user %s' % \`
		`+ (currentUser['name'], notificationUser['name']))`
		`+`
		`+ # sanitize input`
		`+ user_id = notificationUser['id']`
		`+ if package_id is not None:`
		`+ package_id = get_package_id(package_id, strict=True)`
		`+ if tag_id is not None:`
		`+ tag_id = get_tag_id(tag_id, strict=True)`
		`+`
		`+ # check existing notifications to not have same twice`
		`+ for block in get_build_notification_blocks(user_id):`
		`+ if (block['package_id'] == package_id and block['tag_id'] == tag_id):`
		`+ raise koji.GenericError('notification already exists')`
		`+`
		`+ insert = InsertProcessor('build_notifications_block')`
		`+ insert.set(user_id=user_id, package_id=package_id, tag_id=tag_id)`
		`+ insert.execute()`
		`+`
		`+ def deleteNotificationBlock(self, id):`
		`+ """Delete the notification block with the given ID. If the currently logged-in`
		`+ user is not the owner of the notification or an admin, raise a GenericError."""`
		`+ block = self.getBuildNotificationBlock(id, strict=True)`
		`+ currentUser = self.getLoggedInUser()`
		`+ if not currentUser:`
		`+ raise koji.GenericError('not logged-in')`
		`+`
		`+ if not (block['user_id'] == currentUser['id'] or`
		`+ self.hasPerm('admin')):`
		`+ raise koji.GenericError('user %i cannot delete notification blocks for user %i' % \`
		`+ (currentUser['id'], block['user_id']))`
		`+ delete = """DELETE FROM build_notifications_block WHERE id = %(id)i"""`
		`+ _dml(delete, locals())`
		`+`
		`def _prepareSearchTerms(self, terms, matchType):`
		`r"""Process the search terms before passing them to the database.`
		`If matchType is "glob", "_" will be replaced with "\_" (to match literal`

tests/test_cli/data/list-commands.txt

file modified

+3 -1

		`@@ -124,9 +124,11 @@`

		`monitor commands:`
		`add-notification Add user's notification`
		`+ block-notification Block user's notifications`
		`edit-notification Edit user's notification`
		`- list-notifications List user's notifications`
		`+ list-notifications List user's notifications and blocks`
		`remove-notification Remove user's notifications`
		`+ unblock-notification Unblock user's notification`
		`wait-repo Wait for a repo to be regenerated`
		`watch-logs Watch logs in realtime`
		`watch-task Track progress of particular tasks`

tests/test_cli/test_list_notifications.py

file modified

+34 -10

		`@@ -25,17 +25,22 @@`
		`{'id': 2, 'tag_id': None, 'package_id': 11, 'email': 'email@test.com', 'success_only': False},`
		`{'id': 3, 'tag_id': 1, 'package_id': None, 'email': 'email@test.com', 'success_only': True},`
		`]`
		`+ self.session.getBuildNotificationBlocks.return_value = []`
		`self.session.getTag.return_value = {'id': 1, 'name': 'tag'}`
		`self.session.getPackage.return_value = {'id': 11, 'name': 'package'}`

		`anon_handle_list_notifications(self.options, self.session, ['--mine'])`

		`actual = stdout.getvalue()`
		`- expected = ''' ID Tag Package E-mail Success-only`
		`+ expected = '''\`
		`+ Notifications`
		`+ ID Tag Package E-mail Success-only`
		`--------------------------------------------------------------------------------------------`
		`- 1 tag package email@test.com yes`
		`- 2 * package email@test.com no`
		`- 3 tag * email@test.com yes`
		`+ 1 tag package email@test.com yes`
		`+ 2 * package email@test.com no`
		`+ 3 tag * email@test.com yes`
		`+`
		`+ No notification blocks`
		`'''`

		`self.maxDiff=None`
		`@@ -54,18 +59,37 @@`
		`{'id': 2, 'tag_id': None, 'package_id': 11, 'email': 'email@test.com', 'success_only': False},`
		`{'id': 3, 'tag_id': 1, 'package_id': None, 'email': 'email@test.com', 'success_only': True},`
		`]`
		`- self.session.getTag.return_value = {'id': 1, 'name': 'tag'}`
		`- self.session.getPackage.return_value = {'id': 11, 'name': 'package'}`
		`+ self.session.getBuildNotificationBlocks.return_value = [`
		`+ {'id': 11, 'tag_id': None, 'package_id': 22},`
		`+ {'id': 12, 'tag_id': None, 'package_id': None},`
		`+ ]`
		`+ self.session.getTag.side_effect = [`
		`+ {'id': 1, 'name': 'tag'},`
		`+ {'id': 3, 'name': 'tag3'},`
		`+ ]`
		`+ self.session.getPackage.side_effect = [`
		`+ {'id': 11, 'name': 'package'},`
		`+ {'id': 11, 'name': 'package'},`
		`+ {'id': 22, 'name': 'package'},`
		`+ ]`
		`self.session.getUser.return_value = {'id': 321}`

		`anon_handle_list_notifications(self.options, self.session, ['--user', 'random_name'])`

		`actual = stdout.getvalue()`
		`- expected = ''' ID Tag Package E-mail Success-only`
		`+ expected = '''\`
		`+ Notifications`
		`+ ID Tag Package E-mail Success-only`
		`--------------------------------------------------------------------------------------------`
		`- 1 tag package email@test.com yes`
		`- 2 * package email@test.com no`
		`- 3 tag * email@test.com yes`
		`+ 1 tag package email@test.com yes`
		`+ 2 * package email@test.com no`
		`+ 3 tag3 * email@test.com yes`
		`+`
		`+ Notification blocks`
		`+ ID Tag Package`
		`+ ----------------------------------------------------------`
		`+ 11 * package`
		`+ 12 * *`
		`'''`

		`self.maxDiff=None`

tests/test_hub/test_get_build_notifications.py

file modified

-2

		`@@ -8,8 +8,6 @@`


		`class TestGetBuildNotifications(unittest.TestCase):`
		`-`
		`-`
		`@mock.patch('kojihub.get_user', return_value={'id': 1})`
		`@mock.patch('kojihub.get_build_notifications')`
		`def test_loggedin_user(self, get_build_notifications, get_user):`

tests/test_hub/test_notifications.py

file modified

+330 -26

		`@@ -53,6 +53,7 @@`
		`self.exports.getUser = mock.MagicMock()`
		`self.exports.hasPerm = mock.MagicMock()`
		`self.exports.getBuildNotification = mock.MagicMock()`
		`+ self.exports.getBuildNotificationBlock = mock.MagicMock()`

		`def tearDown(self):`
		`mock.patch.stopall()`
		`@@ -60,7 +61,7 @@`

		`@mock.patch('kojihub.get_user')`
		`@mock.patch('kojihub.readPackageList')`
		`- def test_get_notification_recipients(self, readPackageList, get_user):`
		`+ def test_get_notification_recipients_watchers(self, readPackageList, get_user):`
		`# without build / tag_id`
		`build = None`
		`tag_id = None`
		`@@ -72,7 +73,7 @@`
		`# only query to watchers`
		`self.assertEqual(len(self.queries), 1)`
		`q = self.queries[0]`
		`- self.assertEqual(q.columns, ['email'])`
		`+ self.assertEqual(q.columns, ['email', 'user_id'])`
		`self.assertEqual(q.tables, ['build_notifications'])`
		`self.assertEqual(q.clauses, ['package_id IS NULL',`
		`'status = %(users_status)i',`
		`@@ -83,20 +84,38 @@`
		`self.assertEqual(q.values['state'], state)`
		`self.assertEqual(q.values['build'], build)`
		`self.assertEqual(q.values['tag_id'], tag_id)`
		`+`
		`+ '''`
		`+ q = self.queries[1]`
		`+ self.assertEqual(q.columns, ['user_id'])`
		`+ self.assertEqual(q.tables, ['build_notifications_block'])`
		`+ self.assertEqual(q.clauses, ['user_id IN %(user_ids)s'])`
		`+ self.assertEqual(q.joins, [])`
		`+ self.assertEqual(q.values['user_ids'], None)`
		`+ '''`
		`readPackageList.assert_not_called()`


		`+ @mock.patch('kojihub.get_user')`
		`+ @mock.patch('kojihub.readPackageList')`
		`+ def test_get_notification_recipients_build_without_tag(self, readPackageList, get_user):`
		`### with build without tag`
		`- build = {'package_id': 12345, 'owner_name': 'owner_name'}`
		`+ tag_id = None`
		`+ state = koji.BUILD_STATES['CANCELED']`
		`+ build = {'package_id': 12345, 'owner_name': 'owner_name', 'owner_id': 5}`
		`self.queries = []`
		`+ self.set_queries([`
		`+ [{'user_id': 5, 'email': 'owner_name@%s' % self.context.opts['EmailDomain']}],`
		`+ []`
		`+ ])`

		`emails = kojihub.get_notification_recipients(build, tag_id, state)`
		`self.assertEqual(emails, ['owner_name@test.domain.com'])`

		`# there should be only query to watchers`
		`- self.assertEqual(len(self.queries), 1)`
		`+ self.assertEqual(len(self.queries), 2)`
		`q = self.queries[0]`
		`- self.assertEqual(q.columns, ['email'])`
		`+ self.assertEqual(q.columns, ['email', 'user_id'])`
		`self.assertEqual(q.tables, ['build_notifications'])`
		`self.assertEqual(q.clauses, ['package_id = %(package_id)i OR package_id IS NULL',`
		`'status = %(users_status)i',`
		`@@ -108,11 +127,27 @@`
		`self.assertEqual(q.values['state'], state)`
		`self.assertEqual(q.values['build'], build)`
		`self.assertEqual(q.values['tag_id'], tag_id)`
		`+`
		`+ q = self.queries[1]`
		`+ self.assertEqual(q.columns, ['user_id'])`
		`+ self.assertEqual(q.tables, ['build_notifications_block'])`
		`+ self.assertEqual(q.clauses, [`
		`+ 'package_id = %(package_id)i OR package_id IS NULL',`
		`+ 'tag_id IS NULL',`
		`+ 'user_id IN %(user_ids)s',`
		`+ ])`
		`+ self.assertEqual(q.joins, None)`
		`+ self.assertEqual(q.values['user_ids'], [5])`
		`+`
		`readPackageList.assert_not_called()`

		`+ @mock.patch('kojihub.get_user')`
		`+ @mock.patch('kojihub.readPackageList')`
		`+ def test_get_notification_recipients_tag_without_build(self, readPackageList, get_user):`
		`### with tag without build makes no sense`
		`build = None`
		`tag_id = 123`
		`+ state = koji.BUILD_STATES['CANCELED']`
		`self.queries = []`

		`with self.assertRaises(koji.GenericError):`
		`@@ -120,58 +155,128 @@`
		`self.assertEqual(self.queries, [])`
		`readPackageList.assert_not_called()`

		`+ def set_queries(self, return_values):`
		`+ self.query_returns = return_values`
		`+ self.query_returns.reverse()`
		`+ def getQuery(args, *kwargs):`
		`+ q = QP(args, *kwargs)`
		`+ q.execute = mock.MagicMock()`
		`+ q.execute.return_value = self.query_returns.pop()`
		`+ self.queries.append(q)`
		`+ return q`
		`+ self.QueryProcessor.side_effect = getQuery`

		`+ @mock.patch('kojihub.get_user')`
		`+ @mock.patch('kojihub.readPackageList')`
		`+ def test_get_notification_recipients_tag_with_build(self, readPackageList, get_user):`
		`### with tag and build`
		`- build = {'package_id': 12345, 'owner_name': 'owner_name'}`
		`+ build = {'package_id': 12345, 'owner_name': 'owner_name', 'owner_id': 5}`
		`tag_id = 123`
		`- self.queries = []`
		`+ state = koji.BUILD_STATES['CANCELED']`
		`readPackageList.return_value = {12345: {'blocked': False, 'owner_id': 'owner_id'}}`
		`get_user.return_value = {`
		`- 'id': 'owner_id',`
		`+ 'id': 342,`
		`'name': 'pkg_owner_name',`
		`'status': koji.USER_STATUS['NORMAL'],`
		`'usertype': koji.USERTYPES['NORMAL']`
		`}`
		`+ self.set_queries([`
		`+ [{'user_id': 5, 'email': 'owner_name@%s' % self.context.opts['EmailDomain']}],`
		`+ []`
		`+ ])`

		`emails = kojihub.get_notification_recipients(build, tag_id, state)`
		`self.assertEqual(sorted(emails), ['owner_name@test.domain.com', 'pkg_owner_name@test.domain.com'])`


		`# there should be only query to watchers`
		`- self.assertEqual(len(self.queries), 1)`
		`+ self.assertEqual(len(self.queries), 2)`
		`q = self.queries[0]`
		`- self.assertEqual(q.columns, ['email'])`
		`+ self.assertEqual(q.columns, ['email', 'user_id'])`
		`self.assertEqual(q.tables, ['build_notifications'])`
		`self.assertEqual(q.clauses, ['package_id = %(package_id)i OR package_id IS NULL',`
		`'status = %(users_status)i',`
		`'success_only = FALSE',`
		`'tag_id = %(tag_id)i OR tag_id IS NULL',`
		`- 'usertype IN %(users_usertypes)s'])`
		`+ 'usertype IN %(users_usertypes)s',`
		`+ ])`
		`self.assertEqual(q.joins, ['JOIN users ON build_notifications.user_id = users.id'])`
		`self.assertEqual(q.values['package_id'], build['package_id'])`
		`self.assertEqual(q.values['state'], state)`
		`self.assertEqual(q.values['build'], build)`
		`self.assertEqual(q.values['tag_id'], tag_id)`
		`+`
		`+ q = self.queries[1]`
		`+ self.assertEqual(q.columns, ['user_id'])`
		`+ self.assertEqual(q.tables, ['build_notifications_block'])`
		`+ self.assertEqual(q.clauses, [`
		`+ 'package_id = %(package_id)i OR package_id IS NULL',`
		`+ 'tag_id = %(tag_id)i OR tag_id IS NULL',`
		`+ 'user_id IN %(user_ids)s',`
		`+ ])`
		`+ self.assertEqual(q.joins, None)`
		`+ self.assertEqual(sorted(q.values['user_ids']), [5, 342])`
		`readPackageList.assert_called_once_with(pkgID=build['package_id'], tagID=tag_id, inherit=True)`
		`- get_user.asssert_called_once_with('owner_id', strict=True)`
		`+ get_user.asssert_called_once_with(342, strict=True)`

		`+ @mock.patch('kojihub.get_user')`
		`+ @mock.patch('kojihub.readPackageList')`
		`+ def test_get_notification_recipients_blocked_pkg_owner(self, readPackageList, get_user):`
		`# blocked package owner`
		`+ build = {'package_id': 12345, 'owner_name': 'owner_name', 'owner_id': 5}`
		`+ tag_id = 123`
		`+ state = koji.BUILD_STATES['CANCELED']`
		`get_user.return_value = {`
		`- 'id': 'owner_id',`
		`+ 'id': 342,`
		`'name': 'pkg_owner_name',`
		`'status': koji.USER_STATUS['BLOCKED'],`
		`'usertype': koji.USERTYPES['NORMAL']`
		`}`
		`+ self.set_queries([`
		`+ [{'user_id': 5, 'email': 'owner_name@%s' % self.context.opts['EmailDomain']}],`
		`+ []`
		`+ ])`
		`emails = kojihub.get_notification_recipients(build, tag_id, state)`
		`self.assertEqual(emails, ['owner_name@test.domain.com'])`

		`+ @mock.patch('kojihub.get_user')`
		`+ @mock.patch('kojihub.readPackageList')`
		`+ def test_get_notification_recipients_optout(self, readPackageList, get_user):`
		`+ # blocked package owner`
		`+ build = {'package_id': 12345, 'owner_name': 'owner_name', 'owner_id': 5}`
		`+ tag_id = 123`
		`+ state = koji.BUILD_STATES['CANCELED']`
		`+ get_user.return_value = {`
		`+ 'id': 342,`
		`+ 'name': 'pkg_owner_name',`
		`+ 'status': koji.USER_STATUS['NORMAL'],`
		`+ 'usertype': koji.USERTYPES['NORMAL']`
		`+ }`
		`+ self.set_queries([`
		`+ [{'user_id': 5, 'email': 'owner_name@%s' % self.context.opts['EmailDomain']}],`
		`+ [{'user_id': 5}]`
		`+ ])`
		`+ emails = kojihub.get_notification_recipients(build, tag_id, state)`
		`+ self.assertEqual(emails, [])`
		`+`
		`+`
		`+ @mock.patch('kojihub.get_user')`
		`+ @mock.patch('kojihub.readPackageList')`
		`+ def test_get_notification_recipients_machine(self, readPackageList, get_user):`
		`# package owner is machine`
		`+ build = {'package_id': 12345, 'owner_name': 'owner_name', 'owner_id': 5}`
		`+ tag_id = 123`
		`+ state = koji.BUILD_STATES['CANCELED']`
		`get_user.return_value = {`
		`- 'id': 'owner_id',`
		`+ 'id': 342,`
		`'name': 'pkg_owner_name',`
		`'status': koji.USER_STATUS['NORMAL'],`
		`'usertype': koji.USERTYPES['HOST']`
		`}`
		`+ self.set_queries([`
		`+ [{'user_id': 5, 'email': 'owner_name@%s' % self.context.opts['EmailDomain']}],`
		`+ []`
		`+ ])`
		`emails = kojihub.get_notification_recipients(build, tag_id, state)`
		`self.assertEqual(emails, ['owner_name@test.domain.com'])`

		`@@ -337,34 +442,36 @@`

		`self.exports.deleteNotification(n_id)`

		`- self.exports.getBuildNotification.assert_called_once_with(n_id)`
		`+ self.exports.getBuildNotification.assert_called_once_with(n_id, strict=True)`
		`self.exports.getLoggedInUser.assert_called_once_with()`
		`_dml.assert_called_once()`

		`- @mock.patch('kojihub._dml')`
		`- def test_deleteNotification_missing(self, _dml):`
		`+ def test_deleteNotification_missing(self):`
		`user_id = 752`
		`n_id = 543`
		`- self.exports.getBuildNotification.return_value = None`
		`+ self.exports.getBuildNotification.side_effect = koji.GenericError`

		`with self.assertRaises(koji.GenericError):`
		`self.exports.deleteNotification(n_id)`

		`- self.exports.getBuildNotification.assert_called_once_with(n_id)`
		`- _dml.assert_not_called()`
		`+ self.exports.getBuildNotification.assert_called_once_with(n_id, strict=True)`

		`- @mock.patch('kojihub._dml')`
		`- def test_deleteNotification_not_logged(self, _dml):`
		`+ def test_deleteNotification_not_logged(self):`
		`user_id = 752`
		`n_id = 543`
		`self.exports.getBuildNotification.return_value = {'user_id': user_id}`
		`self.exports.getLoggedInUser.return_value = None`
		`+ #self.set_queries = ([`
		`+ # [{'user_id': 5, 'email': 'owner_name@%s' % self.context.opts['EmailDomain']}],`
		`+ #])`

		`with self.assertRaises(koji.GenericError):`
		`self.exports.deleteNotification(n_id)`

		`- self.exports.getBuildNotification.assert_called_once_with(n_id)`
		`- _dml.assert_not_called()`
		`+ self.exports.getBuildNotification.assert_called_once_with(n_id, strict=True)`
		`+ self.assertEqual(len(self.inserts), 0)`
		`+ self.assertEqual(len(self.updates), 0)`
		`+ self.assertEqual(len(self.queries), 0)`

		`@mock.patch('kojihub._dml')`
		`def test_deleteNotification_no_perm(self, _dml):`
		`@@ -377,7 +484,7 @@`
		`with self.assertRaises(koji.GenericError):`
		`self.exports.deleteNotification(n_id)`

		`- self.exports.getBuildNotification.assert_called_once_with(n_id)`
		`+ self.exports.getBuildNotification.assert_called_once_with(n_id, strict=True)`
		`_dml.assert_not_called()`


		`@@ -445,7 +552,7 @@`
		`tag_id = 345`
		`success_only = True`
		`self.exports.getLoggedInUser.return_value = {'id': 1}`
		`- self.exports.getBuildNotification.return_value = None`
		`+ self.exports.getBuildNotification.side_effect = koji.GenericError`

		`with self.assertRaises(koji.GenericError):`
		`self.exports.updateNotification(n_id, package_id, tag_id, success_only)`
		`@@ -523,3 +630,200 @@`

		`self.assertEqual(len(self.inserts), 0)`
		`self.assertEqual(len(self.updates), 0)`
		`+`
		`+ ###########################`
		`+ # Create notification block`
		`+`
		`+ @mock.patch('kojihub.get_build_notification_blocks')`
		`+ @mock.patch('kojihub.get_tag_id')`
		`+ @mock.patch('kojihub.get_package_id')`
		`+ def test_createNotificationBlock(self, get_package_id, get_tag_id,`
		`+ get_build_notification_blocks):`
		`+ user_id = 1`
		`+ package_id = 234`
		`+ tag_id = 345`
		`+ self.exports.getLoggedInUser.return_value = {'id': 1}`
		`+ self.exports.getUser.return_value = {'id': 2, 'name': 'username'}`
		`+ self.exports.hasPerm.return_value = True`
		`+ get_package_id.return_value = package_id`
		`+ get_tag_id.return_value = tag_id`
		`+ get_build_notification_blocks.return_value = []`
		`+`
		`+ r = self.exports.createNotificationBlock(user_id, package_id, tag_id)`
		`+ self.assertEqual(r, None)`
		`+`
		`+ self.exports.getLoggedInUser.assert_called_once()`
		`+ self.exports.getUser.asssert_called_once_with(user_id)`
		`+ self.exports.hasPerm.asssert_called_once_with('admin')`
		`+ get_package_id.assert_called_once_with(package_id, strict=True)`
		`+ get_tag_id.assert_called_once_with(tag_id, strict=True)`
		`+ get_build_notification_blocks.assert_called_once_with(2)`
		`+ self.assertEqual(len(self.inserts), 1)`
		`+ insert = self.inserts[0]`
		`+ self.assertEqual(insert.table, 'build_notifications_block')`
		`+ self.assertEqual(insert.data, {`
		`+ 'package_id': package_id,`
		`+ 'user_id': 2,`
		`+ 'tag_id': tag_id,`
		`+ })`
		`+ self.assertEqual(insert.rawdata, {})`
		`+`
		`+ @mock.patch('kojihub.get_build_notification_blocks')`
		`+ @mock.patch('kojihub.get_tag_id')`
		`+ @mock.patch('kojihub.get_package_id')`
		`+ def test_createNotificationBlock_unauthentized(self, get_package_id, get_tag_id,`
		`+ get_build_notification_blocks):`
		`+ user_id = 1`
		`+ package_id = 234`
		`+ tag_id = 345`
		`+ self.exports.getLoggedInUser.return_value = None`
		`+`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.createNotificationBlock(user_id, package_id, tag_id)`
		`+`
		`+ self.assertEqual(len(self.inserts), 0)`
		`+`
		`+ @mock.patch('kojihub.get_build_notification_blocks')`
		`+ @mock.patch('kojihub.get_tag_id')`
		`+ @mock.patch('kojihub.get_package_id')`
		`+ def test_createNotificationBlock_invalid_user(self, get_package_id, get_tag_id,`
		`+ get_build_notification_blocks):`
		`+ user_id = 2`
		`+ package_id = 234`
		`+ tag_id = 345`
		`+ self.exports.getLoggedInUser.return_value = {'id': 1}`
		`+ self.exports.getUser.return_value = None`
		`+`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.createNotificationBlock(user_id, package_id, tag_id)`
		`+`
		`+ self.assertEqual(len(self.inserts), 0)`
		`+`
		`+ @mock.patch('kojihub.get_build_notification_blocks')`
		`+ @mock.patch('kojihub.get_tag_id')`
		`+ @mock.patch('kojihub.get_package_id')`
		`+ def test_createNotificationBlock_no_perm(self, get_package_id, get_tag_id,`
		`+ get_build_notification_blocks):`
		`+ user_id = 2`
		`+ package_id = 234`
		`+ tag_id = 345`
		`+ self.exports.getLoggedInUser.return_value = {'id': 1, 'name': 'a'}`
		`+ self.exports.getUser.return_value = {'id': 2, 'name': 'b'}`
		`+ self.exports.hasPerm.return_value = False`
		`+`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.createNotificationBlock(user_id, package_id, tag_id)`
		`+`
		`+ self.assertEqual(len(self.inserts), 0)`
		`+`
		`+ @mock.patch('kojihub.get_build_notification_blocks')`
		`+ @mock.patch('kojihub.get_tag_id')`
		`+ @mock.patch('kojihub.get_package_id')`
		`+ def test_createNotificationBlock_invalid_pkg(self, get_package_id, get_tag_id,`
		`+ get_build_notification_blocks):`
		`+ user_id = 2`
		`+ package_id = 234`
		`+ tag_id = 345`
		`+ self.exports.getLoggedInUser.return_value = {'id': 2, 'name': 'a'}`
		`+ self.exports.getUser.return_value = {'id': 2, 'name': 'a'}`
		`+ get_package_id.side_effect = ValueError`
		`+`
		`+ with self.assertRaises(ValueError):`
		`+ self.exports.createNotificationBlock(user_id, package_id, tag_id)`
		`+`
		`+ self.assertEqual(len(self.inserts), 0)`
		`+`
		`+ @mock.patch('kojihub.get_build_notification_blocks')`
		`+ @mock.patch('kojihub.get_tag_id')`
		`+ @mock.patch('kojihub.get_package_id')`
		`+ def test_createNotificationBlock_invalid_tag(self, get_package_id, get_tag_id,`
		`+ get_build_notification_blocks):`
		`+ user_id = 2`
		`+ package_id = 234`
		`+ tag_id = 345`
		`+ self.exports.getLoggedInUser.return_value = {'id': 2, 'name': 'a'}`
		`+ self.exports.getUser.return_value = {'id': 2, 'name': 'a'}`
		`+ get_package_id.return_value = package_id`
		`+ get_tag_id.side_effect = ValueError`
		`+`
		`+ with self.assertRaises(ValueError):`
		`+ self.exports.createNotificationBlock(user_id, package_id, tag_id)`
		`+`
		`+ self.assertEqual(len(self.inserts), 0)`
		`+`
		`+ @mock.patch('kojihub.get_build_notification_blocks')`
		`+ @mock.patch('kojihub.get_tag_id')`
		`+ @mock.patch('kojihub.get_package_id')`
		`+ def test_createNotificationBlock_exists(self, get_package_id, get_tag_id,`
		`+ get_build_notification_blocks):`
		`+ user_id = 2`
		`+ package_id = 234`
		`+ tag_id = 345`
		`+ self.exports.getLoggedInUser.return_value = {'id': 2, 'name': 'a'}`
		`+ self.exports.getUser.return_value = {'id': 2, 'name': 'a'}`
		`+ get_package_id.return_value = package_id`
		`+ get_tag_id.return_value = tag_id`
		`+ get_build_notification_blocks.return_value = [{`
		`+ 'package_id': package_id,`
		`+ 'tag_id': tag_id,`
		`+ }]`
		`+`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.createNotificationBlock(user_id, package_id, tag_id)`
		`+`
		`+ self.assertEqual(len(self.inserts), 0)`
		`+`
		`+ #####################`
		`+ # Delete notification`
		`+ @mock.patch('kojihub._dml')`
		`+ def test_deleteNotificationBlock(self, _dml):`
		`+ user_id = 752`
		`+ n_id = 543`
		`+ self.exports.getBuildNotificationBlock.return_value = {'user_id': user_id}`
		`+`
		`+ self.exports.deleteNotificationBlock(n_id)`
		`+`
		`+ self.exports.getBuildNotificationBlock.assert_called_once_with(n_id, strict=True)`
		`+ self.exports.getLoggedInUser.assert_called_once_with()`
		`+ _dml.assert_called_once()`
		`+`
		`+ def test_deleteNotification_missing(self):`
		`+ user_id = 752`
		`+ n_id = 543`
		`+ self.exports.getBuildNotificationBlock.side_effect = koji.GenericError`
		`+`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.deleteNotificationBlock(n_id)`
		`+`
		`+ self.exports.getBuildNotificationBlock.assert_called_once_with(n_id, strict=True)`
		`+`
		`+ def test_deleteNotification_not_logged(self):`
		`+ user_id = 752`
		`+ n_id = 543`
		`+ self.exports.getBuildNotificationBlock.return_value = {'user_id': user_id}`
		`+ self.exports.getLoggedInUser.return_value = None`
		`+ #self.set_queries = ([`
		`+ # [{'user_id': 5, 'email': 'owner_name@%s' % self.context.opts['EmailDomain']}],`
		`+ #])`
		`+`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.deleteNotificationBlock(n_id)`
		`+`
		`+ self.exports.getBuildNotificationBlock.assert_called_once_with(n_id, strict=True)`
		`+ self.assertEqual(len(self.inserts), 0)`
		`+ self.assertEqual(len(self.updates), 0)`
		`+ self.assertEqual(len(self.queries), 0)`
		`+`
		`+ @mock.patch('kojihub._dml')`
		`+ def test_deleteNotification_no_perm(self, _dml):`
		`+ user_id = 752`
		`+ n_id = 543`
		`+ self.exports.getBuildNotificationBlock.return_value = {'user_id': user_id}`
		`+ self.exports.getLoggedInUser.return_value = {'id': 1}`
		`+ self.exports.hasPerm.return_value = False`
		`+`
		`+ with self.assertRaises(koji.GenericError):`
		`+ self.exports.deleteNotificationBlock(n_id)`
		`+`
		`+ self.exports.getBuildNotificationBlock.assert_called_once_with(n_id, strict=True)`
		`+ _dml.assert_not_called()`

tkopecek commented 3 years ago

What API should handle this? Should we create new calls/CLI or modify existing create/list/deleteNotification ?
It looks to me better to add new calls, but it can be confusing for users what is the difference here.

pretty please pagure-ci rebuild

3 years ago

mikem commented 3 years ago

Should we create new calls/CLI or modify existing create/list/deleteNotification ?

I think separate calls and commands. Seems like it would be messy to cram the two things together.

I can see a possible exception for the list-notifications cli command. It might make sense for that to list blocks.

1 new commit added

API/CLI/tests for notification blocks

3 years ago

tkopecek commented 3 years ago

Added getBuildNotificationBlocks, getBuildNotificationBlock, createNotificationBlock, deleteNotificationBlock to API and block-notification, unblock-notification to CLI + extended list-notifications to show also blocks. I think, that potential edit-notification-block is not much useful here, but it can be added also.

mikem commented 2 years ago

Testing gives:

Traceback (most recent call last):
  File "/home/mike/Devel/koji/koji/hub/kojixmlrpc.py", line 236, in _wrap_handler
    response = handler(environ)
  File "/home/mike/Devel/koji/koji/hub/kojixmlrpc.py", line 279, in handle_rpc
    return self._dispatch(method, params)
  File "/home/mike/Devel/koji/koji/hub/kojixmlrpc.py", line 316, in _dispatch
    ret = koji.util.call_with_argcheck(func, params, opts)
  File "/home/mike/Devel/koji/koji/koji/util.py", line 263, in call_with_argcheck
    return func(*args, **kwargs)
  File "/home/mike/Devel/koji/koji/hub/kojihub.py", line 7610, in get_notification_recipients
    optouts = [r['user_id'] for r in query.execute()]
  File "/home/mike/Devel/koji/koji/hub/kojihub.py", line 8206, in execute
    data = _multiRow(query, self.values, (self.aliases or self.columns))
  File "/home/mike/Devel/koji/koji/hub/kojihub.py", line 4680, in _multiRow
    return [dict(zip(fields, row)) for row in _fetchMulti(query, values)]
  File "/home/mike/Devel/koji/koji/hub/kojihub.py", line 4652, in _fetchMulti
    c.execute(query, values)
  File "/home/mike/Devel/koji/koji/koji/db.py", line 136, in execute
    ret = self.cursor.execute(operation, parameters)
ProgrammingError: can't adapt type 'set'

mikem commented 2 years ago

-    user_ids = set([r['user_id'] for r in recipients])
+    user_ids = list(set([r['user_id'] for r in recipients]))

1 new commit added

use list for db instead of set

2 years ago

tkopecek commented 2 years ago

added commit

Anyway, interesting as it is not problem not only for me (f29) , but also for jenkins' envs.

1 new commit added

fix tests

2 years ago

rebased onto 45357e7

2 years ago

mikem commented 2 years ago

Anyway, interesting as it is not problem not only for me (f29) , but also for jenkins' envs.

I got that by calling get_notification_recipients directly (added a temporary export for it and called with fakehub script). I.e. not any of the unit tests.

Edited 2 years ago by mikem

mikem commented 2 years ago

flake8 found a few items

tests/test_hub/test_notifications.py:450:9: F841 local variable 'user_id' is assigned to but never used
tests/test_hub/test_notifications.py:790:5: F811 redefinition of unused 'test_deleteNotification_missing' from line 449
tests/test_hub/test_notifications.py:791:9: F841 local variable 'user_id' is assigned to but never used
tests/test_hub/test_notifications.py:800:5: F811 redefinition of unused 'test_deleteNotification_not_logged' from line 459
tests/test_hub/test_notifications.py:817:5: F811 redefinition of unused 'test_deleteNotification_no_perm' from line 476

I think the stray user_id assigns are probably copied from elsewhere, but the test redefinitions are a problem.

https://github.com/mikem23/koji-playground/commits/pagure/pr/1417

mikem commented 2 years ago

If the above changes look good, I can merge this.

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

tkopecek commented 2 years ago

:thumbsup:

mikem commented 2 years ago

Commit 916d03e fixes this pull-request

Pull-Request has been merged by mikem

2 years ago