Summary: Fixes spacing in Differential revision detail and Diffusion browse views.
Test Plan:
{F48677}
{F48678}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6359
Summary: This reverts commit e70bb28ea0. We didn't end up using these.
Test Plan: Looked at Differential.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6357
Summary: Ref T3485. Moves flag icon inline in the header.
Test Plan: {F48654}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3485
Differential Revision: https://secure.phabricator.com/D6355
Summary:
Ref T603. Ref T2625. Fixes T3241. Depends on D5451. Depends on D6346.
@wez, this changes the Differential revision list UI substantially and may generate a lot of bikeshedding / who-moved-my-cheese churn. See T3417 for context, for example. The motivations for this change are:
- The list now works on devices, like phones and tablets. This is a requirement to make the rest of Differential work on devices.
- Although ApplicationSearch intentionally presents a simpler interface initially and some options which were one click away before aren't now, it is much more powerful than the search it replaces and allows users to build, save, share, fork, edit, and customize a much wider range of queries. Users who used the old filters frequently can use Advanced Search -> Save Custom Query to create new versions of them, and of any other query. "Edit Queries.." allows users to remove and reorder queries, including builtin queries. Basically, there are like three things which have gone from "1-click" to "a few clicks", and ten trillion things which have gone from "hard/impossible" to "relatively easy".
The local screenshots look a bit iffy, but I think a lot of this is the fakenesss of my test data. If they still feel iffy in production we can tweak them until they feel good, like we did for Maniphest.
Test Plan:
{F48477}
{F48478}
Reviewers: btrahan, chad, wez
Reviewed By: btrahan
CC: aran, s
Maniphest Tasks: T603, T2625, T3241
Differential Revision: https://secure.phabricator.com/D6347
Summary:
Tried out `PhabricatorObjectItemView` for Differential. It looks smexy and smooth.
Refs T2014
- Title and Date as Maniphest
- Author in the handle icon
- Bar color reflects revision status (Needs Review, Accepted, Abandoned etc.) @chad looking for non-blue is faster than keeping watch for everything that's not "Closed" in old table form
- Some status information are in footer icons; currently only stale/old status display as well as saved drafts, maybe more in future; these come into my mind:
- No reviewer warning
- Push Blocking Priority (T2730)
- Trivial, fast review guaranteed
- Sketch / Just looking for advice/help
- Arcanist Project (T2614)
- Denote "Public Send-in" (T1476)
{F37662}
{F37663}
{F37664}
{F37665}
Some flaws:
- Date and reviewers on every entry the same?
- No respect for Differential fields (for some reason, every entry appeared the same, so broke it to parts)
- Plenty of (potential) increase in height - advise reducing paging length from 100 to 50 - or just ignore me
Suggestions for the future:
- Expand the meta information regarding revisions; e.g. the various status displays above
- Uh... T2543, T1279, T793, T731 and what else I want for Differential, because they are awesome!
- T793 should be in particular easy appearance-wise, just copy-paste from Maniphest
Test Plan: By looking at it, of course. Verified there are no errors or crashed
Reviewers: epriestley, chad, btrahan, liguobig
Reviewed By: chad
CC: aran, Korvin, edward, nh
Maniphest Tasks: T2014
Differential Revision: https://secure.phabricator.com/D5451
Conflicts:
src/__celerity_resource_map__.php
Summary: This allows the SavedQuery to modify what the result list looks like (e.g., include display flags and similar).
Test Plan: Looked at some ApplicationSearch apps.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6346
Summary:
Ref T603. Ref T2625. Use cursors to page Differential queries, not offsets.
The trick here is that some queries are ordered. In these cases, we either need to pass some kind of tuple or do a cursor lookup. For example, if you are viewing revisions ordered by `dateModified`, we can either have the next page be something like:
?afterDateModified=2398329373&afterID=292&order=modified
...or some magical token:
?afterToken=2398329373:292&order=modified
I think we did this in Conpherence, but one factor there was that paging orders update with some frequency. In most cases, I think it's reasonable to pass just the ID and do a lookup to get the actual clause value (e.g., go look up object ID 292 and see what its dateModified is) and I think this is much simpler in general.
Test Plan: Set page size in Differential to 3, and paged through result lists ordered by date created and date modified.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625
Differential Revision: https://secure.phabricator.com/D6345
Summary:
Ref T603. Ref T2625. Makes `DifferentialRevisionQuery` do policy checks.
Note that it still uses inefficient offset-based paging, but it's rare to page through revisions. I'll switch to cursor paging in a future diff.
Test Plan: Viewed a bunch of Differential interfaces, home page, etc. This shouldn't actually materially impact anything.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625
Differential Revision: https://secure.phabricator.com/D6344
Summary:
Currently, when querying for responsible users (revisions where a given user is either the author or a reviewer), we do this:
- If the query passes a bunch of hard-coded special cases, use a special hard-coded UNION.
- Otherwise, use a very complicated JOIN/WHERE clause.
This is bad for several reasons:
- Tons and tons of hard-coding and special casing.
- The JOIN/WHERE clause performs very poorly for large datasets.
- (As a material consequence, the homepage issues a responsible query which barely misses the hard-coded special cases and goes down the slow path.)
Instead, //always// use the UNION strategy to execute a "responsible" query. Specifically, if we have responsible PHIDs, temporarily add them to the author list and build a normal query, then repeat for reviewers, then UNION any clauses we built.
Fixes T3377. Ref T603. Ref T2625. Depends on D6342.
There's various folklore about UNION ALL / UNION DISTINCT performance. UNION DISTINCT is simpler here and the number of rows is small, although we could use UNION ALL in the form:
SELECT * FROM ((SELECT ...) UNION ALL (SELECT ...) ORDER) GROUP LIMIT
...if we find that there's some performance benefit at some point.
Test Plan: Used DarkConsole to examine queries. Viewed home page and Differential dashboard/authors/subscribers.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2625, T3377
Differential Revision: https://secure.phabricator.com/D6343
I don't actually have Phabricator installed locally so I have no idea if this works, but in theory this should add one of the + buttons in the left sidebar to quickly add a new macro.
Summary: They seem to look OK?
Test Plan: {F48529} {F48530}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6350
Summary:
Ref T603. This introduces a policy-aware DifferentialDiffQuery and converts most callsites.
I've left unusual callsites (mostly: hard to get the viewer, unusual query, queries related to active diffs) alone for now, so this isn't exhaustive but hits 60-80% of sites.
Test Plan: Created diff; created revision; viewed diffs and revisions; made additional conduit calls.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6338
Summary: Ref T603. This query isn't policy-aware yet, but prepare for it to be one day.
Test Plan: Looked at: home page; differential home; differential detail; diffusion browse. Made differential.query conduit call.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6337
Summary: Ref T603. This is a very old, very bad version of `DifferentialRevisionQuery`. I want to modernize only the latter. Express the remaining callsite of the former in terms of `DifferentialRevisionQuery`.
Test Plan: Executed all four modes of `differential.find`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6335
Summary:
Ref T603.
- Primarily, this gets rid of a `DifferentialRevisionListData` callsite.
- Also modernize and clean up some UI stuff.
Test Plan:
{F48260}
{F48261}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D6334
Summary:
- Add GC support to conduit logs.
- Add Query support to conduit logs.
- Record the actual user PHID.
- Show client name.
- Support querying by specific method, so I can link to this from a setup issue.
@wez, this migration may not be fast. It took about 8 seconds for me to migrate 800,000 rows in the `conduit_methodcalllog` table. This adds a GC which should keep the table at a more manageable size in the future.
You can safely delete all data older than 30 days from this table, although you should do it by `id` instead of `dateCreated` since there's no key on `dateCreated` until this patch.
Test Plan:
- Ran GC.
- Looked at log UI.
- Ran Conduit methods.
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Differential Revision: https://secure.phabricator.com/D6332
Summary:
Ref T603. Ref T2625.
Long chain of "doing the right thing" here: I want to clean this up, so I can clean up the Conduit logs, so I can add a setup issue for deprecated method calls, so I can remove deprecated methods, so I can get rid of `DifferentialRevisionListData`, so I can make Differntial policy-aware.
Adds modern infrastructure and UI to all of the Conduit interfaces (except only partially for the logs, that will be the next diff).
Test Plan:
{F48201}
{F48202}
{F48203}
{F48204}
{F48206}
This will get further updates in the next diff:
{F48205}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603, T2625
Differential Revision: https://secure.phabricator.com/D6331
Summary:
Depends on D6329. This fixes `http://www.example.com/D123`, which currently gets the "D123" rendered, after addition of the Asana rule. It also removes a hack for object refernces.
Basically, the "hyperlink" rule needs to happen after rules which specialize hyperlinks (Youtube, Asana) but before rules which apply to general text (like the Differential and Maniphest rules). Allow these rules to specify that they have higher or lower priority.
Test Plan: Asana rules, Differential rules and Diffusion rules now all markup correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6330
Summary: See discussion in IRC. Not 100% sure what's going on here because of email ghost theives, but conceivably a commit with no changes will end up with `null` changesets instead of `array()` changesets, which throws. Such diffs are certianly possible (`git commit --allow-empty`) even if they aren't the issue in this specific case. See T3416. Initialize changesets to `array()` to avoid throwing.
Test Plan:
Viewed some commits?
iiam
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6339
Summary:
Addes a button group to filter tasks by agents, non-agents or all.
Fixes T3394
Test Plan: View task list, filter by agents, filter by non agents. Make sure the correct tasks display.
Reviewers: epriestley, dctrwatson
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3394
Differential Revision: https://secure.phabricator.com/D6328
Summary: Fixes T3400. Users are crafty. Attempt to outwit them.
Test Plan: Added all kinds of nonsense to my PATH to hit all the errors. Verified sensible-looking error messages which I couldn't figure out any way to misread or outwit.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3400
Differential Revision: https://secure.phabricator.com/D6318
Summary: I have simply copied the existing css into the oblivious skin. I don't know if this is the right approach (code duplication), but considering this skin should be isolated (and will potentially differ) I think this makes sense.
Test Plan: Use a code block on a phame post.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Differential Revision: https://secure.phabricator.com/D6313
Summary:
Fixes T3436. Currently, when installs have configuration options which we don't know about, we raise a fairly confusing/ambiguous message about the options being unknown. Instead:
- Keep a list of previously valid (but now deleted) config, with explanatory reasons for what happened to it. Present this information, along with altenate wording ("Obsolete Config" instead of "Unknown Config") where applicable.
- Show a list of all the places the config is defined.
- Provide an active link to delete it from the web UI.
- Provide a command to delete it from the CLI.
- Allow `bin/config delete` to delete configuration options which no longer have a definition.
Test Plan:
- Set an auth key in database, local and file config.
- Walked through the setup issue, cleaning it up.
- Set an invalid key and made sure I still got a reasonable error (this now has better cleanup instructions).
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3436
Differential Revision: https://secure.phabricator.com/D6317
Summary: Currently you can't refresh the live blog or a blog post after clicking "View Live" due to POST action. I have removed the setRenderAsForm call on the "View Live" actions. I am unsure if this has any unintended consequences but I have tested and not found any.
Test Plan: Click the "View Live" action within a blog post or blog, and observe that the request occurs via GET not POST
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6314
Summary: Ref T2852. Setting followers (like CCs) is a separate API call, but we don't need to do anything complicated.
Test Plan: Synchronized revisions and verified the parent task got followers.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6308
Summary:
Ref T2852.
Before trying related users, try using the feed story's actor. This is the most correct voice to act in.
Test Plan: Ran `feed/republish`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6305
Summary:
Ref T2852.
The parent task is open unless the revision is in the states "closed" or "abandoned". If it's in "needs review", it remains open. This last bit is slightly unlike Differential, but consistent with the Google Doc and generally seems like a better fit. There's no way to put the task in a "Waiting on Others" state in Asana like we can in Differential.
The subtasks are closed unless the revision is in the state "needs review". This is generally consistent with Differential.
Test Plan:
Made a series of changes to a revision and synchronized it repeatedly:
- requested changes
- commandeered
- requested review
- abandoned
Verified task and subtasks synchronized states correctly in Asana.
{F47554}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6304
Summary:
Ref T2852. Depends on D6302. This now creates, destroys, and synchronizes subtasks.
- After finishing the parent task stuff, we pull a list of all known subtasks.
- We load all those subtasks.
- If we fail to load any, we delete their objects and edges on the Phabricator side.
- Of the remaining subtasks, we find subtasks for users who aren't related to the object any more and delete them in Asana and locally (for example, if alincoln is removed as a reviewer, we delete his subtask).
- For all the related users, we either synchronize their existing task or create a new one for them.
- Then we write edges for any new tasks we added.
This doesn't handle a few weird edge cases in any specific way:
- If a subtask is moved under a different parent, we ignore it.
- If a new subtask is created that we don't know about, we ignore it.
- If a subtask we know about is deleted, we just respawn it. This is consistent with "DON'T EDIT THESE". You can force sync to stop by deleting the parent.
Addititionally:
- Make the "don't edit" warning more compelling and visceral.
Test Plan:
- Kind of ran it a bit.
- There are like 3,000 edge cases here so this is hard to test exhaustively.
- Forced a few of the edge cases to happen.
- Nothing seems immediately broken in an obvious way?
{F47551}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6303
Summary:
Ref T2852. This is highly incomplete but seems structurally sound. Some additional context is available in the Google doc.
- Add a workspace ID configuration. Without it, nothing else activates.
- Add a worker which reacts to feed stories.
- Feed stories about things which aren't Differential objects are ignored.
- We load the revision, or fail permanently if we can't.
- We get all the related user PHIDs (author, reviewers, CCs).
- We check if any of them have linked Asana accounts, or fail permanently if they don't.
- We check for an "ASANATASK" edge from the revision.
- If we do not find one, we create a new task.
- If we do find one, we load the task.
- If we succeed, we check the chronological key of the most recent synchronized feed story ("cursor").
- If this story is the same or newer, we update the task to synchronize it to the current state of the revision.
- If we fail to load the task, we fail permanently ("asana task has been deleted").
- We then publish the actual story text to the task.
Not in yet:
- Updating followers requires separate API calls which we don't do yet.
- No subtasks yet.
- No sync of open/closed state.
Test Plan: {F47546}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6302
Summary: Currently if disqus is used and a phame post is loaded over ssl, the disqus api is not loaded over https. This fixes that by honouring the protocol being used by the html document.
Test Plan: Open a phame post over https
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6311
Summary: Ref T2852. Reduce the number of magical strings in use, and prepare the Asana bridge for eventual workspace/project support (a little bit).
Test Plan: Verified enriched links still work properly.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6301
Summary:
Ref T2852. Add a `log()` method to `PhabricatorWorker` to make debugging easier.
I renamed the similar Drydock-specific method.
Test Plan:
Used logging in a future revision:
...
<<< [36] <http> 211,704 us
Updating main task.
>>> [37] <http> https://app.asana.com/api/1.0/tasks/6153776820388
...
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6296
Summary:
Ref T1536. Ref T2852. Currently, after refreshing the token we don't actually return it. This means that code relying on token refresh fails once per hour (for Asana) in a sort of subtle way. Derp.
Update `bin/auth refresh` to make this failure more clear.
Test Plan: Set `force refresh` flag and verified a return value.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536, T2852
Differential Revision: https://secure.phabricator.com/D6295
Summary:
Ref T2852.
- Broadly, we support "I have a Ref, I need a PHID" well but not "I have a PHID, I need a Ref".
- Add DoorkeeperExternalObjectQuery, and use it to query ExternalObjects.
- Allow external objects to be imported by their internal PHIDs. Basically, if we have an edge pointing at an ExternalObject, we can say "load all the data about this" from just the PHID and have it hit all the same code.
- Allow construction of Refs from ExternalObjects. This makes the "I have a PHID, I need a Ref" easier.
Test Plan:
- Verified Asana links still enrich properly at display time.
- Used in future revision.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6294
Summary:
Ref T2852. I want to model Asana integration as a response to feed events. Currently, we queue one feed event for each HTTP hook.
Instead, always queue one feed event and then have it queue any necessary followup events (now, http hooks; soon, asana).
Add a script to make it easy to reproducibly fire feed event publishing.
Test Plan:
Republished a feed event and verified it hit configured HTTP hooks correctly.
$ ./bin/feed republish 5765774156541908292 --trace
>>> [2] <connect> phabricator2_feed
<<< [2] <connect> 1,660 us
>>> [3] <query> SELECT story.* FROM `feed_storydata` story JOIN `feed_storyreference` ref ON ref.chronologicalKey = story.chronologicalKey WHERE (ref.chronologicalKey IN (5765774156541908292)) GROUP BY story.chronologicalKey ORDER BY story.chronologicalKey DESC
<<< [3] <query> 595 us
>>> [4] <connect> phabricator2_differential
<<< [4] <connect> 760 us
>>> [5] <query> SELECT * FROM `differential_revision` WHERE phid IN ('PHID-DREV-ywqmrj5zgkdloqh5p3c5')
<<< [5] <query> 478 us
>>> [6] <query> SELECT * FROM `differential_revision` WHERE phid IN ('PHID-DREV-ywqmrj5zgkdloqh5p3c5')
<<< [6] <query> 449 us
>>> [7] <connect> phabricator2_user
<<< [7] <connect> 1,062 us
>>> [8] <query> SELECT * FROM `user` WHERE phid in ('PHID-USER-lqiz3yd7wmk64ejugvov')
<<< [8] <query> 540 us
>>> [9] <connect> phabricator2_file
<<< [9] <connect> 951 us
>>> [10] <query> SELECT * FROM `file` WHERE phid IN ('PHID-FILE-gq6dlsysvxbn3dgwvky7')
<<< [10] <query> 498 us
>>> [11] <query> SELECT * FROM `user_status` WHERE userPHID IN ('PHID-USER-lqiz3yd7wmk64ejugvov') AND UNIX_TIMESTAMP() BETWEEN dateFrom AND dateTo
<<< [11] <query> 507 us
Republishing story...
>>> [12] <query> SELECT story.* FROM `feed_storydata` story JOIN `feed_storyreference` ref ON ref.chronologicalKey = story.chronologicalKey WHERE (ref.chronologicalKey IN (5765774156541908292)) GROUP BY story.chronologicalKey ORDER BY story.chronologicalKey DESC
<<< [12] <query> 685 us
>>> [13] <query> SELECT * FROM `differential_revision` WHERE phid IN ('PHID-DREV-ywqmrj5zgkdloqh5p3c5')
<<< [13] <query> 489 us
>>> [14] <query> SELECT * FROM `differential_revision` WHERE phid IN ('PHID-DREV-ywqmrj5zgkdloqh5p3c5')
<<< [14] <query> 512 us
>>> [15] <query> SELECT * FROM `user` WHERE phid in ('PHID-USER-lqiz3yd7wmk64ejugvov')
<<< [15] <query> 601 us
>>> [16] <query> SELECT * FROM `file` WHERE phid IN ('PHID-FILE-gq6dlsysvxbn3dgwvky7')
<<< [16] <query> 405 us
>>> [17] <query> SELECT * FROM `user_status` WHERE userPHID IN ('PHID-USER-lqiz3yd7wmk64ejugvov') AND UNIX_TIMESTAMP() BETWEEN dateFrom AND dateTo
<<< [17] <query> 551 us
>>> [18] <query> SELECT story.* FROM `feed_storydata` story JOIN `feed_storyreference` ref ON ref.chronologicalKey = story.chronologicalKey WHERE (ref.chronologicalKey IN (5765774156541908292)) GROUP BY story.chronologicalKey ORDER BY story.chronologicalKey DESC
<<< [18] <query> 507 us
>>> [19] <query> SELECT * FROM `differential_revision` WHERE phid IN ('PHID-DREV-ywqmrj5zgkdloqh5p3c5')
<<< [19] <query> 428 us
>>> [20] <query> SELECT * FROM `differential_revision` WHERE phid IN ('PHID-DREV-ywqmrj5zgkdloqh5p3c5')
<<< [20] <query> 419 us
>>> [21] <query> SELECT * FROM `user` WHERE phid in ('PHID-USER-lqiz3yd7wmk64ejugvov')
<<< [21] <query> 591 us
>>> [22] <query> SELECT * FROM `file` WHERE phid IN ('PHID-FILE-gq6dlsysvxbn3dgwvky7')
<<< [22] <query> 406 us
>>> [23] <query> SELECT * FROM `user_status` WHERE userPHID IN ('PHID-USER-lqiz3yd7wmk64ejugvov') AND UNIX_TIMESTAMP() BETWEEN dateFrom AND dateTo
<<< [23] <query> 593 us
>>> [24] <http> http://127.0.0.1/derp/
<<< [24] <http> 746,157 us
[2013-06-24 20:23:26] EXCEPTION: (HTTPFutureResponseStatusHTTP) [HTTP/500] Internal Server Error
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6291
Summary: Currently setting "disqus.shortname" via config isn't actually used in the instantiation of disqus. This fix uses the shortname configured.
Test Plan: Open a phame post with disqus enabled and a shortname other than "phabricator" specified.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6310
Summary: When I ignore setup issues, I want them to look dealt with, and keep yellow for new ones. Also updated callout colors.
Test Plan: Ignored a number of issues.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6300
Summary: Ref T3322. Depends on D6297. Here are some Phabricator tweaks to complment D6297.
Test Plan: {F47522}
Reviewers: garoevans
Reviewed By: garoevans
CC: aran, chad
Maniphest Tasks: T3322
Differential Revision: https://secure.phabricator.com/D6298
Summary:
Ref T1536. This is equivalent to logging out and logging back in again, but a bit less disruptive for users. For some providers (like Google), this may eventually do something different (Google has a "force" parameter which forces re-auth and is ostensibly required to refresh long-lived tokens).
Broadly, this process fixes OAuth accounts with busted access tokens so we can do API stuff. For other accounts, it mostly just syncs profile pictures.
Test Plan:
Refreshed LDAP and Oauth accounts, linked OAuth accounts, hit error conditions.
{F47390}
{F47391}
{F47392}
{F47393}
{F47394}
{F47395}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6290
Summary:
Ref T1536.
- Allow providers to customize the look of external accounts.
- For username/password auth, don't show the account view (it's confusing and not useful).
- For OAuth accounts, show token status.
Test Plan:
{F47374}
{F47375}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6289
Summary:
Ref T2852. Give OAuth providers a formal method so you can ask them for tokens; they issue a refresh request if necessary.
We could automatically refresh these tokens in daemons as they near expiry to improve performance; refreshes are blocking in-process round trip requests. If we do this for all tokens, it's a lot of requests (say, 20k users * 2 auth mechanisms * 1-hour tokens ~= a million requests a day). We could do it selectively for tokens that are actually in use (i.e., if we refresh a token in response to a user request, we keep refreshing it for 24 hours automatically). For now, I'm not pursuing any of this.
If we fail to refresh a token, we don't have a great way to communicate it to the user right now. The remedy is "log out and log in again", but there's no way for them to figure this out. The major issue is that a lot of OAuth integrations should not throw if they fail, or can't reasonably be rasied to the user (e.g., activity in daemons, loading profile pictures, enriching links, etc). For now, this shouldn't really happen. In future diffs, I plan to make the "External Accounts" settings page provide some information about tokens again, and possibly push some flag to accounts like "you should refresh your X link", but we'll see if issues crop up.
Test Plan: Used `bin/auth refresh` to verify refreshes. I'll wait an hour and reload a page with an Asana link to verify the auto-refresh part.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6280
Summary: Ref T2852. Provide a script for inspecting/debugging OAuth token refresh.
Test Plan: Ran `bin/auth refresh` with various arguments, saw token refreshes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6276
Summary:
Ref T2852. Primarily, this expands API access to Asana. As a user-visible effect, it links Asana tasks in Remarkup.
When a user enters an Asana URI, we register an onload behavior to make an Ajax call for the lookup. This respects privacy imposed by the API without creating a significant performance impact.
Test Plan: {F47183}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6274
Summary:
- `DoorkeeperObjectRef` is a convenience object to keep track of `<applicationType, applicationDomain, objectType, objectID>` tuples.
- `DoorkeeperBridge` provides pull/push between Phabricator and external systems.
- `DoorkeeperBridgeAsana` is a bridge to Asana.
Test Plan:
Ran this snippet and got a task from Asana:
{P871}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6273
Summary:
Ref T2852. This table holds data about external objects and allows us to write edges to them.
Objects are identified with an `<applicationType, applicationDomain, objectType, objectID>` tuple. For example, Asana tasks will be, e.g., `<asana, asana.com, asana:task, 93829279873>` or similar.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6271
Summary: ...also make it so in Pholio when you add an inline comment the preview refreshes. Fixes T2649.
Test Plan: played around in pholio leaving commentary. noted that a new inline comment would refresh the preview.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2649
Differential Revision: https://secure.phabricator.com/D6267
Summary: Ref T1536. This is missing a call.
Test Plan: Viewed a public blog with Facebook comments.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6288
Summary: Ref T2222. Currently, we load inline comments by `commentID` here, but we always pass every commentID associated with the revision. Instead, just load non-draft comments by revision ID. This simplifies querying a little bit and is likely faster anyway (draft comments are currently loaded separately).
Test Plan: Looked at some revisions and verified inlines showed up correctly and in the right places.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6270
Summary:
Ref T2222. My path forward here wasn't very good -- I was thinking I could set `transactionPHID` for the inline comments as I migrated, but it must be unique and an individual DifferentialComment may have more than one inline comment. Dropping the unique requirement just creates more issues for us, not fewer.
So the migration in D6266 isn't actually useful. Undo it -- this can't be a straight revert because some installs may already have upgraded.
Test Plan: Ran new migrations, verified the world ended up back in the same place as before (made comments, viewed reivsions).
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6269
Summary: Ref T1536. After DB-driven auth config, we need to load this differently.
Test Plan: Ran `bin/auth ldap`.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6287
Summary: Ref T2852. Asana supports a link directly to this panel, I just wasn't able to find it.
Test Plan:
Clicked the link and got to the apps panel.
{F47346}
Reviewers: isaac_asana, btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2852
Differential Revision: https://secure.phabricator.com/D6285
Summary:
Validates the keys on page load, alerts if there is a problem.
Fixes T3432
Test Plan: Use some good and bad keys, ensure we get the error at the right time.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3432
Differential Revision: https://secure.phabricator.com/D6284
Summary:
D6278 kind of got closed and commited, this is the actual direction.
Ref T3432
Depends on D6277
Test Plan: Keep using the site
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, mbishopim3
Maniphest Tasks: T3432
Differential Revision: https://secure.phabricator.com/D6283
Summary: Ref T2222. I didn't translate this query properly; reproduce the original.
Test Plan: When viewing a revision with non-draft inline comments by a user other than the viewer, the inline comments now appear on the changesets themselves.
Reviewers: kawakami, btrahan, garoevans
Reviewed By: garoevans
CC: aran, mbishopim3
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6281
Summary: Casting the defaul custom field storage to an int, need to stop that!
Test Plan:
Use some custom fields, make sure they work as expected.
Fixes T3444
Reviewers: epriestley, mbishopim3
Reviewed By: mbishopim3
CC: aran, Korvin, mbishopim3
Maniphest Tasks: T3444
Differential Revision: https://secure.phabricator.com/D6282
Summary: I assume this is a bug!
Test Plan: Look at it
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6279
Summary:
Ref T2222. This adds PHIDs to all Differential comments so I can migrate the inlinecommment table to transaction_comment in the next diff.
@wez, this will issue a few million queries for Facebook (roughly, one for each Differential comment ever made). It's safe to skip the `.php` half of the patch, bring Phabricator up normally, and then apply this patch with Phabricator running if that eases the migration, although the next few diffs will probably be downtime-required migrations so maybe it's easier to just schedule some downtime.
Test Plan: Ran migration locally. Verified existing comments and new comments received PHIDs.
Reviewers: btrahan
Reviewed By: btrahan
CC: wez, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6266
Summary:
Currently it's not allowed to be left blank (even with required: false)
Fixes T3343
Test Plan: Use the custom date field.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T3343
Differential Revision: https://secure.phabricator.com/D6251
Summary:
Ref T2222. See D6260.
Push all this junk behind a Query so I can move the storage out from underneath it.
Test Plan: Viewed home page, list view, revision. Made draft, looked at preview, submitted draft, viewed inline, replied to inline.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6262
Summary:
Ref T2222. Ref T1460. Depends on D6260.
This creates the new tables, but doesn't start using them. I added three new fields for {T1460}, to represent fixed/done/replied states.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T1460, T2222
Differential Revision: https://secure.phabricator.com/D6261
Summary:
Ref T2222.
I'm thinking about how I want to approach the Asana sync, and I want to try to do T2222 first so that we can build it cleanly on top of ApplicationTransactions. I think we can at least walk down this road a little bit and if it turns out to be scary we can take another approach.
I was generally very happy with how the auth migration turned out (seemingly, it was almost completely clean), and want to pursue a similar strategy here. Basically:
- Wrap the new objects in the old objects for reads/writes.
- Migrate all the existing data to the new table.
- Everything hard is done; move things over a piece at a time at a leisurely pace in lots of smallish, relatively-easy-to-understand changes.
This deletes or abstracts all reads of the DifferentialComment table. In particular, these things are **deleted**:
- The script `undo_commits.php`, which I haven't pointed anyone at in a very long time.
- The `differential.getrevisionfeedback` Conduit method, which has been marked deprecated for a year or more.
- The `/stats/` interface in Differential, which should be rebuilt on Fact and has never been exposed in the UI. It does a ton of joins and such which are prohibitively difficult to migrate.
This leaves a small number of reading interfaces, which I replaced with a new `DifferentialCommentQuery`. Some future change will make this actually load transactions and wrap them with DifferentialComment interfaces.
Test Plan: Viewed a revision; made revision comments
Reviewers: btrahan
Reviewed By: btrahan
CC: edward, chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6260
Summary: Ref T1536. Like Google, GitHub is actually strict about callback URIs too. Keep them pointed at the old URIs until we can gradually migrate.
Test Plan: Logged in with GitHub.
Reviewers: garoevans, davidreuss, btrahan
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6265
Summary: Changes it to a dialog view, tweaks some layout bugs on full width forms.
Test Plan: Tested loging in and resetting my password. Chrome + Mobile
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, nrp
Differential Revision: https://secure.phabricator.com/D6257
Summary: Used Differential emails as a formatting guide. This also includes "Image X: <inline comment>" similarly to how Differential has "<file path><line(s)>: <inline comment>" or what have you. Fixes T3138.
Test Plan: inserted some debugging code and verified the mail body format looked okay ish
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3138
Differential Revision: https://secure.phabricator.com/D6258
Summary: in applyExternalEffects, for subscriber transactions, we now re-load subscribers. also fixes a bug where a user can get emailed 2x when they take an action on a mock they created.
Test Plan: made some mocks. verified one copy sent to creator and one to each subscriber. (note having problems with email so I verified the phids mail was supposed to be sent to and did not get the actual email delivered)
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3315
Differential Revision: https://secure.phabricator.com/D6206
Summary:
Ref T1536.
- When users try to add a one-of provider which already exists, give them a better error (a dialog explaining what's up with reasonable choices).
- Disable such providers and label why they're disabled on the "new provider" screen.
Test Plan:
{F47012}
{F47013}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6256
Summary: Ref T1536. Mostly, this puts "username/password" (which is probably a common selection) first on the list.
Test Plan: {F47010}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6254
Summary: Ref T1536. This "should never happen", but can if you're developing custom providers. Improve the robustness of this interface in the presence of missing provider implementations.
Test Plan: {F47008}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6253
Summary: Generally prefer 'cards' to represent individual 'items' or 'action items', so I think it works here.
Test Plan: Reload setup issues pages.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6252
Summary:
Ref T1536.
- Move all the provider-specific help into contextual help in Auth.
- This provides help much more contextually, and we can just tell the user the right values to use to configure things.
- Rewrite account/registration help to reflect the newer state of the word.
- Also clean up a few other loose ends.
Test Plan: {F46937}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6247
Summary: Ref T1536. This is the last major migration. Moves us over to the DB and drops all the config stuff.
Test Plan:
- Ran the migration.
- Saw all my old config brought forward and respected, with accurate settings.
- Ran LDAP import.
- Grepped for all removed config options.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, wez
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6243
Summary:
Ref T1536. This sets us for the "Config -> Database" migration. Basically:
- If stuff is defined in the database, respect the database stuff (no installs have anything defined yet since they can't reach the interfaces/code).
- Otherwise, respect the config stuff (all installs currently do this).
Test Plan: Saw database stuff respected when database stuff was defined; saw config stuff respected otherwise.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6240
Summary:
Adds the phids of users entered into any user project query to handles phids for handle loading
Fixes T3395
Test Plan: Load page that was previous breaking
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3395
Differential Revision: https://secure.phabricator.com/D6250
Summary:
D6057 introduced images in the typeahead results, but not all
projects return a valid result. This silently broke /owners/new because
the exception "Call to a member function loadProfileImageURI() on a non-object"
is swallowed somewhere in the handler.
Test Plan: go to /owners/new and type something in the primary owner field
Reviewers: epriestley, nh, Afaque_Hussain
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6245
Summary: Added in color variables in most used places. Tweaked green to be a bit more serious.
Test Plan: Tested Tags, Error View, Timeline, Object Views, and Color Palette.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6244
Summary:
Ref T1536. Currently, when you install Phabricator you're dumped on the login screen and have to consult the documentation to learn about `bin/accountadmin`.
Instead, detect that an install is running first-time setup:
- It has no configured providers; and
- it has no user accounts.
We can safely deduce that such an install isn't configured yet, and let the user create an admin account from the web UI.
After they login, we raise a setup issue and lead them to configure authentication.
(This could probably use some UI and copy tweaks.)
Test Plan:
{F46738}
{F46739}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6228
Summary: This adds an 83% Light set of colors for highlights, warnings, etc.
Test Plan: Tested Notifications, Error View, and Color Palette page. Test is out, not quite sure on notifications.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6239
Summary: Ref T1536. Make this UI a bit more human-friendly.
Test Plan: {F46873}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6237
Summary: We end up with both "user.id" and "email.id". Disambiguate for ORDER.
Test Plan: Ran Conduit user.query query with "email".
Reviewers: wez, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6234
Summary:
Ref T1536. This is extremely reachable and changes the login code to the new stuff.
Notes:
- I've hard-disabled password registration since I want installs to explicitly flip it on via config if they want it. New installs will get it by default in the future, but old installs shouldn't have their auth options change.
- Google doesn't let us change the redirect URI, so keep the old one working.
- We need to keep a bit of LDAP around for now for LDAP import.
- **Facebook:** This causes substantive changes in what login code is executed.
Test Plan:
- Logged in / logged out / registered, hit new flows.
- Logged in with google.
- Verified no password registration by default.
Reviewers: btrahan, chad
Reviewed By: chad
CC: wez, nh, aran, mbishopim3
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6222
Summary:
Ref T1536. Because Facebook publishes data from Phabricator to user profiles and that data is sensitive, it wants to require secure browsing to be enabled in order to login.
Respect the existing option, and support it in the UI.
The UI part isn't reachable yet.
Test Plan: {F46723}
Reviewers: chad, btrahan
Reviewed By: chad
CC: arice, wez, aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6224
Summary: Picked a set of standard colors. Based on our current Maniphest color set, but tweaked to the same hue with http://color.hailpixel.com/
Test Plan: Not intended to be end all be all, but a decent first cut. Applied to Maniphest and Tags.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6229
Summary: Ref T1536. Love me some LDAP.
Test Plan: Viewed and edited form. Looked through transactions.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6227
Summary:
Ref T1536.
- When we render a dialog on a page by itself, put it on a dust background.
- Currently, we render "Logout" in two different places. Stop doing that.
- Make sure the surviving one has workflow so we get a modal ajax dialog if possible.
Test Plan: {F46731}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6226
Summary:
Ref T1536. None of this code is reachable.
For the new web UI for auth edits, give providers more and better customization options for handling the form. Allow them to format transactions.
Also fix the "Auth" application icon.
Test Plan: {F46718}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6221
Summary:
Ref T1536. Above, we load all providers, which is intentional (if a user has a link with a previously-enabled but now-disabled provider, we should enrich it with provider information).
However, before showing linking options we should drop disabled providers.
Test Plan: Disabled Disqus, reloaded, didn't see Disqus anymore.
Reviewers: mbishopim3
Reviewed By: mbishopim3
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6219
Summary:
Ref T1536.
- When linking accounts after initially failing, make the crumb say "Link Account" instead of "Login".
- When on the LDAP failure form, show a "Cancel" button returning to start (if logging in) or settings (if linking accounts).
- Allow providers to distinguish between "start", "login" and "link" rendering.
Test Plan: Linked and logged in with LDAP and other registration mechainsms.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6214
Summary:
Ref T1536.
- Add a "Cancel" button, to get back to login.
- Add a crumb showing the registering provider.
- Add an account card when registering with an external account
- Tailor some language to make it less ambiguous ("Phabricator Username", "Register Phabricator Account").
Test Plan:
{F46618}
{F46619}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6213
Summary: Ref T1536. This gets the single queries out of the View and builds a propery Query class for ExternalAccount.
Test Plan: Linked/unlinked accounts, logged out, logged in.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6212
Summary: Ref T1536. This script basically exists to restore access if/when users shoot themselves in the foot by disabling all auth providers and can no longer log in.
Test Plan: {F46411}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6205
Summary: Ref T1536. When auth providers are edited, show the edit history.
Test Plan: {F46400}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6203
Summary: Ref T1536. Many rough / broken edges, but adds the rough skeleton of the provider edit workflow.
Test Plan: {F46333}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6200
Summary: Ref T1536. Adds an initial "choose a provider type" screen for adding a new provider. This doesn't go anywhere yet.
Test Plan: {F46316}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6199
Summary:
Ref T1536. Currently, we have about 40 auth-related configuration options. This is already roughly 20% of our config, and we want to add more providers. Additionally, we want to turn some of these auth options into multi-auth options (e.g., allow multiple Phabricator OAuth installs, or, theoretically multiple LDAP servers).
I'm going to move this into a separate "Auth" tool with a minimal CLI (`bin/auth`) interface and a more full web interface. Roughly:
- Administrators will use the app to manage authentication providers.
- The `bin/auth` CLI will provide a safety hatch if you lock yourself out by disabling all usable providers somehow.
- We'll migrate existing configuration into the app and remove it.
General goals:
- Make it much easier to configure authentication by providing an interface for it.
- Make it easier to configure everything else by reducing the total number of available options.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6196
Summary: Ref T1536. These can probably use some design tweaking and there's a bit of a bug with profile images for some providers, but generally seems to be in the right ballpark.
Test Plan:
{F46604}
{F46605}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6210
Summary:
Ref T1536. Currently, we have separate panels for each link/unlink and separate controllers for OAuth vs LDAP.
Instead, provide a single "External Accounts" panel which shows all linked accounts and allows you to link/unlink more easily.
Move link/unlink over to a full externalaccount-based workflow.
Test Plan:
- Linked and unlinked OAuth accounts.
- Linked and unlinked LDAP accounts.
- Registered new accounts.
- Exercised most/all of the error cases.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6189
Summary:
Ref T1536. Facebook currently does a check which should be on-login in registration hooks, and this is generally a reasonable hook to provide.
The "will login" event allows listeners to reject or modify a login, or just log it or whatever.
NOTE: This doesn't cover non-web logins right now -- notably Conduit. That's presumably fine.
(This can't land for a while, it depends on about 10 uncommitted revisions.)
Test Plan: Logged out and in again.
Reviewers: wez, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6202
Summary: Ref T1536. Error state is a bit gross but we need to sort that out in general.
Test Plan:
{F46549}
{F46550}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6208
Summary: Ref T1536. Like D6080, we don't need to store the registration key itself. This prevents a theoretical attacker who can read the database but not write to it from hijacking registrations.
Test Plan: Registered a new account.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6188
Summary:
Ref T1445. Ref T1536. Although we have separate CSRF protection and have never been vulnerable to OAuth hijacking, properly implementing the "state" parameter provides a little more certainty.
Before OAuth, we set a random value on the client, and pass its hash as the "state" parameter. Upon return, validate that (a) the user has a nonempty "phcid" cookie and (b) the OAuth endpoint passed back the correct state (the hash of that cookie).
Test Plan: Logged in with all OAuth providers, which all apparently support `state`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, arice
Maniphest Tasks: T1445, T1536
Differential Revision: https://secure.phabricator.com/D6179
Summary: Ref T1536. We can safely replace the old login validation controller with this new one, and reduce code dplication while we're at it.
Test Plan: Logged in with LDAP, logged in with OAuth, logged in with username/password, did a password reset.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6178
Summary:
Ref T1536. LDAP is very likely the worst thing in existence.
This has some rough edges (error handling isn't perfect) but is already better than the current LDAP experience! durrr
Test Plan: Registered and logged in using LDAP.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6177
Summary: Ref T1536. Support for GitHub on new flows.
Test Plan: Registered and logged in with GitHub.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6166
Summary: Ref T1536. Adds Disqus as a Provider.
Test Plan: Registered and logged in with Disqus.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6165
Summary:
Ref T1536. Ref T1930. Code is not reachable.
This provides password authentication and registration on the new provider/adapter framework.
I sort of cheated a little bit and don't really route any password logic through the adapter (instead, this provider uses an empty adapter and just sets the type/domain on it). I think the right way to do this //conceptually// is to treat username/passwords as an external black box which the adapter communicates with. However, this creates a lot of practical implementation and UX problems:
- There would basically be two steps -- in the first one, you interact with the "password black box", which behaves like an OAuth provider. This produces some ExternalAccount associated with the username/password pair, then we go into normal registration.
- In normal registration, we'd proceed normally.
This means:
- The registration flow would be split into two parts, one where you select a username/password (interacting with the black box) and one where you actually register (interacting with the generic flow). This is unusual and probably confusing for users.
- We would need to do a lot of re-hashing of passwords, since passwords currently depend on the username and user PHID, which won't exist yet during registration or the "black box" phase. This is a big mess I don't want to deal with.
- We hit a weird condition where two users complete step 1 with the same username but don't complete step 2 yet. The box knows about two different copies of the username, with two different passwords. When we arrive at step 2 the second time we have a lot of bad choices about how to reoslve it, most of which create security problems. The most stragihtforward and "pure" way to resolve the issues is to put password-auth usernames in a separate space, but this would be incredibly confusuing to users (your login name might not be the same as your username, which is bizarre).
- If we change this, we need to update all the other password-related code, which I don't want to bother with (at least for now).
Instead, let registration know about a "default" registration controller (which is always password, if enabled), and let it require a password. This gives us a much simpler (albeit slightly less pure) implementation:
- All the fields are on one form.
- Password adapter is just a shell.
- Password provider does the heavy lifting.
We might make this more pure at some point, but I'm generally pretty satisfied with this.
This doesn't implement the brute-force CAPTCHA protection, that will be coming soon.
Test Plan: Registered with password only and logged in with a password. Hit various error conditions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1536, T1930
Differential Revision: https://secure.phabricator.com/D6164
Summary:
Ref T1536. None of this code is reachable.
Implements new-auth login (so you can actually login) and login validation (which checks that cookies were set correctly).
Test Plan: Manually enabled FB auth, went through the auth flow to login/logout. Manually hit most of the validation errors.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6162
Summary:
Ref T1536. Code is intentionally made unreachable (see PhabricatorAuthProviderOAuthFacebook->isEnabled()).
This adds:
- A provider-driven "start" screen (this has the list of ways you can login/register).
- Registration actually works.
- Facebook OAuth works.
@chad, do you have any design ideas on the start screen? I think we poked at it before, but the big issue was that there were a limitless number of providers. Today, we have:
- Password
- LDAP
- Facebook
- GitHub
- Phabricator
- Disqus
- Google
We plan to add:
- Asana
- An arbitrary number of additional instances of Phabricator
Users want to add:
- OpenID
- Custom providers
And I'd like to have these at some point:
- Stripe
- WePay
- Amazon
- Bitbucket
So basically any UI for this has to accommodate 300 zillion auth options. I don't think we need to solve any UX problems here (realistically, installs enable 1-2 auth options and users don't actually face an overwhelming number of choices) but making the login forms less ugly would be nice. No combination of prebuilt elements seems to look very good for this use case.
Test Plan: Registered a new acount with Facebook.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6161
Summary:
Ref T1536. None of this code is reachable.
`PhabricatorAuthLoginController` provides a completely generic login/link flow, similar to how D6155 provides a generic registration flow.
`PhabricatorAuthProvider` wraps a `PhutilAuthAdapter` and glues the generic top-level flow to a concrete authentication provider.
Test Plan: Static only, code isn't meaningfully reachable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6159
Summary:
Currently, registration and authentication are pretty messy. Two concrete problems:
- The `PhabricatorLDAPRegistrationController` and `PhabricatorOAuthDefaultRegistrationController` controllers are giant copy/pastes of one another. This is really bad.
- We can't practically implement OpenID because we can't reissue the authentication request.
Additionally, the OAuth registration controller can be replaced wholesale by config, which is a huge API surface area and a giant mess.
Broadly, the problem right now is that registration does too much: we hand it some set of indirect credentials (like OAuth tokens) and expect it to take those the entire way to a registered user. Instead, break registration into smaller steps:
- User authenticates with remote service.
- Phabricator pulls information (remote account ID, username, email, real name, profile picture, etc) from the remote service and saves it as `PhabricatorUserCredentials`.
- Phabricator hands the `PhabricatorUserCredentials` to the registration form, which is agnostic about where they originate from: it can process LDAP credentials, OAuth credentials, plain old email credentials, HTTP basic auth credentials, etc.
This doesn't do anything yet -- there is no way to create credentials objects (and no storage patch), but I wanted to get any initial feedback, especially about the event call for T2394. In particular, I think the implementation would look something like this:
$profile = $event->getValue('profile')
$username = $profile->getDefaultUsername();
$is_employee = is_this_a_facebook_employee($username);
if (!$is_employee) {
throw new Exception("You are not employed at Facebook.");
}
$fbid = get_fbid_for_facebook_username($username);
$profile->setDefaultEmail($fbid);
$profile->setCanEditUsername(false);
$profile->setCanEditEmail(false);
$profile->setCanEditRealName(false);
$profile->setShouldVerifyEmail(true);
Seem reasonable?
Test Plan: N/A yet, probably fatals.
Reviewers: vrana, btrahan, codeblock, chad
Reviewed By: btrahan
CC: aran, asherkin, nh, wez
Maniphest Tasks: T1536, T2394
Differential Revision: https://secure.phabricator.com/D4647
Summary: Ref T1536. This is similar to D6172 but much simpler: we don't need to retain external interfaces here and can do a straight migration.
Test Plan: TBA
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6173
Summary: make it work with z-index and make it grey.
Test Plan:
clicked the grey button and it worked! Safari and Chrome
clicked around and observed loading mask functioning correctly
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3396
Differential Revision: https://secure.phabricator.com/D6207
Summary: Ref T1536. Migrates the OAuthInfo table to ExternalAccount, and makes `PhabricatorUserOAuthInfo` a wrapper for an ExternalAccount.
Test Plan: Logged in with OAuth, registered with OAuth, linked/unlinked OAuth accounts, checked OAuth status screen, deleted an account with related OAuth.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6172
Summary:
Ref T1536. Move all access to the underlying storage to inside the class. My plan is:
- Migrate the table to ExternalAccount.
- Nuke the table.
- Make this class read from and write to ExternalAccount instead.
We can't get rid of OAuthInfo completely because Facebook still depends on it for now, via registration hooks.
Test Plan: Logged in and registered with OAuth.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6171
Summary:
Ref T1536. Currently, we store OAuth tokens along with their expiry times and status. However, all we use this for is refreshing profile pictures and showing a silly (and probably somewhat confusing) interface about token status.
I want to move this storage over to `PhabricatorExternalAccount` to make the cutover easier. Drop it for now, including all the profile image stuff (I plan to rebuild that in a more sensible way anyway).
Test Plan: Viewed screen; linked/unlinked accounts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6170
Summary:
Ref T1536. This is the schema code for `PhabricatorExternalAccount` which was previously in D4647. I'm splitting it out so I can put it earlier in the sequence and because it's simple and standalone.
Expands `PhabricatorExternalAccount` to have everything we need for the rest of registration.
Test Plan: Implemented the remainder of new registration on top of this.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1536
Differential Revision: https://secure.phabricator.com/D6169
Summary: Ref T3377. MySQL ignores indexes if we hand it mismatched datatypes. This seems colossally dumb, but give it what it expects.
Test Plan: wat
Reviewers: wez, btrahan
Reviewed By: wez
CC: aran
Maniphest Tasks: T3377
Differential Revision: https://secure.phabricator.com/D6201
Summary: Rough pass at a PHUIButtonView Class. Keeps phutil_tag intact and adds some image features if you use the class.
Test Plan: UIExamples
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6192
Summary:
Ref T3354. There's no way for us to test most of the config options which actually affect this limit, so the Phabricator config is basically a canary value to indicate "the administrator hasn't configured anything yet".
Raise a setup issue if it isn't set. There's a trail to get here from Files, but we've de-emphasized the old-school upload form so it's hard to unearth.
Emphasize the warning that you need to read the documentation and configure like 30 other things to make this work.
Test Plan: Cleared my config, verified I got the issue, read it, set my config, issue went away.
Reviewers: jamesr, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3354
Differential Revision: https://secure.phabricator.com/D6185
Summary: Fixes T3242. Changes the red and orange objects to match the transactions. Also adds a highlight color to 'cards'.
Test Plan: Review my audits in my sandbox
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3242
Differential Revision: https://secure.phabricator.com/D6184
Summary: Decided to just remove the hover grey to white, seems fine with the new white icons.
Test Plan: use homepage + icons
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6181
Summary: the people widget was returning a comma-delimited list of HTML nodes so kill that noise with some hsprintf action. We also weren't consistently updating the latest transaction id so simplify those codepaths (widgets vs pontificate) a bit. Fixes T3336.
Test Plan: left some messages, added some participants. noted that the people widget looked good and only the pertinent transactions were pulled down on updates.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3336
Differential Revision: https://secure.phabricator.com/D6180
Summary: Fixes T3330
Test Plan: Test desktop and mobile menus in chrome and ios.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3330
Differential Revision: https://secure.phabricator.com/D6157
Summary:
Ref T1703. This sets the stage for (but does not yet implement) custom UI types for config. In particular, a draggable list for custom fields.
I might make all the builtin types go through this at some point too, but don't really want to bother for the moment. It would be very slightly cleaner but woudn't get us much of anything.
Test Plan:
UI now renders via custom code, although that code does nothing (produces an unadorned text field):
{F45693}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6154
Summary:
Ref T1703.
- Adds "Title".
- Adds "Blurb".
- Adds `user.fields` config for selecting and reordering. This will get UI in the next patch.
Test Plan:
{F45689}
{F45690}
Edited the fields, too.
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6153
Summary:
Adds a profile edit controller (with just one field and on links to it) that uses ApplicationTransactions and CustomField.
{F45617}
My plan is to move the other profile fields to this interface and get rid of Settings -> Profile. Basically, these will be "settings":
- Sex
- Language
- Timezone
These will be "profile":
- Real Name
- Title
- Blurb
- Profile Image (but I'm going to put this on a separate UI)
- Other custom fields
Test Plan: Edited my realname using the new interface.
Reviewers: chad, seporaitis
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6152
Summary: Used more logical icons for subscribe, auto, and delete instead of the mail icons. Fixes T3329
Test Plan: Tested subscribing and unsubscribing in Maniphest.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3329
Differential Revision: https://secure.phabricator.com/D6151
Summary:
None of this code is reachable yet. See discussion in D6147. Ref T1703.
Provide tighter integration between ApplicationTransactions and CustomField. Basically, I'm just trying to get all the shared stuff into the base implementation.
Test Plan: Code not reachable.
Reviewers: chad, seporaitis
Reviewed By: chad
CC: aran
Maniphest Tasks: T1703
Differential Revision: https://secure.phabricator.com/D6149
Summary: Adds collapsing of the sidebar, also allows you to say where it goes on mobile (above or below content). ToC for example, above. General Navbar, below. Up to you.
Test Plan: Review UIExamples and Diviner.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6150
Summary: We need to make sure the symbol is always saved before the atom. I mucked this up recently and didn't catch it locally since I'd already generated the atoms.
Test Plan: Ran `bin/diviner generate` after truncating all diviner tables.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6146
Summary: Gets TOC populated for articles, at least, and fixes a few other things.
Test Plan: {F45474}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6144
Summary: Allow users to set a default by dragging it to the top. When they land on a page without a saved query, choose their default.
Test Plan: Hit `/paste/`, got my default results, etc.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6140
Summary:
Also you have to drop them. So drag, and then drop.
This needs some cleanup and reconciliation/generalization with the Maniphest implementation. In particular, you can't drag things to the very top right now, and they should share more CSS and more behaviors.
Test Plan:
Look I alphabetized them:
{F45286}
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6136
Summary: See discussion in D6131, D6130. This turned into 35 layers of mess so throw it away and just tweak the JS to be more flexible.
Test Plan:
- Clicked "Show More Applications".
- Clicked "Show Fewer Applications".
- Edited tasks using popup dialog.
- Tried to drag tasks using pencil icon (correctly no longer works).
- Changed threads in Conpherence.
- Not sure how to actually hit the Conpherence "Load ... Threads" thing since
it seems to auto-load? But that works, at least, and the code doesn't really
care what you hit.
- Added a conpherence participant.
- Added a new calendar item.
- Poked around other menus.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6133
Summary: This adds examples and abstracts out CSS for common nav re-use.
Test Plan: Tested DocumentExample and ListExample
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6138
Summary: I made changes here recently to improve robustness in the presence of missing files, but accidentally caused the results to re-key. Some callers depend on the mapping, and every other query is consistent about it. Restore the original behavior.
Test Plan: `Pnnn` works again in remarkup.
Reviewers: dctrwatson, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6134
Summary:
Ref T3320. Request from Dropbox. You can currently prefill "title" and some other stuff (including most of the fields by using templates) but there's no way to prefill description and assigned to right now.
- Allow "Description" to be prefilled with a string ("description=...").
- Allow "Assigned To" to be prefilled with a user rather than a PHID ("assign=username").
Test Plan:
Hit `/maniphest/task/create/?assign=epriestley&description=derpderp&title=blarpblarp` locally and got prefills.
{F45259}
Reviewers: chad, deuresti
Reviewed By: chad
CC: aran, euresti
Maniphest Tasks: T3320
Differential Revision: https://secure.phabricator.com/D6132
Summary:
This diff covers a bit of ground.
- PHUIDocumentExample has been added
- PHUIDocument has been extended with new features
- PhabricatorMenuView is now PHUIListView
- PhabricatorMenuItemView is now PHUIItemListView
Overall - I think I've gotten all the edges covered here. There is some derpi-ness that we can talk about, comments in the code. Responsive design is missing from the new features on PHUIDocument, will follow up later.
Test Plan: Tested mobile and desktop menus, old phriction layout, new document views, new lists, and object lists.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6130
Summary: At the global level, truncate emails at a user-configured size.
Test Plan: Untested, as I could not get PHP to send emails on my box, but if you can this should be very easy to test. Just set the max size to something like .001 kilobytes and make sure it does the right thing.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T1392
Differential Revision: https://secure.phabricator.com/D6118
Summary:
Applications come with builtin queries, but users might want to get rid of them. Allow users to disable named queries if they prefer.
This has one funky behavior, which is that the first time you disable a named query it goes to the top of your list. That will be fixed in the next diff, which will make them reorderable.
Test Plan: Added/edited/removed named queries, disabled/enabled builtin named queries.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6128
Summary: Ref T988. Mostly backend changes, with a very rough frontend on top of them. See Conpherence discussion.
Test Plan: {F45010}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6113
Summary: Adds 'Add Project...' if no projects present on Maniphest items. Also - I can't seem to get a dialog to pop, what am I missing? Fixes T3308
Test Plan: Click add project, get edit form.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3308
Differential Revision: https://secure.phabricator.com/D6121
Summary: D6114 fixed some bugs but on production it shows up as a new bug where Saturday is the first day? stop messing with the DateTime object so much and do some old school epoch manipulation. This works correctly on my laptop and my still fail in production, but it will rule out DateTime suckage.
Test Plan: still works on laptop
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6115
Summary: Pholio stories now reference the mock they're talking about.
Test Plan: Generate each type of story and check that they make sense in feed/inline.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2988
Differential Revision: https://secure.phabricator.com/D6117
Summary: See D6115.
Test Plan:
- Ran unit tests.
- Viewed reports.
- Created a countdown.
- Searched chatlog.
- Searched pastes by created date.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6116
Summary: I needed to port my extremely clever "last sunday as of tomorrow" trick to the display layer. Also found a fun bug in testing where +N days was changing it to 1:00 AM from 00:00 AM with my timezone configuration. Presumably all sorts of whacky hyjinx ensue when you modify DateTime and you need to re-specify the timezone after to get it to work
Test Plan: verified that Today, SUNDAY, we see TODAY -> Saturday and it all looks good. Verified midnite -> just before midnight status events span but a single day.
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6114
Summary: Missed this in the PHUIDocumentView thing.
Test Plan: Previewed a Phriction edit; grepped more/harder.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6110
Summary: Ref T3155. "last sunday" is "last sunday at 00:00" so you have to include a day for Sunday itself.
Test Plan: calendar renders correctly today - saturday - which is the edge case of this
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T3155
Differential Revision: https://secure.phabricator.com/D6108
Summary: Ref T988. Lots of rough edges still, but this pulls the right data and dumps it into a reasonable-looking shell.
Test Plan: {F44883}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D6104
Summary:
Ref T988. Fixes T3150. I want to use this element in Diviner, so separate it from Phriction.
This makes no changes to the actual display except for fixing {T3150} by adding `overflow: hidden;`.
Test Plan: Viewed Phriction documents in mobile and desktop views.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988, T3150
Differential Revision: https://secure.phabricator.com/D6101
Summary: this does a few things. Fixes T3253. Including the Sunday -> Saturday list view part. Cleans up the display when there are no events, getting rid of this spacer thing. Also fixes Calendar CSS for device-tablet where we had a 2px gap on the calendar from the header.
Test Plan: played with calendar widget a bunch
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T3253
Differential Revision: https://secure.phabricator.com/D6102
Summary: Last of the methods. Fixes T3166.
Test Plan: updated a thread in all the various ways except remove and it worked. removed myself and it worked! tried to remove someone else and it yelled at me.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3166
Differential Revision: https://secure.phabricator.com/D6103
Summary: Ref T2625. This is now "query" instead of "filter" with the new search stuff.
Test Plan: Uploaded a file. Grepped for 'file/filter'.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6105
Summary: This mainly affects crumbs, makes it more clear that the Conpherence doesn't have a set title.
Test Plan: Clear a title, see [No Title]
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6099
Summary: Ref T3166. Returned all the data I could think of, though notable "metadata" isn't used in conpherence (yet afaik) AND its somewhat silly to return the conpherence id / phid you specified, but seems handy.
Test Plan: played with conduit console.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3166
Differential Revision: https://secure.phabricator.com/D6098
Summary: Ref T988. Ref T2625. Rough cut of ApplicationSearch in Diviner, for detailed Atom queries. This isn't useful yet, and isn't linked in the UI.
Test Plan: {F44836}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988, T2625
Differential Revision: https://secure.phabricator.com/D6094
Summary:
Ref T2625. Fixes T2812. Implement ApplicationSearch in People.
{F44788}
Test Plan: Made People queries. Used Conduit. Used `@mentions`.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2625, T2812
Differential Revision: https://secure.phabricator.com/D6092
Summary: Ref T2625. Ref T1163. A couple of small generalization nudges, but this is almost entirely straightforward.
Test Plan: Executed various File queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1163, T2625
Differential Revision: https://secure.phabricator.com/D6091
Summary:
Ref T2625.
- Build the mobile menu from the delegating controller.
- Make the result header look a little better (still a bit funky).
Test Plan:
{F44774}
{F44775}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6090
Summary: nice title. also adds a description to the create thread method which I forgot to add... Ref T3166.
Test Plan: queried threads by ids, by phids, and by offset / limit tweakage. Got the right stuff!
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3166
Differential Revision: https://secure.phabricator.com/D6096
Summary:
Fixes T1945. Ref T2947. At various times, installs (Disqus, Dropbox, etc.) have asked for a way to edit tasks more quickly. Provide edit-from-lists.
{F44700}
{F44701}
The one rough edge on this is that if you change the task priority we update it inline but don't move it. It's probably infeasible to actually move it, but maybe we could give it some sort of visual style to indicate that it's dirty.
Test Plan: Edited tasks normally and via this action thing.
Reviewers: chad, btrahan
Reviewed By: chad
CC: tido, deuresti, ahoffer, aran
Maniphest Tasks: T1945, T2947
Differential Revision: https://secure.phabricator.com/D6086
Summary:
Ref T2625. Ref T3273. This is mostly a UI foil for T3273. Right now, to find tasks without owners or without projects you search for the magic strings "upforgrabs" and "noproject". Unsurprisingly, no users have ever figured this out. I want to get rid of it. Instead, these interfaces will look like:
Assigned: [ Type a user name... ]
[ X ] Find unassigned tasks.
Projects: [ Type a project name... ]
[ X ] Find tasks with no projects.
Seems reasonable, I think?
Test Plan: Searched for "rainbow, js", "rainbow + no language", "no language", date ranges, etc.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T2625, T3273
Differential Revision: https://secure.phabricator.com/D6085
Summary:
This was mentioned in T2928 and nobody objected.
It just references the task instead of fixing it as that would be too aggressive.
It also doesn't check assignee of the task (by purpose).
Test Plan: Created diff from a branch named T2928.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2928
Differential Revision: https://secure.phabricator.com/D5640
Summary:
Ref T1163. Ref T2625. This could probably use some tweaks, but I kept things mostly-generic.
I added a new control for freeform dates so we can have it render help or whatever later on.
Test Plan: See screenshots.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625, T1163
Differential Revision: https://secure.phabricator.com/D6084
Summary:
This prevents security by obscurity.
If I have read-only access to the database then I can pretend to be any logged-in user.
I've used `PhabricatorHash::digest()` (even though we don't need salt as the hashed string is random) to be compatible with user log.
Test Plan:
Applied patch.
Verified I'm still logged in.
Logged out.
Logged in.
$ arc tasks
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6080
Summary: Ref T3166. I moved the create logic into a static method in the editor class to keep things tidy.
Test Plan: created a conpherence from UI. purdy. tried errors and got UI to show "required". for conduit, created a thread with all the bells and whistles and it worked. verified i got proper exceptions with bum conduit calls
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3166
Differential Revision: https://secure.phabricator.com/D6083
Summary:
Fixes T3279. For ApplicationSearch (and in some other cases) I'd like users to be able to provide an optional date. This isn't currently possible.
Add a checkbox which disables or enables the input.
Test Plan: Used UIExample to enter dates. Used Calendar to enter dates.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3279
Differential Revision: https://secure.phabricator.com/D6082
Summary: nice title. Fixes T3203. If its been N days and now its Tuesday, it just shows a single marker for Tuesday.
Test Plan: Viewed a conpherence and there were date dividers!
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3203
Differential Revision: https://secure.phabricator.com/D6081
Summary: Ref T2625. Works out the last kinks of generalization and gives Macros the more powerful new query engine. Overall, this feels pretty good to me.
Test Plan: Executed, saved and edited a bunch of Macro queries.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6078
Summary:
Ref T2625. Lifts almost all of the search logic out of Paste controllers and into Search.
This uses controller delegation for generalization. We use this in a few places, but don't use it very much yet. I think it's pretty reasonable as-is, but I might be able to make even more stuff free.
There are some slightly rough edges around routes, still, but I want to hit Phame and Differential (which both have multiple application search engines) before trying to generalize that.
Test Plan: Executed, browsed and managed Paste searches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6073
Summary: Fixes T3280 - when a pontificate brought back multiple transactions, we were rendering a comma. Yay hsprintf. Also fixes the noconpherences view, which broke at some point recently.
Test Plan: sent comment, then replied from different browser. when both comments loaded noted no comma. loaded a conpherence view with no conpherences and verified it looked good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T3280
Differential Revision: https://secure.phabricator.com/D6079
Summary:
Fixes T3252. Other enhancements:
- Header in widget panel was 2px too short.
- Typeahead in add people only allowed one person
- Typeahead in add people was cutoff by overflow:hidden
- X in remove has been changed to unicode (multiply)
- Add people dialog form fields are full width
- Some other CSS tweaks.
Test Plan: Add, Remove people.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3252
Differential Revision: https://secure.phabricator.com/D6076
Summary: and now you can add more than one at a time! Also adds the 'add participants' and 'new calendar event' options to mobile view. Fixes T3251. Ref T3253.
Test Plan: loaded up these "adders" on both desktop and device-ish views and it went well!
Reviewers: epriestley, chad
Reviewed By: chad
CC: chad, aran, Korvin
Maniphest Tasks: T3251, T3253
Differential Revision: https://secure.phabricator.com/D6075
Summary:
Currently, the author of an image macro is read from the attached file. This is messy and necessitates a join, and is not always correct. Instead, store the data when the macro is created.
This lays the groundwork for generalizing ApplicationSearch here. Ref T2625.
Test Plan: Migrated existing macros, created a new macro, checked web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6071
Summary: Fixes T3253 by shifting the display to the "next 3 days". Also adds in the "create" functionality for calendar on desktop view only, ref T3251. As part of T3251, I plan to make this work on mobile too.
Test Plan: added statuses and noted errors showed up. noted on success the widget pane refreshed. also made sure the regular old /calendar/status/create/ page still worked.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3251, T3253
Differential Revision: https://secure.phabricator.com/D6072
Summary:
Ref T2625. @chad, you might have some feedback here. The behaviors this implements are:
- When the user selects "Advanced Search", we show the full search UI and no results (for performance and clarity).
- When the user submits a search which //is not// a named search, we show the full search UI and the "Save Custom Query..." button.
- When the user submits a search which //is// a named search, we show "Results for search X." with an "Edit Query..." button. The button expands the search form.
- When the user selects a builtin query (like "All Pastes"), we don't show any search UI, but I'm probably going to make this behave more like named searches.
Test Plan:
{F44346}
{F44347}
Reviewers: chad, btrahan, blc
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6063
Summary: this diff tries to polish the poo out of the JS layer while achieving fixes T3157 accolades.
Test Plan: introduced sleeps in the various controllers and clicked about. verified good "loading" UI in the menu / message / widget section as appropros. Loaded up in device size and resize and desktop sized and resized and all was good.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3164, T3157
Differential Revision: https://secure.phabricator.com/D6069
Summary:
We can lose file data through various means; one reasonable way is if files get deleted from disk with 'local-disk' storage. If data goes missing,
Ref T3265. Also, reduce some code duplication.
Test Plan:
Ran `bin/files purge`, `bin/files migrate`, `bin/files rebuild` with various args.
Deleted a file with "local-disk" storage, ran `bin/files purge`, made sure it got picked up.
Reviewers: dctrwatson, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3265
Differential Revision: https://secure.phabricator.com/D6068
Summary:
We allow files to be deleted from the web UI, including paste files. When a paste's file is deleted, we currently continue to show it in the paste list and then 400 when it's clicked on.
Instead, remove it from the list. (Note that it may stick around if it's cached.)
Test Plan: Purged general cache, deleted a paste's file, viewed paste list, saw no pastes.
Reviewers: dctrwatson, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3265
Differential Revision: https://secure.phabricator.com/D6067
Summary: Highlights which day is today on the calendar list in conpherence. Fixes T3254
Test Plan: Made sure today was Tuesday.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3254
Differential Revision: https://secure.phabricator.com/D6065
Summary:
In Releeph, `ReleephRequestHeaderView` will render you a "mark-manually-picked" button if (a) you are authoritative (chuckr) or (b) you are the person who created this Releeph request.
However `ReleephRequestActionController`, which handles these button presses, will only do something if you are (a). This patch honors your button pressing if you are (b) as well.
Test Plan: Push buttons. There's another bug in the Javascript do-stuff-without-reloading-the-page code that handles these button pushes, but I'd like to fix that separately from handling this hi-pri bug.
Reviewers: epriestley, wez
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6036
Summary:
Ref T2625. The specialized buildSearchForm() method has significant amounts of generic form construction responsibility right now. Lift the generic stuff above the Engine level. Also:
- Rename "users" to "authors".
- Use "users", not "searchowners" (which incorrectly includes "upforgrabs").
- No need for "set_" prefixes anymore since we do GET redirects with query keys.
- Use newer style for search stuff.
Test Plan:
Searched for stuff?
{F44342}
Reviewers: btrahan, blc
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6061
Summary: Ref T2625. We currently hard-code the URI; instead, derive it from the Engine. I weakened the strength of getQueryResultsPageURI to let it build from a NamedQuery or a SavedQuery, because constructing a SavedQuery for a builtin NamedQuery is a bit of a pain.
Test Plan: Clicked links on the saved queries page, got query results.
Reviewers: btrahan, blc
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6060
Summary:
Ref T2625. Currently, custom saved queries can be edited but not deleted. Allow them to be deleted. Also:
- Clean up an unused property in `PhabricatorPasteViewController`.
- Fix an issue with left nav highlighting of builtin queries.
- Improve submit behavior for edits.
- Add a cancel button on edits.
Test Plan: Saved, edited and deleted queries.
Reviewers: btrahan, blc
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6059
Summary:
Ref T2625. Currently, Paste hard-codes its filters as a separate layer above the query layer. Instead, expose these as "Builtin" queries which we construct at runtime. They act like normal saved queries, except in cases where it doesn't make sense.
(I'm probably going to let you hide them too, and maybe even rename them, although for now they're just immutable.)
Test Plan: {F44340}
Reviewers: btrahan, blc
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6058
Summary: Ref T2625. Rename the "Name" controller to "Edit" and allow it to edit named queries. Also some UI touchups. Add an edit link to the saved query browse view.
Test Plan: Created and edited named queries.
Reviewers: btrahan, blc
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6056
Summary:
Ref T2625. Currently, after saving a query the user is redirected to "/search/", which isn't especially useful. Instead, redirect them back into the application they came from and to the query results page.
Also, query hashes may contain ".", which does not match `\w`. Use `[^/]` instead.
Test Plan: Saved some custom queries.
Reviewers: btrahan, blc
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6055
Summary: Same as D6051, but for SavedQueries instead of NamedQueries. These are POLICY_PUBLIC because you need to know the hash to access them, and because we want to let users copy/paste query URLs. Ref T2625.
Test Plan: Saved a query, reused a saved query.
Reviewers: btrahan, blc
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6054
Summary:
Ref T2625.
- Show saved queries in the left nav.
- Highlight the correct stuff in the left nav.
Test Plan: Clicked all left-nav stuff.
Reviewers: btrahan, blc
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6053
Summary: Adds a first-class Query object for querying NamedQueries. xzibit would be proud.
Test Plan: Updated query edit interface to use this query, verified it works correctly.
Reviewers: btrahan, blc
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D6051
Summary:
These are a bit tricky because we don't want to require you to install a VCS you don't use just to use Phabricator. Test that repositories exist before performing the checks.
I'll couple this with additional checks during repository creation.
Test Plan: Changed binary names to nonexistent ones, verified setup issues raised properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6040
Summary:
A few more of these issues have cropped up recently. Basically:
- Webservers often (by default, I guess?) have a different or nonexistent $PATH.
- Users have a hard time figuring this out, since it's not obvious that the webserver might have a different configuration than the CLI, and they can run "git" and such themselves fine, and they don't normally use SetEnv or similar in webserver config.
I've been pursuing one prong of attack here (better detection and more tailored errors); this is a second prong (try to just guess the configuration correctly).
In 99% of cases, the binaries in question are in one of these three places, so just make them the default appended paths. If users have wacky configs they can override the setting.
Test Plan: Viewed config locally.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6039
Summary:
Ref T3201. D6009 added this flag, but I don't think it actually works as advertised? We either need a Conduit patch to interpret `null` as `upforgrabs` (which seems 100% reasonable to me) or this (which is kind of nasty).
@garoevans, was there a Phabricator-side diff for the conduit part that just got dropped? I'll probably replace this with that if not, but figured I'd check before I poke anything.
Test Plan: Ran `arc diff --unassigned`
Reviewers: garoevans
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T3201
Differential Revision: https://secure.phabricator.com/D6062
Summary: Search typeahead now returns project objects with project images :). I have set the displaytype to "Project" if that's okay. Alternatively We can also set it to some short description of a given project. All this time, I was thinking Jump Nav to be the search typeahead ... I am so dumb ... Lolz :P
Test Plan: {F44336}
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2948
Differential Revision: https://secure.phabricator.com/D6057
Summary: Semi-decent pass at cleaning up the Conpherence dropdown and widgets. Will continue to update but have diff questions.
Test Plan: Testing Conpherence in my sb.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6043
Summary:
Fixes T3139. See that task for discussion.
If all mentions are removed because they're already subscribed, we currently generate an empty transaction, which later gets picked up as having no effect and the user gets yelled at.
Instead, don't generate a transaction if no PHIDs remain after filtering already-subscribed PHIDs.
Test Plan: Followed plan in T3139.
Reviewers: garoevans, btrahan
Reviewed By: garoevans
CC: aran
Maniphest Tasks: T3139
Differential Revision: https://secure.phabricator.com/D6048
Summary: Add a button to the Remarkup area that explains how to attach an image and remove the separate upload field.
Test Plan: Check that the dialog pops up correctly and that dropping images onto the Remarkup area works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T879
Differential Revision: https://secure.phabricator.com/D6049
Summary:
Kind of a quick look at an idea for T2184
Ref T2184
Test Plan: Make sure the site still loads
Reviewers: epriestley
CC: aran, Korvin, mbishopim3
Maniphest Tasks: T2184
Differential Revision: https://secure.phabricator.com/D6045
Summary: Ref T3184. See discussion in D6042.
Test Plan: {F44265}
Reviewers: garoevans, btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3184
Differential Revision: https://secure.phabricator.com/D6047
Summary:
Fixes T3184
We make sure that we're not working with a global policy that just sets the title correctly. Otherwise we have a PHID to work with and set the handle as required.
Test Plan: Change policies on a mock and see the title render correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3184
Differential Revision: https://secure.phabricator.com/D6042
Summary:
Kind of just guessed what to do here. It seemed reasonable.
Take the transaction, use the author to get an actor image. Get the main object handle type, use that for the app icon. Use the transaction's created date as a time.
Ref T2988
@chad I think there's still no icon for mock?
Test Plan: Look at my feed. See some info.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: chad, aran, epriestley, Korvin
Maniphest Tasks: T2988
Differential Revision: https://secure.phabricator.com/D6044
Summary: We were missing this option on the response, so notifications about commits (e.g., tokens given) don't clear when viewing the commit.
Test Plan: Gave a commit a token with user A, viewed notification with user B, viewed commit with user B and verified notification cleared.
Reviewers: garoevans, btrahan
Reviewed By: garoevans
CC: aran
Differential Revision: https://secure.phabricator.com/D6041
Summary: Adds support for the "encoding" field to the new transactional interface.
Test Plan:
{F44189}
{F44190}
Some of the encodings in the second screen are from testing, and can no longer be set.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6035
Summary:
At one point this was sort of a one-line summary but it isn't really anymore (and doesn't appear on the list view). We could add a summary in the future if we wanted.
- Change the control from a text area to a remarkup area.
- Change the display to remarkup.
Test Plan:
{F44183}
{F44184}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6034
Summary: I want to use some of these for instructional text in Diffusion. Provide a concrete class to make one-offs cacheable and switch Releeph to use it.
Test Plan: {F44175}
Reviewers: btrahan, chad, edward
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6032
Summary:
Ref T2231, T603. Plan of attack here is pretty much:
- Built out a new (currently not linked in the UI) edit interface in Diffusion which is transaction-based and has a sensible layout.
- Build out a new create interface based on PagedForm which dumps into the new edit interface.
- Throw the old stuff away.
- Everyone lives happily ever after.
Test Plan:
{F44163}
{F44164}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2231
Differential Revision: https://secure.phabricator.com/D6029
Summary: Ref T3155. Also re-adds the ability to update Conpherence titles by letting user click the title and fill out a little dialogue. Also fixes a bunch of random bugs and what have you. I tried to make the javascript less mysterious by trying to code what's actually happening more explicitly. Still a work in progress all over the place but a good stopping point for feedback.
Test Plan: played around with Conpherence. In particular, went to /conpherence/ and re-sized and went to /conpherence/X/ and re-sized. Also loaded up my no conpherneces user.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3155
Differential Revision: https://secure.phabricator.com/D6022
Summary: I introduced this helper at some point, clean up all the code duplication around content sources.
Test Plan: Grepped; hit edit interfaces for most/all of these.
Reviewers: btrahan, chad, edward
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6030
Summary:
A few users have come into IRC asking about this: basically, we show "children + grandchildren" until you have more than 50, then we show only children. This creates a few issues:
- This view is surprisingly useful, and becomes less useful after it folds.
- The folding behavior and program motivation for folding isn't obvious.
In the long term, some finessier approach is probably better here (configuration, CMS stuff, etc), but for now just bump the limit up.
Test Plan: Viewed Phriction, saw child paegs.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D6028
Summary: I'll build out a PHUI Class for this soonish.
Test Plan: UIExamples
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D6023
Summary: Added constants to PhabricatorEventType. Modified DifferentialRevisionEditor and DifferentialCommentEditor.
Test Plan:
Created a revision. Edited and made a comment on that revision. It's updating as usual. I think nothing broke may be it's working.
Let me know if I have done it correclty.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2899
Differential Revision: https://secure.phabricator.com/D5869
Summary: Inline comments weren't haven't remarkup applied. Fixes T3137
Test Plan: Added some inline comments, checked that they had remarkup applied. Also checked in the real time preview.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3137
Differential Revision: https://secure.phabricator.com/D6024
Summary:
Spent an hour or two helping a user figure this out. Make sure I never do that again.
If the webserver is configured with an empty or bogus PATH, binaries like 'which' and 'diff' (and 'git', and 'svn', etc.) may not be available. In most cases, this is fine, because we get an error like "sh: whatever-command not found", which is obvious to diagnose.
In the case of 'diff', we don't get this, because 'diff' is expected to exit with a nonzero code for differing files -- so we interpret the "sh: whatever-command not found" as "files differ" and then try to parse the empty output.
Explicitly check for 'which' (on Windows, 'where') and 'diff' during setup (I plan to refine the behavior around 'git', 'svn' and 'hg' at some point, but this is less pressing since the errors are trivial to support).
Test Plan: Faked failures on all modes, verified setup warnings look reasonable.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D6008
Summary:
Ref T2232. Very busy day on IRC so I feel like I've made 20 minutes of progress in 1-minute spurts here, but this adds the basics for a form that can have multiple pages and automatically handle pagination and reading to/from the request, objects and responses.
The UIExample is reasonably instructive. Basically, you make a form, add pages to the form, and add controls to the pages. The core flow control looks like this:
if ($request->isFormPost()) {
$form->readFromRequest($request); // (1)
if ($form->isComplete()) { // (2)
$response = $form->writeToResponse($response); // (3)
// Process result here. // (4)
}
} else {
$form->readFromObject($object); // (5)
}
The key parts are:
# This reads the form state from the request, including reading all the inactive pages.
# This tests if all pages are valid and the user just clicked "Done" on the last page.
# This produces a "response", which might be writing to an object (for simpler forms) or creating a transaction record (for more complex forms).
# Here, we would save the object or apply the transactions.
# When the user views the form for the first time, we preload all the values from some object (which might just be empty).
Ultimate goal here is to fix repository creation to not be a terrible pit of awfulness.
There are probably a lot of rough edges and missing features still, but this seems to not be totally crazy.
I'm using two submit buttons with different names which doesn't work on IE7 or something, but we can JS our way out of that if we need to.
Test Plan: Paged forward and backward through the form.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2232
Differential Revision: https://secure.phabricator.com/D6003
Summary: If we're unassigning an owner from a task it should set the column to `NULL` rather than an empty string. Fixes T3239
Test Plan: Assigned and Unassigned a task. Make sure the db is doing as excpected. Ran the patch, checked the db.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3239
Differential Revision: https://secure.phabricator.com/D6017
Summary:
Now has policy support (not really)
Now uses CountdownQuery
Now has handles
Now uses common way for remarkup
Remarkup still looks terrible
Test Plan: Added countdowns, edited countdowns. Did even embed some. Couldn't break it
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2624
Differential Revision: https://secure.phabricator.com/D6012
Summary:
removes the whole custom image thing, instead using a more standard application crumbs. Gives this glorious space back to the compose area which is now tens of pixels taller. Also defaults it to the people widget. Basically, fixes T3160.
For now, you **CAN NOT** edit the title of a conpherence. I didn't want to jam in too much here. Next diff will be to change the widget icons into the dropdown switcher, which will also bring back the editing of titles.
Test Plan: looked at conpherence and it was pretty. Resized it vigorously and it wasn't too bad.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T3160
Differential Revision: https://secure.phabricator.com/D5998
Summary: I used the `PhabricatorEnv::getProductionURI()` in too many places to build Releeph URIs. The only places that should need full URIs are the links generated for Releeph emails, and in Conduit responses that link to Releeph objects.
Test Plan:
- Grep for `getProductionURI()` in Releeph, and make sure only sensible, non-DOM building places use it.
- Inspect the Releeph DOM to make sure hrefs etc. are relative.
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5999
Summary:
Ref T2784. Relatively complicated one as this bad boy is used in a repository daemon.
While testing, I noticed bugs in the expandshortname query stuff. Those variables are private to the parent class so they need some setX love.
Also, was unable to find links to the "before" stuff, but made them by hand by looking at some of these T2784 diffs, browsing a file at a specific revision, then hacking the "before" variable to be some known commit that also touched the file. This produced sensical results. On the process of doing that I upgraded a query to use the proper policy query.
Test Plan: In git, mercurial, svn, verified on a commit page the "parents" showed up correctly. played around with ?before parameter on specific file browse page, with commits known to have interesting history and stuff looked good
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5988
Summary: Run through Projects in ALL_CAPS
Test Plan: see summary
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5990
Summary: The JsShrink change swapped an "== 0" for "=== 0" in JS, but we're currently sending a string down. Fixes T3230.
Test Plan: I'm not 100% sure this actually fixes it but probably? Let me know if you still see it.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T3230
Differential Revision: https://secure.phabricator.com/D5997
Summary:
Ref T2787.
I //think// we could also use WePay as a recurring payment provider, but this is somewhat messy (OAuth + requires account) -- basically it's "add a WePay account" instead of "add a credit card".
The WePay checkout workflow is a bit upsell-y but basically reasonable.
I like that their API just has a `request($method, $params)` method instead of 30,000 lines of methods for each request type. I did hit one bug; I'll send a pull for that.
Test Plan: Got as far as the charge callback in testing; the rest isn't implemented for any provider yet.
Reviewers: btrahan, vrana, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5982
Summary: This swaps out ObjectItemListView for PHUIFeedStory when viewing posts in a Phame blog.
Test Plan: Write blog posts, published or not, and test in Phame. Web and iOS tested.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5989
Summary: phts, crumbs, dust, standard navs.
Test Plan: Used owners on web and mobile.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5980
Summary: This adds the ability to have a multi-column full height container that is responsive based on PHUIBox's shadow box.
Test Plan: Tested new examples in UIExamples and Workboards.
Reviewers: epriestley, edward, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5996
Summary: Fixes T2965, see that task for discussion. This is dumb but seems like our best bet.
Test Plan:
- Installed newish version of Git.
- Set HOME on the websever to `/var/root` (or any other unreadable directory).
- Hit the error described in T2965 when viewing Diffusion.
- Applied this patch.
- Diffusion works.
Reviewers: btrahan, joel
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T2965
Differential Revision: https://secure.phabricator.com/D5994
Summary: wordy title. Ref T3214. Not exactly sure what a valid test is here as the user has a borked setup... Shouldn't fatal though.
Test Plan: arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3214
Differential Revision: https://secure.phabricator.com/D5995
Summary: Ref T2784. Also sneaks in a fix for branch query -- forgot to catch the unsupported VCS exception. One strange thing is I have test Phabricator repositories and the mercurial one is showing different data than git for the same commit. The data shown is consistent pre and post this diff though so its an existing issue. Also note the mercurial is an import of git so maybe its busted-ish?
Test Plan: viewed commits in mercurial, svn, and git that were merge commits. saw the right stuff in mercurial and git.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5985
Summary:
Ref T3217. @hlau has an email client which quotes text with:
________________________________________
From: ...
Add a regular expression to correctly detect this as quoted body text.
Test Plan: Add unit test; executed unit test.
Reviewers: btrahan, chad
Reviewed By: chad
CC: hlau, aran
Maniphest Tasks: T3217
Differential Revision: https://secure.phabricator.com/D5986
Summary: My `\w` shenanigans don't work with Diffusion, which has no explicit prefix.
Test Plan: Typed `rXnnn` and got the whole thing properly marked up.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5984
Summary: Ref T2784
Test Plan: for each flavor of VCS, I loaded up the repository home page. verified I saw some parent action where appropos. next, clicked through to 'view history' and verified it loaded up A-OK.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5960
Summary:
Ref T988. This adds basics for the non-static publishing target:
- Storage (called "Live", e.g. `DivinerLiveAtom` to distinguish it from shared classes like `DivinerAtom`).
- Mostly populate the storage.
- Some minor fixes and improvements.
Test Plan: Generated docs, looked at DB, saw mostly-sensible output.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D5973
Summary:
See <https://github.com/facebook/phabricator/issues/323>. We have a very old cache management script which doesn't purge all the modern caches (and does purge some caches which are no longer in use). Update it so it purges all the modern caches (remarkup, general, changeset), no longer purges outdated caches, and is easier to use.
Also delete a lot of "this script has moved" scripts from the last few rounds of similar cleanup, I believe all of these have been in master for at least several months, which should be enough time for users to get used to the new stuff.
Test Plan: Ran `bin/cache` with various arguments. Verified caches were purged.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5978
Summary: We can't show this stuff on the web UI because it has password reset links and private reply-to addresses, but we can provide easier CLI tools than "root around in the database". Land a rough version of `bin/mail show-inbound` and `bin/mail show-outbound`.
Test Plan: Used both commands to examine mail from the CLI.
Reviewers: btrahan
Reviewed By: btrahan
CC: tido, euresti, aran
Differential Revision: https://secure.phabricator.com/D5963
Summary: Convert to responsive layout, pht, etc.
Test Plan: Test Herald on desktop and mobile.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5976
Summary: Ponder was pretty unusable on mobile, I fixed most of the issues and ran some pht's as well.
Test Plan: Use Ponder //shudder//
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5977
Summary:
Fixes T3218.
- Currently, Paste pages don't clear notifications about the paste (notably, token notifications).
- Currently, Paste pages don't show tooltips on tokens.
- `buildApplicationPage()` stopped respecting `pageObjects` (which controls whether "this page has been updated" is shown). Restore that.
- Make `pageObjects` imply "clear notifications on this stuff".
Test Plan: Viewed a tokened Paste. Verified it cleared the notification and hovering over a token showed a tip.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3218
Differential Revision: https://secure.phabricator.com/D5971
Summary:
Ref T2785
Looks for hosts in `conduit.servers` config and if any exist route any conduit calls through any one of the hosts.
Test Plan:
Make some curl calls to public methods (`conduit.ping`), watch the access log for two requests. Make some calls from the UI that require authentication, watch the access log a bit more.
Also ran the unit tests.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2785
Differential Revision: https://secure.phabricator.com/D5970
Summary: We need to be able to request history for more than the master branch (the default). This adds branch as an option to the API.
Test Plan: Test by sending recentcommitsbypath a non-master branch along with callsign.
Reviewers: epriestley, chad, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5967
Summary: Adds mobile support to Audit, converts tables to object item views. I also colored 'concerns' and 'audit required' in the list, but nothing else. We can add more if needed but I'm assuming these are the two most important cases.
Test Plan: Tested as much as I could, a little unsure of a few things since my local repo isn't super filled. Will let epriestley run through.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5962
Summary:
Ref T3190. #shipit
Likely future work:
- Extract project mentions from remarkup for ApplicationTransactions; this isn't relevant in any apps right now (but will be in Pholio before tooo long). Ref T3189.
- Allow projects to have alternate short names. As written, this is fine for most projects ("Differential" is `#differential`) but not so great for other projects ("Phabricator Public & Media Relations" is `#phabricator_public_media_relations`). This also breaks refs when you rename a project. Better would be letting long project names have short aliases (`#pr`) as permitted alternatives.
Since this mention uses `#` instead of a letter, I needed to do a small amount of regexp gymnastics.
Test Plan:
you only #yolo once
{F43615}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3189, T3190
Differential Revision: https://secure.phabricator.com/D5954
Summary: Ref T2784. We need to always return array() here so the foreach doesn't crap out
Test Plan: chad - can you confirm this fixes the error? I have ref-heavy git repos it seems. should work though.
Reviewers: chad, epriestley
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T3215, T2784
Differential Revision: https://secure.phabricator.com/D5961
Summary: Consistent look for panels, test for mobile, forms consistency
Test Plan: test Macro on web and iOS sim
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5959
Summary: Ref T2784.
Test Plan: loaded up a git commit with refs and they showed up! loaded up a git commit without revs and nothing showed up.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5957
Summary: Ref T2784. This is a lower-level one from drequest so it gets the conditional initialization treatment. Consolidated SVN as well even though SVN is issuing database queries; I felt better about the code de-duplication despite the small performance hit when we could just query the DB directly in the SVN case.
Test Plan: browsed around my Phabricator repositories in Mercurial, Git, and SVN flavors. Looked good.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5956
Summary: Ref T2784.
Test Plan: loaded up my git and mercurial copies of Phabricator. Searched for "diff". Observed many results and pagination working correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5955
Summary:
Moves all remaining mail handling into ReplyHandlers.
Farewell, `getPhabricatorToInformation()`! You were a bad method and no one liked you.
Ref T1205.
Test Plan:
- Used test console to send mail to Revisions, Tasks, Conpherences and Commits (these all actually work).
- Used test console to send mail to Requests, Macros, Questions and Mocks (these accept the mail but don't do anything with it, but didn't do anything before either).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5953
Summary:
See <https://github.com/facebook/phabricator/issues/320>. We have a soft dependency on 'fileinfo', which we try to recover from (with `file`) but won't be able to on Windows and apparently FreeBSD systems. Since users can ignore setup checks anyway now, just raise a warning during install.
I believe almost all installs should have this extension, it has been part of the core for a long time.
Test Plan: Faked setup failure, looked at warning. "Solved" setup failure, saw it go away.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5952
Summary:
See <https://github.com/facebook/phabricator/issues/320>. Files can end up with a bad MIME type, and we don't update it when uploading another copy of the file since obviously the new copy has the same data and thus the same MIME type.
- Rename `bin/files metadata` to `bin/files rebuild` to make it a more consistent verb.
- Let it rebuild MIME types so users who hit issues like this can run `bin/files rebuild --all --rebuild-mime` to straighten things out.
Test Plan: Ran `bin/files` in various modes, examined output.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5951
Summary: Fixes T3210
Test Plan: Run the conduit call via the UI and ensure it no longer breaks. Check I get an error returned if no params are set
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3210
Differential Revision: https://secure.phabricator.com/D5950
Summary: This does the wrong thing (fatals) if there are no passed PHIDs.
Test Plan: No more fatal.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5940
Summary: Ref T1205. Moves the handling logic for these email types to reply handlers.
Test Plan: Used test form to send conpherence and maniphest mail.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5945
Summary: Currently this is fairly hard-coded. Instead, make it use available receivers. Ref T1205.
Test Plan: Used mail form to send mail to various objects (Dnn, Tnn, Cnn, etc.). Only some of these work right now because the receiver thing still hard-codes a bunch of junk.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5944
Summary:
Ref T1205. Finally able to delete a big chunk of this nastiness.
Make MailReceivers responsible for validating senders. For object creation receivers (bugs, conpherences) this just means that users must not be disabled. For other receivers the senders must be able to see the objects, have the right hashes, etc., according to policy.
Test Plan: Added a bunch of test cases (everything except policy). Verified behavior via the Receive test console.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5943
Summary: This doesn't do anything, but touches a bunch of files so I split it out to reduce the size of the next diff. Basically, make `MailReceiver` classes responsible for loading their application objects. Ref T1205.
Test Plan: Inspection / next diff / code is not reached.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5941
Summary:
Normalize the unit test environment by installing all applications.
The immediate issue this fixes is that `testDropUnknownSenderMail` depends on Maniphest being installed. Some possible fixes are:
# Don't rely on the Maniphest mail receiver for the test (e.g., write a stub/dummy/mock receiver).
# Explicitly make sure Maniphest is installed before running the test.
# Normalize the test environment to install all applications.
I don't like (1) much because it turns a pretty good 10 line test into a bunch of stub classes or mock junk. I'll do it if we have more uses after a few more diffs, but so far running these tests against real code hasn't created a dependency mess and we get more coverage.
I don't like (2) much because I think requiring tests to do this will do more harm than good. The number of issues we'll hypothetically uncover by exposing unrealized application interdependencies is probably very small or maybe zero, and they're probably all trivial. But tests with an undeclared but implicit dependency on an application (e.g., Differential tests depend on Differential) are common.
So here's (3), which I think is reasonable.
I also simplified some of this code a little bit, and moved the Application object cache one level down (this was sort of a bug -- installation status is variant across requests).
Test Plan: Added unit test.
Reviewers: wez, btrahan
Reviewed By: wez
CC: aran
Differential Revision: https://secure.phabricator.com/D5938
Summary: Image URLs were accidentally inaccurate due to the macro ID being overwritten by the file ID when searching for users
Test Plan: Visited the macro page and searched for my own macros. They are now clickable.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3193
Differential Revision: https://secure.phabricator.com/D5936
Summary: Copies sender identification logic into MailReceivers and makes it basically sane. The mess we run into after this try/catch is terrifying so I'm avoiding actually getting rid of any of it quite yet. Ref T1205.
Test Plan: Added a bit of test coverage. Used Receiver test console to verify some additional behaviors.
Reviewers: btrahan
Reviewed By: btrahan
CC: Afaque_Hussain, aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5931
Summary: forgot to specify the commit! On github as https://github.com/facebook/phabricator/pull/318, though this is a tighter fix.
Test Plan: tried to save an owners package with a folder - FAIL - then with the patch - GREAT SUCCESS
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5933
Summary: Adding Email reply support for external users.
Test Plan: Please let me know if I have approached it correctly. Had few doubts. Will proceed after your comments :)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan, jennis.mekwan3
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5912
Summary: Ref T2784. This one was a wee bit complicated. Had to add PhabricatorUser and concept of initFromConduit (or not) to DiffusionRequest.
Test Plan: foreach repo, visited CALLSIGN and clicked a commit and verified they laoded correctly. Hacked code to hit NOT via Conduit and repeated tests to great success.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5928
Summary: Ref T3183. We should accept addresses like `"New Bug" <bugs@example.com>` to match `bugs@example.com`.
Test Plan: Ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3183
Differential Revision: https://secure.phabricator.com/D5923
Summary: title does not say it all; also added conduit wrappers for rawdiffquery and lastmodifiedquery. These get the wrapper treatment since they are used in daemons. Ref T2784.
Test Plan: for each of the 3 VCS, did the following: 1) loaded up CALLSIGN and verified 'Modified' column data showed up correctly in Browse Repository box. 2) loaded up the "change" view for a specific file and verified content showed up correctly. 3) loaded up a specific commit and noted the changes ajax loaded A-OK
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5896
Summary: This piggybacks onto device-phone's CSS rules to enable a full width form (for smaller spaces).
Test Plan: Convert New Message dialog to fullWidth.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5924
Summary:
Ref T1205. Continuation of D5915.
Currently, `PhabricatorMetaMTAReceivedMail` has //all// the logic for routing mail. In particular:
- New mail receivers in applications must edit it.
- Mail receivers don't drop out when applications are uninstalled.
Applications have some logic in subclasses of `PhabricatorMailReplyHandler`, but this class is a bit of a mess. It is also heavily based on the assumption that mail receivers are objects (like revisions), but this is not true in at least two cases today (creating new tasks with `bugs@`, creating a new Conpherence thread) and likely other cases in the future (e.g., revision-by-mail).
Move this logic into a new `PhabricatorMailReceiver` classtree. This is similar to `PhabricatorMailReplyHandler` but a bit cleaner and more general. I plan to heavily reduce the responsibilities of `PhabricatorMailReplyHandler` or possibly eliminate it entirely.
For now, the new classtree doesn't do much of interest. The only behavioral change this diff causes is that Phabricator will now reject mail to an application when that application is uninstalled.
I also moved all the `ReplyHandler` classes into `mail/` directories in their respective applications.
Test Plan: Unit tests, used receive test to route mail to various objects.
Reviewers: btrahan
Reviewed By: btrahan
CC: Afaque_Hussain, edward, aran
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5922
Summary:
We/I broke a couple of things here recently (see D5911) and are doing some work here in general (see D5912, etc.).
Generally, this code is pretty oldschool and not especially well architected for modern application-oriented Phabricator. It hardcodes a lot of stuff which should be applications' responsibilites.
Take the first steps toward making it more solid to reduce the risk here. In particular:
- Factor out the "self mail" and "duplicate mail" checks and add unit tests.
- Make Message-ID hash handling automatic.
Test Plan: Ran unit tests.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5915
Summary: Fixes T3188. See that task for details.
Test Plan: Viewed packages with and without project membership; no more exception.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3188
Differential Revision: https://secure.phabricator.com/D5918
Summary:
Fixes T3181.
- Inbound `bugs@` mail is broken right now if it doesn't use the new external user stuff, because it calls `$user->getPhabricatorUser()` on an object which is already a `PhabricatorUser`. Instead, build the right `$user` object from the external user earlier on.
- Maniphest mail is nuking or otherwise awkwardly altering CCs. Make this work properly.
- Make sure "!unsubscribe" works correctly.
Test Plan: Sent `bugs@` mail. Sent CC mail. Sent "!unsubscribe" mail.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran, tido
Maniphest Tasks: T3181
Differential Revision: https://secure.phabricator.com/D5911
Summary:
`number_format()` returns a string, so if passed a number greater than 999 by default it will add commas, which parsed by %d will only return the 1000's delimter.
```echo sprintf("%d", number_format(1000)); // Outputs 1
echo sprintf("%s", number_format(1000)); // Outputs 1,000
Test Plan: Looked at repos with over 999 commits.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5913
Summary: Maniphest sends email to External users.
Test Plan:
{F42649}
It seems that maniphest tries to send an email, my install is not configured to deliver email.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5856
Summary:
Ref T2784. Begins pulling discovery into Engines and covering it with tests. In particular:
- Discovery is currently a one-shot process where we find all the new commits and write them to the database in one go. Split it apart so we find and return the new commits first, then write them to the database separately. This makes things simpler and more testable.
- This diff only brings SVN into an engine (and only the "find the commits" part), since it's simpler than Git or Mercurial.
- Creates a base Engine class and moves common functionality there.
- Restores the `--verbose` flag to `repository pull`.
Test Plan: Added unit tests. Ran `bin/repository discover`. Ran `bin/phd debug pulllocal`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5906
Summary:
Ref T2784. This moves us toward being able to test the background and Conduit pipelines for repositories. In particular:
- Separate the logic for pulling repositories (`git pull`, `hg pull`) out of `PhabricatorRepositoryPullLocalDaemon` and put it in `PhabricatorRepositoryPullEngine`. This allows repositories to be pulled directly without invoking the daemons.
- Add tests for the engine, including a future-looking base test case.
- Add basic `PhutilDirectoryFixture`-based repositories.
Next steps:
# Do the same for repo discovery.
# Then we can start writing tests against specific Conduit methods.
Test Plan: Ran unit tests. Ran `bin/repository pull` on SVN, Hg and Git repositories. Ran `bin/phd debug pulllocal`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, nh
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5904
Summary: Added a space in between "document" and "DocumentName"
Test Plan: Edit a document
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D5909
Summary: Converts to ObjectList for display, pht's most everything, some responsive design when possible. Tables still are tables, but scroll on touch.
Test Plan: Use iOS simulator on Diffusion, Chrome
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5847
Summary: This is just a bit of gardening in order to make the responsive-UI diff easier; I'll be putting `getColorFor($status)` type things in this class, following the pattern in `ManiphestTaskStatus`.
Test Plan: Poke around Releeph.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5893
Summary:
Migrate to `PhabricatorApplicationTransactions` (`ReleephRequestTransactions` applied by `ReleephRequestTransactionalEditor`, instead of `ReleephRequestEvents` created by `ReleephRequestEditor`) and migrate all the old events into transactions. Email is supported in the standard way (no more `ReleephRequestMail`) as well.
This also collapses the Releeph request create and edit controllers into one class, as well as breaking everyone's subject-based mail rules by standardising them (but which should be more easily filtered by looking at headers.)
Test Plan:
* Make requests, then pick them.
* Pick and revert the same request so that discovery happens way after `arc` has told Releeph about what's been happening.
* Try to pick something that fails to pick in a project with pick instructions (and see the instructions are in the email.)
* Load all of FB's Releeph data into my DB and run the `storage upgrade` script.
* Request a commit via the "action" in a Differential revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: epriestley, aran, Korvin, wez
Maniphest Tasks: T3092, T2720
Differential Revision: https://secure.phabricator.com/D5868
Summary: Oops, `pht()` interpolates percent escape sequences, but this is a literal "%N".
Test Plan: Edit a Releeph project; notice no errors.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5897
Summary: forgot to declare this method as static. PHP manages to make it work functionally anyway. Ref T2784. Fixes T3175.
Test Plan: loaded diffusion, observed no errors
Reviewers: chad, epriestley
CC: aran, Korvin
Maniphest Tasks: T3175, T2784
Differential Revision: https://secure.phabricator.com/D5899
Summary: Can name saved queries.
Test Plan: Try naming some saved queries using the form.
Reviewers: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D5878
Conflicts:
src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
Summary: title. Ref T2784.
Test Plan: foreach of SVN, Mercurial, and Git, loaded up a repository. Verified that only git had a tags box and it showed up correctly. Went to CALLSIGN/tags and verified that only git had a tags box and it showed up correctly. Went to various commits across vcs and verified it said "none" unless it was a git commit that also was tagged.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5894
Summary: I wrote some of the Releeph-project edit-page's help in remarkup. This renders the remarkup using the `PhabricatorMarkupEngine` pipeline.
Test Plan:
* Edit a Releeph project.
* Feel the cool base of a laptop whose CPU is no longer being thrashed by onerous remarkup rendering duties.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3098
Differential Revision: https://secure.phabricator.com/D5898
Summary:
We have a few interfaces where add "Edit", "Delete" or some other action to a list. Currently, this happens via icons, but these are cumbersome and weird, are inconsistent, can't be workflow'd, are hard to hit on desktops and virtually impossible to hit on mobile, and generally just feel iffy to me. Prominent examples are Projects and Flags. I'd like to try adding an "edit" action to Maniphest (to provide quick edit from list views, basically). It looks like some of Releeph would benefit here, as well.
Instead, provide first-class actions:
{F42978}
They produce targets which my meaty ham-fists can plausibly hit on mobile, too:
{F42979}
(We could do some kind of swipe-to-expose thing eventually, but I think putting them by default is OK?)
Test Plan: Added UIExamples. Checked desktop/mobile.
Reviewers: chad, btrahan, edward
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D5890
Summary: see title. Ref T2784.
Test Plan:
In diffusion, for each of SVN, Mercurial, and Git, I loaded up /diffusion/CALLSIGN/. I verified the README was displayed and things looked good. Next I clicked on "browse" on the top-most commit and verified things looked correct. Also clicked through to a file for a good measure and things looked good.
In owners, for each of SVN, Mercurial, and Git, I played around with the path typeahead / validator. It worked correctly!
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5883
Summary: Adds the PHIDs of the tasks that the current task depends on.
Test Plan: Use conduit to query a task with and without tasks it depends on.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5892
Summary:
Fixes T3151. Javelin treats a behavior without parameters as a global behavior and invokes it only once no matter how many times it is initialized (this is necessarily correct for any reasonable behavior, as the inputs do not vary). A recent patch changed `differential-dropdown-menus` from a zero-argument global behavior to an implicitly nonzero-argument behavior by adding `pht`.
Currently, we initialize the behavior next to dropdown menu creation, so this resulted in `O(N^2)` initializations of the menus. For large diffs, this locks browsers. Instead, initialize outside of the dropdown loop so we ginitialize each menu just once.
Test Plan: Viewed a 2,000 file diff without browser lock.
Reviewers: wez, vrana, btrahan
Reviewed By: wez
CC: aran
Maniphest Tasks: T3151
Differential Revision: https://secure.phabricator.com/D5885
Summary: Fixes the button spacing issue (doesn't seem related to forms?) and moves fonts and sizes over to Helvetica.
Test Plan: Submit many inline comments.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5882
Summary:
Fixes T3143. When a user deletes a file, delete all transforms of the file too. In particular, this means that deleting an image deletes all the thumbnails of the image.
In most cases, this aligns with user expectations. The only sort of weird case I can come up with is that memes are transformations of the source macro image, so deleting grumpycat will delete all the hilarious grumpycat memes. This seems not-too-unreasonable, though, and desirable if someone accidentally uploads an inappropriate image which is promptly turned into a meme.
Test Plan:
Added a unit test which covers both inbound and outbound transformations.
Uploaded a file and deleted it, verified its thumbnail was also deleted.
Reviewers: chad, btrahan, joseph.kampf
Reviewed By: btrahan
CC: aran, joseph.kampf
Maniphest Tasks: T3143
Differential Revision: https://secure.phabricator.com/D5879
Summary: Wanted to clean this up a little to make Deedy's diff hopefully easier. Removes some unneeded CSS, and Deedy's should remove more with object list.
Test Plan: Search for people, documents, tasks, etc. Test mobile and desktop layouts
Reviewers: epriestley, DeedyDas
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5880
Summary:
Ref T3023
Token support for Phriction Documents, Ponder Questions, and Phame Blogs
Test Plan: Token notifications and visual display seems to be working for the above types
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T3023
Differential Revision: https://secure.phabricator.com/D5862
Summary: Instead of being able to ask if someone was a pusher or not, ask if they are "authoritative" enough to make decisions about Releeph requests. A person is authoritative if a project has pushers, and they are a pusher, or in the case of pusher-less projects, everyone is authoritative.
Test Plan: Make a request in a project with no pushers (it is immediately ready to be picked) and a project with pushers (where it requires approval.)
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: epriestley, aran
Differential Revision: https://secure.phabricator.com/D5877
Summary:
Ref T3141. This is a hacky fix until we can get something a little more sensible in place.
Currently, we sometimes end up with an empty text list, which causes file content to not display. Instead, generate the text list from the corpus if it isn't available.
Test Plan: Looked at a previously-broken source listing, saw code.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3141
Differential Revision: https://secure.phabricator.com/D5871
Summary:
Fixes T3132. Currently, if a user deletes a file which is present in a mock, that mock throws an exception when loading. If the file is also the cover photo, the mock list throws an exception as well.
In other applications, we can sometimes deal with this (a sub-object vanishing) by implicitly hiding the parent object (for example, we can just vanish feed stories about objects which no longer exist). We can also sometimes deal with it by preventing sub-objects from being directly deleted.
However, neither approach is reasonable in this case.
If we vanish the whole mock, we'll lose all the comments and it will generally be weird. Vanishing a mock is a big deal compared to vanishing a feed story. We'll also need to load more data on the list view to prevent showing a mock on the list view and then realizing we need to vanish it on the detail view (because all of its images have been deleted).
We permit total deletion of files to allow users to recover from accidentally uploading sensitive files (which has happened a few times), and I'm hesitant to remove this capability because I think it serves a real need, so we can't prevent sub-objects from being deleted.
So we're left in a relatively unique situation. To solve this, I've added a "builtin" mechanism, which allows us to expose some resource we ship with as a PhabricatorFile. Then we just swap it out in place of the original file and proceed forward normally, as though nothing happened. The user sees a placeholder image instead of the original, but everything else works reasonably and this seems like a fairly acceptable outcome.
I believe we can use this mechanism to simplify some other code too, like default profile pictures.
Test Plan: Deleted a Pholio mock cover image's file. Implemented change, saw functional Pholio again with beautiful life-affirming "?" art replacing soul-shattering exception.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T3132
Differential Revision: https://secure.phabricator.com/D5870
Summary: Makes all forms on People app consistent with rest of site.
Test Plan: Click each page
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5865
Summary: Adding mail-keys; required for `PhabricatorApplicationTransaction` support.
Test Plan: Upgrade an old database with this patch, observe the matrix: {F42620}
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: epriestley, aran
Maniphest Tasks: T2720
Differential Revision: https://secure.phabricator.com/D5852
Summary:
Removing a bunch of cache-style columns from `ReleephRequest`, where it's actually much easier to just load the information at runtime.
This makes sense for migrating to `PhabricatorApplicationTransactions`, where each xaction changes one aspect of a `ReleephRequest` at a time (rather than multiple columns at once.)
Test Plan: Request something, run `arc releeph` and watch the picks, pass on some RQs, run `arc releeph` and watch the reverts.
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: epriestley, aran
Maniphest Tasks: T2720
Differential Revision: https://secure.phabricator.com/D5851
Summary: Adds mobile support, new forms.
Test Plan: Clicked on each page after converting. Tested mobile menu
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5863
Summary: Ref T2784. This is probably pretty good except the fancy lint error saver now issue serial queries via Conduit.
Test Plan: reparsed commits on 3 repos - yay. viewed readme from diffusion UI on 3 repos - yay. viewed file content from diffusion UI on 3 repos - yay.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5824
Summary:
This creates a common form look and feel across the site. I spent a bit of time working out a number of kinks in our various renderings. Some things:
- Font Styles are correctly applied for form elements now.
- Everything lines up!
- Selects are larger, easier to read, interact.
- Inputs have been squared.
- Consistant CSS applied glow (try it!)
- Improved Mobile Responsiveness
- CSS applied to all form elements, not just Aphront
- Many other minor tweaks.
I tried to hit as many high profile forms as possible in an effort to increase consistency. Stopped for now and will follow up after this lands. I know Evan is not a super fan of the glow, but after working with it for a week, it's way cleaner and responsive than the OS controls. Give it a try.
Test Plan: Tested many applications, forms, mobile and tablet.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5860
Summary: External user gets on the CC list of a task.
Test Plan: {F42630}
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2804
Differential Revision: https://secure.phabricator.com/D5853
Summary: Not every commit requested in Releeph has to have come from a Differential revision. If you have a Releeph request for a commit without a Differential revision, you currently get this lovely error in the logs: {P824}. This diff silences the howling.
Test Plan: Load a Releeph branch page; observe log silence.
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: epriestley, aran, SEJeff, Korvin
Differential Revision: https://secure.phabricator.com/D5823
Summary: Use a single `PhabricatorMarkupEngine` to render any markup in Releeph's fields, rather than rendering everything from scratch every time.
Test Plan: Check out the "Services" tab in the dark console when rendering a page on a branch page with 500x RQs!
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: epriestley, aran
Maniphest Tasks: T3098
Differential Revision: https://secure.phabricator.com/D5822
Summary:
`ReleephRequest`s contain the PHID for a `PhabricatorRepositoryCommit`, and commits have an edge to a `DifferentialRevision`.
Commits are loaded with the `loadOneRelative()` method that loads the commits for every `ReleephRequest` in a `LiskDAOSet`, but the edges are loaded indivdually. A page with N RQs on it makes one DB query for the commits, but N queries for the `TYPE_COMMIT_HAS_DREV` edges.
This diff uses `loadRelativeEdges` instead to load the edges all in one query.
Test Plan: {F42290}
Reviewers: wez, epriestley
Reviewed By: epriestley
CC: epriestley, vrana, aran
Maniphest Tasks: T2714
Differential Revision: https://secure.phabricator.com/D5820
Summary:
Ref T1751. This still doesn't do anything very interesting, but loads the acutal Commit objects that a commit message claims to revert.
The only tricky thing here is that we need to interpret "reverts rnnn" or "reverts nnn" in an SVN repository as "reverts rXnnn", where "X" is the current repository. This adds a method to do allow `DiffusionCommitQuery` to do that.
Test Plan:
Used `reparse.php --message` to reparse several commits with revert language and verify they loaded the correct affected commits.
In an SVN repository, created a commit with ambiguous revert language ("reverts n", "reverts rn", "reverts n, n") and verified it identified the affected commits correctly despite ambiguity.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1751
Differential Revision: https://secure.phabricator.com/D5842
Summary:
Ref T2787. See discussion in D5834.
- Replace `PhortuneUtil` with `PhortuneCurrency`, which feels a little better and more flexible / future-proof.
- Add unit tests.
- Display prices explicitly as "$... USD".
Test Plan: Hit product list, cart, purchase flow, verified displays.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5841
Summary: There are a few places a third text row in ObjectItemListView is needed or make things easier to read. Built and rolled out in Config.
Test Plan: Tested in Config Groups.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5837
Summary: Remove lines that set from field.
Test Plan: Check that emails are sent without from field.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5845
Summary:
Ref T1751. When commit messages include language like "reverts X", parse it. This change doesn't do anything with the commits yet.
I attempted to cover all "natural" VCS messages and all reasonable human variations of these messages.
Test Plan: Added unit tests. Added `var_dump()` and used `scripts/repository/reparse.php --message X` to reparse some commit messages, with expected results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1751
Differential Revision: https://secure.phabricator.com/D5840
Summary:
Paypal doesn't let us capture cards in a PCI-free way like Stripe and Balanced do, but we can provide a "pay with paypal" option at checkout. (For subscriptions, we'll have to invoice monthly to retain control over billing, but this doesn't seem wildly unreasonable.) The bitcoin provider MtGox works in a similar way, as do some other providers we might some day want to implement.
This adds:
- Hooks to providers so they can offer "pay once at checkout" workflows.
- Hooks so providers can have controllers, for redirect-based third-party workflows.
- Basic Paypal integration using the "Express Checkout Merchant API", which seems like the best fit for our use case. This only goes as far as shoving the user through the payment flow; we don't actually capture payments yet (paypal has around 35 different APIs, but this one seems to be the only PCI-free one which wouldn't give users an awful experience).
This diff is fairly checkpointey, but Phortune doesn't really bill anything yet anyway. Ref T2787.
Test Plan: Ran through Paypal sandbox workflow; "paid" for stuff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5834
Summary: Added PHID_TYPE_XUSR to loadObjectsofType() of PhabricatorObjectHandle data. Was trying to solve the setActor() issue. Created a getPhabricator method in PhabricatorExternalAccount. When I try to set authorPHID using setAuthorPHID() in PhabricatorExternalAccount it's saying bad setter call, if I don't, then it's saying authorPHID can't be null. Sending you diff for comments on this.
Test Plan:
{F42523}
{F42525}
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5839
Summary: Partially complete advanced search (building a form that might be right).
Test Plan: Check that form appears for advanced filter.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Differential Revision: https://secure.phabricator.com/D5807
Summary:
There's a crash when `arc diff` is running it's lint / unit test
phase which describes that this function should be public instead
of private:
```Fatal error: Access level to PhabricatorPholioMockTestDataGenerator::loadPhabrictorUserPHID() must be public (as in class PhabricatorTestDataGenerator) in /srv/www/tychaia.com/phab/phabricator/src/applications/pholio/lipsum/PhabricatorPholioMockTestDataGenerator.php on line 102```
Test Plan: `arc diff` should not crash.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5828
Summary: Went and pht'd most everything I could find, except exceptions. Fixes T3091
Test Plan: Bopped around the application, didn't seem to break anything but didn't test all flows. Should be safe.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3091
Differential Revision: https://secure.phabricator.com/D5825
Summary:
Use imagemagick convert to first detect the number of gif frames,
then +adjoin to split them in tmpfile, then call applyMemeTo to
each frame, then convert them back.
Test Plan:
Add memes in sandbox
with both gifs and non-gifs. Note that gifs are quite slow.
CC: aran, epriestley, chad
Differential Revision: https://secure.phabricator.com/D5821
Summary:
countdown_timer table named to countdown.
datepoint and related stuff renamed to epoch.
Countdowns now have phids.
Various UI items changed from timer to countdown.
Test Plan: Did run storage upgrade and added some countdowns.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2624
Differential Revision: https://secure.phabricator.com/D5812
Summary:
Same as //Subscribe//, //Unsubscribe// and //Automatically Subscribed// in differential.
Manually updated library map as windows is fun!
Test Plan: Subscribe, Unsubscribe!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5809
Summary:
getDiffDict method returns the JSON for the changesets in a diff
The JSON descriping a changeset didn't containt the changeset ID
in some scenarios, knowing the changeset ID is really useful for the
clients.(i.e. when you want to open a standalone view of the changeset,
you need to know its ID)
Test Plan: existing
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5816
Summary: Fixes T3087. I guess I didn't load up the actual Conpherence UI last time I was in here with my 'noconpherences' user. :/
Test Plan: noconpherences user loaded /conpherence/ with no error
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T3087
Differential Revision: https://secure.phabricator.com/D5815
Summary: ref T2784. This one had a few fun spots where I had to move data around. Also, is there some common object (or should I add it?) that can do this toDictionary newFromConduit stuff? Also, this assumes D5803 is largely correct at the time of this diff.
Test Plan: browsed mercurial and git repository page. saw the branches i expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5810
Summary: le title. However, this also "is the first" conversion so sets the precedence for how this will all work. See comments in the code. I think this helps us keep the new code we're writing to a minimum. Wondering if the conduit end point could be more generic, and rather than have a switch statement on VCS type, one can just implement the "handleSubversion" version and have that called? Ref T2784
Test Plan: slapped an "or true" in the conditional protecting this code path. verified it worked on all 3 vcs systems, including typing in garbage and getting a 404
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D5803
Summary: Fixes T2416. I was tired of looking at this task in the list, interns be damned. :D
Test Plan: it is now a workflow when you click it
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2416
Differential Revision: https://secure.phabricator.com/D5811
Summary: Ref T2683. Provides access to `formatWhereClause()`, etc. I plan to eventually unify the various DiffusionQuery execute methods as well, which all have special names right now (`loadModifiedPaths()`, `loadWhateverBlah()`, etc).
Test Plan: Ran EverythingImplemented test. Browsed Diffusion.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D5251
Summary: Enable saved query objects to actually be saved to the database.
Test Plan: Insert a call to save() and check that the query is written correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T2625
Differential Revision: https://secure.phabricator.com/D5775
Conflicts:
src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
Summary:
Highlights the current day :)
I am not great in choosing colors. @chad can suggest some improvements :)
Test Plan: {F41738}
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin, AnhNhan
Maniphest Tasks: T2627
Differential Revision: https://secure.phabricator.com/D5790
Summary: Using PhabricatorExternalAccount in place maniphest.default-public-author.
Test Plan:
Using receivemail to see if the a new entry is made in the 'phabircator_user.user_externalaccount' table. Few things, I noticed that phabricator creates table 'user_externalaccout'. And now it throws up error 'Unknown column 'dateCreated' in 'field list''. Awaiting your comments.
{F41370}
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T1205
Differential Revision: https://secure.phabricator.com/D5747
Summary: this is D5750 but just the conpherence part. fixes a few random conpherence bugs / quirks as well. Also messes with ApplicationTransactionEditor to expose the xactions so Conpherence doesn't over-update participation rows. Fixes T2429.
Test Plan: set LIMIT to 3. verified I could scroll down all conpherences. next, picked a conpherence "in the middle" to load. verified I could page up and down. next, picked a conpherence in the middle then had another user update that conpherence. verified as I paged up the conpherence re-loaded properly selected
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin, vrana
Maniphest Tasks: T2429
Differential Revision: https://secure.phabricator.com/D5783
Summary: These should be in (true, false) order, not (false, true) order. This was a transcritpion error in porting the config over.
Test Plan: Edited config from the web, verified the labels matched the underlying values.
Reviewers: tberman, btrahan
Reviewed By: tberman
CC: aran
Differential Revision: https://secure.phabricator.com/D5786
Summary: Provide a bare implementation so that you can add PhortuneTestProvider as a payment method. Ref 2787.
Test Plan: Added "cards" through the test provider.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5772
Summary:
This has no real behavioral changes (except better error handling), it just factors things out to be a bit cleaner. In particular:
- Move more shared form behaviors into the common JS form component.
- Move more error handling into shared pathways.
- Make the specialized Stripe / Balanced methods do less work.
This needs some more polish for nontrival errors (especially on the Balanced side) but none of the error behavior is worse than it was and a lot of it is much better.
Ref T2787.
Test Plan: Hit all invalid form errors, added valid payment methods with Stripe and Balacned.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5771
Summary:
Allows Balanced payment methods to be added. This works essentially the same way as Stripe, except everything is a little bit different.
Slightly more stuff could be shared, but I feel //mostly// good about this. I'll probably do a bit more cleanup next. Some of the error handling is messy, in particular.
Ref T2787.
Test Plan: Added Balanced and Stripe payment methods.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5765
Summary:
General cleanup and separation into generic vs Stripe blocks of code.
- There was an old CC form view for Stripe stuff that I never cleaned up; clean that up.
- Move non-Stripe CC form rendering into a base class (Balanced can reuse it).
- Move non-Stripe CC form JS into a shareable class.
- Simplify JS a bit (JX.Workflow can add extra parameters to a request, so we don't need hidden inputs).
- Genericize CSS.
- Depend on Stripe JS directly, if they're down we're not going to be able to add cards anyway.
Ref T2787.
Test Plan: Hit all Stripe errors and added new cards.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5758
Summary:
Ref T2787. For payment methods that allow you to add a billable method (i.e., a credit card), move all the logic into the provider. In particular:
- Providers may (Stripe, Balanced) or may not (Paypal, MtGox) allow you to add rebillable payment methods. Providers which don't allow rebillable methods will appear at checkout instead and we'll just invoice you every month if you don't use a rebillable method.
- Providers which permit creation of rebillable methods handle their own data entry, since this will be per-provider.
- "Add Payment Method" now prompts you to choose a provider. This is super ugly and barely-usable for the moment. When there's only one choice, we'll auto-select it in the future.
Test Plan: Added new Stripe payment methods; hit all the Stripe errors.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5756
Summary: Abstract out the Stripiness of payment providers so we can add a Test provider (and maybe MtGox / Balanced / Paypal / etc). Ref T2787.
Test Plan: Ran unit tests.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5752
Summary: Although I imagine we aren't really going to have an "add to cart" type storefront, putting this in place makes a lot of other workflows simpler. No storage yet, just allows reasonable construction of a "buy stuff" page.
Test Plan: {F41342}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T2787
Differential Revision: https://secure.phabricator.com/D5745
Summary: This provides some new display methods and examples to PHUIFeedStory.
Test Plan: Tested UIExamples Page, mobile layouts, and existing Feed Pages (feed, profile, etc). I want to add a bit more but am stopping here since it's not a priority.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5749
Summary:
There wasn't an easy way to see all the packages that cover a particular
directory. This adds the option
This is useful to see what's covered within a repo.
Test Plan:
* Queried with a valid repository and path - got the results
as before.
* Queried with a repository and no path - got an error as before.
* Queried with a repository and an asterix as the path - got all the
packages within the repository.
Revert Plan: sure
Other Notes:
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T3016
Differential Revision: https://secure.phabricator.com/D5763
