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

RESOLVED FIXED

Status

()

RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: dkl, Assigned: dkl)

Tracking

({conduit-triaged})

Production
conduit-triaged

Details

(Whiteboard: [phabricator-backlog])

Attachments

(1 attachment)

(Assignee)

Description

7 months ago
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
(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?
^

ckolos / glob, feel free to weigh in. dkl?
Flags: needinfo?(dkl)
Keywords: conduit-triaged
Whiteboard: [phabricator-backlog]
(Assignee)

Comment 3

7 months ago
(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
Flags: needinfo?(dkl)
(Assignee)

Comment 4

7 months ago
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
Assignee: nobody → dkl
Status: NEW → ASSIGNED
(Assignee)

Comment 5

7 months ago
Created attachment 8987993 [details] [review]
github pr #629
Component: Phabricator → Extensions: PhabBugz
Product: Conduit → bugzilla.mozilla.org
Version: unspecified → Production
(Assignee)

Comment 7

7 months ago
Merged to master.
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.