Paths

Table of Contentst

Differential D25636

Fix exception awarding empty badge to user
Needs ReviewPublic
Actions

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

Details

Reviewers

None

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15825: Exception awarding badge to user without selecting a badge

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

aklapper created this revision.May 12 2024, 08:16

Owners added a reviewer: O1: Blessed Committers.May 12 2024, 08:16

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptMay 12 2024, 08:16

aklapper requested review of this revision.May 12 2024, 08:16

Harbormaster completed remote builds in B1252: Diff 1977.May 12 2024, 08:16

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.

Harbormaster completed remote builds in B1255: Diff 1982.May 13 2024, 18:43

Remove variable definition now unneeded outside of the if clause

Harbormaster completed remote builds in B1256: Diff 1983.May 13 2024, 18:50

In D25636#17842, @aklapper wrote:

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

In D25636#17849, @valerio.bozzolan wrote:

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

Revision Contents
Changeset List

Path

Size

Packages

src/

applications/

badges/

controller/

PhabricatorBadgesAwardController.php

61 lines

O1: Blessed Committers

Diff 1983

View Options

src/applications/badges/controller/PhabricatorBadgesAwardController.php

Fix exception awarding empty badge to userNeeds ReviewPublicActions