1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 05:02:44 +01:00
Commit graph

6897 commits

Author SHA1 Message Date
epriestley
c39b10aa7a Fix non-public capabilities in Application edit
Summary: Ref T603. I nuked this check by accident and neglected to test the negative case.

Test Plan: Saved a non-public policy (Herald Global) and a public policy (Maniphest View).

Reviewers: asherkin, btrahan

Reviewed By: asherkin

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7278
2013-10-09 16:23:36 -07:00
Asher Baker
e3005fad09 Hide Audit information on Home when the application is uninstalled
Test Plan: Looked at Home with Audit installed and uninstalled.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7277
2013-10-09 15:25:03 -07:00
epriestley
f4582dc49d Allow "Default View" policies to be set to Public
Summary: Ref T603. Currently, we hard-code defense against setting policies to "Public" in several places, and special case only the CAN_VIEW policy. In fact, other policies (like Default View) should also be able to be set to public. Instead of hard-coding this, move it to the capability definitions.

Test Plan: Set default view policy in Maniphest to "Public", created a task, verified default policy.

Reviewers: btrahan, asherkin

Reviewed By: asherkin

CC: asherkin, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7276
2013-10-09 15:06:18 -07:00
epriestley
11fbd213b1 Custom Policy Editor
Summary:
Ref T603. This isn't remotely usable yet, but I wanted to get any feedback before I build it out anymore.

I think this is a reasonable interface for defining custom policies? It's basically similar to Herald, although it's a bit simpler.

I imagine users will rarely interact with this, but this will service the high end of policy complexity (and allow the definition of things like "is member of LDAP group" or whatever).

Test Plan: See screenshots.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran, asherkin

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7217
2013-10-09 14:05:10 -07:00
epriestley
436a403357 Add a "default view" policy to Differential
Summary:
Ref T603. Allows the Differential view policy to be configured with a default.

I've omitted "edit" because I want to wait and see how comment/comment-action policies work out. I could imagine locking "edit" down to only the owner at some point, and providing a wider "interact" capability, or something like that, which would cover accept/reject/commandeer. Users in this group could still edit indirectly by commandeering first.

Test Plan: Created new revisions from the CLI and conduit.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7269
2013-10-09 13:58:00 -07:00
epriestley
650dc0cc30 Remove the "create rules" Herald capability
Summary:
Ref T603. In thinking about this, I think I went mad with power in creating this capability. I can't imagine any reason to give users access to Herald but not let them create rules.

We can restore this later if some install comes up with a good reason to have it, but in the interest of keeping policies as simple as possible, I think we're better off without it. In particular, if you don't want a group of users creating rules, just lock them out of the application entirely.

The "Manage Global Rules" capability is still around, I think that one's super good.

Test Plan: Edited Herald policies, created a rule.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7268
2013-10-09 13:55:44 -07:00
epriestley
1ee455c441 Add defualt view and default edit policies for tasks
Summary: Ref T603. Allow global default policies to be configured for tasks.

Test Plan:
  - Created task via web UI.
  - Created task via Conduit.
  - Created task via email.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7267
2013-10-09 13:53:17 -07:00
epriestley
3147a6ca57 Improve messaging of special policy rules in applications
Summary: Ref T603. When the user encounters an action which is controlled by a special policy rule in the application, make it easier for applications to show the user what policy controls the action and what the setting is. I took this about halfway before and left a TODO, but turn it into something more useful.

Test Plan: See screenshots.

Reviewers: btrahan, chad

Reviewed By: chad

CC: chad, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7265
2013-10-09 13:52:04 -07:00
epriestley
45f38c549b Use header status/policy elements in Applications meta-application
Summary: Ref T603. Use more modern elements.

Test Plan: See screenshot.

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7264
2013-10-09 13:46:45 -07:00
epriestley
7a97a71e20 Move Herald application capabilities to newer infrastructure
Summary: Ref T603. Use the new hotness.

Test Plan: Edited Herald in Applications, tried to create rules / global rules without capabilities, got reasonable error messages.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7263
2013-10-09 13:44:41 -07:00
epriestley
82a061b485 Make UIStatusList background element transparent instead of white
Summary: Fixes T3933.

