Page MenuHomePhorge

Fix exception awarding empty badge to user
Needs ReviewPublic

Authored by aklapper on Sun, May 12, 08:16.
Tags
None
Referenced Files
F2195971: D25636.1716163736.diff
Sun, May 19, 00:08
Unknown Object (File)
Sat, May 18, 06:31
Unknown Object (File)
Sat, May 18, 04:21
Unknown Object (File)
Fri, May 17, 16:25
Unknown Object (File)
Thu, May 16, 23:41
Unknown Object (File)
Thu, May 16, 16:30
Unknown Object (File)
Thu, May 16, 16:00
Unknown Object (File)
Thu, May 16, 15:54

Details

Summary

When awarding a badge to a user without having actually selected a batch to award, do not fail on running a database query with no parameter. Instead, silently fail without an error and reload the page, similar to the behavior of Add Recipients on /badges/recipients/1/ in this case.
Do not throw an Aphront404Response as it would make the Award Badge button in the dialog inaccessible and keep the dysfunctional overlay dialog displayed.

EXCEPTION: (AphrontParameterQueryException) Array for %Ls conversion is empty. Query: badges.phid IN (%Ls) at [<phorge>/src/infrastructure/storage/xsprintf/qsprintf.php:383]

Closes T15825

Test Plan

Go to a user's badges, click the Award Badge button, in the dialog do not select any Badge and click the Award button.

Diff Detail

Repository
rP Phorge
Branch
badgesEmptyException (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1256
Build 1256: arc lint + arc unit

Event Timeline

I think the 404 is possibly better then "silently fail without error".

We have ways to signal errors in form inputs in most places - e.g. https://we.phorge.it/source/phorge/browse/master/src/applications/people/controller/PhabricatorPeopleNewController.php$167

Thanks for the hints how to improve this! Appreciated.

I think the 404 is possibly better then "silently fail without error".

Indeed but the 404 just is not exposed anywhere to the user. I'm afraid that code never did what it was supposed to. (Leaving that 404 in with that previous version of my patch would have just disabled the Award button and blocked the page until the user reloads the page.)

Here's a revision which does not throw a red box via ->setFormErrors() at the user but at least it is displaying the Required text and turning it into bold red after clicking Award without having defined a recipient. So the line $errors[] = pht('Badge name is required.'); is currently never ever shown.

Ideally this would be done via validateTransactions using ->setFormErrors($errors) on a PHUIObjectBoxView but all seems quite broken for badges anyway, see T15827.

Remove variable definition now unneeded outside of the if clause

Ideally this would be done via validateTransactions using ->setFormErrors($errors) on a PHUIObjectBoxView but all seems quite broken for badges anyway, see T15827.

Nice catch. Basically the question is, why is this not working?

https://we.phorge.it/source/phorge/browse/master/src/applications/badges/xaction/PhabricatorBadgesBadgeNameTransaction.php;8a3b70aa7d4c3376dabf5dfd5d3a0df68998711c$36-39