Fixes T10798. Separates these two since they don't need to be combined and it allows for more flexibility / scalability.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T12270: Unbeta Badges
T10798: Limit, order recipients list in BadgeView - Commits
- rPd2a420d13abd: Remove needRecipients and needAwards from Badges
- 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.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- badges-limit (branched from master)
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 15848 Build 20972: Run Core Tests Build 20971: arc lint + arc unit
Event Timeline
Comment Actions
Thinking about this, I should get rid of attaching recipients to badges outright and just always do targeted searches?
Comment Actions
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?)
Comment Actions
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 | ||
|---|---|---|
| 83–84 ↗ | (On Diff #41946) | 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:
I think that will fatal on $user->getUsername(). |
Comment Actions
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
Comment Actions
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.
