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

14425 commits

Author SHA1 Message Date
epriestley
415ad78484 Remove old code for "Request Review" action from Differential
Summary: Ref T10967. This moves all remaining "request review" pathways (just `differential.createcomment`) to the new code, and removes the old action.

Test Plan: Requested review on a revision, `grep`'d for the action constant.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17567
2017-03-28 11:50:40 -07:00
epriestley
aea46e55da Fix an issue where "Request Review" of a fully-accepted revision would transition to "Accepted"
Summary:
Ref T10967. This is explained in more detail in T10967#217125

When an author does "Request Review" on an accepted revision, void (in the sense of "cancel out", like a bank check) any "accepted" reviewers on the current diff.

Test Plan:
  - Create a revision with author A and reviewer B.
  - Accept as B.
  - "Request Review" as A.
  - (With sticky accepts enabled.)
  - Before patch: revision swithced back to "accepted".
  - After patch: the earlier review is "voided" by te "Request Review", and the revision switches to "Review Requested".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17566
2017-03-28 11:50:15 -07:00
epriestley
7d3956bec1 Correct spelling of "Dasbhoard"
Summary: Before the speling pollice lock us in prisun.

Test Plan: Used a dicationairey.

Reviewers: chad, jmeador

Reviewed By: jmeador

Differential Revision: https://secure.phabricator.com/D17570
2017-03-28 10:04:26 -07:00
Mukunda Modell
9e2f263bb4 Add repositories to fulltext search index.
Summary:
This implements a simplistic `PhabricatorRepositoryFulltextEngine`
Currently only the repository name, description, timestamps and
status are indexed.

Note: I had to change the `search index` workflow to disambiguate
PhabricatorRepository from PhabricatorRepositoryCommit

Test Plan:
* ran `./bin/search index --type PhabricatorRepository --force`
 * searched for some repositories. Saw reasonable results matching on either title or description.
* Edited a repository in the web ui
 * Added unique key words to the repo description.
 * I was then able to find that repo by searching for the new keywords.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Tags: #search, #diffusion

Differential Revision: https://secure.phabricator.com/D17300
2017-03-28 07:58:22 +00:00
Chad Little
9e2ab4f80e Scope syntax css rules to direct descendants only in diffs
Summary: Fixes T11641. We're overbroad here (and this may need more scoping?) but this seems to resolve the immediate issue.

Test Plan: Upload a few diffs and ask disabled accounts to comment on them inline.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11641

Differential Revision: https://secure.phabricator.com/D17565
2017-03-27 10:25:37 -07:00
Chad Little
76404c5fdb Cleaner fullscreen / preview states for Remarkup bar
Summary: General CSS and usability touchup of the Remarkup bar states for fullscreen and preview. Larger fonts, more spacing, some hint of the underlying page. Disable buttons that can't be used in preview mode.

Test Plan:
Formal test coming with mobile, browsers. This is a kick the tires upload.

{F4283448}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17563
2017-03-27 09:19:23 -07:00
Mukunda Modell
e41c25de50 Support multiple fulltext search clusters with 'cluster.search' config
Summary:
The goal is to make fulltext search back-ends more extensible, configurable and robust.

When this is finished it will be possible to have multiple search storage back-ends and
potentially multiple instances of each.

Individual instances can be configured with roles such as 'read', 'write' which control
which hosts will receive writes to the index and which hosts will respond to queries.

These two roles make it possible to have any combination of:

* read-only
* write-only
* read-write
* disabled

This 'roles' mechanism is extensible to add new roles should that be needed in the future.

In addition to supporting multiple elasticsearch and mysql search instances, this refactors
the connection health monitoring infrastructure from PhabricatorDatabaseHealthRecord and
utilizes the same system for monitoring the health of elasticsearch nodes. This will
allow Wikimedia's phabricator to be redundant across data centers (mysql already is,
elasticsearch should be as well).

