1. 12 Jan, 2017 2 commits
    • epriestley's avatar
      Render revision and audit state icons in Maniphest · 45c740ac
      epriestley authored
      Summary:
      Fixes T7076. This could probably use some tweaking but should get the basics in place.
      
      This shows overall object state (e.g., "Needs Review"), not individual viewer state (e.g., "you need to review this"). After the bucketing changes it seems like we're mostly in a reasonable place on showing global state instead of viewer state. This makes the overall change much easier than it might otherwise have been.
      
      Test Plan: {F2351867}
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T7076
      
      Differential Revision: https://secure.phabricator.com/D17193
      45c740ac
    • epriestley's avatar
      Restore "[Action]" mail subject lines to Differential/Diffusion · 7d3d0224
      epriestley authored
      Summary: Ref T11114. Ref T10978. These hadn't made it over to EditEngine yet.
      
      Test Plan:
        - Took various actions on revisions and commits.
        - Used `bin/mail show-outbound --id ...` to examine the "Vary Subject", saw it properly generate "[Accepted]", "[Resigned]", etc.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T11114, T10978
      
      Differential Revision: https://secure.phabricator.com/D17191
      7d3d0224
  2. 11 Jan, 2017 1 commit
  3. 10 Jan, 2017 2 commits
  4. 09 Jan, 2017 4 commits
    • epriestley's avatar
      Restore missing behavior for Differential keyboard navigation · fda83094
      epriestley authored
      Summary: Fixes T12086. This got dropped by accident while cleaning up haunting.
      
      Test Plan: Loaed a revision, hit "?", hit n/j/p/etc
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T12086
      
      Differential Revision: https://secure.phabricator.com/D17166
      fda83094
    • epriestley's avatar
      When updating revisions in response to commits, reuse previously generated diffs · 2dfe79cf
      epriestley authored
      Summary:
      Fixes T10968. In rare situations, we can generate a diff, then hit an error which causes this update to fail.
      
      When it does, we tend to get stuck in a loop creating diffs, which can fill the database up with garbage. We saw this once in the Phacility cluster, and one instance hit it, too.
      
      Instead: when we create a diff, keep track of which commit we generated it from. The next time through, reuse it if we already built it.
      
      Test Plan:
        - Used `bin/differential attach-commit <commit> <revision>` to hit this code.
        - Simulated a filesystem write failure, saw the diff get reused.
        - Also did a normal update, which worked properly.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T10968
      
      Differential Revision: https://secure.phabricator.com/D17164
      2dfe79cf
    • epriestley's avatar
      Fix an issue which could prevent blocking reviewers from being removed from revisions · 425deeb5
      epriestley authored
      Summary: Ref T11114. After evaluating typeahead tokens, we could process blocking reviewer removals incorrectly: we may get structures back.
      
      Test Plan: Removed blocking reviewers from the web UI.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T11114
      
      Differential Revision: https://secure.phabricator.com/D17163
      425deeb5
    • epriestley's avatar
      Mark "v3" API methods as stable; mark obsoleted methods as "Frozen" · aa6e788f
      epriestley authored
      Summary:
      Ref T12074. The "v3" API methods (`*.search`, `*.edit`) are currently marked as "unstable", but they're pretty stable and essentially all new code should be using them.
      
      Although these methods are seeing some changes, almost all changes are additive (support for new constraints or attachemnts) and do not break backward compatibility. We have no major, compatibility-breaking changes planned.
      
      I don't want to mark the older methods "deprecated" yet since `arc` still uses a lot of them and there are some capabilities not yet available on the v3 methods, but introduce a new "frozen" status with pointers to the new methods.
      
      Overall, this should gently push users toward the newer methods.
      
      Test Plan: {F2325323}
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T12074
      
      Differential Revision: https://secure.phabricator.com/D17158
      aa6e788f
  5. 05 Jan, 2017 1 commit
    • epriestley's avatar
      Fix a case where "Accept + Comment" would ignore the "Accept" · 1f230699
      epriestley authored
      Summary:
      Ref T11114. When you comment, we try to upgrade your review status to "commented".
      
      This can conflict with upgrading it to "accepted" or "rejected", or removing it entirely.
      
      For now, just avoid making this update. After T10967, I expect "you commented" to be orthogonal to accepted/rejected so it should stop conflicting on its own.
      
      Test Plan:
        - As an "added" reviewer, accepted a revision with a comment in the same transaction.
        - Before patch: accept didn't stick.
        - After patch: accept sticks.
      
      This may be somewhat magical/order-dependent but I was able to reproduce it locally.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T11114
      
      Differential Revision: https://secure.phabricator.com/D17146
      1f230699
  6. 04 Jan, 2017 3 commits
    • Chad Little's avatar
      Bring up contrast on light green / red diffs · 96fbf37d
      Chad Little authored
      Summary: Minor color saturation here, ideal for low quality monitors.
      
      Test Plan:
      Review new colors in various scenarios.
      
      {F2305178}
      
      Reviewers: epriestley
      
      Reviewed By: epriestley
      
      Subscribers: Korvin
      
      Differential Revision: https://secure.phabricator.com/D17141
      96fbf37d
    • epriestley's avatar
      Show an info view warning for ongoing or failed builds in Differential · 2855470b
      epriestley authored
      Summary:
      Fixes T10136. This reinforces ongoing or failed builds in the comment action area.
      
      We already emit a similar message for unit test failures from `arc unit`. This should probably obsolete that, eventually.
      
      Test Plan:
      {F2304809}
      
      {F2304810}
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T10136
      
      Differential Revision: https://secure.phabricator.com/D17140
      2855470b
    • epriestley's avatar
      Allow Harbormaster builds to publish to a different object · ef05bf33
      epriestley authored
      Summary:
      Fixes T9276. Fixes T8650. The story so far:
      
        - We once published build updates to Revisions.
        - An unrelated fix (D10911) sent them to the Diffs instead of Revisions, which isn't useful, since you can't see a diff's timeline anywhere.
        - This also caused a race condition, where the RevisionEditor and DiffEditor would update the diff simultaneously (T8650).
        - The diff update was just disabled to avoid the race (part of D13441).
        - Instead, allow the updates to go somewhere else. In this case, we send commit updates to the commit but send diff updates to the revision so you can see 'em.
        - Since everything will be using the revision editor now, we should either get proper lock behavior for free or it should be easy to add if something whack is still happening.
        - Overall, this should pretty much put us back in working order like we were before D10911.
      
      This behavior is undoubtedly refinable, but this should let us move forward.
      
      Test Plan:
      Saw a build failure in timeline:
      
      {F2304575}
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
      
      Maniphest Tasks: T9276, T8650
      
      Differential Revision: https://secure.phabricator.com/D17139
      ef05bf33
  7. 02 Jan, 2017 2 commits
    • epriestley's avatar
      Define Differential email action in terms of EditEngine · 50de3071
      epriestley authored
      Summary: Ref T11114. Move email/command actions, like "!reject", to modular transactions + editengine.
      
      Test Plan: Used `bin/mail receive-test` to pipe "!stuff" to an object, saw appropraite effects in web UI.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T11114
      
      Differential Revision: https://secure.phabricator.com/D17133
      50de3071
    • epriestley's avatar
      Make some Differential comment actions (like "Accept" and "Reject") conflict with one another · 35750b9c
      epriestley authored
      Summary:
      Ref T11114. When a user selects "Accept", and then selects "Reject", remove the "Accept". It does not make sense to both accept and reject a revision.
      
      For now, every one of the "actions" conflicts: accept, reject, resign, claim, close, commandeer, etc, etc. I couldn't come up with any combinations that it seems like users are reasonably likely to want to try, and we haven't received combo-action requests in the past that I can recall.
      
      Test Plan:
        - Selected "Accept", then selected "Reject". One replaced the other.
        - Selected "Accept", then selected "Change Subscribers". Both co-existed happily.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T11114
      
      Differential Revision: https://secure.phabricator.com/D17132
      35750b9c
  8. 01 Jan, 2017 4 commits
    • epriestley's avatar
      Use extended policies in Differential diffs · 65c1c758
      epriestley authored
      Summary:
      Fixes T9648. Diffs currently use `return $this->getRevision()->getViewPolicy();` to inherit their revision's view policy.
      
      After the introduction of object policies, this is wrong for policies like "Subscribers", because it means "Subscribers to this object, the diff". Since Diffs have no subscribers, this always fails.
      
      Instead, use extended policies so that the object policy evaluates in the context of the correct object (the revision).
      
      Test Plan:
        - Create a revision.
        - Subscribe `alice` to it.
        - Set view policy to "Subscribers".
        - View revision as `alice`.
        - Before patch: nonsense fatal about missing diff because of policy error.
        - After patch: `alice` can see the revision.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T9648
      
      Differential Revision: https://secure.phabricator.com/D17123
      65c1c758
    • epriestley's avatar
      Always parse the first line of a commit message as a title · 81e2a1cf
      epriestley authored
      Summary: Fixes T10312. If your first line is "Reviewers: xyz", it's a title, not a "Reviewers" field.
      
      Test Plan: Added unit test.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T10312
      
      Differential Revision: https://secure.phabricator.com/D17122
      81e2a1cf
    • epriestley's avatar
      Be more lenient when accepting "Differential Revision" in the presence of... · ab17a7d4
      epriestley authored
      Be more lenient when accepting "Differential Revision" in the presence of custom ad-hoc commit message fields
      
      Summary:
      Fixes T8360. We will now parse revisions out of "Differential Revision: X" followed by other ad-hoc fields which we do not recognize. Previously, these fields would be treated as part of the value.
      
      (In the general case, other fields may line wrap so we can't assume that fields are only one line long. However, we can make that assumption safely for this field.)
      
      Also maybe fix whatever was going on in T9965 although that didn't really have a reproduction case.
      
      Test Plan: Added unit tests.
      
      Reviewers: chad
      
      Reviewed By: chad
      
      Maniphest Tasks: T8360
      
      Differential Revision: https://secure.phabricator.com/D17121
      ab17a7d4
    • epriestley's avatar
      Use a more conventional spelling of "CLOSED" · 7bf49d25
      epriestley authored
      Summary: Ref T11114. Wow!
      
      Test Plan: Spelling!
      
      Reviewers: chad, eadler
      
      Reviewed By: chad, eadler
      
      Maniphest Tasks: T11114
      
      Differential Revision: https://secure.phabricator.com/D17125
      7bf49d25
  9. 31 Dec, 2016 13 commits
  10. 29 Dec, 2016 1 commit
  11. 16 Dec, 2016 7 commits