Test Plan: See screenshot.

Reviewers: chad

Reviewed By: chad

CC: chad, aran

Maniphest Tasks: T3933

Differential Revision: https://secure.phabricator.com/D7272
2013-10-08 17:18:57 -07:00
Chad Little
ac868f56db Maniphest mobile CSS tweaks
Summary: Fixes a few minor quirks when viewing maniphest on mobile.

Test Plan: shrink maniphest screen, easier to read.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7270
2013-10-08 14:35:56 -07:00
Chad Little
fe73f9da73 Adds status colors to navbar tabs
Summary: Adds the abilit to set a status color of warning or fail to navbar tab lists (for objectheaders)

Test Plan: uiexamples, photoshop

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7266
2013-10-07 19:29:05 -07:00
epriestley
de67f00d0e Remove AphrontRedirectException
Summary: Fixes T3909. Waiting on Facebook to confirm this is unused.

Test Plan: `grep`

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3909

Differential Revision: https://secure.phabricator.com/D7193
2013-10-07 13:29:05 -07:00
epriestley
b1b1ff83f2 Allow applications to define new policy capabilities
Summary:
Ref T603. I want to let applications define new capabilities (like "can manage global rules" in Herald) and get full support for them, including reasonable error strings in the UI.

Currently, this is difficult for a couple of reasons. Partly this is just a code organization issue, which is easy to fix. The bigger thing is that we have a bunch of strings which depend on both the policy and capability, like: "You must be an administrator to view this object." "Administrator" is the policy, and "view" is the capability.

That means every new capability has to add a string for each policy, and every new policy (should we introduce any) needs to add a string for each capability. And we can't do any piecemeal "You must be a {$role} to {$action} this object" becuase it's impossible to translate.

Instead, make all the strings depend on //only// the policy, //only// the capability, or //only// the object type. This makes the dialogs read a little more strangely, but I think it's still pretty easy to understand, and it makes adding new stuff way way easier.

Also provide more context, and more useful exception messages.

Test Plan:
  - See screenshots.
  - Also triggered a policy exception and verified it was dramatically more useful than it used to be.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7260
2013-10-07 13:28:58 -07:00
epriestley
68c854b967 Remove dead rejectImpossiblePolicy() method
Summary: Ref T603. Apparently we made all policies possible at some point. Go us! This has no callsites.

Test Plan: `grep`, notice it's a private method

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7259
2013-10-07 12:52:01 -07:00
epriestley
0598600476 Always pass handles to tokenizers, not <phid -> name> maps
Summary: Ref T1279. Prerequisite for adding icons or other type information to tokenizers, since we don't currently have enough information to prefill them when rendering things from the server side. By passing handles in, the tokenizer can extract type information.

Test Plan:
- Searched by user in Audit.
- Sent Conpherence from profile page.
- Tried to send an empty conpherence.
- Searched Countdown by user.
- Edited CCs in Differential.
- Edited reviewers in Differential.
- Edited a commit's projects.
- Searched lint by owner.
- Searched feed by owner/project.
- Searched files by owner.
- Searched Herald by owner.
- Searched Legalpad by owner.
- Searched Macro by owner.
- Filtered Maniphest reports by project.
- Edited CCs in Maniphest.
- Searched Owners by owner.
- Edited an Owners package.
- Searched Paste by owner.
- Searched activity logs by owner.
- Searched for mocks by owner.
- Edited a mock's CCs.
- Searched Ponder by owner.
- Searched projects by owner.
- Edited a Releeph project's pushers.
- Searched Releeph by requestor.
- Edited "Uses Symbols" for an Arcanist project.
- Edited all tokenizers in main search.
- Searched Slowvote by user.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7248
2013-10-07 12:51:24 -07:00
epriestley
2abbd51868 Don't raise a policy exception if a user can't see the parent revision of a new diff
Summary: Ref T603. We currently bomb out here, but should just continue forward. I'm fairly certain we don't even use this for anything anymore (it has been replaced by "depends on") but need to check that.

Test Plan: Created a new revision with `arc diff`.