The real-world use-case I have in mind here is writing to two indexes (two elasticsearch clusters
in different data centers) but reading from only one. Then toggling the 'read' property when
we want to migrate to the other data center (and when we migrate from elasticsearch 2.x to 5.x)

Hopefully this is useful in the upstream as well.

Remaining TODO:

* test cases
* documentation

Test Plan:
(WARNING) This will most likely require the elasticsearch index to be deleted and re-created due to schema changes.

Tested with elasticsearch versions 2.4 and 5.2 using the following config:

```lang=json
  "cluster.search": [
    {
      "type": "elasticsearch",
      "hosts": [
        {
          "host": "localhost",
          "roles": { "read": true, "write": true }
        }
      ],
      "port": 9200,
      "protocol": "http",
      "path": "/phabricator",
      "version": 5
    },
    {
      "type": "mysql",
      "roles": { "write": true }
     }
  ]

Also deployed the same changes to Wikimedia's production Phabricator instance without any issues whatsoever.
```

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, epriestley

Tags: #elasticsearch, #clusters, #wikimedia

Differential Revision: https://secure.phabricator.com/D17384
2017-03-26 08:16:47 +00:00
epriestley
a41d158490 Only hibernate the Taskmaster after 15 seconds of inactivity
Under some workloads, the taskmaster may hibernate and launch more rapidly
than it should. Require 15 seconds of inactivity before hibernating. Also
hibernate for longer.

Auditors: chad
2017-03-25 05:01:32 -07:00
epriestley
2cda280cde Make the default Trigger hibernation 3 minutes instead of 5 seconds
The `min()` vs `max()` fix in D17560 meant that the Trigger daemon only
hibernates for 5 seconds, so we do a full GC sweep every 5 seconds. This ends
up eating a fair amount of CPU for no real benefit.

The GC cursors should move to persistent storage, but just bump this default
up in the meantime.

Auditors: chad
2017-03-25 04:14:32 -07:00
epriestley
b4effdf26c Fix a rendering fatal for unknown edge constants
If we try to render an edge transaction which uses unknown edge constants,
it turns out we fatal. Degrade instead. This happened when viewing very old
badges.

Auditors: chad
2017-03-24 16:58:48 -07:00
Chad Little
186460888d Funbeta Badges
Summary: Ships Badges. I can write up some basic docs too if needed.

Test Plan: /applications/

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12270

Differential Revision: https://secure.phabricator.com/D17360
2017-03-24 21:15:42 +00:00
epriestley
080bf064c4 Remove obsolete Badges edge types
Summary: Ref T12270. These no longer have any callsites.

Test Plan: Used `grep` to search for each edge class constant, found no hits.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12270

Differential Revision: https://secure.phabricator.com/D17562
2017-03-24 14:11:46 -07:00
epriestley
6f80a04699 Paginate the profile badges view
Summary: Ref T12270. Adds a pager, plus a few little cleanups from copy/paste and accumulated cruft.

Test Plan:
  - Paginated a user with 180 badges.
  - Viewed a user with 0 badges.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12270

Differential Revision: https://secure.phabricator.com/D17561
2017-03-24 14:10:59 -07:00
epriestley
8b553d2f18 Allow taskmaster daemons to hibernate
Summary: Ref T12298. Like PullLocal daemons, this allows the last daemon in the pool to hibernate if there's no work to be done, and awakens the pool when work arrives.

Test Plan:
  - Ran `bin/phd debug task --trace`.
  - Saw the pool hibernate and look for tasks.
  - Commented on an object.
  - Saw the pool wake up and process the queue.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12298

Differential Revision: https://secure.phabricator.com/D17559
2017-03-24 13:51:37 -07:00
epriestley
3cdabb9588 Provide a hint that submitting a Conduit call shows you how to encode particular parameters
Summary: Ref T12447.

Test Plan: {F4270003}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12447

