On Tue, 2021-08-03 at 01:27 +1000, Daniel Axtens wrote: > As with the UI. > > Signed-off-by: Daniel Axtens <d...@axtens.net> > --- > patchwork/tests/test_xmlrpc.py | 90 ++++++++++++++++++++++++++++++++++ > patchwork/views/xmlrpc.py | 4 ++ > 2 files changed, 94 insertions(+) > > diff --git a/patchwork/tests/test_xmlrpc.py b/patchwork/tests/test_xmlrpc.py > index 4726fdffa5d5..eea0b4eaf560 100644 > --- a/patchwork/tests/test_xmlrpc.py > +++ b/patchwork/tests/test_xmlrpc.py > @@ -10,6 +10,9 @@ from django.conf import settings > from django.test import LiveServerTestCase > from django.urls import reverse > > +from patchwork.models import Person > +from patchwork.models import Project > +from patchwork.models import State > from patchwork.tests import utils > > > @@ -81,6 +84,93 @@ class XMLRPCAuthenticatedTest(LiveServerTestCase): > self.assertTrue(result['archived']) > > > +@unittest.skipUnless(settings.ENABLE_XMLRPC, > + 'requires xmlrpc interface (use the ENABLE_XMLRPC ' > + 'setting)') > +class XMLRPCStateSettingTest(LiveServerTestCase): > + > + def url_for_user(self, user): > + return ('http://%s:%s@' + self.url[7:]) % \ > + (user.username, user.username) > + > + def setUp(self): > + self.url = self.live_server_url + reverse('xmlrpc') > + # url is of the form http://localhost:PORT/PATH > + # strip the http and replace it with the username/passwd of a user. > + self.projects = {} > + self.maintainers = {} > + self.delegates = {} > + self.submitters = {} > + self.patches = {} > + self.rpcs = {} > + > + for project_type in (Project.SUBMITTER_NO_STATE_CHANGES, > + Project.SUBMITTER_ALL_STATE_CHANGES): > + project = utils.create_project( > + submitter_state_change_rules=project_type) > + self.projects[project_type] = project > + self.maintainers[project_type] = utils.create_maintainer(project) > + submitter = utils.create_user(project) > + self.submitters[project_type] = submitter > + delegate = utils.create_user(project) > + self.delegates[project_type] = delegate > + > + self.rpcs[project_type] = { > + 'maintainer': ServerProxy(self.url_for_user( > + self.maintainers[project_type])), > + 'delegate': ServerProxy(self.url_for_user(delegate)), > + 'submitter': ServerProxy(self.url_for_user(submitter)), > + } > + > + patch = utils.create_patch(project=project, > + submitter=Person.objects.get( > + user=submitter), > + delegate=delegate) > + self.patches[project_type] = patch > + > + utils.create_state(name="New") > + utils.create_state(name="RFC") > + > + def tearDown(self): > + for project_type in self.rpcs: > + rpc_dict = self.rpcs[project_type] > + for user in rpc_dict: > + rpc_dict[user].close() > + > + def can_set_state(self, patch, rpc): > + new_state = State.objects.get(name="New") > + rfc_state = State.objects.get(name="RFC") > + patch.state = new_state > + patch.save() > + > + result = rpc.patch_get(patch.id) > + self.assertEqual(result['state_id'], new_state.id) > + > + try: > + rpc.patch_set(patch.id, {'state': rfc_state.id}) > + except xmlrpc_client.Fault: > + return False > + > + # reload the patch > + result = rpc.patch_get(patch.id) > + self.assertEqual(result['state_id'], rfc_state.id) > + return True > + > + def test_allset(self): > + rpc_dict = self.rpcs[Project.SUBMITTER_ALL_STATE_CHANGES] > + patch = self.patches[Project.SUBMITTER_ALL_STATE_CHANGES] > + self.assertTrue(self.can_set_state(patch, rpc_dict['maintainer'])) > + self.assertTrue(self.can_set_state(patch, rpc_dict['delegate'])) > + self.assertTrue(self.can_set_state(patch, rpc_dict['submitter'])) > + > + def test_noset(self): > + rpc_dict = self.rpcs[Project.SUBMITTER_NO_STATE_CHANGES] > + patch = self.patches[Project.SUBMITTER_NO_STATE_CHANGES] > + self.assertTrue(self.can_set_state(patch, rpc_dict['maintainer'])) > + self.assertTrue(self.can_set_state(patch, rpc_dict['delegate'])) > + self.assertFalse(self.can_set_state(patch, rpc_dict['submitter'])) > + > +
Do we need this level of validation at this layer? LiveServerTests are expensive to setup. Rather than doing this, could we simply have a positive and negative test case (ideally folded into 'XMLRPCPatchTest', but that might be easier said than done) and then rely on unit tests for 'can_set_state' to ensure we have a full test matrix? > class XMLRPCModelTestMixin(object): > > def create_multiple(self, count): > diff --git a/patchwork/views/xmlrpc.py b/patchwork/views/xmlrpc.py > index 6701bf20f386..d73cfa7a8441 100644 > --- a/patchwork/views/xmlrpc.py > +++ b/patchwork/views/xmlrpc.py > @@ -713,6 +713,10 @@ def patch_set(user, patch_id, params): > if not patch.is_editable(user): > raise Exception('No permissions to edit this patch') > > + if 'state' in params: > + if not patch.can_set_state(user): > + raise Exception('No permissions to set patch state') > + [Thinking out loud] I wonder if the additional verbosity provided by having a separate check for this one validation is worth the additional complexity in code (~90 lines of code and tests in this patch alone). Perhaps rather than having a separate check, 'is_editable' could raise a (custom) exception with the additional details about what validation, if any, failed, rather than returning a simple boolean. 'is_editable' isn't a Django-provided thing fwict, so we don't have a particular contract we need to adhere to. wdyt? Stephen > for (k, v) in params.items(): > if k not in ok_params: > continue _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork