Page MenuHomePhabricator

Remove needRecipients and needAwards from Badges
ClosedPublic

Authored by chad on Mar 2 2017, 2:25 AM.
Tags
None
Attached Files
F10754778: D17447.id41970.diff
Tue, May 24, 10:24 PM
F10753572: D17447.id41970.diff
Tue, May 24, 1:35 PM
Unknown Object (File)
Tue, May 24, 9:20 AM
Unknown Object (File)
Tue, May 24, 8:41 AM
Unknown Object (File)
Apr 14 2017, 11:25 PM
Unknown Object (File)
Apr 14 2017, 6:03 PM
Unknown Object (File)
Apr 14 2017, 12:34 PM
Unknown Object (File)
Apr 14 2017, 3:13 AM
Subscribers

Details

Summary

Fixes T10798. Separates these two since they don't need to be combined and it allows for more flexibility / scalability.

Test Plan
  • Add Badge
  • Edit Badge
  • Add myself as Recipient
  • Remove myself
  • Go to my profile
  • Award Badge from there
  • Assign myself a badge, try to re-assign it, see validation error.

Also, validation errors on dialog forms are ugly.

pasted_file (1×2 px, 272 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

  • validate awards being re-granted
chad edited the test plan for this revision. (Show Details)

Thinking about this, I should get rid of attaching recipients to badges outright and just always do targeted searches?

Ideally, yes (and you can do pagination that way, with executeWithCursorPager(...)), although that might be a little bit tricky in the transaction (but maybe not too bad?)

In more detail:

  • The "Recipients" UI should either use full-blown ApplicationSearch (but probably way overkill / tons of work?) or PhabricatorBadgesAwardQuery + executeWithCursorPager() (probably more reasonable and not very much work) to load a page at a time with pagination. We don't have too many manually paginated interfaces left, but I think the one in PhabricatorTokenGivenController is fine as a model. I think this isn't too tricky, and should only be a little extra code.
  • Your award validation is good as written (except for one small bug, see inline).
  • You should be able to remove getAwards() from the transaction stuff now (in applyExternalEffects()) because you're validating better, so we don't need to only apply awards that don't exist: we know all the awards don't exist yet.
  • Then, remove needRecipients() and getAwards() completely if possible.
src/applications/badges/xaction/PhabricatorBadgesBadgeAwardTransaction.php
75–90

Minor thing: we should continue; here. Otherwise, we may do the new check with a null value for $user, which could fatal on line 96 when we $user->getUsername().

This would be hard to hit, but you could do it like this, I think:

  • Award a badge to @alice.
  • Open another browser window with an award for @alice, but don't submit the dialog yet.
  • Destroy @alice with bin/remove destroy @alice.
  • Try to submit the dialog now.

I think that will fatal on $user->getUsername().

Minor thing: we should continue; here. Otherwise, we may do the new check with a null value for $user, which could fatal on line 96 when we $user->getUsername().

what in tarnation

C5yIGKOWgAApIlh.jpg (802×816 px, 68 KB)

Did you ever hear the tragedy of Darth Plagueis "the wise"? I thought not. It's not a story the Jedi would tell you. It's a Sith legend. Darth Plagueis was a Dark Lord of the Sith, so powerful and so wise he could use the Force to influence the midichlorians to create life... He had such a knowledge of the dark side that he could even keep the ones he cared about from dying. The dark side of the Force is a pathway to many abilities some consider to be unnatural. He became so powerful... the only thing he was afraid of was losing his power, which eventually, of course, he did. Unfortunately, he taught his apprentice everything he knew, then his apprentice killed him in his sleep. It's ironic he could save others from death, but not himself.

chad planned changes to this revision.Mar 2 2017, 11:38 PM
chad retitled this revision from Limit recipient queries in badges to Remove needRecipients and needAwards from Badges.Mar 3 2017, 6:43 AM
chad edited the summary of this revision. (Show Details)
chad edited the test plan for this revision. (Show Details)
epriestley added inline comments.
src/applications/badges/query/PhabricatorBadgesQuery.php
48–49

You can just get rid of this completely now.

src/applications/badges/view/PhabricatorBadgesRecipientsListView.php
14

Could use an array typehint.

This revision is now accepted and ready to land.Mar 3 2017, 12:08 PM
This revision was automatically updated to reflect the committed changes.
chad marked 2 inline comments as done.