Differential Revision: https://secure.phabricator.com/D17557
2017-03-24 13:15:03 -07:00
epriestley
24b6c7d718 Allow users to resign if they have authority over any reviewer
Summary:
Ref T11050. The old rule was "you can only resign if you're a reviewer".

With the new behavior of "resign", the rule should be "you can resign if you're a reviewer, or you have authority over any reviewer". Make it so.

Also fixes T12446. I don't know how to reproduce that but I'm pretty sure this'll fix it?

Test Plan:
  - Could not resign from a revision with no authority/reviewer.
  - Resigned from a revision with myself as a reviewer.
  - Resigned from a revision with a package I owned as a reviewer.
  - Could not resign from a revision I had already resigned from.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12446, T11050

Differential Revision: https://secure.phabricator.com/D17558
2017-03-24 13:14:47 -07:00
epriestley
daeb94561f When destroying Calendar events, destroy invitees and notifications
Summary: Fixes T12395.

Test Plan: Ran `bin/remove destroy E... --trace`, saw invitee and notification destruction.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12395

Differential Revision: https://secure.phabricator.com/D17555
2017-03-24 09:21:13 -07:00
epriestley
0ffde484e5 Give Daemons a mobile menu
Summary: Fixes T12422.

Test Plan: {F4269080}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12422

Differential Revision: https://secure.phabricator.com/D17554
2017-03-24 09:19:56 -07:00
epriestley
f13637627d Improve daemon "waiting" message, config reload behavior
Summary:
Ref T12298. Two minor daemon improvements:

  - Make the "waiting" message reflect hibernation.
  - Don't trigger a reload right after launching.

Test Plan:
- Read "waiting" message.
- Ran "bin/phd start", didn't see an immediate SIGHUP in the log.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12298

Differential Revision: https://secure.phabricator.com/D17550
2017-03-24 08:32:08 -07:00
Chad Little
2707681b48 Restrict Audit buckets to just ApplicationSearch views
Summary: Fixes T9363. This drops empty buckets from dashboard panel context. Still see full results in Audit.

Test Plan: Create an "Active Audits" panel, add to Dashboard. See no commits found. Check Audit, see all buckets.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T9363

Differential Revision: https://secure.phabricator.com/D17545
2017-03-23 12:46:19 -07:00
Chad Little
4f2bca58fc Fix typo in diviner user guide / diffusion
Summary: Fixes T12445. Reads better.

Test Plan: Read it a few more times.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12445

Differential Revision: https://secure.phabricator.com/D17546
2017-03-23 12:43:40 -07:00
Chad Little
ffab52f17e Restrict Differential buckets to just ApplicationSearch views
Summary: Ref T9363, If we're in a dashboard panel, only show buckets with data, or a fallback if nothing exists.

Test Plan: Test 'active revisions' panel in a dashboard and in Differential.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T9363

Differential Revision: https://secure.phabricator.com/D17544
2017-03-23 12:09:44 -07:00
epriestley
9099485a71 Allow the PullLocal daemon to hibernate, and wake it when repositories need an update
Summary: Ref T12298. This allows the PullLocal daemon to hibernate like the Trigger daemon, but automatically wakes it back up when it needs to do something.

Test Plan:
  - Ran `bin/phd debug pulllocal --trace`.
  - Saw the daemon hibernate after doing a checkup on repositories.
  - Saw periodic queries to look for new update messages.
  - After clicking "Update Now" in the web UI to schedule an update, saw the daemon wake up immediately.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12298

Differential Revision: https://secure.phabricator.com/D17540
2017-03-23 10:52:28 -07:00
epriestley
9326b4d131 Fix some range issues and 32-bit issues with avatar generation
Summary:
Ref T12444. A few issues:

   - `x % (y - z)` doesn't generate values in the full range: the largest value is never generated. Instead, use `x % (1 + y - z)`.
   - `digestToRange(1, count)` never generates 0. After fixing the first bug, it could generate `count`. The range of the arrays is `0..(count-1)`, inclusive. Generate the correct range instead.
   - `unpack('L', ...)` can unpack a negative number on a 32-bit system. Use `& 0x7FFFFFFF` to mask off the sign bit so the result is always a positive integer.
   - FileFinder might return arbitrary keys, but we rely on sequential keys (0, 1, 2, ...)

