Funbeta Badges
ClosedPublic

Authored by chad on Feb 15 2017, 6:57 PM.

Details

Summary

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

Test Plan

/applications/

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
chad created this revision.Feb 15 2017, 6:57 PM
chad retitled this revision from Unbeta Badges to Funbeta Badges.Feb 15 2017, 7:13 PM
epriestley requested changes to this revision.Feb 15 2017, 8:17 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?

  • "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.

  • 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.

  • 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:

  • 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":

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

  • 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:

  • Hovercard does not limit displayed badges:

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

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

  • 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?):

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

  • 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:

This revision now requires changes to proceed.Feb 15 2017, 8:17 PM
chad added a comment.EditedFeb 15 2017, 8:22 PM

chad requested review of this revision.Thu, Mar 9, 4:12 AM

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.

epriestley accepted this revision.Fri, Mar 24, 9:11 PM

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.Fri, Mar 24, 9:11 PM
This revision was automatically updated to reflect the committed changes.