Reviewers: ljalonen, btrahan, #blessed_reviewers, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7255
2013-10-07 12:51:04 -07:00
epriestley
c6f9316a77 Add missing case for personal ruls which create blocking reviewers
Summary: Ref T1279. I only tested the global case. :O

Test Plan: Created a personal "add me as blocking" rule.

Reviewers: btrahan, zeeg

Reviewed By: zeeg

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7261
2013-10-07 11:18:00 -07:00
Chad Little
bc46403cef Tweak status list highlight
Summary: Removes highlight from the 'notes' row, leaves for status and name.

Test Plan: Tested on a diff. Faked a note.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7258
2013-10-07 08:57:39 -07:00
Chad Little
e2824648e4 Fix retina ObjectHeader status icons
Summary: There are reversed on retina displays/

Test Plan: Review on retina Mac

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7257
2013-10-07 08:47:40 -07:00
Chad Little
8fdfe1f547 Minor Results table CSS updates
Summary: Missed these in the previous pass. Long term I'd like to move the results to tabs, will probably mock those up today and ask for your help coding.

Test Plan: Tested the changes on a diff.

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7256
2013-10-07 08:41:01 -07:00
epriestley
c4ecdfa2a5 Fix missing property on revision adapter. 2013-10-07 03:41:00 -07:00
epriestley
2a69b91000 Fix minor issue with data attachment on project creation 2013-10-06 17:13:51 -07:00
epriestley
953ff197bf Allow Herald rules to be disabled, instead of deleted
Summary:
Ref T603. Ref T1279. Further improves transaction and policy support for Herald.

  - Instead of deleting rules (which wipes out history and can't be undone) allow them to be disabled.
  - Track disables with transactions.
  - Gate disables with policy controls.
  - Show policy and status information in the headers.
  - Show transaction history on rule detail screens.
  - Remove the delete controller.
  - Support disabled queries in the ApplicationSearch.

Test Plan:
  - Enabled and disabled rules.
  - Searched for enabled/disabled rules.
  - Verified disabled rules don't activate.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279, T603

Differential Revision: https://secure.phabricator.com/D7247
2013-10-06 17:10:29 -07:00
epriestley
c6add6ae73 Make "reject" and "blocking reviewer" block acceptance in Differential
Summary:
Ref T1279. This is a logical change.

  - "Reject" (nee "Request Changes") is now sticky. The review won't transition to "Accepted" until the reviewer clears their objection. In practice, I think it always worked like this anyway (without technical enforcement, users just followed this rule naturally, since disobeying this rule is kind of a dick move) so I don't expect this to change much. I think this rule is easier to understand than the old rule now, given the multi-reviewer status and blocking reviewers.
  - "Blocking Reviewer" and "Reject" now prevent a revision from transitioning to "Accepted". When reviewers accept, resign, or are removed, we do a check to see if the reivsion has: at least one user reviewer who has accepted; zero rejects; and zero blocks. If all conditions are satisfied, we transition it to "accepted".

Practically, the primary net effect of this is just to make blocking reviews actually block.

This is pretty messy, but there's not much we can do about it until after T2222, since we have two completely separate editor pathways which are both responsible for adjusting status. Eventually, these can merge into a single sane editor which implements reasonable rules in reaonable ways. But that day is not today.

Test Plan: With three users and a project, made a bunch of accepts, rejects, resigns and reviewer removals. I think I probably covered most of the pathways? There are a lot of interactions here.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, wisutsak.jaisue.7

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7245
2013-10-06 17:09:56 -07:00
epriestley
8aa8ef49da Provide an "Add blocking reviewer..." Herald action
Summary: Ref T1279. These reviewers don't actually create a logical block yet (that is, revisions still transition to "accepted" even in their presence), but this handles everything except that.

Test Plan: Added Herald rules and updated revisions; see screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7244
2013-10-06 17:09:24 -07:00
epriestley
929ad86b57 Allow accepting accepted revisions, and rejecting rejected revisions
Summary:
Ref T1279. With the new per-reviewer status, you can always accept or reject a revision.

This is primarily cosmetic/UI changes. In particular, you've always been able to reject a rejected revision, the UI just didn't show you an option.

Test Plan: Accepted accepted revisions; rejected rejected revisions. See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7243
2013-10-06 17:09:02 -07:00
epriestley
d518f3c9de When a user accepts a revision, accept for all projects the user has authority over
Summary: Ref T1279. If you accept a revision, also accept on behalf of all the projects you have authority to accept for.

Test Plan:
  - Accepted a revision which I was a reviewer on, saw my own status and an authority project's status change to "Accepted".
  - Accepted a revision which I was not a reviewer on, saw my own status be added (as "Accepted") and the project's status update.

Also, see screenshot.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, wisutsak.jaisue.7

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7242
2013-10-06 17:08:30 -07:00
epriestley
3d3d3b6d80 Move determination of reviewer authority into DifferentialRevisionQuery
Summary:
Ref T1279. We currently determine reviewers at display time, but this is bad for several reasons:

  - It puts queries very close to the display layer.
  - We have to query for each revision if we want to figure out authority for several.
  - We need to figure it out in several places, so we'll end up with copies of this logic.
  - The logic isn't trivial (exceptions for the viewer, exceptions to that rule for install configuration).
  - We already do this "figure it out when we need it" stuff in Diffusion for audits and it's really bad: we have half-working copies of the logic spread all over the place.

Instead, put it in the Query. Callers query for it and get the data attached to the reviewer objects.

Test Plan:
  - Looked at some revisions, verified the correct lines were highlighted.
    - Looked at a revision I created and verified that projects I was a member of were not highlighted.
      - With self-accept enabled, these //are// highlighted.
    - Looked at a revision I did not create and verified that projects I was a member of were highlighted.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7241
2013-10-06 17:08:14 -07:00
epriestley
515f9a36ab When editing objects which use files, attach the files to the objects
Summary: Ref T603. Fixes T3921. Tightens up policy controls for file/object relationships in existing applications.

Test Plan:
  - Uploaded new project image, verified it got an edge to the project.
  - Uploaded new profile image, verified it got an edge to me.
  - Uploaded new macro image, verified it got an edge to the macro.
  - Uploaded new paste via web UI and conduit, verified it got attached.
  - Replaced, added images to a mock, verified they got edges.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3921, T603

Differential Revision: https://secure.phabricator.com/D7254
2013-10-06 17:07:55 -07:00
epriestley
c587b8a9c8 Remove ProjectProfile->loadProfileImageURI()
Summary: Ref T603. Gets rid of a sketchy, non-policy-aware, unbatched file query.

Test Plan: Looked at projects, typeahead, edited project profile images.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7253
2013-10-06 17:07:43 -07:00
epriestley
80f6d00940 Remove PhabricatorProject->loadProfile
Summary: Ref T603. Do modern, sensible queries here.

Test Plan: Viewed project profile, list, member edit, profile edit, used typeahead, changed project image.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7252
2013-10-06 17:07:20 -07:00
epriestley
64e4b3aef4 Remove loadMemberPHIDs from PhabricatorProject
Summary:
Ref T603. Move toward stamping out all the Project / ProjectProfile query irregularities with respect to policies.

  - Fixes a bug with Asana publishing when the remote task is deleted.
  - Fixes an issue with Herald commit rules.

Test Plan:
  - Viewed projects;
  - edited projects;
  - added and removed members from projects;
  - republished Asana-bridged feed stories about commits.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7251
2013-10-06 17:07:08 -07:00
Chad Little
f3a4d27cfe Fix border on Actions List.
Summary: This adds setActionList to PropertyListView and properly places it in an archaic HTML 1.0 table.

Test Plan: test layouts with actions really tall or properties really tall. Always see a full height border.

Reviewers: epriestley, btrahan

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7239
2013-10-06 11:12:00 -07:00
epriestley
b7297a3278 Add an "Author's projects" Herald field to Differential
Summary:
Ref T1279. This allows installs to implement two different flavors of project review. They can either implement this rule:

  When:
    [ ... ] [ ... ]
  Take Action:
    [ Add blockign reviewers ] [ Security ]

...which means "every revision matching X needs to be signed off by someone else on the Security team, //even if the author is on that team//". The alternative is to implement this rule:

  When:
    [ Author's projects ] [ do not include ] [ Security ]
    [ ... ] [ ... ]
  Take Action:
    [ Add blocking reviewers ] [ Security ]

...which means that people on the Security team don't need a separate signoff from someone else on the team.

I think this weaker version maps to some of what, e.g., Google does (you need to be reviewed by someone with "readability" in a language, but if you have it that's good enough), but I could imagine cases like "Security" wanting to prevent self-review from satisfying the requirement.

@zeeg, not sure which of these use cases is relevant here, but either one should work after this.

Test Plan: Created rules with this field, verified it populated properly in the transcript.

Reviewers: btrahan

Reviewed By: btrahan

CC: zeeg, aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7238
2013-10-05 14:10:53 -07:00
epriestley
c80a4f51c1 Highlight reviews the viewer is responsible for in Differential
Summary: Ref T1279. No logical changes, but cosmetically highlight stuff you have authority for, like we do in Diffusion.

Test Plan: See screenshot.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7237
2013-10-05 14:10:52 -07:00
epriestley
4c0ec01ce5 Allow Herald rules to add reviewers
Summary:
Ref T1279. Although I think this is a bad idea in general (we once supported it, removed it, and seemed better off for it) users expect it to exist and want it to be available. Give them enough rope to shoot themselves in the foot.

I will probably write some lengthy treatise on how you shouldn't use this rule later.

Implementation is straightforward because Differential previously supported this rule.

This rule can also be used to add project reviewers.

Test Plan: Made some "add reviewers" rules, created revisions, saw reviewers trigger.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7235
2013-10-05 14:10:51 -07:00
epriestley
2d733f88a1 Split users apart from projects/packages in reviewer and audit UIs
Summary: Ref T1279. Show separate sections for "Reviewers" and "Project Reviewers" (Differential) and for "Auditors" and "Package/Project Auditors" (Diffusion/Audit).

Test Plan:
  - Looked at a commit. Saw separation.
  - Looked at a revision. Saw separation.

Reviewers: chad, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7233
2013-10-05 14:10:49 -07:00
epriestley
9434df9d7c Accommodate project reviewers in Differential search
Summary:
Ref T1279. Two changes to the search/query for Differential:

  - "Reviewers" now accepts users and projects.
  - "Responsible Users" now includes revisions where a project you are a member of is a reviewer.

Test Plan:
  - Searched for project reviewers.
  - Verified that the dashboard now shows reviews which I'm only part of via project membership.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7231
2013-10-05 14:10:47 -07:00
epriestley
cf4eb3109e Allow projects to review revisions
Summary:
Ref T1279. No actual logical changes, but:

  - You can now add projects as reviewers from the revision view typeahead ("Add Reviewers" action).
  - You can now add projects as reviewers from the revision detail typeahead.
  - You can now add projects as reviewers from the CLI (`#yoloswag`).
  - Generated commit messages now list project reviewers (`Reviewers: #yoloswag`).

I'll separate projects from users in the "Reviewers" tables in the next revision.

Test Plan:
  - Added projects as reviewers using the web UI and CLI.
  - Used `arc amend --show --revision Dnnn` to generate commit messages.
  - Viewed revision with project reviewers in web UI.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7230
2013-10-05 14:10:46 -07:00
epriestley
370c7635a7 Track "accepted" and "commented" in per-reviewer status
Summary: Ref T1279. Updates status to 'accepted' or 'commented' when the user takes those actions.

Test Plan:
  - Commented on a revision, got a comment icon.
  - Accepted a revision, got an accept icon.
  - Commented again, icon stayed as "accept".
  - Faked the "old diff" states.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7229
2013-10-05 14:10:45 -07:00
epriestley
4d8707df13 Use status list UI to show reviewers in Differential
Summary:
Ref T1279. No logical changes, just updates the reviewer display style.

We currently keep track of only "requested changes".

Test Plan: See screenshot.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7228
2013-10-05 14:10:44 -07:00
epriestley
65ddefad8b Migrate all Differential reviewer data to edges
Summary:
Ref T1279. @champo did a lot of this work already; we've been doing double writes for a long time.

Add "double reads" (reading the edge table as both the "relationship" table and as the "reviewer status" table), and migrate all the data.

I'm not bothering to try to recover old reviewer status (e.g., we could infer from transactions who accepted old revisions) because it wold be very complicated and doesn't seem too valuable.

Test Plan:
  - Without doing the migration, used Differential. Verified that reads and writes worked. Most of the data was there anyway since we've been double-writing.
  - Performed the migration. Verified that everything was still unchanged.
  - Dropped the edge table, verified all reviweer data vanished.
  - Migrated again, verified the reviewer stuff was restored.
  - Did various cc/reviewer/subscriber queries, got consistent results.

Reviewers: btrahan

Reviewed By: btrahan

CC: champo, aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7227
2013-10-05 13:54:02 -07:00
epriestley
bd8071bb33 Add a (dst, type, src) key to Differential's edge table
Summary:
Ref T1279. This came to me in a dream.

The existing `differential_relationship` table has an `(objectPHID, type)` column, which theoretically is useful for queries like "revisions with X as a reviewer". In practice, I'm not sure it gets used much, but I can get it to show up in at least some query plans.

Add a similar index to the `edge` table. This sequences //before// D7227, which actually migrates the data.

Test Plan:
  - Ran `bin/storage upgrade`.
  - EXPLAIN'd a bunch of queries against different versions of the schema, this seemed helpful overall.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7232

Conflicts:
	src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
2013-10-05 13:53:43 -07:00
epriestley
e6d8e1a00a Make Herald rules obey policies during application
Summary:
Ref T603. This closes the other major policy loophole in Herald, which was that you could write a rule like:

  When [Always], [Add me to CC]

...and end up getting email about everything. These rules are now enforced:

  - For a //personal// rule to trigger, you must be able to see the object, and you must be able to use the application the object exists in.
  - In contrast, //global// rules will //always// trigger.

Also fixes some small bugs:

  - Policy control access to thumbnails was overly restrictive.
  - The Pholio and Maniphest Herald rules applied only the //last// "Add CC" or "Add Project" rules, since each rule overwrote previous rules.

Test Plan:
  - Created "always cc me" herald and maniphest rules with a normal user.
  - Created task with "user" visibility, saw CC.
  - Created task with "no one" visibility, saw no CC and error message in transcript ("user can't see the object").
  - Restricted Maniphest to administrators and created a task with "user" visibility. Same deal.
  - Created "user" and "no one" mocks and saw CC and no CC, respectively.
  - Thumbnail in Pholio worked properly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7224
2013-10-05 12:55:34 -07:00
Chad Little
ae27ce0f7d Tweak mobile timeline
Summary: Improves timeline legebility by pulling date inline with title in timeline mobile.

Test Plan: shrink screen for a task, see new layout

Reviewers: epriestley, btrahan

Reviewed By: epriestley

CC: Korvin, aran

Differential Revision: https://secure.phabricator.com/D7236
2013-10-05 11:18:07 -07:00
epriestley
7dde01df76 Fix issues with first-time account registration
Summary: This worked originally, but the migration broke slightly after the
config was deprecated, and there was another minor issue during setup.
2013-10-05 08:02:41 -07:00
epriestley
f88a2b735d Remove spurious "+x" from files that shouldn't have it
Summary: We have a bunch of files with +x that aren't actually executable.
Remove +x from PNGs, etc.
2013-10-05 05:18:17 -07:00
Juan Pablo Civile
562da7e98b Stop using loadRelationships in differential related herald adapters
Summary:
Used `DifferentialRevisionQuery` with the relevant `need*()` calls in the test controller.
And started assuming the revision has reviewers and CC phids in `HeraldDifferentialRevisionAdapter`.

Test Plan:
Added herald rules that use revisions (one for revisions another for commit) and reviewers.
Created, accepted and landed a revision that matched the rules and checked all rules were applied.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D6468

Conflicts:
	src/applications/herald/adapter/HeraldCommitAdapter.php
	src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
	src/applications/herald/controller/HeraldTestConsoleController.php
2013-10-04 16:40:42 -07:00