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
