#1173 hub: [groupListRemove] raise Error when no group for tag
Merged 2 years ago by tkopecek. Opened 3 years ago by julian8628.
julian8628/koji issue/1090  into  master

file modified
+20 -1
@@ -1658,6 +1658,16 @@ 

  

  def grplist_remove(taginfo, grpinfo, force=False):

      """Remove group from the list for tag

+     Permission required: admin

+ 

+     :param taginfo: tag id or name which group is removed from

+     :type taginfo: int or str

+     :param grpinfo: group id or name which is removed

+     :type grpinfo: int or str

+     :param bool force: If False(default), GenericException will be raised when

+                        no group found in the list for tag. If True, revoking

+                        will be force to execute, no matter if the relationship

+                        exists.

  

      Really this shouldn't be used except in special cases

      Most of the time you really want to use the block or unblock functions
@@ -1667,13 +1677,22 @@ 

      _grplist_remove(taginfo, grpinfo, force)

  

  

- def _grplist_remove(taginfo, grpinfo, force):

+ def _grplist_remove(taginfo, grpinfo, force=False):

      """grplist_remove without permission check"""

      tag = get_tag(taginfo, strict=True)

      group = lookup_group(grpinfo, strict=True)

      tag_id = tag['id']

      grp_id = group['id']

      clauses = ['group_id=%(grp_id)s', 'tag_id=%(tag_id)s']

+     if not force:

+         query = QueryProcessor(columns=['group_id', 'tag_id', 'active'],

+                                tables=['group_config'],

+                                values=locals(),

+                                clauses=clauses + [eventCondition(None)])

+         old_grp_conf = query.executeOne()

+         if not old_grp_conf:

+             raise koji.GenericError("No group: %s found for tag: %s"

+                                     % (tag['name'], group['name']))

      update = UpdateProcessor('group_config', values=locals(), clauses=clauses)

      update.make_revoke()

      update.execute()

@@ -18,6 +18,13 @@ 

          self.queries.append(query)

          return query

  

+     def getEmptyQuery(self, *args, **kwargs):

+         query = QP(*args, **kwargs)

+         query.execute = mock.MagicMock()

+         query.execute.return_value = None

+         self.queries.append(query)

+         return query

+ 

      def getInsert(self, *args, **kwargs):

          insert = IP(*args, **kwargs)

          insert.execute = mock.MagicMock()
@@ -30,6 +37,11 @@ 

          self.updates.append(update)

          return update

  

+     def reset_db_processors(self):

+         self.queries = []

+         self.updates = []

+         self.inserts = []

+ 

      def setUp(self):

          self.context = mock.patch('kojihub.context').start()

          self.get_tag = mock.patch('kojihub.get_tag').start()
@@ -171,13 +183,42 @@ 

          self.lookup_group.assert_called_once_with(group, strict=True)

  

          # db

+         self.assertEqual(len(self.queries), 1)

          self.assertEqual(len(self.inserts), 0)

          self.assertEqual(len(self.updates), 1)

+         query = self.queries[0]

+         self.assertEqual(' '.join(str(query).split()),

+                          'SELECT group_id, tag_id, active FROM group_config'

+                          ' WHERE (group_id=%(grp_id)s)'

+                          ' AND (tag_id=%(tag_id)s)'

+                          ' AND ((active = TRUE))')

          update = self.updates[0]

          self.assertEqual(update.table, 'group_config')

          self.assertEqual(update.data, {'revoke_event': 42, 'revoker_id': 24})

          self.assertEqual(update.rawdata, {'active': 'NULL'})

  

+         # no group for tag found

+         self.reset_db_processors()

+         with mock.patch('kojihub.QueryProcessor', side_effect=self.getEmptyQuery):

+             with self.assertRaises(koji.GenericError) as cm:

+                 kojihub.grplist_remove(tag, group)

+ 

+         self.assertEqual(len(self.queries), 1)

+         self.assertEqual(len(self.inserts), 0)

+         self.assertEqual(len(self.updates), 0)

+         self.assertEqual(cm.exception.args[0],

+                          'No group: tag found for tag: group')

+ 

+         # force = True

+         self.reset_db_processors()

+         with mock.patch('kojihub.QueryProcessor',

+                         side_effect=self.getEmptyQuery):

+             kojihub.grplist_remove(tag, group, force=True)

+ 

+         self.assertEqual(len(self.queries), 0)

+         self.assertEqual(len(self.inserts), 0)

+         self.assertEqual(len(self.updates), 1)

+ 

      def test_grplist_unblock(self):

          # identical with test_grplist_add except blocked=True

          tag = 'tag'

fixes: #1090

using force=True to avoid throwing error when no item found in DB

docstrings! :heart:

It's customary to have these at the very end, mind moving "Permission required: admin" up into its own paragraph above the Sphinx lines?

rebased onto c77573e061143e1c347ca25b6d212b2a8abcbe93

3 years ago

rebased onto e4272fb9943b886d4364da38a85717ea8a702fcc

3 years ago

This seems to be a substitution misfire

It's not completely clear what force should mean here. I think this is a relic of some previous behavior in the function.

In any case, I'm not sure that recycling this param to mean "not strict" is the right thing here.

rebased onto bad108f

3 years ago

This seems to be a substitution misfire

my bad :dizzy_face:
updated

It's not completely clear what force should mean here. I think this is a relic of some previous behavior in the function.
In any case, I'm not sure that recycling this param to mean "not strict" is the right thing here.

IMO,
- strict: for get* APIs, to raise Error when input/output not found
- force: for create/update/delete APIs, to avoid raising Error and do the action when some conflict happens

1 new commit added

  • fix unavailable id column
3 years ago

Commit fa7c207 fixes this pull-request

Pull-Request has been merged by tkopecek

2 years ago