Test Plan:
  - Used `bin/people profileimage ... --force` to regenerate images.
  - Added some debugging to verify that the math seemed to be working.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12444

Differential Revision: https://secure.phabricator.com/D17543
2017-03-23 10:51:33 -07:00
epriestley
1953ab98be Don't show the "Override Lock" prompt when creating objects
Summary:
Fixes T12369. When you create objects they may technically be locked: either because the default state is legitimately locked, or because the default policies prevent you from viewing so we sort of technically end in a locked state.

Regardless, don't prompt during creation, since this prompt isn't useful even if the lock detection is completely legitimate.

Test Plan:
  - In {nav Applications > Maniphest > Configure}, set "Default View Policy" to "No One".
  - Tried to create a task.
  - Before patch: prompted to override lock.
  - After patch: no override prompt.

Reviewers: chad

Reviewed By: chad

Subscribers: d.maznekov

Maniphest Tasks: T12369

Differential Revision: https://secure.phabricator.com/D17541
2017-03-23 06:40:14 -07:00
epriestley
aa91dc992e Record which user accepted on behalf of packages/owners reviewers
Summary:
Ref T12271. Don't do anything with this yet, but store who accepted/rejected/whatever on behalf of reviewers.

In the future, we could use this to render stuff like "Blessed Committers (accepted by epriestley)" or whatever. I don't know that this is necessarily super useful, but it's easy to track, seems likely to be useful, and would be a gigantic pain to backfill later if we decide we want it.

Test Plan: Accepted/rejected a revision, saw reviewers update appropriately.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12271

Differential Revision: https://secure.phabricator.com/D17537
2017-03-22 14:26:37 -07:00
epriestley
fab37aa4e3 When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.

There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.

Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.

In cases where project/package reviewers aren't in use, this doesn't change anything.

For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.

Test Plan:
  - Accepted normally.
  - Accepted a subset.
  - Tried to accept none.
  - Tried to accept bogus reviewers.
  - Accepted with myself not a reviewer
  - Accepted with only one reviewer (just got normal "this will be accepted" text).

{F4251255}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12271

Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 14:25:04 -07:00
epriestley
e1ee8ba428 Fix a bad getStatus() call which is fataling during Herald rule evaluation
Summary: Hit this while `arc diff`'ing something which is triggering 2+ rules which add reviewers, I think.

Test Plan: Dug this out of a production stack trace; will push and `arc diff` again.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D17534
2017-03-22 10:03:38 -07:00
epriestley
9c998e988b Don't require mentioned objects to have all required fields when editing comments
Summary: Fixes T12439. This pathway was just missing a `setContinueOnMissingFields(...)` to skip enforcement of required fields.

Test Plan:
  - Added a required custom field.
  - Mentioned any task without a field value in a comment.
  - Edited that comment.
  - Saved changes.
  - Before fix: fatal in log.
  - After fix: clean edit.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12439

Differential Revision: https://secure.phabricator.com/D17536
2017-03-22 09:59:40 -07:00
Chad Little
5e16e46039 Remove "Aleo" as specialized font for headers
Summary: Fixes T11865. Part of a 'clean up remarkup' pass, removing Aleo helps simplify coding, is lighter on the wire, and gives a more consistent, clean look.

Test Plan: run celerity, grep for 'aleo' and 'Aleo', test Phriction, tasks

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11865

Differential Revision: https://secure.phabricator.com/D17535
2017-03-22 09:57:00 -07:00
epriestley
3e7b63aa73 Add a <reviewer, revision> key to the reviewers table
Summary:
Ref T10967. I'm not 100% sure we need this, but the old edge table had it and I recall an issue long ago where not having this key left us with a bad query plan.

