Summary:
Depends on D20844. Ref T13425. When we line up two blocks and they can be interdiffed (generally: they both have the same type of content), let the Engine interdiff them.
Then, make the Jupyter engine interdiff markdown.
Test Plan: {F6898583}
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20845
Summary:
Depends on D20843. Ref T13425. Add very basic support for "Show Hidden Context", in the form of folding it behind an unclickable shield. This isn't ideal, but should be better than nothing.
Prepare for "intraline" diffs on content blocks.
Fix newline handling in Markdown sections in Jupyter notebooks.
Remove the word "visibile" from the codebase.
Test Plan: {F6898192}
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20844
Summary:
Ref T13425. Some diff checks currently sequence incorrectly:
- When we're rendering block lists, syntax highlighting isn't relevant.
- The "large change" guard can prevent rendering of otherwise-renderable changes.
- Actual errors in the document engine (like bad JSON in a ".ipynb" file) aren't surfaced properly.
Improve sequencing somewhat to resolve these issues.
Test Plan:
- Viewed a notebook, no longer saw a "highlighting disabled" warning.
- Forced a notebook to fail, got a useful inline error instead of a popup dialog error.
- Forced a notebook to have a large number of differences, got a rendering out of it.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20843
Summary:
See PHI1442. If you have a bulk-editable datasource field with a composite datasource, it can currently fatal on the bulk edit workflow because the viewer is not passed correctly.
The error looks something like this:
> Argument 1 passed to PhabricatorDatasourceEngine::setViewer() must be an instance of PhabricatorUser, null given, called in /Users/epriestley/dev/core/lib/phabricator/src/applications/typeahead/datasource/PhabricatorTypeaheadCompositeDatasource.php on line 231
Test Plan: Configured a Maniphest custom field with a composite datasource, then tried a bulk edit. Things worked cleanly instead of fataling.
Differential Revision: https://secure.phabricator.com/D20841
Summary:
Ref T13425. Currently, code inputs and all outputs are grouped into a single block. This is fine for display notebooks but not great for diffing notebooks.
Instead, split source code input into individual lines with one line per block, and each output into its own block.
This allows you to leave actual line-by-line inlines on source, and comment on outputs individually.
Test Plan: {F6888583}
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20840
Summary: Depends on D20838. Fixes T13414. Instead of doing coarse diffing with "PhutilEditDistanceMatrix", use hash-and-diff with "DocumentEngine".
Test Plan:
- On a large document (~3K top level blocks), saw a more sensible diff, instead of the whole thing falling back to "everything changed" mode.
- On a small document, still saw a sensible granular diff.
{F6888249}
Maniphest Tasks: T13414
Differential Revision: https://secure.phabricator.com/D20839
Summary:
Depends on D20835. Ref T13425. Ref T13414. When a document has a list of content blocks, we may not be able to diff it directly, but we can hash each block and then diff the hashes (internally "diff" also does approximately the same thing).
We could do this ourselves with slightly fewer layers of indirection, but: diff already exists; we already use it; we already have a bunch of abstractions on top of it; and it's likely much faster on large inputs than the best we can do in PHP.
Test Plan: {F6888169}
Maniphest Tasks: T13425, T13414
Differential Revision: https://secure.phabricator.com/D20836
Summary:
Depends on D20834. Ref T13425. After the change from "th" to "td" for accessibility, the algorithm picks which cells it should highlight slightly improperly (it picks too many cells since it can no longer find the line numbers).
Ideally, it would probably highlight //only// the source content, but there isn't an easy way to do this right now. Settle for an incremental improvement for the moment.
Test Plan: Hovered over line numbers, saw a more accurate highlight area.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20835
Summary: Depends on D20833. Ref T13425. This look like it "just works"?
Test Plan: Left inline comments on a Juptyer notebook. Nothing seemed broken? Confusing.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20834
Summary:
Depends on D20832. Ref T13425. Emit Jupyter notebooks as diffable blocks with block keys.
No diffing or proper inlines yet.
Test Plan: {F6888058}
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20833
Summary:
Depends on D20831. Ref T13425. As an escape hatch to get out of future DocumentEngine rendering behavior, provide a "View As.." option.
Now I can break DocumentEngine real bad and no one can complain.
Test Plan: Used "View As" to swap document engines for image files.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20832
Summary:
Depends on D20830. Ref T13425. Have the image engine elect into block rendering, then emit blocks.
This is rough (the blocks aren't actually diffed yet) but image diffs were already pretty rough so this is approximately a net improvement.
Test Plan: Viewed image diffs, saw nothing worse than before.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20831
Summary:
Ref T13425. Allow DocumentEngines to claim they can produce diffs. If both sides of a change can be diffed by the same document engine and it can produce diffs, have it diff them.
This has no impact on runtime behavior because no upstream engine elects into diff generation yet.
Test Plan: Loaded some revisions, nothing broke.
Maniphest Tasks: T13425
Differential Revision: https://secure.phabricator.com/D20830
Summary:
Fixes T13284. See that task for substantial discussion. There are currently two cases where we'll skip over commits which we should publish:
- if a branch is not permanent, then later made permanent; or
- in some cases, the first time we examine branches in a repository.
In both cases, this error is one-shot and things work correctly going forward. The root cause is conflation between the states "this ref currently permanent" and "this ref was permanent the last time we updated refs".
Separate these pieces of state and cover all these cases. Also introduce a "--rebuild" flag to fix the state of bad commits.
Test Plan:
See T13284 for the three major cases:
- initial import;
- push changes to a nonpermanent branch, update, then make it permanent;
- push chanegs to a nonpermanent branch, update, push more changes, then make it permanent.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13284
Differential Revision: https://secure.phabricator.com/D20829
Summary: Fixes T13420. Allow installs to provide username change instructions if there's someone you should contact to get this done.
Test Plan: {F6885027}
Maniphest Tasks: T13420
Differential Revision: https://secure.phabricator.com/D20828
Summary: Ref T13420. This workflow currently dead-ends for non-administrators. Instead, provide explanatory text.
Test Plan:
- Clicked "Change Username" as an administrator, same workflow as always.
- Clicked "Change Username" as a non-administrator, got explanatory text.
Maniphest Tasks: T13420
Differential Revision: https://secure.phabricator.com/D20827
Summary:
Ref T13420. These warnings are currently more severe than they need to be; weaken them.
Among other cases, the upstream supports and encourages changing usernames when users change human names.
The login/password instructions are also out of date since sessions were decoupled from usernames about a year ago.
Test Plan: Hit dialog as an administrator
Maniphest Tasks: T13420
Differential Revision: https://secure.phabricator.com/D20826
Summary: Fixes T13423. The "Query" class for conduit call logs is missing a "withIDs()" method.
Test Plan: Paged through Conduit call logs.
Maniphest Tasks: T13423
Differential Revision: https://secure.phabricator.com/D20823
Summary: Ref T13279. Facts is still fairly rough, but not broken/policy-violating, so it can be unprototyped to fix the issue where Maniphest reports (which are now driven by Facts) don't work if prototypes are disabled.
Test Plan: Viewed Maniphest reports and Project reports with prototypes on/off and Fact installed/uninstalled.
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20822
Summary:
Depends on D20818. Ref T13279. The behavior of the "burndown" chart has wandered fairly far afield; make it look more like a burndown.
Move the other thing into an "Activity" chart.
Test Plan: {F6865207}
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20819
Summary:
Ref T13279. Allow engines to choose how areas in a stacked area chart stack on top of one another.
This could also be accomplished by using multiple stacked area datasets, but datasets would still need to know if they're stacking "up" or "down" so it's probably about the same at the end of the day.
Test Plan: {F6865165}
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20818
Summary: Ref T13279. Fix some tabular stuff, draw areas better, make the "compose()" API more consistent, unfatal the demo chart, unfatal the project burndown, make the project chart do something roughly physical.
Test Plan: Looked at charts, saw fewer obvious horrors.
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20817
Summary: Depends on D20815. Ref T13279. Give datapoints "refs", which allow us to figure out where particular datapoints came from even after the point is transformed by functions. For now, show the raw points in a table below the chart.
Test Plan: Viewed chart data, saw reasonable-looking numbers.
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20816
Summary:
Depends on D20814. Currently, "min()" and "max()" are still "min(f, n)". This is no longer consistent with the construction of functions a function-generators that are composed at top level.
Turn them into "min(n)" and "max(n)" (i.e., not higher-order functions).
Then, mark all the functions which are pure mathematical functions and not higher-order as "pure". These functions have no function parameters and do not reference external data. For now, this distinction has no immediate implications, but it will simplify the next change (which tracks where data came from when it originated from an external source -- these pure functions never have any source information, since they only apply pure mathematical transformations to data).
Test Plan: Loaded a burnup chart, nothing seemed obviously broken.
Subscribers: yelirekim
Differential Revision: https://secure.phabricator.com/D20815
Summary: Ref T13279. We currently draw a point on the chart for each datapoint, but this leads to many overlapping circles. Instead, aggregate the raw points into display points ("events") at the end.
Test Plan: Viewed a stacked area chart with many points, saw a more palatable number of drawn dots.
Subscribers: yelirekim
Maniphest Tasks: T13279
Differential Revision: https://secure.phabricator.com/D20814
Summary: Ref T13411. This is a leftover from recent policy rendering changes.
Test Plan: Viewed feed with application policy stories, no more fatal.
Maniphest Tasks: T13411
Differential Revision: https://secure.phabricator.com/D20811
Summary:
Fixes T13415. Provide a way for subtypes to customize the behavior of "Change Subtype" actions that appear above comment areas.
Subtypes may disable this action by specifying `"mutations": []`, or provide a list of subtypes.
The bulk editor and API can still perform any change.
Test Plan:
- Tried to define an invalid "mutations" list with a bad subtype, got a sensible error.
- Specified a limited mutations list and an empty mutations list, verified that corresponding tasks got corresponding actions.
- Used the bulk editor to perform a freeform mutation.
- Verified that tasks of a subtype with no "mutations" still work the same way they used to (allow mutation into any subtype).
Maniphest Tasks: T13415
Differential Revision: https://secure.phabricator.com/D20810
Summary: See PHI1434. For objects that support subtypes and have subtypes configured, allow Herald rules to act on subtypes.
Test Plan:
- Configured task and project subtypes, wrote Herald rules, saw "Subtypes" as an option, saw appropriate typeahead values and detail page rendering.
- Unconfigured project subtypes, saw field vanish from UI for new rules.
- Wrote a "subtype"-depenent rule that added a comment, interacted with tasks of that subtype and a different subtype. Saw Herald act only on tasks with the correct subtype.
Differential Revision: https://secure.phabricator.com/D20809
Summary:
Fixes T7961. Currently, we present Herald users with actions like "Require legalpad signatures" and "Run build plans" even if Legalpad and Harbormaster are not installed.
Instead, allow fields and actions to be made "unavailable", which means that we won't present them as options when adding to new or existing rules.
If you edit a rule which already uses one of these fields or actions, it isn't affected.
Test Plan:
- Created a rule with a legalpad action, uninstalled legalpad, edited the rule. Action remained untouched.
- Created a new rule, wasn't offered the legalpad action.
- Reinstalled the application, saw the action again.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T7961
Differential Revision: https://secure.phabricator.com/D20808
Summary:
Fixes T13411. This looks like the last case where you hit a policy explanation and have permission to see the policy, but we don't currently show you the policy rules.
This implementation is slightly clumsy, but likely harmless.
Test Plan: {F6856421}
Maniphest Tasks: T13411
Differential Revision: https://secure.phabricator.com/D20807
Summary:
Ref T13411. Currently, if you hit a policy exception because you can't view an object, we disclose details about the view policy of the object, particularly which project's members can see the object for project policies.
Although there's a large amount of grey area here, this feels like a more substantial disclosure than we offer in other contexts. Instead, if you encounter a policy exception while testing "CAN_VIEW" or don't have "CAN_VIEW", present an "opaque" explanation which omits details that viewers who can't view the object shouldn't have access to. Today, this is the name of "Project" policies (and, implicitly, the rulesets of custom policies, which we now disclose in other similar contexts).
Test Plan:
- Hit policy exceptions for "CAN_VIEW" on an object with a project view policy, saw an opaque explanation.
- Hit policy exceptions for "CAN_EDIT" on an object with a project edit policy and a view policy I satisfied, saw a more detailed explanation.
Maniphest Tasks: T13411
Differential Revision: https://secure.phabricator.com/D20806
Summary: Ref T13411. When users click a link to explain a capability (like the policy header on many objects, or the link next to specific capabilities in "Applications", "Diffusion", etc), inline the full ruleset for the custom policy into the dialog if the object has a custom policy.
Test Plan: {F6856365}
Maniphest Tasks: T13411
Differential Revision: https://secure.phabricator.com/D20805
Summary:
Ref T13411. This cleans up policy name rendering. We ultimately render into three contexts:
- Plain text contexts, like `bin/policy show`.
- Transaction contexts, where we're showing a policy change. In these cases, we link some policies (like project policies and custom policies) but the links go directly to the relevant object or a minimal explanation of the change. We don't link policies like "All Users".
- Capability contexts, where we're describing a capability, like "Can Push" or cases in Applicaitons. In these cases, we link all policies to the full policy explanation flow.
Test Plan:
- Used `bin/policy show` to examine the policy of an object with a project policy, no longer saw HTML.
- Viewed the transaction logs of Applications (ModularTransactions) and Tasks (not ModularTransactions) with policy edits, including project and custom policies.
- Clicked "Custom Policy" in both logs, got consistent dialogs.
- Viewed application detail pages, saw all capabities linked to explanatory capability dialogs. The value of having this dialog is that the user can get a full explanation of special rules even if the policy is something mundane like "All Users".
Maniphest Tasks: T13411
Differential Revision: https://secure.phabricator.com/D20804
Summary: Ref T13411. This pathway has an unused "icon" parameter with no callsites. Throw it away to ease refactoring.
Test Plan: Grepped for callsites, found none using this parameter.
Maniphest Tasks: T13411
Differential Revision: https://secure.phabricator.com/D20803
Summary:
Ref T13411. These three applications render an "Editable By: <policy>" field in their descriptions.
The pages that these appear on all have "Edit <thing>" actions which either tell you the policy or allow you to discover the policy, and this field is unusual (the vast majority of objects don't have it). I think it largely got copy/pasted or used as space-filler and doesn't offer much of value.
Remove it to simplify/standardize these pages and make changes to how this field works simpler to implement.
Test Plan: Viewed a Credential, Blog, and Space; no longer saw the "Editable By" field.
Maniphest Tasks: T13411
Differential Revision: https://secure.phabricator.com/D20802
Summary:
Ref T13411. Since circa D19829, transactions have rendered policy changes in a modern way, notably making "Custom Policy" clickable to show the policy rules.
Edit transactions in Applications still use a separate, older approach to render policies. This produces policy renderings which don't use modern quoting rules and don't link in a modern way.
Make Applications use the same rendering code that other transactions (like normal edit/view edits) use.
Test Plan: Edited policies in Applications, saw more useful transactions in the log. Clicked "Custom Policy" in the transaction log and got a useful explanation of the policy.
Maniphest Tasks: T13411
Differential Revision: https://secure.phabricator.com/D20801
Summary:
Ref T13410. See PHI1431. Currently, when you move a document in Phriction, the target shows a "This document was moved from ..." banner until it is edited.
This banner isn't particularly useful, and it's distracting and it isn't obvious how to dismiss it, and making a trivial edit to dismiss it is awkward.
This information is also already available in the transaction log.
Just remove this banner since it doesn't really serve any clear purpose.
Test Plan:
- Moved a page in Phriction, then loaded the destination page. Before change: header banner. After change: nothing.
- Viewed a normal (non-moved) page, saw normal behavior.
- Reviewed transactions, saw "Moved from ..." in the timeline.
Maniphest Tasks: T13410
Differential Revision: https://secure.phabricator.com/D20800
Summary:
Fixes T8808. Currently, all project use the default ("Briefcase") project icon when they appear in a policy dropdown.
Since project policies are separated out into a "Members of Projects" section of the dropdown anyway, there is no reason not to use the actual project icon, which is often more clear.
Test Plan: {F6849927}
Maniphest Tasks: T8808
Differential Revision: https://secure.phabricator.com/D20799
Summary:
Fixes T9136.
- Fix a bug where the name is rendered improperly.
- Put disabled rules at the bottom.
- Always show the rule monogram so you can distingiush between rules with the same name.
Test Plan: {F6849915}
Maniphest Tasks: T9136
Differential Revision: https://secure.phabricator.com/D20798
Summary:
Fixes T8952. These feed stories are not interesting and tend to be generated as collateral damage when a non-story update is made to an old task and someone has a "subscribe me" Herald rule.
Also clean up some of the Herald field/condition indexing behavior slightly.
Test Plan: Wrote a "Subscribe X" herald rule, made a trivial update to a task. Before: low-value feed story; after: no feed story.
Maniphest Tasks: T8952
Differential Revision: https://secure.phabricator.com/D20797
Summary: Fixes T13409. This is a companion to the existing "Mark with flag" rule.
Test Plan: Used a "remove flag" rule on an object with no flag (not removed), the right type of flag (removed), and a different type of flag (not removed).
Maniphest Tasks: T13409
Differential Revision: https://secure.phabricator.com/D20796
Summary:
Fixes T13408. Currently, when a package (or other object) appears in a field (rather than an action), it is not indexed.
Instead: index fields too, not just actions.
Test Plan:
- Wrote a rule like "[ Affected packages include ] ...".
- Updated the search index.
- Saw rule appear on "Affected By Herald Rules" on the package detail page.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13408
Differential Revision: https://secure.phabricator.com/D20795
Summary: Fixes T13412. If you search for "https://phabricator.example.com" with no trailing slash, we currently redirect you to "", which is fouled by a safety check in the redirection flow.
Test Plan:
- Searched for "https://local.phacility.com"; before: fatal in redirection; after: clean redirect.
- Searched for other self-URIs, got normal redirects.
Maniphest Tasks: T13412
Differential Revision: https://secure.phabricator.com/D20794
Summary: Fixes T13405. We currently offer non-global custom saved queries here, but this doesn't make sense as a global default setting.
Test Plan: Saved a global search query, edited global search settings, no longer saw the non-global query as an option.
Maniphest Tasks: T13405
Differential Revision: https://secure.phabricator.com/D20793
Summary:
Ref T13404. Except for one known issue in Multimeter, Phabricator appears to function properly in this mode. It is broadly desirable that we run in this mode; it's good on its own, and enabled by default in at least some recent MySQL.
Additionally, "ONLY_FULL_GROUP_BY" and "STRICT_ALL_TABLES" shared a setup key, so ignoring one would ignore both. Change the key so that existing ignores on "ONLY_FULL_GROUP_BY" do not mask "STRICT_ALL_TABLES" warnings.
Test Plan: Grepped for `ONLY_FULL_GROUP_BY`.
Maniphest Tasks: T13404
Differential Revision: https://secure.phabricator.com/D20791
Summary: Ref T13404. Enabling "STRICT_ALL_TABLES" is good, but if you don't want to bother it doesn't matter too much. All upstream development has been on "STRICT_ALL_TABLES" for a long time.
Test Plan: {F6847839}
Maniphest Tasks: T13404
Differential Revision: https://secure.phabricator.com/D20790
Summary: Fixes T13406. On the logout screen, test for no configured providers and warn users they may be getting into more trouble than they expect.
Test Plan:
- Logged out of a normal install and a fresh (unconfigured) install.
{F6847659}
Maniphest Tasks: T13406
Differential Revision: https://secure.phabricator.com/D20789
Summary:
See D20779, https://discourse.phabricator-community.org/t/3089. `bin/config set` complains about
missing config file as if it's un-writable.
Test Plan: run `bin/config set` with missing, writable, unwritable conf.json and parent dir.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20788
Summary: Fixes T13405. The default behavior of the global search bar isn't currently configurable, but can be made configurable fairly easily.
Test Plan: Changed setting as an administrator, saw setting reflected as a user with no previous preference. As a user with an existing preference, saw preference retained.
Maniphest Tasks: T13405
Differential Revision: https://secure.phabricator.com/D20787
Summary: Ref T13366. The "authorities" mechanism was replaced, but I missed this callsite. Update it to use the request cache mechanism.
Test Plan: As a user without permission to view some initiatives, viewed a list of initiatives.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20783
Summary: Ref T13404. This query is invalid under "sql_mode=ONLY_FULL_GROUP_BY". Rewrite it to avoid interacting with `actorIdentity` at all; this is a little more robust in the presence of weird data and not really more complicated.
Test Plan:
- Enabled "ONLY_FULL_GROUP_BY".
- Hit system actions (e.g., login).
- Before: error.
- After: clean login.
- Tried to login with a bad password many times in a row, got properly limited by the system action rate limiter.
Maniphest Tasks: T13404
Differential Revision: https://secure.phabricator.com/D20782
Summary: Ref T13403. We currently emit a useful error message, but it's not tailored and has a stack trace. Since this is a relatively routine error and on the first-time-setup path, tailor it so it's a bit nicer.
Test Plan:
- Ran `bin/config set ...` with an unwritable "local.json".
- Ran `bin/config set ...` normally.
Maniphest Tasks: T13403
Differential Revision: https://secure.phabricator.com/D20779
Summary:
Ref T13286. The current (very safe / conservative) rules for retrying git reads generalize to git writes, so we can use the same ruleset in both cases.
Normally, writes converge rapidly to only having good nodes at the head of the list, so this has less impact than the similar change to reads, but it generally improves consistency and allows us to assert that writes which can be served will be served.
Test Plan:
- In a cluster with an up node and a down node, pushed changes.
- Saw a push to the down node fail, retry, and succeed.
- Did some pulls, saw appropriate retries and success.
- Note that once one write goes through, the node which received the write always ends up at the head of the writable list, so nodes need to be explicitly thawed to reproduce the failure/retry behavior.
Maniphest Tasks: T13286
Differential Revision: https://secure.phabricator.com/D20778
Summary: Ref T13286. When retrying a read request, keep retrying as long as we have canididate services. Since we consume a service with each attempt, there's no real reason to abort early, and trying every service allows reads to always succeed even if (for example) 8 nodes of a 16-node cluster are dead because of a severed network link between datacenters.
Test Plan: Ran `git pull` in a clustered repository with an up node and a down node; saw retry count dynamically adjust to available node count.
Maniphest Tasks: T13286
Differential Revision: https://secure.phabricator.com/D20777
Summary:
Depends on D20775. Ref T13286. When a Git read request fails against a cluster and there are other nodes we could safely try, try more nodes.
We DO NOT retry the request if:
- the client read anything;
- the client wrote anything;
- or we've already retried several times.
Although //some// requests where bytes went over the wire in either direction may be safe to retry, they're rare in practice under Git, and we'd need to puzzle out what state we can safely emit.
Since most types of failure result in an outright connection failure and this catches all of them, it's likely to almost always be sufficient in practice.
Test Plan:
- Started a cluster with one up node and one down node, pulled it.
- Half the time, hit the up node and got a clean pull.
- Half the time, hit the down node and got a connection failure followed by a retry and a clean pull.
- Forced `$err = 1` so even successful attempts would retry.
- On hitting the up node, got a "failure" and a decline to retry (bytes already written).
- On hitting the down node, got a failure and a real retry.
- (Note that, in both cases, "git pull" exits "0" after the valid wire transaction takes place, even though the remote exited non-zero. If the server gave Git everything it asked for, it doesn't seem to care if the server then exited with an error code.)
Maniphest Tasks: T13286
Differential Revision: https://secure.phabricator.com/D20776
Summary:
Ref T13286. To support request retries, allow the service lookup method to return an ordered list of structured service references.
Existing callsites continue to immediately discard all but the first reference and pull a URI out of it.
Test Plan: Ran `git pull` in a clustered repository with an "up" node and a "down" node, saw 50% serivce failures and 50% clean pulls.
Maniphest Tasks: T13286
Differential Revision: https://secure.phabricator.com/D20775
Ref T13401. The checkout UI didn't get fully updated to the new View objects,
and account handles are still manually building a URI that goes to the wrong
place.
Summary:
Ref T13393. See some previous discussion in T13366.
Caching is hard and all approaches here have downsides, but the request cache likely has fewer practical downsides for this kind of policy check than other approaches. In particular, the grant approach (at least, as previously used in Phortune) has a major downside that "Query" classes can no longer fully enforce policies.
Since Phortune no longer depends on grants and they've now been removed from instances, drop the mechanism completely.
Test Plan: Grepped for callsites, found none.
Maniphest Tasks: T13393
Differential Revision: https://secure.phabricator.com/D20754
Summary:
Ref T13393. While doing a shard migration in the Phacility cluster, we'd like to stop writes to the migrating repository. It's safe to continue serving reads.
Add a simple maintenance mode for making repositories completely read-only during maintenance.
Test Plan: Put a repository into read-only mode, tried to write via HTTP + SSH. Viewed web UI. Took it back out of maintenance mode.
Maniphest Tasks: T13393
Differential Revision: https://secure.phabricator.com/D20748
Summary:
Fixes T13336.
- Prevent `--no-indexes` from being combined with `--for-replica`, since combining these options can only lead to heartbreak.
- In `--for-replica` mode, dump caches too. See discussion in T13336. It is probably "safe" to not dump these today, but fragile and not correct.
- Mark the "MarkupCache" table as having "Cache" persistence, not "Data" persistence (no need to back it up, since it can be fully regenerated from other datasources).
Test Plan: Ran `bin/storage dump` with various combinations of flags.
Maniphest Tasks: T13336
Differential Revision: https://secure.phabricator.com/D20743
Summary:
Fixes T13389. Currently, we try to "newSubtypeMap()" unconditionally, even if the underlying object does not support subtypes.
- Only try to build a subtype map if subtype transactions are actually being applied.
- When subtype transactions are applied to a non-subtypable object, fail more explicitly.
Test Plan: Clicked "Make Editable" in a fresh Calendar transaction form, got an editable form instead of a fatal from "newSubtypeMap()". (Calendar events are not currently subtypable.)
Maniphest Tasks: T13389
Differential Revision: https://secure.phabricator.com/D20741
Summary:
Depends on D20739. Ref T13366. Slightly modularize/update components of order views, and make orders viewable from either an account context (existing view) or an external context (new view).
The new view is generally simpler so this mostly just reorganizes existing code.
Test Plan: Viewed orders as an account owner and an external user.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20740
Summary: Depends on D20738. Ref T13366. Fixes T8389. Now that the infrastructure is in place, actually send email to external addresses.
Test Plan: Used `bin/phortune invoice` to generate invoices and saw associated external accounts receive mail in `bin/mail list-outbound`.
Maniphest Tasks: T13366, T8389
Differential Revision: https://secure.phabricator.com/D20739
Summary: Depends on D20737. Ref T13367. Allow external addresses to have their access key rotated. Account managers can disable them, and anyone with the link can permanently unsubscribe them.
Test Plan: Enabled/disabled addresses; permanently unsubscribed addresses.
Maniphest Tasks: T13367
Differential Revision: https://secure.phabricator.com/D20738
Summary: Ref T13366. This gives each account email address an "external portal" section so they can access invoices and receipts without an account.
Test Plan: Viewed portal as user with authority and in an incognito window.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20737
Summary:
Depends on D20734. Ref T13366. This makes the cart/order flow work under the new policy scheme with no "grantAuthority()" calls.
It prepares for a "Void Invoice" action, although the action doesn't actually do anything yet.
Test Plan: With and without merchant authority, viewed and paid invoices and went through the other invoice interaction workflows.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20735
Summary: Refresh the 404 text since it hasn't been updated in a while, and swap the "Save Query" button back to grey since I never got used to blue.
Test Plan: Hit 404 page, saved a query.
Differential Revision: https://secure.phabricator.com/D20734
Summary:
Depends on D20732. Ref T13366. This generally makes the "Merchant" UI look and work like the "Payment Account" UI.
This is mostly simpler since the permissions have largely been sorted out already and there's less going on here and less weirdness around view/edit policies.
Test Plan: Browsed all Merchant functions as a merchant member and non-member.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20733
Summary:
Ref T13366. Depends on D20721. Continue applying UI and policy updates to the last two Phortune objects.
Charges aren't mutable and Carts are already transactional, so this is less involved than prior changes.
Test Plan: Viewed various charge/order interfaces as merchants and account members.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20732
Summary:
Depends on D20720. Ref T13366.
- Use modern policies and policy interfaces.
- Use new merchant authority cache.
- Add (some) transactions.
- Move MFA from pre-upgrade-gate to post-one-shot-check.
- Simplify the autopay workflow.
- Use the "reloading arrows" icon for subscriptions more consistently.
Test Plan: As a merchant-authority and account-authority, viewed, edited, and changed autopay for subscriptions.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20721
Summary:
Depends on D20718. Ref T13366. Ref T13367.
- Phortune payment methods currently do not use transactions; update them.
- Give them a proper view page with a transaction log.
- Add an "Add Payment Method" button which always works.
- Show which subscriptions a payment method is associated with.
- Get rid of the "Active" status indicator since we now treat "disabled" as "removed", to align with user expectation/intent.
- Swap out of some of the super weird div-form-button UI into the new "big, clickable" UI for choice dialogs among a small number of options on a single dimension.
Test Plan:
- As a mechant-authority and account-authority, created payment methods from carts, subscriptions, and accounts. Edited and viewed payment methods.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13367, T13366
Differential Revision: https://secure.phabricator.com/D20719
Summary:
Depends on D20717. Ref T13366. Make PhortunePaymentMethod use an extended policy interface for consistency with modern approaches. Since Accounts have hard-coded policy behavior (and can't have object policies like "Subscribers") this should have no actual impact on program behavior.
This leaves one weird piece in the policy dialog UIs, see T13381.
Test Plan: Viewed and edited payment methods as a merchant and account member. Merchants can only view, not edit.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20718
Summary: Depends on D20716. Ref T13366. This implements the new policy behavior cleanly in all top-level Phortune payment account interfaces.
Test Plan: As a merchant with an account relationship (not an account member) and an account member, browsed all account interfaces and attempted to perform edits. As a merchant, saw a reduced-strength view.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20717
Summary:
Depends on D20715. Ref T13366. See that task for discussion.
Replace the unreliable "grantAuthority()"-based check with an actual "can the viewer edit any merchant this account has a relationship with?" check.
This makes these objects easier to use from a policy perspective and makes it so that the `Query` alone can fully enforce permissions properly with no setup, so general infrastructure (like handles and transactions) works properly with Phortune objects.
Test Plan: Viewed merchants and accounts as users with no authority, direct authority on the account, and indirect authority via a merchant relationship.
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20716
Summary:
Depends on D20713. Ref T13366. When a payment account establishes a relationship with a merchant by creating a cart or subscription, create an edge to give the merchant access to view the payment account.
Also, migrate all existing subscriptions and carts to write these edges.
This aims at straightening out Phortune permissions, which are currently a bit wonky on a couple of dimensions. See T13366 for detailed discussion.
Test Plan:
- Created and edited carts/subscriptions, saw edges write.
- Ran migrations, saw edges write.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20715
Summary: Depends on D20697. Ref T8389. Add support for adding "billing@enterprise.com" and similar to Phortune accounts.
Test Plan: Added and edited email addresses for a payment account.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T8389
Differential Revision: https://secure.phabricator.com/D20713
Summary:
Ref T13366. Some of the information architecture is a little muddy here, notably an item called "Billing / History" which contains payment methods.
Split things up a bit to prepare for adding support for "Email Addresses".
Test Plan: {F6676988}
Maniphest Tasks: T13366
Differential Revision: https://secure.phabricator.com/D20697
Summary: See PHI1396. Ideally this would be some kind of general-purpose tie-in to object relationships, but see D18456 for precedent.
Test Plan: Used `maniphest.edit` to edit associated commits for a task.
Differential Revision: https://secure.phabricator.com/D20731
Summary:
Fixes T13386. See PHI1391. These constraints largely exist already, but are not yet exposed to Conduit.
Also, tweak some keys to support the underlying query.
Test Plan: Ran `differential.revision.search` queries with the new constraints.
Maniphest Tasks: T13386
Differential Revision: https://secure.phabricator.com/D20730
Summary: Ref T13382. Currently, the "Make Administrator" action in the web UI does state-based MFA. Convert it to one-shot MFA.
Test Plan: Empowered and unempowered a user from the web UI, got one-shot MFA'd. Empowered a user from the CLI, no MFA issues.
Maniphest Tasks: T13382
Differential Revision: https://secure.phabricator.com/D20729
Summary:
Ref T13385. Currently, if you run `arc diff` in a CWD with more than 255 characters, the workflow fatals against the length of the `sourcePath` database column.
In the long term, removing this property is likely desirable.
For now, truncate long values and continue. This only meaningfully impacts relatively obscure interactive SVN workflows negatively, and even there, "some arc commands are glitchy in very long working directories in SVN" is still better than "arc diff fatals".
Test Plan:
- Modified `arc` to submit very long source paths.
- Ran `arc diff`.
- Before: Fatal when inserting >255 characters into `sourcePath`.
- After: Path truncated at 255 bytes.
Maniphest Tasks: T13385
Differential Revision: https://secure.phabricator.com/D20727
Summary:
Ref T13382.
- Remove "bin/people profileimage" which previously generated profile image caches but now feels obsolete.
- Replace it with "bin/user", with "enable" and "empower" flows. This command is now focused on regaining access to an install after you lock your keys inside.
- Document the various ways to unlock objects and accounts from the CLI.
Test Plan:
- Ran `bin/user enable` and `bin/user empower` with various flags.
- Grepped for `people profileimage` and found no references.
- Grepped for `bin/people` and found no references.
- Read documentation.
Maniphest Tasks: T13382
Differential Revision: https://secure.phabricator.com/D20724
Summary: Fixes T13383. Provide a basic "drydock.resource.search". Also allow "drydock.lease.search" to be queried by resource PHID.
Test Plan: Called "drydock.resource.search" and "drydock.lease.search" with various constraints.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13383
Differential Revision: https://secure.phabricator.com/D20723
Summary:
Fixes T13378. If we join Ferret tables and page, we can end up with an ambiguous `id` column here.
Explicitly refer to "project.x" in all cases that we're interacting with the project table.
Test Plan:
- Changed page size to 3.
- Issued a Projects query for "~e", matching more than 3 results.
- Clicked "Next Page".
- Before: ambiguous id column fatal.
- After: next page.
Maniphest Tasks: T13378
Differential Revision: https://secure.phabricator.com/D20714
Summary:
Ref T13369. See that task for discussion.
When the discovery daemon finds more than 64 commits to import, demote the worker queue priority of the resulting tasks.
Test Plan:
- Pushed one commit, ran `bin/repository discover --verbose --trace ...`, saw commit import with "at normal priority" message and priority 2500 ("PRIORITY_COMMIT").
- Pushed 3 commits, set threshold to 3, ran `bin/repository discover ...`, saw commist import with "at lower priority" message and priority 4000 ("PRIORITY_IMPORT").
Maniphest Tasks: T13369
Differential Revision: https://secure.phabricator.com/D20712
Summary:
Ref T13373. When you "bin/config set x ..." a value, the success message ("Set x ...") is somewhat ambiguous and can be interpreted as "First, you need to set x..." rather than "Success, wrote x...".
Make the messaging more explicit. Also make this string more translatable.
Test Plan: Ran `bin/config set ...` with various combinations of flags, saw more clear messaging.
Maniphest Tasks: T13373
Differential Revision: https://secure.phabricator.com/D20711
Summary: Fixes T13374. The "Temporary Failures" row is missing a cell definiton from the addition of "Average Queue Time".
Test Plan: Viewed "/daemon/" with some temporary failures and and odd number of rows above the "Temporary Failures" row. Saw cell properly zebra-striped.
Maniphest Tasks: T13374
Differential Revision: https://secure.phabricator.com/D20710
Summary:
Ref T13349. This is almost the same change as D20678, but for project profiles instead of user profiles.
The general reproduction case is "view a project where you can't see more than 50 of the 500 most recent feed stories".
Test Plan:
- Forced all queries to overheat.
- Viewed a project profile page.
- Before: overheating fatal near top level.
- After: damage contained to feed panel.
Maniphest Tasks: T13349
Differential Revision: https://secure.phabricator.com/D20704
Summary: See downstream <https://phabricator.wikimedia.org/T229757>. The "autofocus" attribute mostly just works, so add it to this input.
Test Plan: As a user with TOTP enabled, established a new session. Saw browser automatically focus the "App Code" input on the TOTP prompt screen.
Differential Revision: https://secure.phabricator.com/D20703
Summary:
Fixes T13370. We currently show an "Award Badge" button conditionally, based on whether the viewer can award any badges or not.
The query to test this may overheat and this pattern isn't consistent with other UI anyway. Stop doing this test.
Test Plan:
- Created 12 badges.
- As a user who could not edit any of the badges, viewed the "Badges" section of a user profile.
Maniphest Tasks: T13370
Differential Revision: https://secure.phabricator.com/D20702
Summary: Fixes T13368. Some workflows (like "Move tasks to...") execute board layout without objects to update. In these cases, we can hit a warning because `objectPHIDs` is not initialized to `array()`.
Test Plan: Went through the "Move tasks to..." workflow on a workboard, no longer saw a warning when trying to iterate over an empty `objectPHIDs` list.
Maniphest Tasks: T13368
Differential Revision: https://secure.phabricator.com/D20701
Summary:
Ref T13368. Currently, both visible and hidden columns are shown in the "Move tasks to..." dropdown on workflows from workboards.
When the dropdown contains hidden columns, move them to a separate section to make it clear that they're not likely targets.
Test Plan:
- Used "Move tasks to project..." targeting a board with no hidden columns. Saw a single ungrouped dropdown.
- Used "Move tasks to project..." targeting a board with hidden columns. Saw a dropdown grouped into "Visible" and "Hidden" columns.
Maniphest Tasks: T13368
Differential Revision: https://secure.phabricator.com/D20700
Summary:
Ref T13368. Proxy columns should not be selectable from this workflow. If you want to move tasks to milestone/subproject X, do "Move tasks to project..." and pick X as the project.
(This could be made to work some day.)
Test Plan: Went through a "Move tasks to project..." workflow targeting a project with subprojects. No longer saw subproject columns presented as dropdown options.
Maniphest Tasks: T13368
Differential Revision: https://secure.phabricator.com/D20699
Summary: Ref T13368. The column options presented to the user are currently incorrect because the wrong set of columns are drawn from.
Test Plan: On a workboard, used "Move tasks to project..." to target another board, saw that board's columns.
Maniphest Tasks: T13368
Differential Revision: https://secure.phabricator.com/D20698
Summary:
Fixes T13341. Currently, cart emails (invoices/receipts) are sent to members of the associated merchant account. This was just a simple way to keep an eye on things when this was first written.
The system works fine, and recent changes (almost certainly D20525) stopped these emails from working (presumably because of the slightly weird merchant permissions model).
This could be sorted out in more detail, but it looks like the path forward is to introduce a side channel for email anyway (via T8389), and that's a better way to implement this behavior since it means the normal recipients won't see a bunch of random staff/merchant email addresses on their receipts.
Test Plan: Grepped for `merchant` in this editor.
Maniphest Tasks: T13341
Differential Revision: https://secure.phabricator.com/D20696
Summary:
Ref T13339. If a search pattern matches more than once on a line, we currently render the line incorreclty, duplicating some of the text.
`substr()` is being called as though the third parameter was `end_offset`, but it's actually `length`. Correct the parameter.
Test Plan:
Before:
{F6676625}
After:
{F6676623}
Maniphest Tasks: T13339
Differential Revision: https://secure.phabricator.com/D20695
Summary:
Fixes T8830. Fixes T13364.
- The inability to destroy objects from the web UI is intentional. Make this clear in the messaging, which is somewhat out of date and partly reflects an earlier era when things could be destroyed.
- `bin/remove destroy` can't rewind time. Document expectations around the "put the cat back in the bag" use case.
Test Plan: Read documentation, clicked through both workflows.
Maniphest Tasks: T13364, T8830
Differential Revision: https://secure.phabricator.com/D20694
Summary: Ref T13358. This is very minimal, but technically works. The eventual goal is to generate PDF invoices to make my life easier when I have to interact with Enterprise Vendor Procurement.
Test Plan: {F6672439}
Maniphest Tasks: T13358
Differential Revision: https://secure.phabricator.com/D20692
Summary:
Fixes T13356. This option is supported and works fine, it just isn't documented.
Add documentation and fix the config option to actually link to it to make life a little easier.
Test Plan: Read documentation.
Maniphest Tasks: T13356
Differential Revision: https://secure.phabricator.com/D20691
Summary: Fixes T13355. This didn't appear to be a ton of extra work, we just didn't get it for free in the original implementation in D14635.
Test Plan:
- Saw "date" custom fields appear in Conduit API documentation for "maniphest.edit".
- Set custom "date" field to null and non-null values via the API.
{F6666582}
Maniphest Tasks: T13355
Differential Revision: https://secure.phabricator.com/D20690
Summary: Ref T4900. We may execute a bad query here if the task has no projects at all.
Test Plan: Edited a task with no new or old projects. Instead of an exception, things worked.
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20689
Summary:
Fixes T13353. If you:
- Visit a blog post and save the URI.
- Move the blog post to a different blog.
- Revisit the old URI.
...we currently 404. We know what you're trying to do and should just redirect you to the new URI instead. We already do this if you visit a URI with a noncanonical slug.
Test Plan:
- Created post A.
- Copied the live URI.
- Moved it to a different blog.
- Visited the saved URI from the earlier step.
- Before: 404.
- After: Redirect to the canonical URI.
Maniphest Tasks: T13353
Differential Revision: https://secure.phabricator.com/D20688
Summary: Depends on D20686. Fixes T13350. Now that "slowvote.poll.search" exists, deprecate this old method.
Test Plan: Reviewed method description in Condiut API console in the web UI.
Maniphest Tasks: T13350
Differential Revision: https://secure.phabricator.com/D20687
Summary:
Depends on D20685. Ref T13350. Currently:
- When a SearchEngine parameter is marked as hidden from Conduit, we may still render a table of possible values. Instead, only render the table if the parameter is actually usable.
- The table header is hard-coded to say `'statuses'`, which is just a silly mistake. (Most commonly, this table does have `statuses` constants.)
Test Plan: Viewed the Conduit API documentation for the new "slowvote.poll.search" API method, saw more sensible display behavior.
Maniphest Tasks: T13350
Differential Revision: https://secure.phabricator.com/D20686
Summary: Ref T13350. Add a modern "*.search" API method for Slowvote so "slowvote.info" can be deprecated with a reasonable replacement.
Test Plan: Used Conduit test console to call method, saw reasonable results.
Maniphest Tasks: T13350
Differential Revision: https://secure.phabricator.com/D20685
Summary:
Depends on D20680. Ref T4900. The "BoardLayoutEngine" operates on PHIDs without knowledge of the underlying objects, but this means it has to be sensitive to PHID input order when falling back to a default layout order.
We use "default layout order" on workboards which are sorted by "Natual" order but which have one or more cards which no user has ever reordered. For example, if you add 10 tasks to a project, then create a board, there's no existing order for those tasks in the "Backlog" column. The layout engine uses the input order to place them in the column, with the expectation that input order is ID/creation order, so new cards will end up on top.
I think this code never really made an explicit effort to guarantee that the LayoutEngine received objects in ID order, and it just sort of happened to by coincidence and good fortune. Some recent change has disrupted this, so the edit operation can end up with the PHIDs arranged in arbitrary order.
Explicitly put them in ID order so we always get an implicit default layout order to fall back to. Also, update to `msortv()`.
Test Plan:
- Tagged several tasks with project X, a project without a board yet.
- Created the project X workboard.
- (Did not drag any tasks around on the project X board!)
- Viewed the board in "Natural" order.
This creates a view of the board where tasks are ordered by implicit/virtual/input order. The expectation, and "view" behavior of this board, is that this order is "newest on top".
- Edited one of the cards on the board, changing the title (don't reorder it!)
- Before: page state synchronized with cards in arbitrary/random/different order.
- After: page state synchronized with cards in the same order as before ("newest on top").
Reviewers: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20681
Summary:
Ref T4900. When a card is edited, we currently emit an update notification for all the projects the task is tagged with. This isn't quite the right set:
- We want to emit notifications for projects the task //was previously// tagged with, so it can be removed from boards it should no longer be part of.
- We want to emit notifications for ancestors of projects the task is or was tagged with, so parent project boards can be updated.
- However, we don't need to emit notifications for projects that don't actually have workboards.
Adjust the notification set to align better to these rules.
Test Plan:
- Removal of Parent Project: Edited a task on board "A > B", removing the "B" project tag. Saw board A update in another window.
- Normal Update: Edited a task title on board X, saw board X update in another window.
- Used `bin/aphlict debug` to inspect the notification set, saw generally sensible-seeming data going over the wire.
Reviewers: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20680
Summary: Ref T13350. This ancient API method is missing modern policy checks.
Test Plan:
- Set visibility of vote X to "Only: epriestley".
- Called "slowvote.info" as another user.
- Before: retrieved poll title and author.
- After: policy error.
- Called "slowvote.info" on a visible poll, got information before and after.
Maniphest Tasks: T13350
Differential Revision: https://secure.phabricator.com/D20684
Summary:
Fixes T13348. Currently, the Harbormaster UI shows "Restart All Builds", but it really means "Restart Restartable Builds", which is often fewer than "All" builds (because of autobuilds, permissions, and/or configuration).
Remove the misleading term "All" and make the workflow preview exactly which builds will and will not be affected, and why.
Test Plan:
{F6636313}
{F6636314}
{F6636315}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13348
Differential Revision: https://secure.phabricator.com/D20679
Summary:
Depends on D20673. Ref T13343. Since we're now putting log IDs in email, make the UI a little better for working with log IDs.
Some day, this page might have actions like "report this as suspicious" or whatever, but I'm not planning to do any of that for now.
Test Plan: {F6608631}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20674
Summary:
Depends on D20672. Ref T13343. When a user requests an account access link via email:
- log it in the activity log; and
- reference the log in the mail.
This makes it easier to ban users misusing the feature, provided they're coming from a single remote address, and takes a few steps down the pathway toward a button in the mail that users can click to report the action, suspend account recovery for their account, etc.
Test Plan:
- Requested an email recovery link.
- Saw request appear in the user activity log.
- Saw a reference to the log entry in the mail footer.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20673
Summary: Depends on D20671. Ref T13343. Now that log types are modular, provide a datasource/tokenizer for selecting them since we already have a lot (even after I purged a few in D20670) and I'm planning to add at least one more ("Request password reset").
Test Plan: {F6608534}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20672
Summary:
Depends on D20670. Ref T13343. The user activity message log types are currently hard-coded, so only upstream code can really use the log construct.
Under the theory that we're going to keep this log around going forward (just focus it a little bit), modularize things so the log is extensible.
Test Plan:
Grepped for `UserLog::`, viewed activity logs in People and Settings.
(If I missed something here -- say, misspelled a constant -- the effect should just be that older logs don't get a human-readable label, so stakes are very low.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20671
Summary: Fixes T13349. If the user profile page feed query overheats, it currently takes the whole page with it. Contain the blast to a smaller radius.
Test Plan: {F6633322}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13349
Differential Revision: https://secure.phabricator.com/D20678
Summary: Humble user cannot silence/mute project if he/she has no CAN_EDIT permissions in it. You can actually leave it but if project is locked - then you're scr*wed.
Test Plan:
1. On a testing phabricator instance created a dummy project
2. Changed that project permissions CAN_EDIT to be by admin only
3. Added poor soul with no CAN_EDIT permissions
4. Logged it in with poor soul
5. Tried to silence the project
6. The Project is successfully silenced
7. User is happy :)
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, Pawka
Differential Revision: https://secure.phabricator.com/D20675
Summary:
Depends on D20669. Ref T13343. Currently, the user activity log includes a number of explicit administrative actions which some administrator (not a normal user or a suspicious remote address) takes. In most/all cases, these changes are present in the user profile transaction log too, and that's //generally// a better place for them (for example, it doesn't get GC'd after a couple months).
Some of these are so old that they have no writers (like DELETE and EDIT). I'd generally like to modernize this a bit so we can reference it in email (see T13343) and I'd like to modularize the event types as part of that -- partly, cleaning this up makes that modularization easier.
There's maybe some hand-wavey argument that administrative vs non-administrative events could be related and might be useful to see in a single log, but I can't recall a time when that was actually true, and we could always build that kind of view later by just merging the two log sources, or by restoring double-writes for some subset of events. In practice, I've used this log mostly to look for obvious red flags when users report authentication difficulty (e.g., many unauthorized login attempts), and removing administrative actions from the log is only helpful in that use case.
Test Plan: Grepped for all the affected constants, no more hits in the codebase.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20670
Summary: Depends on D20668. Ref T13343. Just an easy cleanup/simplification while I'm here.
Test Plan: `grep` for `getActionConstant()`
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20669
Summary:
Depends on D20667. Ref T13343. Password auth currently uses an older rate limiting mechanism, upgrade it to the modern "SystemAction" mechanism.
This mostly just improves consistency, although there are some tangential/theoretical benefits:
- it's not obvious that making the user log GC very quickly could disable rate limiting;
- if we let you configure action limits in the future, which we might, this would become configurable for free.
Test Plan:
- With CAPTCHAs off, made a bunch of invalid login attempts. Got rate limited.
- With CAPTCHAs on, made a bunch of invalid login attempts. Got downgraded to CAPTCHAs after a few.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20668
Summary:
Depends on D20666. Ref T13343. In D20666, I limited the rate at which a given user account can be sent account recovery links.
Here, add a companion limit to the rate at which a given remote address may request recovery of any account. This limit is a little more forgiving since reasonable users may plausibly try multiple variations of several email addresses, make typos, etc. The goal is just to hinder attackers from fishing for every address under the sun on installs with no CAPTCHA configured and no broad-spectrum VPN-style access controls.
Test Plan: {F6607846}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20667
Summary:
Depends on D20665. Ref T13343. We support CAPTCHAs on the "Forgot password?" flow, but not everyone configures them (or necessarily should, since ReCAPTCHA is a huge external dependency run by Google that requires you allow Google to execute JS on your domain) and the rate at which any reasonable user needs to take this action is very low.
Put a limit on the rate at which account recovery links may be generated for a particular account, so the worst case is a trickle of annoyance rather than a flood of nonsense.
Test Plan: {F6607794}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20666
Summary:
Depends on D20664. Ref T13343. There's a reasonable value for the default "Email Login" auth message (generic "you reset your password" text) that installs may reasonably want to replace. Add support for a default value.
Also, since it isn't completely obvious where this message shows up, add support for an extended description and explain what's going on in more detail.
Test Plan:
- Viewed message detail page, saw more detailed information.
- Sent mail (got default), overrode message and sent mail (got custom message), deleted message (got default again).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20665
Summary:
Depends on D20663. Ref T13343. Currently, if an Auth message hasn't been customized yet, clicking the message type takes you straight to an edit screen to create a message.
If an auth message has already been customized, you go to a detail screen instead.
Since there's no detail screen on the "create for the first time" flow, we don't have anywhere to put a more detailed description or a preview of a default value.
Add a view screen that works if a message is "empty" so we can add this stuff.
(The only reason we don't already have this is that it took a little work to build; this also generally improves the consistency and predictability of this interface.)
Test Plan: {F6607665}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20664
Summary:
Depends on D20662. Ref T13343. Installs may reasonably want to change the guidance users receive in "Email Login"/"Forgot Password" email.
(In an upcoming change I plan to supply a piece of default guidance, but Auth Messages need a few tweaks for this.)
There's probably little reason to provide guidance on the "Set Password" flow, but any guidance one might issue on the "Email Login" flow probably doesn't make sense on the "Set Password" flow, so I've included it mostly to make it clear that this is a different flow from a user perspective.
Test Plan:
- Set custom "Email Login" and "Set Password" messages.
- Generated "Email Login" mail by using the "Login via email" link on the login screen.
- Generated "Set Password" email by trying to set a password on an account with no password yet.
- Saw my custom messages in the resulting mail bodies.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20663
Summary:
Ref T13343. This makes "Password Reset" email a little more consistent with other modern types of email. My expectation is that this patch has no functional changes, just organizes code a little more consistently.
The new `setRecipientAddress()` mechanism deals with the case where the user types a secondary (but still verified) address.
Test Plan:
- Sent a normal "login with email" email.
- Sent a "login with email to set password" email by trying to set a password on an account with no password yet.
- Tried to email reset a bot account (no dice: they can't do web logins so this operation isn't valid).
- Tested existing "PeopleMailEngine" subclasses:
- Created a new user and sent a "welcome" email.
- Renamed a user and sent a "username changed" email.
- Reviewed all generated mail with `bin/mail list-outbound`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20662
Summary:
See D20650. Long ago, this got added as "pastebin", but that's the name of another product/company, not a generic term for paste storage.
Rename the database to `phabricator_paste`.
(An alternate version of this patch would rename `phabricator_search` to `phabricator_bing`, `phabricator_countdown` to `phabricator_spacex`, `phabricator_pholio` to `phabricator_adobe_photoshop`, etc.)
Test Plan:
- Grepped for `pastebin`, now only found references in old patches.
- Applied patches.
- Browsed around Paste in the UI without encountering issues.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20661
Summary:
Depends on D20654. Ref T4900. When a task is edited, emit a "workboards" event for all boards it appears on (in a future change, this should also include all boards it //previously// appeared on, and all parents of both sets of boards -- but I'm just getting things working for now).
When we receive a "workboards" event, check if the visible board should be updated.
Aphlict has a complicated intra-window leader/follower election system which could let us process this update event exactly once no matter how many windows a user has open with the same workboard. I'm not trying to do any of this since it seems fairly rare. It makes sense for events like "you have new notifications" where we don't want to generate 100 Ajax calls if the user has 100 windows open, but very few users seem likely to have 100 copies of the same workboard open.
Test Plan:
- Ran `bin/aphlict debug`.
- Opened workboard A in two windows, X and Y.
- Edited and moved tasks in window X.
- Saw "workboards" messages in the Aphlict log.
- Saw window Y update in nearly-real-time (locally, this is fast enough that it feels instantaneous).
Then:
- Stopped the Aphlcit server.
- Edited a task.
- Started the Aphlict server.
- Saw window Y update after a few moments (i.e., update in response to a reconnect).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20656
Summary:
Fixes T13342. This does a few different things, although all of them seem small enough that I didn't bother splitting it up:
- Support export of "remarkup" custom fields as text. There's some argument here to export them in some kind of structure if the target is JSON, but it's hard for me to really imagine we'll live in a world some day where we really regret just exporting them as text.
- Support export of "date" custom fields as dates. This is easy except that I added `null` support.
- If you built PHP from source without "--enable-zip", as I did, you can hit the TODO in Excel exports about "ZipArchive". Since I had a reproduction case, test for "ZipArchive" and give the user a better error if it's missing.
- Add a setup check for the "zip" extension to try to avoid getting there in the first place. This is normally part of PHP so I believe users generally won't hit it, I just hit it because I built from source. See also T13232.
Test Plan:
- Added a custom "date" field. On tasks A and B, set it to null and some non-null value. Exported both tasks to Excel/JSON/text, saw null and a date, respectively.
- Added a custom "remarkup" field, exported some values, saw the values in Excel.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13342
Differential Revision: https://secure.phabricator.com/D20658
Summary:
Depends on D20653. Ref T4900. Pass ordering details to the reload endpoint so it can give the client accurate ordering/header information in the response.
The removed comment mentions this, but here's why this is a difficult mess:
- In window A, view a board with "Group by: Owner" and no tasks owned by "Alice". Since "Alice" owns no tasks, this means the columns do not have an "Assigned to: Alice" header!
- In window B, edit task T and assign it to Alice.
- In window A, press "R".
Window A now not only needs to update to properly reflect the state of task T, it actually needs to draw a new "Assigned to: Alice" header in every column.
Fortunately, the "group by" code anticipates this being a big mess, is fairly careful about handling it, and the client can handle this state change and the actual code change here isn't too involved. This is just causing a lot of not-very-obvious indirect effects in the pipeline to handle these situations that need complex redraws.
Test Plan:
- After making various normal edits/creates/moves in window A, pressed "R" in window B. Saw ordering reflected correctly after sync.
- Went through the whole "Group by: Owner" + assign to unrepresented owner flow above. After pressing "R", saw "Assigned to: Alice" appear on the board.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20654
Summary:
Depends on D20652. Ref T4900. When the user presses "R", send a list of cards currently visible on the client and their version numbers.
On the server:
- Compare the client verisons to the server versions so we can skip updates for objects which have not changed. (For now, the client version is always "1" and the server version is always "2", so this doesn't do anything meaningful, and every card is always updated.)
- Compare the client visible set to the server visible set and "remove" any cards which have been removed from the board.
I believe this means that "R" always puts the board into the right state (except for some issues with client orderings not being fully handled yet). It's not tremendously efficient, but we can make versioning better (using the largest object transaction ID) to improve that and loading the page in the first place doesn't take all that long so even sending down the full visible set shouldn't be a huge problem.
Test Plan:
- In window A, removed a card from a board.
- In window B, pressed "R" and saw the removal reflected on the client.
- (Also added cards, edited cards, etc., and didn't catch anything exploding.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20653
Summary:
Depends on D20639. Ref T4900. Currently, "BoardResponseEngine" has a `setObjectPHID()` method. This is called after edit operations to mean "we just edited object X, so we know it needs to be updated".
Move toward `setUpdatePHIDs(...)` in all cases, with `setUpdatePHIDs(array(the-object-we-just-edited))` as a special case of that. After this change, callers pass:
- An optional list of PHIDs they know need to be updated on the client. Today, this is always be a card we just edited (on edit/move flows), or a sort of made-up list of PHIDs for the moment (when you press "R"). In the future, the "R" endpoint will do a better job of figuring out a more realistic update set.
- An optional list of PHIDs currently visible on the client. This is used to update ordering details and mark cards for removal. This is currently passed by edit/move, but not by pressing "R" (it will be in the future).
- An optional list of objects. The "R" workflow has to load these anyway, so we can save a couple queries by letting callers pass them. For now, the edit/move flows still rely on the engine to figure out what it needs to load.
This does very little to actually change client behavior, it mostly just paves the way for the next update to the "R" workflow to make it handle add/remove cases properly.
Test Plan:
- Edited and moved cards on a workboard.
- Pressed "R" to reload a workboard.
Neither of these operations seem any worse off than they were before. They still don't fully work:
- When you edit a card and delete the current workboard project from it, it remains visible. This is also the behavior on `master`. This is sort of intentional since we don't necessarily want to make these cards suddenly disappear? Ideally, we would probably have some kind of "tombstone" state where the card can still be edited but can't be dragged, and the next explicit user interaction would clean up old tombstones. This interaction is very rare and I don't think it's particularly important to specialize.
- When a card is removed from the board, "R" can't currently figure out that it should be removed from the client. This is because the client does not yet pass a "visiblePHIDs" state. It will in an upcoming change.
- The "R" flow always sends a full set of card updates, and can not yet detect that some cards have not changed.
- There's a TODO, but some ordering stuff isn't handled yet.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20652
Summary:
Depends on D20638. Ref T4900. This is an incremental step toward proper workboard updates.
Currently, the client can mostly update its view because we do updates when you edit or move a card, and the client and server know how to send lists of card updates, so a lot of the work is already done.
However, the code assumes we're only updating/redrawing one card at a time. Make the client accept and process multiple card updates.
In future changes, I'll add versioning (so we only update cards that have actually changed), fix the "TODO" around ordering, and move toward actual Aphlict-based real-time updates.
Test Plan:
- Opened the same workboard in two windows.
- Edited cards in one window, pressed "R" (capital letter, with no modifier keys) to reload the second window.
- Saw edits and moves reflected accurately after sync, except for some special cases of header/order interaction (see "TODO").
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20639
Summary: I was poking around in `PhabricatorAuthProviderViewController` and noticed that none of the subclass-specific rendering was working. Figured out that no one ever calls `PhabricatorAuthProviderConfigTransaction->setProvider()`, so instead of adding all those calls, just pull the provider out of the config object.
Test Plan:
Before: {F6598145}
After: {F6598147}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20655
Summary: Ref T2784. These are lookin' pretty stable. Subclasses like `DiffusionGetLintMessagesConduitAPIMethod` have their warnings about unstable methods, so just remove this warning in the base class.
Test Plan: Loaded `/conduit`, observed lack of unstable warnings. Only unstable methods are now `diffusion.getlintmessages`, `diffusion.looksoon`, and `diffusion.updatecoverage`.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T2784
Differential Revision: https://secure.phabricator.com/D20651
Summary: Forgot to post this after D20394. Fixes T7667.
Test Plan:
* Edited some providers with the config locked and unlocked.
* Opened the edit form with the config unlocked, locked the config, then saved, and got a sensible error: {F6576023}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T7667
Differential Revision: https://secure.phabricator.com/D20645
Summary: See <https://discourse.phabricator-community.org/t/phd-status-calls-to-undefined-method-when-theres-no-instance/2918>. This call should be `logInfo()`.
Test Plan:
- Purged `PHABRICATOR_INSTANCE` from my environment. In a Phacility development environment, it comes from loading `services/`.
- Ran `bin/phd stop` with all daemons already stopped.
- Before: bad call.
- After: helpful error.
- Ran some other `bin/phd start`, `bin/phd status`, etc., to kick the tires.
- Grepped for remaining `writeInfo()` calls (found none).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20649
Summary: See rPaacc62463d61. D20551 added some `CAN_INTERACT` checks, but `CAN_INTERACT` needs to be checked with `canInteract()` to fall back to `CAN_VIEW` properly. D20558 cleaned up most of this but missed one callsite; fix that up too.
Test Plan: Removed a comment on a commit.
Reviewers: amckinley, 20after4
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20648
Summary: Ref T13332. This fix isn't terribly satisfying, but resolves the issue: this behavior may attempt to build HTML blocks with metadata after Javascript footer rendering has started. Use `hsprintf()` to flatten the markup earlier.
Test Plan: Put a `T123` reference in the description of a Pholio image, then loaded a mock.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13332
Differential Revision: https://secure.phabricator.com/D20647
Summary:
Ref D20645. Start making this view a little more useful:
{F6573605}
Test Plan: Mk. 1 eyeball
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D20646
Summary:
Depends on D20636. Ref T4900. Previously, some workflows didn't know how to identify the default state for the board, so they needed explicit ("force") parameters.
Everything uses the same state management code now so we can rip out the old stuff.
Test Plan: Changed board filters, selected a custom filter, edited a custom filter.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20637
Summary:
Depends on D20635. Ref T4900. Fixes T13316.
Currently, "Move Tasks to Column..." first prompts you to select a project, then prompts you for a column. The first step is prefilled with the current project, so the common case (moving to another column on the same board) requires you to confirm that you aren't doing an off-project move by clicking "Continue", then you can select a column.
This isn't a huge inconvenience and the workflow isn't terribly common, but it's surprising enough that it has come up a few times as a stumbling block. Particularly, we're suggesting to users that they're about to pick a column, then we're asking them to pick a project. The prompt also says "Project: XYZ", not "Project: Keep in current project" or something like that.
Smooth this out by splitting the action into two better-cued flows:
- "Move Tasks to Project..." is the current flow: pick a project, then pick a column.
- The project selection no longer defaults to the current project, since we now expect you to usually use this flow to move tasks to a different project.
- "Move Tasks to Column..." prompts you to select a column on the same board.
- This just skips step 1 of the workflow.
- This now defaults to the current column, which isn't a useful selection, but is more clear.
In both cases, the action cue ("Move tasks to X...") now matches what the dialog actually asks you for ("Pick an X").
Test Plan:
- Moved tasks across projects and columns within the same project.
- Hit all (I think?) the error cases and got sensible error and recovery behavior.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13316, T4900
Differential Revision: https://secure.phabricator.com/D20636
Summary: Depends on D20634. Ref T4900. Ref T13316. I'm planning to do a bit of additional cleanup here in followups, but this separates the main workflow out of the common controller.
Test Plan:
- Used "Move Tasks to Column..." to move some tasks on a board.
- Tried to move an empty column, hit an error.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13316, T4900
Differential Revision: https://secure.phabricator.com/D20635
Summary: Depends on D20633. Ref T4900. Separate the "Bulk Edit Tasks..." flow out of the main workboard controller.
Test Plan:
- Used "Bulk Edit Tasks" on a column with some tasks, got an appropraite edit operation.
- Used "Bulk Edit Tasks" on an empty column, got an error.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20634
Summary:
Depends on D20632. Ref T4900. As with other workflows on the board controller, this one is currently in the giant main "do everything" method. Move it to a separate controller.
This makes one material improvement: previously, we built the full board and did layout on all the cards before building the query. However, we do not actually need to do this: we don't need the cards. Instead, just do layout without handing over any card PHIDs. This is slightly faster, particularly on large boards.
Test Plan:
- Clicked "View as Query" on a board, got a query page for the column.
- Applied a custom filter, then clicked "View as Query" on a board. Got a query page merging the two filters.
- Applied a custom filter, then clicked "Veiw as Query" on a board, in a subproject column. Got a query page merging the two filters, respecting the project-ness of the column.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20633
Summary:
Depends on D20629. Ref T4900. Currently, the "Advanced Filter..." workflow on workboards (where you build a custom query) is inline in the main board controller.
This is because the filter flow depends on some of the board view state: we want to start with the current filter applied to the board, and preserve other state after you change the filter.
Now that `ViewState` can handle state management, we can separate this stuff out pretty easily.
Test Plan:
- Changed filters on a board.
- Applied a custom filter to a board.
- Changed the ordering of a board, then applied a custom filter. Verified "Cancel" and "Apply Filter" both preserve the order state.
- Changed the ordering of a board, then applied a custom filter, intentionally making a mistake in configuring the filter by entering an invalid date. Saw a dialog with an error. After correcting the error, saw state preserved properly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20632
Summary:
Depends on D20628. Ref T4900. Currently, the "Save Current Order/Filter As Default" flows on workboards duplicate some state construction, and require parameters to be passed to them explicitly.
Now that state management is separate, they can reuse a bit more code and be made to look more like other similar controllers.
Test Plan:
- Changed the default order of a workboard.
- Changed the default filter of a workboard.
- Changed the order of a board to something non-default, then changed the filter, then saved the new filter as the default. Saw the modified order preserved and the modified filter removed, so I ended up in the right ("most correct") place: on the board, with my custom order in a URI parameter, and no filter URI parameter so I could see my new default filter behavior. This is an edge case that's not terribly important to get right, but we do get it right.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20629
Summary:
Depends on D20627. Ref T4900. If a user orders a board by "Sort by Title", then toggles the visibility of hidden columns, we want to keep the board sorted by title. To accomplish this, we pass the board state around to all the workflows here.
Pull the "bag of state properties" code out of the View controller. This class basically:
- reads state from a request (order, hidden, filter);
- manages defaults;
- provides the application with the current settings; and
- generates URIs with "?order=X&hidden=Y&filter=Z" to preserve state.
This is still a little questionable/transitional since some of the controllers need more cleanup.
Test Plan:
Toggled state, order, filters, clicked around various workflows and saw the filters preserved.
A lot of these workflows are pretty serious edge cases. For example, here's a feature this implements:
- Changed workboard order to "Title".
- Selected "Bulk Edit Tasks..." in an empty column and command-clicked it to open the link in a new window.
- Hovered over "Cancel".
- Saw the link properly generate with "?order=title", preserving the order.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20628
Summary: Depends on D20626. Ref T4900. On this controller, "id" is a separate property, but serves little purpose and complicates separating state management. Remove it.
Test Plan: Bulk edited a column, managed filters, did show/hide on columns, edited a column.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20627
Summary:
Ref T4900. The Workboard view controller currently has a lot of different responsibilities (it's ~1,500 lines long) because it has to manage the board filter/sort state.
I'd like to split it up and make it easier to move some workboard features (like "move all tasks in column...") to other Controllers, so we can have smaller controllers implementing specific workflows.
I think the state handling isn't really all that bad, it just needs to be separated a little better than it currently is.
To start with, remove the unused "slug" property.
Test Plan: Searched for "slug", got no hits. This class is final and the property is private, so this is certainly unused.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T4900
Differential Revision: https://secure.phabricator.com/D20626
Summary: Depends on D20624. Fixes T13330. The OAuth client pages are using some out-of-date rendering conventions; update them to modern conventions.
Test Plan:
Viewed a page, saw a modern header layout + curtain:
{F6534135}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13330
Differential Revision: https://secure.phabricator.com/D20625
Summary:
Ref T13330. Handles for "OAuthServerClient" objects currently do not have a URI, which causes some obscure fallout like a missing "Close" button when examining their transactions.
Add a URI.
Test Plan:
- Viewed an OAuth server client detail page.
- Edited a policy, changing it to a custom policy.
- Clicked "Custom Policy" in the resulting transaction to view a dialog explaining the changes.
- Before change: dialog has no close button.
- After change: dialog has a close button.
{F6534121}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13330
Differential Revision: https://secure.phabricator.com/D20624
Summary:
See D20540. I mistakenly multiplied some strenghts by 100 and others by 1000 when converting them to integers for `PhutilSortVector`.
Multiply them all by 100 (that is, divide the ones which were multiplied by 1000 by 10) to put things back the way they were.
Test Plan: quick mafs
Reviewers: amckinley, richardvanvelzen
Reviewed By: richardvanvelzen
Differential Revision: https://secure.phabricator.com/D20622
Summary:
Fixes T13326. In D20571, I slightly generalized construction of an iterator over a set of files, but missed some code in other "bin/files ..." commands which was also affected.
Today, basically all of these workflows define their own "--all" and "names" flags. Pull these definitions up and implement them more consistently.
Test Plan: Ran multiple different `bin/files` commands with different combinations of arguments, saw consistent handling of iterator construction.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13326
Differential Revision: https://secure.phabricator.com/D20614
Summary:
Fixes T13327. Currently, when we try to bill an account and all members are disabled, we fail temporarily and the task retries forever.
At least for now, just treat this as a permanent failure.
Test Plan:
- Used `bin/phortune invoice` to generate a normal invoice for a regular subscription.
- Disabled all the account members, then tried again. Got a helpful permanent failure:
```
$ ./bin/phortune invoice --subscription PHID-PSUB-kbedwt5cyepoc6tohjq5 --auto-range
Set current time to Mon, Jun 24, 2:47 PM.
Preparing to invoice subscription "localb.phacility.com" from Fri, May 31, 10:14 AM to Sun, Jun 30, 10:14 AM.
WARNING
Manually invoicing will double bill payment accounts if the range overlaps an
existing or future invoice. This script is intended for testing and
development, and should not be part of routine billing operations. If you
continue, you may incorrectly overcharge customers.
Really invoice this subscription? [y/N] y
[2019-06-24 14:47:57] EXCEPTION: (PhabricatorWorkerPermanentFailureException) All members of the account ("PHID-ACNT-qp54y3unedoaxgkkjpj4") for this subscription ("PHID-PSUB-kbedwt5cyepoc6tohjq5") are disabled. at [<phabricator>/src/applications/phortune/worker/PhortuneSubscriptionWorker.php:88]
arcanist(head=experimental, ref.master=d92fa96366c0, ref.experimental=db4cd55d4673), corgi(head=master, ref.master=6371578c9d32), instances(head=stable, ref.master=ba9e4a19df1c, ref.stable=37fb1f4917c7), libcore(), phabricator(head=master, ref.master=65bc481c91de, custom=11), phutil(head=master, ref.master=7adfe4e4f4a3), services(head=master, ref.master=5424383159ac)
#0 PhortuneSubscriptionWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:124]
#1 PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:163]
#2 PhabricatorWorker::scheduleTask(string, array, array) called at [<phabricator>/src/applications/phortune/management/PhabricatorPhortuneManagementInvoiceWorkflow.php:169]
#3 PhabricatorPhortuneManagementInvoiceWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:457]
#4 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:349]
#5 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/setup/manage_phortune.php:21]
$
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13327
Differential Revision: https://secure.phabricator.com/D20613
Summary: Ref T13321. The daemons no longer write PID files, so we no longer need to pass any of this stuff to them.
Test Plan: Grepped for affected symbols.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13321
Differential Revision: https://secure.phabricator.com/D20608
Summary:
Ref T13321. This gets rid of the last pidfile readers in Phabricator; we just use the process list instead.
These commands always only work on the current instance since they don't make much sense otherwise.
Test Plan: Ran `bin/phd start` and `bin/phd reload` with and without daemons running.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13321
Differential Revision: https://secure.phabricator.com/D20606
Summary:
Ref T13321. Fixes T11037. Realign "bin/phd status" to just mean "show daemon processes on this host".
The value of `bin/phd status` as a mixed remote/local command isn't clear, and the current output is a confusing mess (see T11037).
This also continues letting us move away from PID files.
Test Plan: Ran `bin/phd status`, saw sensible local process status.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13321, T11037
Differential Revision: https://secure.phabricator.com/D20604
Summary:
Ref T13321. Previously, the behavior was:
- `bin/phd stop --gently`: Stop all daemons with PID files that belong to the current instance.
- `bin/phd stop`: Stop all daemons with PID files that belong to the current instance. Complain if there are more processes.
- `bin/phd stop --force`: Stop all processes that look like daemons, ignoring instances.
The new behavior is:
- `bin/phd stop`: Stop all processes that look like daemons and belong to the current instance.
- `bin/phd stop --force`: Stop all processes that look like daemons, period.
Test Plan: Grep / documentation only.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13321
Differential Revision: https://secure.phabricator.com/D20602
Summary:
Ref T13321. Depends on D20600. Make `bin/phd stop` mean:
- `bin/phd stop`: Stop all processes which have daemon process titles. If we're instanced, only stop daemons for the current instance.
- `bin/phd stop --force`: Stop all processes which have deamon process titles for any instance.
We no longer read or care about PID files on disk, and this moves us away from PID files.
This makes unusual flag `--gently` do nothing. A followup will update the documentation and flags to reflect actual usage/behavior.
This also removes the ability to stop specific PIDs. This was somewhat useful long, long ago when you might explicitly run different copies of the `PullLocal` daemon with flags to control which repositories they updated, but with the advent of clustering it's no longer valid to run custom daemon loadouts.
Test Plan: Ran `bin/phd start`, then `bin/phd stop`. Saw instance daemons stop. Ran `bin/phd stop --force`, saw all daemons stop.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13321
Differential Revision: https://secure.phabricator.com/D20601
Summary:
See <https://discourse.phabricator-community.org/t/cannot-audit-a-git-commit/2848>. In D20581, I made some audit behavior dependent upon identities, but the actual edit flow doesn't load them. This can cause us to raise an "attach identities first" exception in the bowels of the edit workflow and trigger unexpected behavior at top level.
Load identities when editing a commit so that the transaction flows have access to identity information and can use it to figure out if a user is an author, etc.
Test Plan:
- As an auditor, applied an "Accept Commit" action to an open audit after D20581.
- Before patch: accept no-ops internally since the preconditions throw.
- After patch: accept works properly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20612
Summary:
Ref T13291. See PHI1312. Currently, if you link to a JIRA or Asana issue with an anchor (`#asdf`) or query parameters (`?a=b`), we:
- treat the link as an external object reference and attempt a lookup on it;
- if the lookup succeeds, we discard the fragment or parameters when re-rendering the rich link (with the issue/task title).
Particularly, the re-rendering part uses the canonical URI of the object, and can discard these parameters/fragments, which is broken/bad.
As a first pass at improving this, just don't apply special behavior for links with anchors or parameters -- simply treat them as links.
In some future change, we could specialize this behavior and permit certain known parameters or anchors or something, but these use cases are likely fairly marginal.
Test Plan:
Before:
{F6516392}
After:
{F6516393}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13291
Differential Revision: https://secure.phabricator.com/D20592
Summary: Ref T13319. Ref PHI1302. Migrate `PhabricatorEditEngineConfigurationTransaction` to modular transactions and add some additional transaction rendering to make these edits less opaque.
Test Plan: Hit all the form edit controllers, viewed resulting transaction timeline.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T13319
Differential Revision: https://secure.phabricator.com/D20595
Summary:
Fixes T13324. Ref PHI1288. Currently, if you edit an Owners package that has some paths with no trailing slashes (like `README.md`) so their internal names and display names differ (`/README.md` display, vs `/README.md/` internal), the "Show Details" in the transaction log shows the path as re-normalized even if you didn't touch it.
Instead, be more careful about handling display paths vs internal paths.
(This code on the whole is significantly less clear than it probably could be, but this issue is so minor that I'm hesitant to start ripping things out.)
Test Plan:
- In a package with some paths like `/src/` and some paths like `/src`:
- Added new paths.
- Removed paths.
- Changed paths from `/src/` to `/src`.
- Changed paths from `/src` to `/src/`.
In all cases, the "paths" list and the transaction record identically reflected the edit in the way I expected them to.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13324
Differential Revision: https://secure.phabricator.com/D20596
Summary:
Ref T13319. Currently, transactions about changes to a default form value use a raw internal key for the affected field and don't show the actual value change.
An ideal implementation will likely require us to specialize a great deal of rendering, but we can do much better than we currently do without too much work:
- Try to pull the actual `EditField` object for the key so we can `getLabel()` it and get a human-readable label (like `Visible To` instead of `policy.view`).
- Add a "(Show Changes)" action that dumps the raw values as more-or-less JSON, so you can at least figure out what happened if you're sophisticated enough.
Test Plan:
Before:
{F6516640}
After:
{F6516642}
The quality of "Show Details" varies a lot. For some fields, like "Description", it's pretty good:
{F6516645}
For others, like "Assigned To", it's better than nothing but pretty technical:
{F6516647}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13319
Differential Revision: https://secure.phabricator.com/D20594
Summary:
Fixes T13317. On `admin.phacility.com`, an enterprising user added `noreply@admin.phacility.com` to their account. This caused them to become CC'd on several support issues over the last year, because we send mail "From" this address and it can get CC'd via reply/reply all/whatever else.
The original driving goal here is that if I reply to a task email and CC you on my reply, that should count as a CC in Phabricator, since this aligns with user intent and keeps them in the loop.
This misfire on `noreply@` is ultimately harmless (being CC'd does not grant the user access permission, see T4411), but confusing and undesirable. Instead:
- Don't allow reserved addresses ("noreply@", "ssladmin@", etc) to trigger this subscribe-via-CC behavior.
- Only count verified addresses as legitimate user recipients.
Test Plan:
- Added a `bin/mail receive-test --cc ...` flag to make this easier to test.
- Sent mail as `bin/mail receive-test --to X --as alice --cc bailey@verified.com`. Bailey was CC'd both before and after the change.
- Sent mail as `bin/mail receive-test --to X --as alice --cc unverified@imaginary.com`, an address which Bailey has added to her account but not verified.
- Before change: Bailey was CC'd on the task anyway.
- After change: Bailey is not CC'd on the task.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13317
Differential Revision: https://secure.phabricator.com/D20593
Summary:
Fixes T13312. Currently, {nav Manage > Branches} has a list of branches on the same page. This has a few minor issues:
- Pager is at the top (see T13312), which is weird.
- "Default" icon is mystery meat.
- Table is kind of pointless/redundant in general?
Previously, this table had more information about technical status of each branch (autoclose/track/publish) but most of these details have been simplified/eliminated, and the main "Branches" view now has more information than it did before.
Get rid of this and just link to the main view.
Test Plan: Viewed "Branches" in UI, saw a link to the main view instead of a weird table.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13312
Differential Revision: https://secure.phabricator.com/D20584
Summary:
Fixes T13309. If you void the warranty on a repository on disk and turn it into a shallow clone, Phabricator currently can't serve it.
We don't support hosting shallow working copies, but we should still parse and proxy the protocol rather than breaking in a mysterious way.
Test Plan:
- Created a shallow working copy with `mv X X.full; git clone --depth Y file://.../X.full X` in the storage directory on disk.
- Cloned it with `git clone <uri>`.
- Deleted all the refs inside it so the wire only has "shallow" frames; cloned it.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13309
Differential Revision: https://secure.phabricator.com/D20577
Summary:
See <https://discourse.phabricator-community.org/t/view-task-from-maniphest-e-mail-doesnt-have-url/2827>.
I added "View Task" / "View Commit" buttons recently but the logic for generating URIs isn't quite right. Fix it up.
Test Plan:
- Commented on a task.
- Used `bin/mail show-outbound --id ... --dump-html > out.html` to dump the HTML.
- Previewed the HTML in a browser.
- This time, actually clicked the button to go to the task.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20586
Summary:
See <https://discourse.phabricator-community.org/t/unhandled-exception-when-logging-in-with-mfa/2828>. The recent changes to turn `msort()` on a vector an error have smoked out a few more of these mistakes.
These cases do not meaningfully rely on sort stability so there's no real bug being fixed, but we'd still prefer `msortv()`.
Test Plan: Viewed MFA and External Account settings panels. Did a `git grep 'msort(' | grep -i vector` for any more obvious callsites, but none turned up.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20587
Summary:
Fixes T13315. See that task for discussion.
Without `--background`, we currently treat this as a catastrophic failure, but it's relatively routine for some repository states. We can safely continue reparsing other steps.
Test Plan: Ran `bin/repository reparse --all X --message` with commits faked to all be unreachable. Got warnings instead of a hard failure on first problem.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13315
Differential Revision: https://secure.phabricator.com/D20588
Summary:
Depends on D20580. Fixes T13311. When we choose which actions to show a user, we can either show them "auditor" actions (like "raise concern") or "author" actions (like "request verification").
Currently, we don't show "author" actions if you're the author of the commit via an identity mapping, but we should. Use identity mappings where they exist.
(Because I've implemented `getEffectiveAuthorPHID()` in a way that requires `$data` be attached, it's possible this will make something throw a "DataNotAttached" exception, but: probably it won't?; and that's easy to fix if it happens.)
Test Plan:
See D20580. As `@alice`, viewed the commit in the UI.
- Before: got auditor actions presented to me.
- After: got author actions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13311
Differential Revision: https://secure.phabricator.com/D20581
Summary:
Ref T13311. We currently don't use committer identity mappings when triggering audits, so if a user is only associated with an identity via manual mapping we won't treat them as the author.
Instead, use the identity and manual mapping if they're available.
Test Plan:
- Pushed a commit as `xyz <xyz@example.org>`, an address with no corresponding user.
- In the UI, manually associated that identity with user `@alice`.
- Ran `bin/repository reparse --publish <hash>` to trigger audits and publishing for the commit.
- Before: observed the `$author_phid` was `null`.
- After: observed the `$author_phid` is Alice.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13311
Differential Revision: https://secure.phabricator.com/D20580
Summary:
Fixes T13310. Use cases in the form "users with no access to any spaces can not <do things>" are generally unsupported (that is, we consider this to mean that the install is misconfigured), but "log out" is a somewhat more reasonable sort of thing to do and easy to support.
Drop the requirement that users be logged in to access the Logout controller. This skips the check for access to any Spaces and allows users with no Spaces to log out.
For users who are already logged out, this just redirects home with no effect.
Test Plan:
- As a user with access to no Spaces, logged out. (Before: error; after: worked).
- As a logged-out user, logged out (was redirected).
- As a normal user, logged out (normal logout).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13310
Differential Revision: https://secure.phabricator.com/D20578
Summary:
Fixes T13313. The "Download Raw Diff" workflow in Differential currently uses an older way of interacting with Files that doesn't engage the chunk engine and can't handle 8MB+ files.
Update to `IteratorFileUploadSource` -- we're still passing in a single giant blob, but this approach can be chunked.
This will still break somewhere north of 8MB (it will break at 2GB with the PHP string limit if nowhere sooner, since we're putting the entire raw diff in `$raw_diff` rather than using a rope/stream) but will likely survive diffs in the hundreds-of-megabytes range for now.
Test Plan:
- Added `str_repeat('x', 1024 * 1024 * 9)` to the `$raw_diff` to create a 9MB+ diff.
- Configured file storage with no engine explicitly configured for >8MB chunks (i.e., "reasonably").
- Clicked "Download Raw Diff".
- Before: misleading file storage engine error ("no engine can store this file").
- After: large, raw diff response.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13313
Differential Revision: https://secure.phabricator.com/D20579
Summary: Ref T13303. See B22967. This should be "msortv()" but didn't get updated properly.
Test Plan: The system works!
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13303
Differential Revision: https://secure.phabricator.com/D20585
Summary: Ref T13303. I upgraded this to a vector-based sort but forgot to type a "v", which means the sort has different stability under PHP 5.5. See D20582 for a root cause fix.
Test Plan: Locally, on PHP7, not much changes. I expect this to fix the odd selection of title stories in mail and notification stories on `secure`, which is running PHP 5.5.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13303
Differential Revision: https://secure.phabricator.com/D20583
Summary:
Fixes T13307. We currently require "CAN_EDIT" to sign actions, but it's fine to sign a comment with only "CAN_INTERACT".
Since the actions like "Accept Revision" already work like this, the fix is one line.
Test Plan: {F6488135}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13307
Differential Revision: https://secure.phabricator.com/D20574
Summary:
Ref T13306. Currently, there's no easy way to import a third-party local-disk file dump into a Phacility instance.
Add some more options to `bin/files migrate` to support this. In particular, this enables:
```
$ ./bin/files --from-engine local-disk --engine amazon-s3 --local-disk-source path/to/backup
```
...to import these files into S3 directly.
These are general-purpose options and theoretically useful in other use cases, although realistically those cases are probably very rare.
Test Plan: Used `bin/files` with the new options to move files in and out of local disk storage in an arbitrary backup directory. Got clean exports/imports.
Reviewers: amckinley
Maniphest Tasks: T13306
Differential Revision: https://secure.phabricator.com/D20571
Summary:
Ref T13298. Add a simple profiler as a starting point to catch any egregiously expensive rules or conditions.
This doesn't profile rule actions, so if "Add subscriber" (or whatever) is outrageously expensive it won't show up on the profile. Right now, actions get evaluated inside the Adapter so they're hard to profile. A future change could likely dig them out without too much trouble. I generally expect actions to be less expensive than conditions.
This also can't pin down a //specific// condition being expensive, but if you see that `H123` takes 20s to evaluate you can probably guess that the giant complicated regex is the expensive part.
Test Plan: {F6473407}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13298
Differential Revision: https://secure.phabricator.com/D20566
Summary:
Depends on D20566. Ref T13298. See PHI1280. Currently, there's no clean way to disable problematic personal rules. This comes up occasionally and sometimes isn't really the best approach to solving a problem, but is a generally reasonable capability to provide.
Allow Herald rules (including personal rules) to be disabled/enabled via `bin/herald rule ... --disable/--enable`.
Test Plan: Used the CLI to disable and enable a personal rule.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jmeador
Maniphest Tasks: T13298
Differential Revision: https://secure.phabricator.com/D20567
Summary:
Fixes T13300. Currently, if you create a revision and then immediately land it (either using `--draft` or just beating Harbormaster to the punch) it can be stuck in "Draft" forever.
Instead, count landing changes like this as a publishing action.
Test Plan:
- Used `arc diff --hold` to create a revision, then pushed the commit immediately.
- Before change: revision closed, but was stuck in draft.
- After change: revision closed and was promoted out of draft.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13300
Differential Revision: https://secure.phabricator.com/D20565
Summary:
See PHI1118. That issue may describe more than one bug, but the recent ordering changes to the import pipeline likely make this at least part of the problem.
Previously, commits would always close associated revisions before we made it to the "publish" step. This is no longer true, so we might be triggering audits on a commit before the associated revision actually closes.
Accommodate this by counting a revision in either "Accepted" or "Published (Was Previously Accepted)" as "reviewed".
Test Plan:
- With commit C affecting paths in package P with "Audit Unreviewed Commits and Commits With No Owner Involvement", associated with revision R, with both R and C authored by the same user, and "R" in the state "Accepted", used `bin/repository reparse --publish <hash>` to republish the commit.
- Before change: audit by package P triggered.
- After change: audit by package P no longer triggered.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20564
Summary:
Ref T13303. In D20525 I fixed an issue where transaction rendering could use cached values with the wrong viewer by reloading transactions.
However, reloading transactions may also reorder them as a side effect, since `withPHIDs(...)` does not imply an order. This can make transaction rendering order in mail wrong/inconsistent.
Instead, reorder the transactions before continuing so mail transaction order is consistent.
Test Plan: Applied a group of transactions to a task, saw a more consistent rendering order in mail after the change.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13303
Differential Revision: https://secure.phabricator.com/D20563
Summary:
See downstream <https://phabricator.wikimedia.org/T1050>. Some time ago, we added a "View Revision" button to Differential mail. This hasn't created any problems and generally seems good / desirable.
It isn't trivial to just add everywhere since we need a translation string in each case, but at least add it to Maniphest for now. Going forward, we can fill in more applications as they come up.
Test Plan:
Used `bin/mail show-outbound --id <x> --dump-html`:
{F6470461}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20561
Summary:
See downstream <https://phabricator.wikimedia.org/T88655>. This is very marginal, but we currently allow comments consisting of //only// whitespace.
These are probably always mistakes, so treat them like completely empty comments.
(We intentionally do not trim leading or trailing whitespace from comments when posting them becuase leading spaces can be used to trigger codeblock formatting.)
Test Plan:
- Posted empty, nonempty, and whitespace-only comments.
- Whitespace-only comments now have the same behavior as truly empty comments (e.g., do not actually generate a transaction).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20562
Summary: These instructions are fairly old and can be a little fancier and more clear in the context of modern Phabricator. Drop the reference to "HPHP", link the actual timezone list, wordsmith a little.
Test Plan: d( O_o )b
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20560
Summary:
See downstream <https://phabricator.wikimedia.org/T902>. Currently, timezones are rendered with their raw internal names (like `America/Los_Angeles`) which include underscores.
Replacing underscores with spaces is a more human-readable (and perhaps meaningfully better for things like screen readers, although this is pure speculation).
There's some vague argument against this, like "administrators may need to set a raw internal value in `phabricator.timezone` and this could mislead them", but we already give a pretty good error message if you do this and could improve hinting if necessary.
Test Plan: Viewed timezone list in {nav Settings} and the timezone "reconcile" dialog, saw a more-readable "Los Angeles".
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20559
Summary:
Ref T13289. See D20551. In D20551, I implemented some "CAN_INTERACT" checks against certain edits, but these checks end up testing "CAN_INTERACT" against objects like Conpherence threads which do not support a distinct "CAN_INTERACT" permission. I misrembered how the "CAN_INTERACT" fallback to "CAN_VIEW" actually works: it's not fully automatic, and needs some explicit "interact, or view if interact is not available" checks.
Use the "interact" wrappers to test these policies so they fall back to "CAN_VIEW" if an object does not support "CAN_INTERACT". Generally, objects which have a "locked" state have a separate "CAN_INTERACT" permission; objects which don't have a "locked" state do not.
Test Plan: Created and edited comments in Conpherence (or most applications other than Maniphest).
Reviewers: amckinley
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20558
Summary:
Ref T13289. If you do this:
- Subscribe to a task (so we don't generate a subscribe side-effect later).
- Prepare a transaction group: sign with MFA, change projects (don't make any changes), add a comment.
- Submit the transaction group.
...you'll get prompted "Some actions don't have any effect (the non-change to projects), apply remaining effects?".
If you confirm, you get MFA'd, but the MFA flow loses the "continue" confirmation, so you get trapped in a workflow loop of confirming and MFA'ing.
Instead, retain the "continue" bit through the MFA.
Also, don't show "You can't sign an empty transaction group" if there's a comment.
See also T13295, since the amount of magic here can probably be reduced. There's likely little reason for "continue" or "hisec" to be magic nowadays.
Test Plan:
- Went through the workflow above.
- Before: looping workflow.
- After: "Continue" carries through the MFA gate.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20552
Summary: Ref T11741. See PHI1276. After the switch to "Ferret", this table has no remaining readers or writers.
Test Plan:
- Ran `bin/storage upgrade -f`, no warnings.
- Grepped for class name, table name, `stemmedCorpus` column; got no relevant hits.
- Did a fulltext search.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11741
Differential Revision: https://secure.phabricator.com/D20549
Summary:
Ref T13289. This tightens up a couple of corner cases around locked threads.
Locking is primarily motivated by two use cases: stopping nonproductive conversations on open source installs (similar to GitHub's feature); and freezing object state for audit/record-keeping purposes.
Currently, you can edit or remove comments on a locked thread, but neither use case is well-served by allowing this. Require "CAN_INTERACT" to edit or remove a comment.
Administrators can still remove comments from a locked thread to serve "lock a flamewar, then clean it up", since "Remove Comment" on a comment you don't own is fairly unambiguously an administrative action.
Test Plan:
- On a locked task, tried to edit and remove my comments as a non-administrator. Saw appropriate disabled UI state and error dialogs (actions were disallowed).
- On a locked task, tried to remove another user's comments as an administrator. This works.
- On a normal task, edited comments normally.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20551
Summary:
Ref T13289. See <https://discourse.phabricator-community.org/t/fatal-error-in-pagination-in-drydock-resources-host-logs-all-logs/2735>.
`bin/drydock lease` and the web UI for reviewing all object logs when there is more than one page of logs didn't get fully updated to the new cursors.
- Use a cursor pager in `bin/drydock lease`.
- Implement `withIDs()` in `LeaseQuery` so the default paging works properly.
Test Plan:
- Ran `bin/drydock lease`, got a lease with log output along the way.
- Set page size to 2, viewed host logs with multiple pages, paged to page 2.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20553
Summary:
Depends on D20546. Ref T13283. Currently, if you do something (transactions "A", "B") and Herald does some things in response (transaction "C"), Herald acts only on the things you did ("A", "B") since the thing it did ("C") didn't exist yet, until it ran.
However, if you use the test console to test rules against the object we'll pick up all three transactions since they're all part of the same group. This isn't ideal.
To fix this, skip transactions which Herald applied, since it obviously didn't consider them when it was evaluating.
Test Plan:
- Created a revision, in the presence of a Herald rule that adds reviewers.
- Then, ran the revision through the test console.
- Before: saw the "Herald added reviewers: ..." transaction in the transaction group Herald evaluated.
- After: saw only authentic human transactions.
{F6464064}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20547
Summary:
Ref T13283. Currently, each Editor sets its own group ID, so if you create a revision and then Herald does some stuff, the two groups of transactions get different group IDs.
This means the test console is slightly misleading (it will only pick up the Herald transactions). It's going to be misleading anyway (Herald obviously can't evaluate Herald transactions) but this is at least a little closer to reality and stops Herald actions from masking non-Herald actions.
Test Plan:
- Created a revision. Herald applied one transaction.
- Used the test console.
- Before: The test console only picked up the single most recent Herald transaction.
- After: The test console picked up the whole transaction group.
{F6464059}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20546
Summary:
Ref T13289. When you create a Phriction document, you currently get an email with the whole new content as a "diff".
You also get extra transactions in the email and on the page.
This is because Phriction isn't on EditEngine and doesn't mark "create" transactions in a modern way. Get them marked properly to fix these obviously-broken behaviors. This can all go away once Phriction switches to EditEngine, although I don't have any particular plans to do that in the immediate future.
Test Plan:
- Created a new document, viewed email, no longer saw redundant "edited content" transaction or "CHANGES TO CONTENT" diff.
- Updated a document, viewed email, got interdiff.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13289
Differential Revision: https://secure.phabricator.com/D20548
Summary:
Ref T13290. Prior to recent changes, if we parsed some commit C which was associated with a revision R, but R was already closed, we'd skip the whole set of updates because the "close the revision" transaction would fail and we'd throw because we did not `setContinueOnNoEffect()`.
We now continue on no effect so we can get the edge ("commit has revision" / "revision has commit"), since we want it in all cases, but this means we may also apply an extra "Updated revision to reflect committed changes" transaction and new diff. This can happen even if we're careful about not trying to apply this transaction to closed revisions, since two workers may race. (Today, we aren't too careful about this.)
To fix this, just make this transaction no-op itself if the revision is already closed by the time it tries to apply.
This happened on D20451 because a merge commit with the same hash as the last diff was pushed, but it's easiest to reproduce by just running `bin/repository reparse --message <commit>`, which updates related revisions with a new diff every time.
Test Plan:
- Ran `bin/repository reparse --messsage <commit>` several times, on a commit with an associated revision.
- Before: each run attached a new diff and created a new "updated to reflect committed changes" transaction.
- After: repeated runs had no effects.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13290
Differential Revision: https://secure.phabricator.com/D20545
