Page MenuHomePhabricator

Funbeta Badges
ClosedPublic

Authored by chad on Feb 15 2017, 6:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 3, 8:52 AM
Unknown Object (File)
Sat, Nov 30, 12:10 AM
Unknown Object (File)
Fri, Nov 22, 1:15 PM
Unknown Object (File)
Tue, Nov 19, 12:54 PM
Unknown Object (File)
Thu, Nov 14, 6:02 PM
Unknown Object (File)
Thu, Nov 14, 5:45 PM
Unknown Object (File)
Thu, Nov 14, 5:44 PM
Unknown Object (File)
Thu, Nov 14, 5:44 PM
Subscribers

Details

Summary

Ships Badges. I can write up some basic docs too if needed.

Test Plan

/applications/

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

chad retitled this revision from Unbeta Badges to Funbeta Badges.Feb 15 2017, 7:13 PM

I think we should deal with this stuff first:

  • T9010 (may require migrations)
  • T10798 (limit stuff only, I don't really care about order much -- badges currently won't scale on pages that use needRecipients() and/or aren't paginated if a badge is awarded to a large number of recipients)
  • T10698
  • Badges should switch to ModularTransactions.
  • Width of badges in the profile element is awkward since they just barely don't fit two-up. Maybe use the small icons (like hovercards) instead?

Screen Shot 2017-02-15 at 11.28.19 AM.png (953×326 px, 32 KB)

  • "Flavor text" on "Create Badge" form should be "Flavor Text" (title case) for consistency.
  • Description on new profile view feels real whitespace-y? Maybe just shove the badge itself into the menu like profiles? But then the page is probably junk on mobile so that's probably terrible.

Screen Shot 2017-02-15 at 11.32.03 AM.png (352×1 px, 23 KB)

  • Reverse side of badges is sort of pointless now, maybe just put everything on the front side? We could just have them spin around when clicked. The text "Awarded by X" appears there sometimes but there's no way to describe why a badge was awarded. If we're unprototyping without this feature, I think we should think about getting rid of badge backs.

Screen Shot 2017-02-15 at 11.32.44 AM.png (248×274 px, 6 KB)

  • Mentioning users and uploading files into a badge's description does not properly subscribe the user or attach the File object (this will sort of autofix for free by moving to ModularTransactions).
  • I can repeatedly award a badge to the same recipients with no UI feedback:

Screen Shot 2017-02-15 at 11.35.36 AM.png (102×470 px, 19 KB)

  • Label/width in "Add Recipients" form is unusual? Form is forced into mobile layout or something. I don't think we do this in other cases. Compare "Add Recipients" and "Add Members":

Screen Shot 2017-02-15 at 11.36.44 AM.png (264×594 px, 19 KB)

Screen Shot 2017-02-15 at 11.37.51 AM.png (190×574 px, 18 KB)

  • See also the "Award Badge" dialog on the profile page, which curiously says "Grant Badge" instead of "Award" or "Add Recipients":

Screen Shot 2017-02-15 at 11.43.48 AM.png (203×574 px, 17 KB)

  • Fatal error if I create a badge with a name longer than 255 characters (should be a validation error).
    • Or more than 255 characters of flavor text.
  • Profile page does not limit displayed badges:

Screen Shot 2017-02-15 at 11.44.43 AM.png (1×322 px, 32 KB)

  • Hovercard does not limit displayed badges:

Screen Shot 2017-02-15 at 11.45.24 AM.png (258×422 px, 31 KB)

  • I can award badges to applications, other badges, etc (no validation on recipient PHIDs):

Screen Shot 2017-02-15 at 11.48.17 AM.png (118×442 px, 22 KB)

  • I can change badges to have invalid qualities (missing validation):

Screen Shot 2017-02-15 at 12.16.31 PM.png (78×676 px, 16 KB)

  • Since creators don't have any durable editing or ownership rights, they probably should not be automatically subscribed (instead, explicitly subscribe them as a side effect of badge creation, like Maniphest)?
  • The profile panel loads awards twice (once on People query, then again afterward) and does badge status filtering in PHP. The hovercard does a slightly better job of this.
  • The timeline view does PHP-side filtering again, though.
  • Since the timeline view doesn't even use needBadges on the People query, can we get rid of it so this feature isn't totally tied to the core?
  • The "Qualities" constraint in badges.search accepts numeric strings like "102". Maybe just drop this constraint from Conduit? I don't really love how badge qualities work internally, and if we hide them we don't have to fix this yet.
  • Revoking badges from the "View Recipients" screen takes the user back to the "View Badge" screen, but should probably stay on "View Recipients".
    • Cancel action (if you open-in-new-tab) also goes to "View Badge".
    • "Add Recipients" always cancels to "View Recipients", but could be invoked from either "View Badge" or "View Recipients" (probably not worth fixing).
  • NUX text on search uses the word "instance", but I think we use the word "install" to describe "your install" elsewhere, vs "an instance (on Phacility)".
  • Badges EditEngine edit form is configurable, but this feels pretty silly/useless. Maybe just make it not-configurable until we have use cases?
  • This is pre-existing and not specific to badges, buuuut badges with a very long name break layout (mostly from crumbs?):

Screen Shot 2017-02-15 at 12.04.36 PM.png (1×1 px, 131 KB)

  • Feed story for quality level changes doesn't list the actual change:

Screen Shot 2017-02-15 at 12.06.15 PM.png (112×374 px, 16 KB)

  • Mail sends with subject lines like "[Badge] [Updated] Badge 6", and does not list the name of the badge.
  • Mail includes "BADGE DESCRIPTION" section in every message about a badge with a description. Particularly, edits to the badge description have the edit and then the description. The description should only be included in the initial creation mail, like Maniphest:

Screen Shot 2017-02-15 at 12.14.35 PM.png (431×285 px, 31 KB)

This revision now requires changes to proceed.Feb 15 2017, 8:17 PM
chad requested review of this revision.Mar 9 2017, 4:12 AM
chad edited edge metadata.

probably ready for another pass. don't think there are any more bugs, running through stuff locally again. I'd like to do Herald stuff, but that's a weekend project since it's not important.

With my tremendously valuable contributions in D17561 and D17562, I think we're in good shape here now.

This revision is now accepted and ready to land.Mar 24 2017, 9:11 PM
This revision was automatically updated to reflect the committed changes.