Our data doesn't really provide a way to test this key (we have many revisions and few reviewers, so the query planner always uses revision keys), and building a convincing test case would take a while (lipsum needs some improvements to add reviewers). But in the worst case this key is mostly useless and wastes a few MB of disk space, which isn't a big deal.

So I can't conclusively prove that this key does anything to the dashboard query, but the migration removed it and I'm more comfortable keeping it so I'm not worried about breaking stuff.

At the very least, MySQL does select this key in the query plan when I do a "Reviewers:" query explicitly so it isn't //useless//.

Test Plan: Ran `bin/storage upgrade`, ran dashboard query, the query plan didn't get any worse.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17532
2017-03-22 09:51:06 -07:00
epriestley
8913552970 Store "resigned" as an explicit reviewer state
Summary:
Fixes T11050. Today, when a user resigns, we just delete the record of them ever being a reviewer.

However, this means you have no way to say "I don't care about this and don't want to see it on my dashboard" if you are a member of any project or package reviewers.

Instead, store "resigned" as a distinct state from "not a reviewer", and treat it a little differently in the UI:

  - On the bucketing screen, discard revisions any responsible user has resigned from.
  - On the main `/Dxxx` page, show these users as resigned explicitly (we could just hide them, too, but I think this is good to start with).
  - In the query, don't treat a "resigned" state as a real "reviewer" (this change happened earlier, in D17517).
  - When resigning, write a "resigned" state instead of deleting the row.
  - When editing a list of reviewers, I'm still treating this reviewer as a reviewer and not special casing it. I think that's sufficiently clear but we could tailor this behavior later.

Test Plan:
  - Resigned from a revision.
  - Saw "Resigned" in reviewers list.
  - Saw revision disappear from my dashboard.
  - Edited revision, saw user still appear as an editable reviewer. Saved revision, saw no weird side effects.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11050

Differential Revision: https://secure.phabricator.com/D17531
2017-03-22 09:50:50 -07:00
epriestley
3d35d6d3f9 Remove duplicate "Change Default Values" action in form editing workflow
Summary: Fixes T12434. I accidentally copy/pasted this too much in D17442.

Test Plan: Viewed a form edit page, no longer saw two copies of this action.

Reviewers: chad, cspeckmim

Reviewed By: chad, cspeckmim

Maniphest Tasks: T12434

Differential Revision: https://secure.phabricator.com/D17530
2017-03-22 09:50:38 -07:00
Chad Little
5e423c5fe0 Provide a 'no dashboards' fallback state if you can't add any
Summary: Ref T10390. Catch if the user doesn't have any dashboards they can edit and give them a helpful message instead.

Test Plan: Clean install, no dashboards, Click "Add to Dashboard" on ApplicationSearch results, see no dashboards message

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T10390

Differential Revision: https://secure.phabricator.com/D17528
2017-03-21 11:43:02 -07:00
Chad Little
3a838ba312 Add Dashboards as a default pinned application
Summary: Ref T10390. Dashboard usability is high enough that I think we should pin it by default for users to create custom home pages.

Test Plan: Review order of applications in sandbox.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T10390

Differential Revision: https://secure.phabricator.com/D17527
2017-03-21 11:10:20 -07:00
Chad Little
d6f7da8685 Add some new Dashboard icons
Summary: Ref T10390. Fixes the missing "fa-dashboard" icon and adds a few more for an even 25.

Test Plan: Create new dashboard, see dashboard icon, select new dashboard icon.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T10390

Differential Revision: https://secure.phabricator.com/D17526
2017-03-21 11:00:16 -07:00
Chad Little
7d4c0f002f Allow searching Dashboards by Editable
Summary: Ref T10390. I find myself wanting to find dashboards I can edit, even if I am not the author. I think this is useful for larger installs with multiple admins. Also make disabled Dashboards more grey in UI results.

Test Plan: Log in a test user, create a dashboard with I cannot edit. Log into my account, search for editable dashboards and only see mine. Set dashboard to all users, search under test account and see editable dashboards.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T10390

Differential Revision: https://secure.phabricator.com/D17524
2017-03-21 09:39:04 -07:00
Chad Little
1182bbcae7 Remove FreeNode from "support" options
Summary: Don't think it's fair to send users there anymore, we can use Conpherence better (and searchable).

Test Plan: Remove copy.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D17525
2017-03-20 22:19:21 -07:00
Chad Little
1a5d92184c Try to guess a name for the 'Add to Dashboard' workflow
Summary: Ref T5307. Just makes the dialog a little easier to use. Picks a name if we already have one.

Test Plan: Test a builtin, custom saved, and a new advanced search (no name).

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T5307

Differential Revision: https://secure.phabricator.com/D17523
2017-03-20 18:02:34 -07:00
epriestley
0ceab7d36f Rename "getReviewerStatus()" to "getReviewers()"
Summary:
Ref T10967. Improves some method names:

  - `Revision->getReviewerStatus()` -> `Revision->getReviewers()`
  - `Revision->attachReviewerStatus()` -> `Revision->attachReviewers()`
  - `Reviewer->getStatus()` -> `Reviewer->getReviewerStatus()` (this is mostly to make this more greppable)

Test Plan:
  - bunch o' `grep`
  - Browsed around.
  - If I missed anything, it should fatal in an obvious way. We have a lot of other `getStatus()` calls and it's hard to be sure I got them all.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17522
2017-03-20 17:11:40 -07:00
epriestley
a15df4f8d5 Rename "needReviewerStatus()" into "needReviewers()"
Summary: Ref T10967. The old name was because we had a `getReviewers()` tied to `needRelationships()`, rename this method to use a simpler and more clear name.

Test Plan: `grep`, browsed around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17519
2017-03-20 16:46:16 -07:00
epriestley
d179d0150c Remove obsolete "relationships" code from Differential
Summary:
Ref T10967. There have been two different ways to load reviewers for a while: `needReviewerStatus()` and `needRelationships()`.

The `needRelationships()` stuff was a false start along time ago that didn't really go anywhere. I believe the idea was that we might want to load several different types of edges (subscribers, reviewers, etc) on lots of different types of objects. However, all that stuff pretty much ended up modularizing so that main `Query` classes did not need to know about it, so `needRelationships()` never got generalized or went anywhere.

A handful of things still use it, but get rid of them: they should either `needReviewerStatus()` to get reviewer info, or the ~3 callsites that care about subscribers can just load them directly.

Test Plan:
  - Grepped for removed methods (`needRelationships()`, `getReviewers()`, `getCCPHIDs()`, etc).
  - Browsed Diffusion, Differential.
  - Called `differential.query`.

It's possible I missed some stuff, but it should mostly show up as super obvious fatals ("call needReviewerStatus() before getReviewerStatus()!").

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17518
2017-03-20 16:45:48 -07:00
epriestley
dccd799b1b Move many "reviewers" readers to new storage
Summary:
Ref T10967.

When we query for revisions with particular reviewers, use the new table to drive the query.

When we load revisions for use in the application, also use the new table to drive the query.

This doesn't convert everything: there's some old `loadRelationships()` stuff still using the old table. But this moves the major stuff over.

(This also changes the icon for "commented" from a question mark to a speech bubble.)

Test Plan:
  - Viewed revision lists and detail views on old and new code, saw identical outcomes.
  - Updated revisions, accepted/rejected/commented on revisions.
  - Hit the "Accepted Older" and "Commented Older" states by taking an action and then updating.
  - Grepped for removed methods (like `getEdgeData()` and `getDiffID()`).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17517
2017-03-20 16:45:28 -07:00
epriestley
8ad5d28686 Migrate old reviewer edges to new storage
Summary:
Ref T10967. We still have double writes, so all reviewers are being written to both old and new storage. This migrates all the data in the old storage to the new storage, so both storage tables should have a complete set of data and be getting identical updates as we move forward.

