On Mon, 2019-09-23 at 15:39 -0500, Bjorn Helgaas wrote: > On Sat, Sep 21, 2019 at 07:30:46PM +0100, Stephen Finucane wrote: > > There have been reports of people being unable to delegate patches to > > themselves, despite being a maintainer or the project to which the patch > > is associated. > > > > The issue is a result of how we do a check for whether the user is a > > maintainer of the patch's project [1]. This check is checking if a given > > 'User.id' is in the list of items referenced by > > 'Project.maintainer_project'. However, 'Project.maintainer_project' is a > > backref to 'UserProfile.maintainer_projects'. This means we're comparing > > 'User.id' and 'UserProfile.id'. Boo. > > > > This wasn't seen in testing since we've had a post-save callback [2] for > > some > > time that ensures we always create a 'UserProfile' object whenever we > > create a > > 'User' object. This also means we won't have an issue on deployments > > initially > > deployed after that post-save callback was added, a 'User' with id=N will > > always have a corresponding 'UserProfile' with id=N. However, that's not > > true > > for older deployments such as the ozlabs.org one. > > I tried the instance at https://patchwork.kernel.org/project/linux-pci/list/ > to see if it was new enough to work without this fix. But it also > fails, slightly differently: > > $ git config -l | grep "^pw" > pw.server=https://patchwork.kernel.org/api/1.1 > pw.project=linux-pci > pw.token=... > > $ git-pw patch update --delegate helgaas 11151519 > More than one delegate found: helgaas > > Is this another manifestation of the same bug or something else?
This is a different issue and, unlike the other one, is more feature than bug. This is happening because the search for a particular user is returning multiple matches. We match on username, first name, last name and email, so I imagine you have multiple user accounts on the instance and there might be a conflict between an email address of one account and a username of another? (Let me know if this isn't the case). The easy solution is to use a more specific match. I'd suggest just using the email address associated with your user account ([1] suggests this is 'bhelg...@google.com'). We could also support lookup by user ID (which would guarantee a single match) but I haven't added that to git- pw yet since it didn't seem that usable. Stephen [1] https://patchwork.kernel.org/project/linux-pci/ > > [1] > > https://github.com/getpatchwork/patchwork/blob/89c924f9bc/patchwork/api/patch.py#L108-L111 > > [2] > > https://github.com/getpatchwork/patchwork/blob/89c924f9bc/patchwork/models.py#L204-L210 > > > > Signed-off-by: Stephen Finucane <step...@that.guru> > > Closes: #313 > > Reported-by: Bjorn Helgaas <helg...@kernel.org> > > --- > > patchwork/api/patch.py | 4 ++-- > > patchwork/tests/api/test_patch.py | 24 ++++++++++++++++++++++++ > > 2 files changed, 26 insertions(+), 2 deletions(-) > > > > diff --git a/patchwork/api/patch.py b/patchwork/api/patch.py > > index c9360308..d1c9904d 100644 > > --- a/patchwork/api/patch.py > > +++ b/patchwork/api/patch.py > > @@ -105,8 +105,8 @@ class > > PatchListSerializer(BaseHyperlinkedModelSerializer): > > if not value: > > return value > > > > - if not self.instance.project.maintainer_project.filter( > > - id=value.id).exists(): > > + if not value.profile.maintainer_projects.only('id').filter( > > + id=self.instance.project.id).exists(): > > raise ValidationError("User '%s' is not a maintainer for > > project " > > "'%s'" % (value, self.instance.project)) > > return value > > diff --git a/patchwork/tests/api/test_patch.py > > b/patchwork/tests/api/test_patch.py > > index 82ae0184..edae9851 100644 > > --- a/patchwork/tests/api/test_patch.py > > +++ b/patchwork/tests/api/test_patch.py > > @@ -284,6 +284,30 @@ class TestPatchAPI(utils.APITestCase): > > self.assertContains(resp, 'Expected one of: %s.' % state.name, > > status_code=status.HTTP_400_BAD_REQUEST) > > > > + def test_update_legacy_delegate(self): > > + """Regression test for bug #313.""" > > + project = create_project() > > + state = create_state() > > + patch = create_patch(project=project, state=state) > > + user_a = create_maintainer(project) > > + > > + # create a user (User), then delete the associated UserProfile and > > save > > + # the user to ensure a new profile is generated > > + user_b = create_user() > > + self.assertEqual(user_b.id, user_b.profile.id) > > + user_b.profile.delete() > > + user_b.save() > > + user_b.profile.maintainer_projects.add(project) > > + user_b.profile.save() > > + self.assertNotEqual(user_b.id, user_b.profile.id) > > + > > + self.client.force_authenticate(user=user_a) > > + resp = self.client.patch(self.api_url(patch.id), > > + {'delegate': user_b.id}) > > + self.assertEqual(status.HTTP_200_OK, resp.status_code, resp) > > + self.assertEqual(Patch.objects.get(id=patch.id).state, state) > > + self.assertEqual(Patch.objects.get(id=patch.id).delegate, user_b) > > + > > def test_update_invalid_delegate(self): > > """Update patch with invalid fields. > > > > -- > > 2.21.0 > > _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork