Page MenuHomePhorge

Fix exception awarding empty badge to user
ClosedPublic

Authored by aklapper on May 12 2024, 08:16.

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

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

I don't know but that's stuff one day to investigate in T15827 instead, while this patch for now fixes the exception? :P

Agree with your considerations and tested 🌈

This revision is now accepted and ready to land.Jun 28 2024, 11:18