After this, I can move readers over one at a time and eventually get rid of the old writes and old storage.

This loads all of the edge data into memory in a big chunk. I reached out to one install to get some more information about their data size. Ours is quite manageable and I think even large installs will probably fit into memory, but we can do this in chunks if not.

However, because the Edge table doesn't have an `id` column, we can't use either the `RawMigrationIterator` or the `MigrationIterator`, and would need to write a new `EdgeMigrationIterator`. This isn't tons of work but might not be necessary.

Test Plan: Ran the migration locally, spot-checked the results in the database for sanity and correctness.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17515
2017-03-20 16:44:40 -07:00
epriestley
794b456530 Store "last comment" and "last action" diffs on reviewers
Summary:
Ref T10967. We have a "commented" state to help reviewers get a better sense of who is part of a discussion, and a "last action" state to help distinguish between "accept" and "accepted an older version", for the purposes of sticky accepts and as a UI hint.

Currently, these are first-class states, partly beacuse we were somewhat limited in what we could do with edges. However, a more flexible way to represent them is as flags separate from the primary state flag.

In the new storage, write them as separate state information: `lastActionDiffPHID` stores the Diff PHID of the last review action (accept, reject, etc). `lastCommentDiffPHID` stores the Diff PHID of the last comment (top-level or inline).

Test Plan: Applied storage changes, commented and acted on a revision. Saw appropriate state reflected in the database.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17514
2017-03-20 16:44:05 -07:00
epriestley
77b3efafbd Use ModularTransactions for accept/reject/resign in "differential.createcomment"
Summary:
Ref T10967. `differential.createcomment` is a frozen API method which has been obsoleted by `differential.revision.edit`.

It is the only remaining way to apply an "accept", "reject", or "resign" action using the old "ACTION" code.

Instead of using the old code, sneakly apply a new type of transaction in these cases instead.

Then, remove all the remaining old code for this stuff on the write pathways.

Test Plan:
  - Used "differential.createcomment" to accept, reject, and resign from a revision.
  - Grepped for all removed ACTION_X constants, found them only in rendering code.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17513
2017-03-20 16:43:43 -07:00
epriestley
a9cbbf3e5e Apply Owners reviewers using ModularTransactions
Summary: Ref T10967. See that task for some discussion. This lets us do double writes on this pathway.

Test Plan: Set an Owners package to auto-review. Created revisions which triggered it: one with no reviewers (autoreview added); one with the package as a blocking reviewer explicitly (no automatic stuff happened, as expected).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17512
2017-03-20 16:43:17 -07:00
epriestley
216052baf9 Apply reviewer changes from Herald via ModularTransactions
Summary:
Ref T10967. This converts the reviewer update action in Herald from an older edge write to a newer ModularTransactions write.

The major value from this is that we get a double-write to the new reviewers table.

Test Plan:
  - Wrote a Herald rule to add a reviewer and a blocking reviewer.
  - Saw them added properly to a revision with: no reviewers; both as blocking; A as blocking, B as nonblocking; A as nonblocking, B as blocking.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967

Differential Revision: https://secure.phabricator.com/D17511
2017-03-20 16:42:54 -07:00
Chad Little
e69f8f717b Fix 'Add to Dashboard' issue with builtins
Summary: Ref T5307. Actually check the built in query with query, not engine.

Test Plan: Try a builtin query, and a custom query when making a dashboard panel.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T5307

Differential Revision: https://secure.phabricator.com/D17521
2017-03-20 15:07:26 -07:00
Chad Little
9b07adb8da Add better error checking to 'Add to Dashboard'
Summary: Ref T5307. Adds a better query check query, sets required for the name, adds the correct URI for cancelling.

Test Plan: Test a form without a name, fake a query string, test cancel button.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T5307

Differential Revision: https://secure.phabricator.com/D17520
2017-03-20 14:55:13 -07:00