#1417 notification's optouts
Merged 2 years ago by mikem. Opened 3 years ago by tkopecek.
tkopecek/koji issue1204  into  master

file modified
+117 -17
@@ -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)

@@ -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');

file modified
+7
@@ -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;

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

@@ -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

@@ -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

@@ -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):

@@ -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()

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

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

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.

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'
-    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

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

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.

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

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

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

2 years ago

Commit 916d03e fixes this pull-request

Pull-Request has been merged by mikem

2 years ago