1469373 - Phabbugz fails with undefined error when phab user without linked BMO account accepts BMO linked revision

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Phabricator Integration, defect)

Description

[David Lawrence [:dkl]]

Error:
Can't call method \"id\" on an undefined value at /app/extensions/PhabBugz/lib/Fe
ed.pm line 498.

Log in to a local Phabricator account such as an admin account that is not linked to any BMO user through auth delegation. Accept changes on a revision that has a bug id attached to it. BMO will attempt to set review+ on the attachment for the revision but will fail with the above error when no mapped account exists.

Normally this will be rare as we only have a couple of accounts in Phabricator that are not linked. But in case this does happen, I think we should just fall back to setting review+ from phab-bot instead of trying to show the real BMO user. This will keep the Phabbugz feed from stalling. The developer can click through to the revision to see who the real reviewer was.

dkl

Comment 1

[Steven MacLeod [:smacleod]]

(In reply to David Lawrence [:dkl] from comment #0)
> Normally this will be rare as we only have a couple of accounts in
> Phabricator that are not linked. But in case this does happen, I think we
> should just fall back to setting review+ from phab-bot instead of trying to
> show the real BMO user. This will keep the Phabbugz feed from stalling. The
> developer can click through to the revision to see who the real reviewer was.

As a matter of policy I don't think anyone should be reviewing with their admin account. Probably worth alerting on as well as whatever we do to have it move past, no?

Comment 2

[Steven MacLeod [:smacleod]]

^

ckolos / glob, feel free to weigh in. dkl?

Comment 3

[David Lawrence [:dkl]]

(In reply to Steven MacLeod [:smacleod] from comment #1)
> (In reply to David Lawrence [:dkl] from comment #0)
> > Normally this will be rare as we only have a couple of accounts in
> > Phabricator that are not linked. But in case this does happen, I think we
> > should just fall back to setting review+ from phab-bot instead of trying to
> > show the real BMO user. This will keep the Phabbugz feed from stalling. The
> > developer can click through to the revision to see who the real reviewer was.
> 
> As a matter of policy I don't think anyone should be reviewing with their
> admin account. Probably worth alerting on as well as whatever we do to have
> it move past, no?

I agree that admin accounts should not be performing reviews especially in the way we have revisions and bugs tied 
together in some cases. I do not think it is worth coding it in such a way to enforce this by not allowing an admin
account to submit comments, etc. But we could definitely make it very obvious when 1) a user has admin rights and 2) 
the account does not have a link to a BMO account. Either through a banner or some other warning flag.

dkl

Comment 4

[David Lawrence [:dkl]]

Leaving this bug about the error on phabbugs side where it needs to more gracefully fail when the user is not linked from Pahab to BMO. 

I will file another bug about warning the Phab user about their admin rights when viewing a revision.

dkl

Comment 5

[David Lawrence [:dkl]]

Attachment: github pr #629

Comment 6

[:glob ✱]

dkl filed bug 1471341

Comment 7

[David Lawrence [:dkl]]

Merged to master.