Page MenuHomePhorge

Disallow awarding a badge without selecting recipient
ClosedPublic

Authored by aklapper on Jul 22 2024, 22:09.
Tags
None
Referenced Files
F3801597: D25740.1745957301.diff
Mon, Apr 28, 20:08
F3748963: D25740.1745789802.diff
Sat, Apr 26, 21:36
F3717071: D25740.1745692970.diff
Fri, Apr 25, 18:42
F3717056: D25740.1745692299.diff
Fri, Apr 25, 18:31
F3711891: D25740.1745662259.diff
Fri, Apr 25, 10:10
F3711837: D25740.1745659711.diff
Fri, Apr 25, 09:28
F3709154: D25740.1745644298.diff
Fri, Apr 25, 05:11
F3702011: D25740.1745613597.diff
Thu, Apr 24, 20:39

Details

Summary

Do not continue on missing fields (in this case: the badge recipient) in PhabricatorBadgesEditRecipientsController.

Closes T15827

Test Plan

See steps in T15827.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php
58–59

Probably here we should also add a ->setError($e_message) or something.

Or, a $form->setFormErrors($errors)

In any case, that error should be probably obtained from a try catch over that applyTransactions() call.

Maybe we can take inspiration from PhabricatorProjectColumnEditController or anything similar


Otherwise it's probably an un-managed exception that is put in the log (and I know you do not like it ihih)

Setting as "new validations probably need to be managed". Thanks again

This revision now requires changes to proceed.Jul 23 2024, 07:57

Rebase after merging rP59678094fb777aed3c3eb66e8075c082b3dc393f

Too lazy, just doing the same as in rP43d3fd9eac6e8e44d8a60110d8ad6f01bb240979: Turn "Required" red after the first attempt

lgtm

I thought there was a ->setFormErrors($errors) but it's not the case. Nice as-is. Tested thanks :3

This revision is now accepted and ready to land.Jul 29 2024, 19:38

It's perfect as-is. Just adding last notes since I'm in verbose mode

src/applications/badges/controller/PhabricatorBadgesEditRecipientsController.php
32

Bonus point:

Avoiding duplicates was probably the original intention.

Otherwise, $award_phids is just an exact copy of $add_recipients.

34
41–45

Maybe?

  • Fix the $award_phids thingy to do the $award_phids[$phid] thing
  • I prefer to keep if (!$errors) as that part of code is supposed to check for (any) errors